From b73c497f2094fe0492e92fc1ca37a3c64f91d978 Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Tue, 1 Sep 2026 19:48:00 -0400 Subject: [PATCH 1/2] fix(guard): name every denying layer in one Bash verdict (#297) classify_bash_with_policy returned only the first denial, so a command both layers would block (e.g. npm run dev redirected outside the sandbox) reported just the containment problem, and its actionable scratch hint sent the agent to fix something that could never unblock the command. classify_bash_denials now collects every applicable layer, and the arbiter renders one verdict that names each blocking reason. Single-layer denials keep their exact historic message; the two npm-install byte-pins (empty marker policy trips both the package-install containment and the command policy) move to the combined wording. --- docs/guides/guard.md | 4 +- src/adapters/guard.rs | 2 +- src/sandbox/decide.rs | 110 ++++++++++++++++++--- src/sandbox/policy.rs | 33 ++++++- src/sandbox/policy/tests/command_policy.rs | 75 ++++++++++++++ tests/cli/guard.rs | 2 +- 6 files changed, 204 insertions(+), 22 deletions(-) diff --git a/docs/guides/guard.md b/docs/guides/guard.md index 25d51cd..9e2462a 100644 --- a/docs/guides/guard.md +++ b/docs/guides/guard.md @@ -9,7 +9,9 @@ Without an explicit `guard` field, eval-magic detects packaged profiles from the ## Understand the two policy layers -Containment checks run before command allowances. A command policy cannot override these checks: +Containment checks run before command allowances. A command policy cannot override these checks. +A command that violates both layers is denied once, with a verdict that names each blocking reason, +so the denial never points at a fix that cannot unblock the command: - Direct write and patch tools must target the task environment. - Shell redirects and `tee` targets must resolve inside the task environment. diff --git a/src/adapters/guard.rs b/src/adapters/guard.rs index 6d8f9f2..92b51aa 100644 --- a/src/adapters/guard.rs +++ b/src/adapters/guard.rs @@ -650,7 +650,7 @@ mod tests { assert_eq!( verdict("codex", payload, Some(marker())).expect("should block"), "{\"decision\":\"block\",\"reason\":\"eval guard: blocked Bash \ - (package install/add) — runs outside the eval sandbox\"}" + (package install/add — runs outside the eval sandbox; command not allowed by eval guard policy)\"}" ); } diff --git a/src/sandbox/decide.rs b/src/sandbox/decide.rs index 59b9f52..016dced 100644 --- a/src/sandbox/decide.rs +++ b/src/sandbox/decide.rs @@ -17,7 +17,7 @@ use crate::core::fs::artifact_path; use super::command_policy::COMMAND_POLICY_REASON; use super::policy::{ - OUTPUT_REDIRECTION_REASON, apply_patch_paths, classify_bash_with_policy, is_patch_tool, + OUTPUT_REDIRECTION_REASON, apply_patch_paths, classify_bash_denials, is_patch_tool, is_shell_tool, is_under_any, is_write_tool, path_arg, resolve_path, }; @@ -220,25 +220,49 @@ pub(crate) fn decide_with_cwd( .get("command") .and_then(Value::as_str) .unwrap_or(""); - if let Some(classification) = - classify_bash_with_policy(command, &roots, invocation_cwd, guard_policy) - { - let hint = if classification.reason == OUTPUT_REDIRECTION_REASON { - scratch_hint(&roots) + let denials = classify_bash_denials(command, &roots, invocation_cwd, guard_policy); + if !denials.is_empty() { + // A containment denial stays a self-contained clause when the + // command policy also denies, so one verdict can name every + // blocking layer instead of sending the agent to fix a problem + // that cannot unblock the command. + let clause = |reason: &str| { + if reason == COMMAND_POLICY_REASON { + reason.to_string() + } else { + format!("{reason} — runs outside the eval sandbox") + } + }; + let verdict = if let [only] = denials.as_slice() { + let boundary = if only.reason == COMMAND_POLICY_REASON { + "" + } else { + " — runs outside the eval sandbox" + }; + format!("({}){boundary}", only.reason) } else { - String::new() + let clauses = denials + .iter() + .map(|denial| clause(denial.reason)) + .collect::>() + .join("; "); + format!("({clauses})") }; - let boundary = if classification.reason == COMMAND_POLICY_REASON { - "" + let hint = if denials + .iter() + .any(|denial| denial.reason == OUTPUT_REDIRECTION_REASON) + { + scratch_hint(&roots) } else { - " — runs outside the eval sandbox" + String::new() }; + let resolved_targets = denials + .into_iter() + .flat_map(|denial| denial.resolved_targets) + .collect(); return GuardEvaluation::deny( - format!( - "{GUARD_REASON_PREFIX}blocked {tool_name} ({}){boundary}{hint}", - classification.reason, - ), - classification.resolved_targets, + format!("{GUARD_REASON_PREFIX}blocked {tool_name} {verdict}{hint}"), + resolved_targets, ); } } @@ -407,6 +431,62 @@ mod tests { ); } + /// Issue #297: a command both layers would deny must get one verdict that + /// names both reasons. Naming only the redirect (with its actionable + /// scratch hint) sends the agent to fix a problem that cannot unblock the + /// command, because the command policy was already denying it. + #[test] + fn a_bash_denial_names_every_blocking_layer_in_one_verdict() { + let marker: GuardMarker = serde_json::from_value(json!({ + "active": true, + "allowedRoots": ["/work/.eval-magic/task"], + "guardPolicy": { "allow_tools": ["cargo"] } + })) + .unwrap(); + + let denied = decide_with_cwd( + "Bash", + &json!({ "command": "npm run dev > /tmp/dev-server.log 2>&1 &" }), + Some(&marker), + now_ms(), + Path::new("/work/.eval-magic/task"), + ); + + assert!(!denied.decision.allow); + let reason = denied.decision.reason.unwrap(); + assert!(reason.contains("output redirection to a file"), "{reason}"); + assert!( + reason.contains("command not allowed by eval guard policy"), + "{reason}" + ); + assert!( + reason.ends_with("For temporary or scratch files, use /work/.eval-magic/task/tmp."), + "{reason}" + ); + assert_eq!( + denied.resolved_targets, + vec!["/tmp/dev-server.log".to_string()] + ); + } + + #[test] + fn a_bash_denial_from_containment_alone_keeps_the_single_reason_verdict() { + let d = decide_now( + "Bash", + json!({ "command": "echo hi > /tmp/out.log" }), + Some(&marker()), + ); + + assert_eq!( + d.reason.as_deref(), + Some( + "eval guard: blocked Bash (output redirection to a file) \ + — runs outside the eval sandbox. For temporary or scratch files, use \ + /work/.eval-magic/tmp." + ) + ); + } + #[test] fn allows_bash_with_an_in_bounds_redirect() { let d = decide_now( diff --git a/src/sandbox/policy.rs b/src/sandbox/policy.rs index 4799c67..3978f22 100644 --- a/src/sandbox/policy.rs +++ b/src/sandbox/policy.rs @@ -198,18 +198,43 @@ pub(crate) fn classify_bash_with_cwd( ) } -/// Classify one shell tool call under its resolved eval command policy. +/// Classify one shell tool call under its resolved eval command policy, +/// returning the first applicable denial. pub(crate) fn classify_bash_with_policy( command: &str, allowed_roots: &[String], invocation_cwd: &Path, policy: &crate::core::GuardPolicyConfig, ) -> Option { + classify_bash_denials(command, allowed_roots, invocation_cwd, policy) + .into_iter() + .next() +} + +/// Every layer that denies one shell tool call, containment checks first and +/// the eval command policy second (at most one denial each). +/// +/// The stray-write audit wants only the first denial, but the live guard owes +/// the agent all of them in one verdict: a verdict that names just the +/// redirect it found sends the agent to fix a problem that cannot unblock the +/// command when the command policy was already denying it. +pub(crate) fn classify_bash_denials( + command: &str, + allowed_roots: &[String], + invocation_cwd: &Path, + policy: &crate::core::GuardPolicyConfig, +) -> Vec { if command.is_empty() { - return None; + return Vec::new(); + } + let mut denials = Vec::new(); + if let Some(denial) = classify_fixed_containment(command, allowed_roots, invocation_cwd) { + denials.push(denial); + } + if let Some(denial) = super::command_policy::classify_command_policy(command, policy) { + denials.push(denial); } - classify_fixed_containment(command, allowed_roots, invocation_cwd) - .or_else(|| super::command_policy::classify_command_policy(command, policy)) + denials } fn classify_fixed_containment( diff --git a/src/sandbox/policy/tests/command_policy.rs b/src/sandbox/policy/tests/command_policy.rs index 9ece762..516218d 100644 --- a/src/sandbox/policy/tests/command_policy.rs +++ b/src/sandbox/policy/tests/command_policy.rs @@ -154,3 +154,78 @@ fn shell_wrapper_allowance_cannot_bypass_fixed_containment() { Some("cargo build/test output") ); } + +#[test] +fn denials_reports_every_denying_layer_containment_first() { + let roots = vec!["/work/env".to_string()]; + let policy = crate::core::GuardPolicyConfig { + allow_commands: vec!["cargo build".to_string()], + ..crate::core::GuardPolicyConfig::default() + }; + + let denials = classify_bash_denials( + "npm run dev > /tmp/dev-server.log", + &roots, + Path::new("/work/env"), + &policy, + ); + + let reasons: Vec<&str> = denials.iter().map(|denial| denial.reason).collect(); + assert_eq!( + reasons, + [ + "output redirection to a file", + "command not allowed by eval guard policy" + ] + ); + assert_eq!( + denials[0].resolved_targets, + vec!["/tmp/dev-server.log".to_string()] + ); +} + +#[test] +fn denials_reports_a_single_layer_when_only_one_applies() { + let roots = vec!["/work/env".to_string()]; + + // Containment only: echo is unrecognized by the command policy. + let denials = classify_bash_denials( + "echo hi > /tmp/out.log", + &roots, + Path::new("/work/env"), + &crate::core::GuardPolicyConfig::default(), + ); + assert_eq!(denials.len(), 1); + assert_eq!(denials[0].reason, "output redirection to a file"); + + // Policy only: the redirect target is in bounds, but npm is claimed and + // not allowed. + let policy = crate::core::GuardPolicyConfig { + allow_commands: vec!["cargo build".to_string()], + ..crate::core::GuardPolicyConfig::default() + }; + let denials = classify_bash_denials( + "npm run dev > /work/env/dev-server.log", + &roots, + Path::new("/work/env"), + &policy, + ); + assert_eq!(denials.len(), 1); + assert_eq!( + denials[0].reason, + "command not allowed by eval guard policy" + ); +} + +#[test] +fn denials_is_empty_for_an_allowed_command() { + let roots = vec!["/work/env".to_string()]; + let policy = crate::core::GuardPolicyConfig { + allow_commands: vec!["cargo build".to_string()], + ..crate::core::GuardPolicyConfig::default() + }; + + assert!( + classify_bash_denials("cargo build", &roots, Path::new("/work/env"), &policy).is_empty() + ); +} diff --git a/tests/cli/guard.rs b/tests/cli/guard.rs index b03fce0..79921f7 100644 --- a/tests/cli/guard.rs +++ b/tests/cli/guard.rs @@ -204,7 +204,7 @@ fn guard_codex_block_verdict_bytes_are_stable() { .success() .stdout( "{\"decision\":\"block\",\"reason\":\"eval guard: blocked Bash \ - (package install/add) — runs outside the eval sandbox\"}", + (package install/add — runs outside the eval sandbox; command not allowed by eval guard policy)\"}", ); } From dee12946bf9c3105f60e05f085ce148950db476d Mon Sep 17 00:00:00 2001 From: samiamorwas Date: Tue, 1 Sep 2026 19:48:12 -0400 Subject: [PATCH 2/2] feat(guard): allow dev and start scripts in language/javascript (#297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The packaged profile allowed install, CI, test, build, lint, and typecheck, but not npm run dev — and framework/nextjs only activates on a next dependency, so a plain Vite + React project (the pinned Weeknight fixture) had no packaged way to start its own dev server during a guarded run. dev and start are generic lifecycle script names, not Next.js-specific, so they move into the language profile for npm, pnpm, Yarn, and Bun. --- docs/guides/guard.md | 4 +-- guard-profiles/language-javascript.toml | 8 +++++ src/sandbox/guard_profiles.rs | 30 ++++++++++++++++ tests/run/guard_policy.rs | 47 +++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 2 deletions(-) diff --git a/docs/guides/guard.md b/docs/guides/guard.md index 9e2462a..d24df9d 100644 --- a/docs/guides/guard.md +++ b/docs/guides/guard.md @@ -115,8 +115,8 @@ The packaged profiles are: - `language/rust` is detected from `Cargo.toml`. It allows `cargo build`, `check`, `test`, `run`, `fmt`, and `clippy`. - `language/javascript` is detected from `package.json`. It allows npm install, CI, test, build, - lint, and typecheck commands, plus corresponding pnpm, Yarn, and Bun install, add, test, build, - lint, and typecheck commands. + dev, start, lint, and typecheck commands, plus corresponding pnpm, Yarn, and Bun install, add, + test, build, dev, start, lint, and typecheck commands. - `framework/nextjs` is detected when `package.json` declares a `next` dependency. It allows npm, pnpm, Yarn, and Bun dev, build, and start scripts, plus direct `next` invocations through `npx`, `pnpm exec`, `yarn`, and `bunx`. diff --git a/guard-profiles/language-javascript.toml b/guard-profiles/language-javascript.toml index a05f013..5e6f285 100644 --- a/guard-profiles/language-javascript.toml +++ b/guard-profiles/language-javascript.toml @@ -5,24 +5,32 @@ allow_commands = [ "npm ci", "npm test", "npm run build", + "npm run dev", "npm run lint", + "npm run start", "npm run typecheck", "pnpm install", "pnpm add", "pnpm test", "pnpm run build", + "pnpm run dev", "pnpm run lint", + "pnpm run start", "pnpm run typecheck", "yarn install", "yarn add", "yarn test", "yarn run build", + "yarn run dev", "yarn run lint", + "yarn run start", "yarn run typecheck", "bun install", "bun add", "bun test", "bun run build", + "bun run dev", "bun run lint", + "bun run start", "bun run typecheck", ] diff --git a/src/sandbox/guard_profiles.rs b/src/sandbox/guard_profiles.rs index e805c36..e6dae2d 100644 --- a/src/sandbox/guard_profiles.rs +++ b/src/sandbox/guard_profiles.rs @@ -132,6 +132,36 @@ mod tests { assert!(!expanded.allow_commands.contains(&"npm test".to_string())); } + /// Issue #297: a plain package.json project (the pinned Weeknight fixture + /// is Vite + React) must be able to start its own dev server. `dev` and + /// `start` are generic lifecycle script names, not Next.js-specific, so + /// they belong to the language profile. + #[test] + fn language_javascript_allows_dev_and_start_scripts() { + let policy = GuardPolicyConfig { + profiles: vec!["language/javascript".to_string()], + ..GuardPolicyConfig::default() + }; + + let expanded = expand_policy(&policy).unwrap(); + + for command in [ + "npm run dev", + "npm run start", + "pnpm run dev", + "pnpm run start", + "yarn run dev", + "yarn run start", + "bun run dev", + "bun run start", + ] { + assert!( + expanded.allow_commands.contains(&command.to_string()), + "{command}" + ); + } + } + #[test] fn detection_joins_language_and_framework_profiles_recursively() { let root = tempdir().unwrap(); diff --git a/tests/run/guard_policy.rs b/tests/run/guard_policy.rs index e49e7b7..2a2b66b 100644 --- a/tests/run/guard_policy.rs +++ b/tests/run/guard_policy.rs @@ -105,3 +105,50 @@ fn per_eval_guard_replaces_the_default_and_disables_detection() { &serde_json::json!({ "allow_commands": ["npm run dev"] }) ); } + +/// Issue #297: the pinned default fixture (Vite + React, no `next` dependency) +/// must detect `language/javascript` alone, and the frozen policy must allow +/// the lifecycle scripts a browser check needs. +#[test] +fn a_plain_package_json_detects_language_javascript_with_dev_and_start() { + let tmp = tempfile::TempDir::new().unwrap(); + let evals = r#"{ + "skill_name": "mr-review", + "evals": [{ + "id": "e1", + "prompt": "build the app", + "expected_output": "built", + "files": ["package.json"] + }] + }"#; + let (skill_dir, cwd) = setup(tmp.path(), evals); + fs::write( + skill_dir.join("mr-review/evals/package.json"), + r#"{"dependencies":{"vite":"5.0.0","react":"18.0.0"}}"#, + ) + .unwrap(); + + skill_eval() + .current_dir(&cwd) + .args(["run", "--skill-dir"]) + .arg(&skill_dir) + .args(["--skill", "mr-review", "--mode", "new-skill", "--guard"]) + .assert() + .success(); + + let dispatch = read_json(&iteration_dir(&cwd).join("dispatch.json")); + let policy = &dispatch["tasks"][0]["guard_policy"]; + assert_eq!( + policy["profiles"], + serde_json::json!(["language/javascript"]) + ); + let commands: Vec<&str> = policy["allow_commands"] + .as_array() + .unwrap() + .iter() + .filter_map(|command| command.as_str()) + .collect(); + assert!(commands.contains(&"npm run dev"), "{commands:?}"); + assert!(commands.contains(&"npm run start"), "{commands:?}"); + assert!(!commands.contains(&"npx next dev"), "{commands:?}"); +}