fix: harden conjure CLI guards for hostile manifests and probes#22
Merged
Conversation
- Reject unknown manifest fields (deny_unknown_fields on AdapterManifest, RuntimeAdapter, StreamArgs, Capabilities, SandboxMapping) so typos like `capabilties` fail loudly instead of silently dropping capabilities. - Validate registry versions as strict major.minor.patch semver; `conjure registry add` now refuses non-semver versions before writing sources. - Give runtime probes a 5s timeout so a binary that blocks on stdin can no longer hang `conjure test`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens conjure’s safety/guardrails around adapter manifests and runtime probing to avoid silent misconfiguration and hangs when presented with hostile or malformed input.
Changes:
- Enforces strict manifest parsing by rejecting unknown fields (
deny_unknown_fields) across manifest-related types. - Adds registry version validation (
major.minor.patch) and surfaces invalid versions as explicit validation errors. - Adds a timeout to runtime probe execution so
conjure testcan’t hang indefinitely on non-cooperative binaries.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/coven-runtime-spec/src/validate.rs | Adds adapter version validation and introduces valid_registry_version plus a test. |
| crates/coven-runtime-spec/src/sandbox.rs | Rejects unknown fields for the untagged SandboxMapping enum. |
| crates/coven-runtime-spec/src/manifest.rs | Rejects unknown fields for manifest envelope + adapter/stream args; adds a regression test. |
| crates/coven-runtime-spec/src/capabilities.rs | Rejects unknown fields in Capabilities while preserving defaulting behavior. |
| crates/coven-runtime-cli/tests/validate_cli.rs | Updates CLI test expectations to match earlier parse-time failure on registry-index-as-manifest input. |
| crates/coven-runtime-cli/tests/registry_cli.rs | Adds a regression test ensuring non-semver versions are rejected without writing to sources. |
| crates/coven-runtime-cli/src/commands/test.rs | Implements probe timeout logic and adds a regression test for blocking binaries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+140
to
+164
| fn run_probe_command(executable: &str, flag: &str, timeout: Duration) -> std::io::Result<Output> { | ||
| let mut child = Command::new(executable) | ||
| .arg(flag) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) | ||
| .spawn()?; | ||
| let deadline = Instant::now() + timeout; | ||
|
|
||
| loop { | ||
| if child.try_wait()?.is_some() { | ||
| return child.wait_with_output(); | ||
| } | ||
|
|
||
| if Instant::now() >= deadline { | ||
| let _ = child.kill(); | ||
| let _ = child.wait(); | ||
| return Err(std::io::Error::new( | ||
| std::io::ErrorKind::TimedOut, | ||
| format!("probe `{executable} {flag}` timed out after {timeout:?}"), | ||
| )); | ||
| } | ||
|
|
||
| thread::sleep(Duration::from_millis(10)); | ||
| } | ||
| } |
Comment on lines
+193
to
+196
| use coven_runtime_spec::{Capabilities, SandboxMapping}; | ||
| use std::fs; | ||
| use std::os::unix::fs::PermissionsExt; | ||
| use tempfile::tempdir; |
Comment on lines
+238
to
+252
| #[test] | ||
| fn probe_times_out_blocking_binary() { | ||
| let dir = tempdir().unwrap(); | ||
| let script = dir.path().join("blocks"); | ||
| fs::write(&script, "#!/bin/sh\nsleep 1\n").unwrap(); | ||
| let mut perms = fs::metadata(&script).unwrap().permissions(); | ||
| perms.set_mode(0o755); | ||
| fs::set_permissions(&script, perms).unwrap(); | ||
|
|
||
| let a = adapter(script.to_str().unwrap()); | ||
| match probe_adapter_with_timeout(&a, Some("--version"), Duration::from_millis(50)) { | ||
| ProbeResult::NotRunnable(msg) => assert!(msg.contains("timed out"), "{msg}"), | ||
| other => panic!("expected NotRunnable timeout, got {:?}", DebugProbe(&other)), | ||
| } | ||
| } |
Comment on lines
+277
to
+280
| parts.next().is_none() | ||
| && [major, minor, patch] | ||
| .iter() | ||
| .all(|part| !part.is_empty() && part.parse::<u64>().is_ok()) |
The test builds a /bin/sh script and uses PermissionsExt, neither of which exists on Windows, breaking the windows-latest CI job. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Hardens
conjure's input guards in three places where bad input previously failed silently or hung:deny_unknown_fieldsonAdapterManifest,RuntimeAdapter,StreamArgs,Capabilities, andSandboxMapping— a typo likecapabiltiesused to silently drop declared capabilities; now it's a parse error naming the field. (Verified the untaggedSandboxMappingenum also rejects extras.)major.minor.patchsemver.conjure registry addrefuses non-semver versions before writing anything to sources.--version) can no longer hangconjure test; it's killed and reported as not runnable.Also updates the
validateCLI test: a registry index passed without--registry-indexnow fails at parse time (unknown field \format`) instead of the olderno adapters` message — still loud, earlier.Type
coven-runtime-*crate)If this changes the SDK
cargo fmt --all --checkcleancargo clippy --workspace --all-targets -- -D warningscleancargo test --workspace --lockedgreenadditionalProperties: false; this aligns the Rust parser with it)SCHEMA_VERSIONbumped if the change is backward-incompatible — N/Acoven-runtime-specremains pure (semver check is plain string parsing; probe timeout lives in the CLI crate)Notes for reviewers
Stricter parsing is technically pickier than before: manifests carrying unrecognized fields that previously parsed will now be rejected. All canonical registry manifests (
copilot,coven-code,hermes,opencode) validate cleanly.cargo deny checkalso green locally.