diff --git a/docs/guides/guard.md b/docs/guides/guard.md index 25d51cd..d24df9d 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. @@ -113,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/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/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/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)\"}", ); } 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:?}"); +}