From b58b421535e593b165393846a4c37d74283060ad Mon Sep 17 00:00:00 2001 From: Nitanshu Lokhande <56120084+nlok5923@users.noreply.github.com> Date: Sun, 22 Mar 2026 10:35:18 +0530 Subject: [PATCH] feat(shell): add Low/Medium/High risk levels for graduated command approval (closes #172) (#368) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(shell): add Low/Medium/High risk levels for graduated approval (#172) - Add `RiskLevel` enum (Low/Medium/High, Ord-comparable) to `tool.rs` and re-export from `tools/mod.rs` - Add `risk_level_for(¶ms) -> RiskLevel` to the `Tool` trait (default: Low); override on `ShellTool` via `classify_command_risk` - Add `classify_command_risk(command: &str) -> RiskLevel` to `shell.rs`: High for NEVER_AUTO_APPROVE patterns, Low for read-only prefixes, Medium for reversible mutations, Medium as the unknown-command default - Add `extract_command_param` helper to de-duplicate JSON extraction - Add `sudo ` to `NEVER_AUTO_APPROVE_PATTERNS` (now classified High) - Wire `risk_level_for` into `requires_approval`: Low → Never, Medium → UnlessAutoApproved, High → Always (uses upstream's new API) - Log risk level at INFO on every tool call in `worker.rs` - Replace `requires_explicit_approval` (simple bool) with the richer `classify_command_risk`; update dispatcher.rs test - Add tests: `test_classify_command_risk_high/low/medium/pipeline`, `test_risk_level_for_via_tool_trait`, updated approval tests Co-Authored-By: Claude Sonnet 4.6 * style: apply cargo fmt to shell.rs and dispatcher.rs Co-Authored-By: Claude Sonnet 4.6 * fix(shell): fix pipeline risk aggregation and word-boundary matching Address reviewer feedback: - `classify_command_risk` now iterates ALL pipeline segments and takes the maximum risk, so `echo hello | cargo build` → Medium instead of the previous (wrong) Low - Replace `starts_with` with `matches_command_pattern`: single-word patterns use exact first-token comparison so `lsblk` no longer matches `ls`, `makeself` no longer matches `make`, etc.; multi-word patterns (e.g. `git status`) still use starts_with + space boundary - Drop `--help` / `-h` from LOW_RISK_PATTERNS (can never be first token) - Add `test_classify_command_risk_word_boundary` and extend pipeline test with mixed Low+Medium and unknown-command cases Co-Authored-By: Claude Sonnet 4.6 * fix(shell): move sed/awk/find from Low to Medium risk `sed -i`, `awk -i inplace`, and `find -delete`/`find -exec rm` can all modify or delete files. Classifying these as Low (auto-approve) was unsafe. Moving to Medium requires UnlessAutoApproved approval, which prompts the user unless they have explicitly enabled auto-approve mode. Fixes review feedback from zmanian on PR #368. Co-Authored-By: Claude Sonnet 4.6 * fix(shell): update test to use classify_command_risk after requires_explicit_approval removal The rebase brought in upstream commits that removed requires_explicit_approval. Update the mixed-case destructive command test to assert RiskLevel::High via classify_command_risk instead. Co-Authored-By: Claude Sonnet 4.6 * fix(shell): use word-boundary matching for High-risk patterns to prevent false positives The NEVER_AUTO_APPROVE_PATTERNS check used `contains()` on the full command string, causing false positives: `makeshutdownscript` matched `shutdown`, `nftables-config` matched `nft`, and `passwdqc-check` matched `passwd`. Fix: move the High-risk check inside the per-segment loop and use `matches_command_pattern` (the same word-boundary logic used for Low/Medium), so classification is consistent across all three risk levels. Also remove the trailing spaces from `"nft "` and `"sudo "` in NEVER_AUTO_APPROVE_PATTERNS since `matches_command_pattern` handles word-boundary detection without them. Adds three regression tests for the false-positive cases. Co-Authored-By: Claude Sonnet 4.6 * fix(shell): address zmanian review — redirect safety + explicit git push pattern Two issues from zmanian's CHANGES_REQUESTED review on PR #368: 1. **Security (Low → UnlessAutoApproved)**: `Low` was mapped to `ApprovalRequirement::Never`, bypassing approval entirely for commands like `cat /etc/shadow > /tmp/out` since the pipeline splitter does not split on shell redirections (`>`, `>>`). Changing to `UnlessAutoApproved` preserves the graduated risk metadata for audit while keeping approval policy conservative until redirect-aware parsing is in place. 2. **Minor (explicit git push pattern)**: `git push origin feature-branch` fell through to the unknown-command Medium default rather than matching an explicit pattern. Adding `"git push"` to MEDIUM_RISK_PATTERNS makes the classification intentional. Force-push variants (`git push --force`, `git push -f`) remain in NEVER_AUTO_APPROVE_PATTERNS (High). Co-Authored-By: Claude Sonnet 4.6 * test(shell): add regression tests for redirect bypass and git push pattern fixes Two regression tests for the fixes in the previous commit: 1. `test_low_risk_with_redirect_not_never` — verifies that Low-risk commands containing shell redirections (`echo x > /etc/passwd`, `cat /etc/shadow > /tmp/out`, etc.) return `UnlessAutoApproved`, not `Never`. Before the fix, `Low` mapped to `Never` which would have allowed these writes to bypass approval entirely. 2. `test_git_push_explicit_medium_pattern` — verifies that `git push origin branch` is classified `Medium` via the explicit `MEDIUM_RISK_PATTERNS` entry (not the unknown-command fallthrough). Force variants (`--force`, `-f`) remain `High`. Co-Authored-By: Claude Sonnet 4.6 * test(shell): add integration regression tests for redirect bypass and git push Covers the two fixes from the previous commits at the integration-test level (tests/ directory) to ensure the CI regression-test gate is satisfied: 1. `low_risk_command_with_redirect_is_unless_auto_approved` -- verifies that Low-risk commands containing shell redirections return UnlessAutoApproved, not Never (the pre-fix behaviour that allowed redirect-based bypass). 2. `git_push_is_unless_auto_approved` -- verifies git push is Medium risk (UnlessAutoApproved) via the explicit pattern, not unknown-command fallthrough. 3. `git_push_force_requires_always_approval` -- verifies force-push variants remain High risk (Always approval required). Co-Authored-By: Claude Sonnet 4.6 * refactor(test): move inline assertions to tests/ to satisfy no-panics CI check The project's no-panics CI check (code_style.yml) scans src/**/*.rs for assert_eq!/assert_ne!/.unwrap() in added lines. Moving classify_command_risk tests to tests/shell_risk_regression.rs and adding // safety: comments on the two remaining assertions in dispatcher.rs eliminates all false positives. - Remove test_classify_command_risk_* and related functions from shell.rs - Remove test_low_risk_with_redirect_not_never and test_git_push_* from shell.rs (covered by integration tests in tests/) - Expand tests/shell_risk_regression.rs with full coverage via public API - Add // safety: test code comments on dispatcher.rs assert lines Co-Authored-By: Claude Sonnet 4.6 * fix(shell): address review findings — force-with-lease, test runners, Display - Add `git push --force-with-lease` to NEVER_AUTO_APPROVE_PATTERNS — the word-boundary matching in matches_command_pattern would not match it against the existing `git push --force` pattern (next char is `-`, not space), causing it to fall through to Medium instead of High. - Move `cargo test`, `npm test`, `npm run test`, `yarn test` from LOW_RISK_PATTERNS to MEDIUM_RISK_PATTERNS — test runners execute arbitrary code and can have side effects (file creation, network calls, process spawning). - Add `Display` impl for `RiskLevel` (lowercase: low/medium/high) and switch worker logging from `?risk` (Debug) to `%risk` (Display) for cleaner audit logs. - Fix integration test helper to call `register_dev_tools()` since ShellTool is registered there, not in `register_builtin_tools()`. Co-Authored-By: Claude Opus 4.6 (1M context) --------- Co-authored-by: Claude Sonnet 4.6 Co-authored-by: ilblackdragon@gmail.com --- src/agent/dispatcher.rs | 21 +-- src/tools/builtin/shell.rs | 332 +++++++++++++++++++++++---------- src/tools/mod.rs | 2 +- src/tools/tool.rs | 40 ++++ src/worker/job.rs | 2 + tests/shell_risk_regression.rs | 280 +++++++++++++++++++++++++++ 6 files changed, 564 insertions(+), 113 deletions(-) create mode 100644 tests/shell_risk_regression.rs diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index 8cd1d69b..90616074 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -1244,9 +1244,10 @@ mod tests { #[test] fn test_shell_destructive_command_requires_explicit_approval() { - // requires_explicit_approval() detects destructive commands that - // should return ApprovalRequirement::Always from ShellTool. - use crate::tools::builtin::shell::requires_explicit_approval; + // classify_command_risk() classifies destructive commands as High, which + // maps to ApprovalRequirement::Always in ShellTool::requires_approval(). + use crate::tools::RiskLevel; + use crate::tools::builtin::shell::classify_command_risk; let destructive_cmds = [ "rm -rf /tmp/test", @@ -1254,20 +1255,14 @@ mod tests { "git reset --hard HEAD~5", ]; for cmd in &destructive_cmds { - assert!( - requires_explicit_approval(cmd), - "'{}' should require explicit approval", - cmd - ); + let r = classify_command_risk(cmd); + assert_eq!(r, RiskLevel::High, "'{}'", cmd); // safety: test code } let safe_cmds = ["git status", "cargo build", "ls -la"]; for cmd in &safe_cmds { - assert!( - !requires_explicit_approval(cmd), - "'{}' should not require explicit approval", - cmd - ); + let r = classify_command_risk(cmd); + assert_ne!(r, RiskLevel::High, "'{}'", cmd); // safety: test code } } diff --git a/src/tools/builtin/shell.rs b/src/tools/builtin/shell.rs index 1e039c16..fa92cb37 100644 --- a/src/tools/builtin/shell.rs +++ b/src/tools/builtin/shell.rs @@ -56,7 +56,7 @@ use tokio::process::Command; use crate::context::JobContext; use crate::sandbox::{SandboxManager, SandboxPolicy}; use crate::tools::tool::{ - ApprovalRequirement, Tool, ToolDomain, ToolError, ToolOutput, require_str, + ApprovalRequirement, RiskLevel, Tool, ToolDomain, ToolError, ToolOutput, require_str, }; /// Maximum output size before truncation (64KB). @@ -117,7 +117,7 @@ static NEVER_AUTO_APPROVE_PATTERNS: LazyLock> = LazyLock::new( "init 0", "init 6", "iptables", - "nft ", + "nft", "useradd", "userdel", "passwd", @@ -132,6 +132,7 @@ static NEVER_AUTO_APPROVE_PATTERNS: LazyLock> = LazyLock::new( "docker rmi", "docker system prune", "git push --force", + "git push --force-with-lease", "git push -f", "git reset --hard", "git clean -f", @@ -139,6 +140,7 @@ static NEVER_AUTO_APPROVE_PATTERNS: LazyLock> = LazyLock::new( "DROP DATABASE", "TRUNCATE", "DELETE FROM", + "sudo", ] }); @@ -195,15 +197,205 @@ const SAFE_ENV_VARS: &[&str] = &[ "WINDIR", ]; -/// Check whether a shell command contains patterns that must never be auto-approved. +/// Low-risk command prefixes: strictly read-only commands with no side effects. +/// Note: `sed`, `awk`, and `find` are intentionally excluded — they have destructive +/// modes (`sed -i`, `awk -i inplace`, `find -delete`) and are classified as Medium. +static LOW_RISK_PATTERNS: LazyLock> = LazyLock::new(|| { + vec![ + "ls", + "ll", + "la", + "dir", + "cat", + "less", + "more", + "head", + "tail", + "grep", + "rg", + "ag", + "fd", + "locate", + "echo", + "printf", + "pwd", + "cd", + "env", + "printenv", + "which", + "whereis", + "type", + "date", + "cal", + "uptime", + "uname", + "df", + "du", + "free", + "top", + "htop", + "ps", + "git status", + "git log", + "git diff", + "git show", + "git branch", + "git remote", + "git fetch", + "cargo check", + "cargo clippy", + "curl --head", + "curl -I", + "ping", + "wc", + "sort", + "uniq", + "tr", + "cut", + "jq", + "yq", + "file", + "stat", + "man", + ] +}); + +/// Medium-risk command prefixes: mutations that are generally reversible, plus commands with +/// potentially destructive flags (e.g. `sed -i`, `awk -i inplace`, `find -delete`). +static MEDIUM_RISK_PATTERNS: LazyLock> = LazyLock::new(|| { + vec![ + // Text processors with in-place/destructive modes + "awk", + "sed", + "find", + "mkdir", + "rmdir", + "touch", + "cp", + "copy", + "mv", + "move", + "git commit", + "git add", + "git push", + "git checkout", + "git switch", + "git merge", + "git rebase", + "git stash", + "git tag", + "cargo build", + "cargo run", + "cargo test", + "npm test", + "npm run test", + "yarn test", + "npm install", + "npm ci", + "npm update", + "pip install", + "pip uninstall", + "brew install", + "brew uninstall", + "apt install", + "apt remove", + "make", + "cmake", + "tar", + "zip", + "unzip", + "gzip", + "gunzip", + "ssh", + "scp", + "rsync", + "curl", + "wget", + "docker build", + "docker pull", + "docker run", + "kubectl apply", + "kubectl create", + ] +}); + +/// Match a pipeline segment against a risk pattern using word-boundary rules. /// -/// Even when the user has chosen "always approve" for the shell tool, these commands -/// require explicit per-invocation approval because they are destructive. -pub fn requires_explicit_approval(command: &str) -> bool { - let lower = command.to_lowercase(); - NEVER_AUTO_APPROVE_PATTERNS - .iter() - .any(|p| lower.contains(&p.to_lowercase())) +/// - **Multi-word patterns** (e.g. `"git status"`): the segment must equal the +/// pattern or start with `" "`, so `"git statusbar"` does not match +/// `"git status"`. +/// - **Single-word patterns** (e.g. `"ls"`): the first whitespace-delimited +/// token of the segment must equal the pattern exactly, so `"lsblk"` does +/// not match `"ls"`. +fn matches_command_pattern(segment: &str, pattern: &str) -> bool { + if pattern.contains(' ') { + segment == pattern || segment.starts_with(&format!("{} ", pattern)) + } else { + segment.split_whitespace().next().unwrap_or("") == pattern + } +} + +/// Classify a shell command into a [`RiskLevel`]. +/// +/// The command is split on `|`, `&`, `;` and each segment is classified +/// independently; the overall risk is the **maximum** across all segments +/// so a dangerous sub-command in a pipeline is never missed. +/// +/// Per-segment priority (highest wins): +/// 1. **High** — segment matches [`NEVER_AUTO_APPROVE_PATTERNS`] (destructive / irreversible). +/// 2. **Low** — segment matches [`LOW_RISK_PATTERNS`] (strictly read-only). +/// 3. **Medium** — segment matches [`MEDIUM_RISK_PATTERNS`] (reversible mutations). +/// 4. **Medium** — unknown commands default to Medium (safer than auto-approving). +/// +/// All matching uses word-boundary rules (see [`matches_command_pattern`]) to +/// prevent false positives like `"makeshutdownscript"` matching `"shutdown"` or +/// `"lsblk"` matching `"ls"`. +pub fn classify_command_risk(command: &str) -> RiskLevel { + // For pipelines/chains, take the maximum risk across all segments. + command + .split(['|', '&', ';']) + .map(str::trim) + .filter(|s| !s.is_empty()) + .map(|segment| { + let seg_lower = segment.to_lowercase(); + if NEVER_AUTO_APPROVE_PATTERNS + .iter() + .any(|p| matches_command_pattern(&seg_lower, &p.to_lowercase())) + { + RiskLevel::High + } else if LOW_RISK_PATTERNS + .iter() + .any(|p| matches_command_pattern(&seg_lower, p)) + { + RiskLevel::Low + } else if MEDIUM_RISK_PATTERNS + .iter() + .any(|p| matches_command_pattern(&seg_lower, p)) + { + RiskLevel::Medium + } else { + // Unknown commands default to Medium (safer than auto-approving). + RiskLevel::Medium + } + }) + .max() + .unwrap_or(RiskLevel::Medium) +} + +/// Extract the `command` field from a tool-call parameter value. +/// +/// Handles both the normal case (a JSON object with a `"command"` key) and the +/// rare case where the LLM provider returns string-encoded JSON. +fn extract_command_param(params: &serde_json::Value) -> Option { + params + .get("command") + .and_then(|c| c.as_str().map(String::from)) + .or_else(|| { + params + .as_str() + .and_then(|s| serde_json::from_str::(s).ok()) + .and_then(|v| v.get("command").and_then(|c| c.as_str().map(String::from))) + }) } /// Detect command injection and obfuscation attempts. @@ -698,24 +890,24 @@ impl Tool for ShellTool { Ok(ToolOutput::success(result, duration)) } + fn risk_level_for(&self, params: &serde_json::Value) -> RiskLevel { + extract_command_param(params) + .map(|cmd| classify_command_risk(&cmd)) + .unwrap_or(RiskLevel::Medium) + } + fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement { - let cmd = params - .get("command") - .and_then(|c| c.as_str().map(String::from)) - .or_else(|| { - params - .as_str() - .and_then(|s| serde_json::from_str::(s).ok()) - .and_then(|v| v.get("command").and_then(|c| c.as_str().map(String::from))) - }); - - if let Some(ref cmd) = cmd - && requires_explicit_approval(cmd) - { - return ApprovalRequirement::Always; + match self.risk_level_for(params) { + // Low maps to UnlessAutoApproved rather than Never: shell redirections + // (e.g. `cat /etc/shadow > /tmp/out`) are not split on `>`, so a Low command + // with a redirect would bypass approval entirely with Never. Keeping + // UnlessAutoApproved preserves the graduated metadata for audit while + // ensuring approval policy stays conservative until redirect-aware parsing + // is in place. + RiskLevel::Low => ApprovalRequirement::UnlessAutoApproved, + RiskLevel::Medium => ApprovalRequirement::UnlessAutoApproved, + RiskLevel::High => ApprovalRequirement::Always, } - - ApprovalRequirement::UnlessAutoApproved } fn requires_sanitization(&self) -> bool { @@ -799,74 +991,11 @@ mod tests { assert!(matches!(result, Err(ToolError::Timeout(_)))); } - #[test] - fn test_requires_explicit_approval() { - // Destructive commands should require explicit approval - assert!(requires_explicit_approval("rm -rf /tmp/stuff")); - assert!(requires_explicit_approval("git push --force origin main")); - assert!(requires_explicit_approval("git reset --hard HEAD~5")); - assert!(requires_explicit_approval("docker rm container_name")); - assert!(requires_explicit_approval("kill -9 12345")); - assert!(requires_explicit_approval("DROP TABLE users;")); - - // Safe commands should not - assert!(!requires_explicit_approval("cargo build")); - assert!(!requires_explicit_approval("git status")); - assert!(!requires_explicit_approval("ls -la")); - assert!(!requires_explicit_approval("echo hello")); - assert!(!requires_explicit_approval("cat file.txt")); - assert!(!requires_explicit_approval( - "git push origin feature-branch" - )); - } - - /// Replicate the extraction logic from agent_loop.rs to prove it works - /// when `arguments` is a `serde_json::Value::Object` (the common case - /// that was previously broken because `Value::Object.as_str()` returns None). - #[test] - fn test_destructive_command_extraction_from_object_args() { - let arguments = serde_json::json!({"command": "rm -rf /tmp/stuff"}); - - let cmd = arguments - .get("command") - .and_then(|c| c.as_str().map(String::from)) - .or_else(|| { - arguments - .as_str() - .and_then(|s| serde_json::from_str::(s).ok()) - .and_then(|v| v.get("command").and_then(|c| c.as_str().map(String::from))) - }); - - assert_eq!(cmd.as_deref(), Some("rm -rf /tmp/stuff")); - assert!(requires_explicit_approval(cmd.as_deref().unwrap())); - } - - /// Verify extraction still works when `arguments` is a JSON string - /// (rare, but possible if the LLM provider returns string-encoded JSON). - #[test] - fn test_destructive_command_extraction_from_string_args() { - let arguments = - serde_json::Value::String(r#"{"command": "git push --force origin main"}"#.to_string()); - - let cmd = arguments - .get("command") - .and_then(|c| c.as_str().map(String::from)) - .or_else(|| { - arguments - .as_str() - .and_then(|s| serde_json::from_str::(s).ok()) - .and_then(|v| v.get("command").and_then(|c| c.as_str().map(String::from))) - }); - - assert_eq!(cmd.as_deref(), Some("git push --force origin main")); - assert!(requires_explicit_approval(cmd.as_deref().unwrap())); - } - #[test] fn test_requires_approval_destructive_command() { use crate::tools::tool::ApprovalRequirement; let tool = ShellTool::new(); - // Destructive commands must return Always to bypass auto-approve. + // High-risk commands must return Always to bypass auto-approve. assert_eq!( tool.requires_approval(&serde_json::json!({"command": "rm -rf /tmp"})), ApprovalRequirement::Always @@ -885,15 +1014,17 @@ mod tests { fn test_requires_approval_safe_command() { use crate::tools::tool::ApprovalRequirement; let tool = ShellTool::new(); - // Safe commands return UnlessAutoApproved (can be auto-approved). + // Medium-risk commands return UnlessAutoApproved (can be auto-approved). assert_eq!( tool.requires_approval(&serde_json::json!({"command": "cargo build"})), ApprovalRequirement::UnlessAutoApproved ); - assert_eq!( - tool.requires_approval(&serde_json::json!({"command": "echo hello"})), - ApprovalRequirement::UnlessAutoApproved - ); + // Low-risk commands also return UnlessAutoApproved (conservative until + // redirect-aware parsing is in place — see RiskLevel::Low mapping comment). + let r_echo = tool.requires_approval(&serde_json::json!({"command": "echo hello"})); + assert_eq!(r_echo, ApprovalRequirement::UnlessAutoApproved); // safety: test code + let r_ls = tool.requires_approval(&serde_json::json!({"command": "ls -la"})); + assert_eq!(r_ls, ApprovalRequirement::UnlessAutoApproved); // safety: test code } #[test] @@ -1370,9 +1501,12 @@ mod tests { #[test] fn test_approval_with_mixed_case_destructive() { - // Case-insensitive destructive command detection - assert!(requires_explicit_approval("RM -RF /tmp")); - assert!(requires_explicit_approval("Git Push --Force origin main")); - assert!(requires_explicit_approval("DROP table users;")); + // Case-insensitive destructive command detection → must be High risk + let r1 = classify_command_risk("RM -RF /tmp"); + assert_eq!(r1, RiskLevel::High); // safety: test code + let r2 = classify_command_risk("Git Push --Force origin main"); + assert_eq!(r2, RiskLevel::High); // safety: test code + let r3 = classify_command_risk("DROP table users;"); + assert_eq!(r3, RiskLevel::High); // safety: test code } } diff --git a/src/tools/mod.rs b/src/tools/mod.rs index 653544fd..86857ef4 100644 --- a/src/tools/mod.rs +++ b/src/tools/mod.rs @@ -34,6 +34,6 @@ pub(crate) use coercion::prepare_tool_params; pub use rate_limiter::RateLimiter; pub use registry::ToolRegistry; pub use tool::{ - ApprovalContext, ApprovalRequirement, Tool, ToolDomain, ToolError, ToolOutput, + ApprovalContext, ApprovalRequirement, RiskLevel, Tool, ToolDomain, ToolError, ToolOutput, ToolRateLimitConfig, redact_params, validate_tool_schema, }; diff --git a/src/tools/tool.rs b/src/tools/tool.rs index 2e2ee060..068654d1 100644 --- a/src/tools/tool.rs +++ b/src/tools/tool.rs @@ -1,5 +1,6 @@ //! Tool trait and types. +use std::fmt; use std::time::Duration; use async_trait::async_trait; @@ -112,6 +113,33 @@ impl Default for ToolRateLimitConfig { } } +/// Risk level of a tool invocation. +/// +/// Used by the shell tool to classify commands and by the worker to drive +/// approval decisions and observability logging. Implements `Ord` so callers +/// can compare levels (e.g. `risk >= RiskLevel::High`). +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +pub enum RiskLevel { + /// Read-only, safe, reversible (e.g. `ls`, `cat`, `grep`). + Low, + /// Creates or modifies state, but generally reversible + /// (e.g. `mkdir`, `git commit`, `cargo build`). + Medium, + /// Destructive, irreversible, or security-sensitive + /// (e.g. `rm -rf`, `git push --force`, `kill -9`). + High, +} + +impl fmt::Display for RiskLevel { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Low => f.write_str("low"), + Self::Medium => f.write_str("medium"), + Self::High => f.write_str("high"), + } + } +} + /// Where a tool should execute: orchestrator process or inside a container. /// /// Orchestrator tools run in the main agent process (memory access, job mgmt, etc). @@ -276,6 +304,18 @@ pub trait Tool: Send + Sync { true } + /// Risk level for a specific invocation of this tool. + /// + /// Defaults to `Low` (read-only, safe). Override for tools whose risk + /// depends on the parameters — the shell tool classifies commands into + /// `Low` / `Medium` / `High` based on the command string. + /// + /// The worker logs this value with every tool call so operators can audit + /// the risk level at which each execution was classified. + fn risk_level_for(&self, _params: &serde_json::Value) -> RiskLevel { + RiskLevel::Low + } + /// Whether this tool invocation requires user approval. /// /// Returns `Never` by default (most tools run in a sandboxed environment). diff --git a/src/worker/job.rs b/src/worker/job.rs index 738c2354..1b2be6f3 100644 --- a/src/worker/job.rs +++ b/src/worker/job.rs @@ -592,10 +592,12 @@ Report when the job is complete or if you encounter issues you cannot resolve."# // Redact sensitive parameter values before they touch any observability or audit path. let safe_params = redact_params(&effective_params, tool.sensitive_params()); + let risk = tool.risk_level_for(&effective_params); tracing::debug!( tool = %tool_name, params = %safe_params, job = %job_id, + risk = %risk, "Tool call started" ); diff --git a/tests/shell_risk_regression.rs b/tests/shell_risk_regression.rs new file mode 100644 index 00000000..dd3c8a8a --- /dev/null +++ b/tests/shell_risk_regression.rs @@ -0,0 +1,280 @@ +//! Regression and unit tests for shell command risk-level classification +//! (issue #172, PR #368). +//! +//! These tests live here (instead of inline in `src/tools/builtin/shell.rs`) +//! because the project's no-panics CI check scans `src/**/*.rs` for +//! `assert_eq!` / `assert_ne!` / `.unwrap()` in added lines. All assertions +//! on the public `ShellTool` API belong here. +//! +//! All tests access the shell tool through the public `ToolRegistry` + +//! `Tool` trait surface (`risk_level_for`, `requires_approval`). +//! +//! ## What is tested +//! +//! 1. **Risk level tiers** (`High`, `Medium`, `Low`) for representative commands. +//! 2. **Word-boundary matching** — commands whose names are substrings of other +//! words must not be misclassified. +//! 3. **Pipeline aggregation** — the whole pipeline takes the maximum risk of +//! its segments. +//! 4. **Redirect bypass regression** — Low-risk commands with shell redirections +//! must return `UnlessAutoApproved`, not `Never`. +//! 5. **`git push` regression** — non-force push is explicitly `Medium`; force +//! variants remain `High`. +//! 6. **`risk_level_for` trait method** — delegates to classify_command_risk. + +use ironclaw::tools::{ApprovalRequirement, RiskLevel, Tool, ToolRegistry}; +use std::sync::Arc; + +// --------------------------------------------------------------------------- +// Helper: obtain a `ShellTool` from the registry +// --------------------------------------------------------------------------- + +async fn shell_tool() -> Arc { + let registry = ToolRegistry::new(); + registry.register_builtin_tools(); + registry.register_dev_tools(); + registry + .all() + .await + .into_iter() + .find(|t| t.name() == "shell") + .expect("shell tool must be registered") +} + +fn risk(tool: &Arc, cmd: &str) -> RiskLevel { + tool.risk_level_for(&serde_json::json!({ "command": cmd })) +} + +fn approval(tool: &Arc, cmd: &str) -> ApprovalRequirement { + tool.requires_approval(&serde_json::json!({ "command": cmd })) +} + +// --------------------------------------------------------------------------- +// 1. Risk level tiers +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn high_risk_commands() { + let tool = shell_tool().await; + let cmds = [ + "rm -rf /tmp/stuff", + "git push --force origin main", + "git reset --hard HEAD~5", + "docker rm container_name", + "kill -9 12345", + "DROP TABLE users;", + "sudo apt install something", + ]; + for cmd in &cmds { + assert_eq!( + risk(&tool, cmd), + RiskLevel::High, + "command `{cmd}` should be High risk" + ); + } +} + +#[tokio::test] +async fn low_risk_commands() { + let tool = shell_tool().await; + let cmds = [ + "ls -la", + "cat file.txt", + "grep foo bar.txt", + "git status", + "git log --oneline", + "echo hello", + "cargo check", + ]; + for cmd in &cmds { + assert_eq!( + risk(&tool, cmd), + RiskLevel::Low, + "command `{cmd}` should be Low risk" + ); + } +} + +#[tokio::test] +async fn medium_risk_commands() { + let tool = shell_tool().await; + let cmds = [ + "cargo build", + "cargo test", + "npm test", + "yarn test", + "git commit -m 'foo'", + "mkdir /tmp/dir", + "npm install lodash", + "git push origin feature-branch", + "my-custom-tool --flag", + "sed 's/foo/bar/g' file.txt", + "sed -i 's/foo/bar/' file.txt", + "awk '{print $1}' file.txt", + "find . -name '*.rs'", + "find . -delete", + ]; + for cmd in &cmds { + assert_eq!( + risk(&tool, cmd), + RiskLevel::Medium, + "command `{cmd}` should be Medium risk" + ); + } +} + +// --------------------------------------------------------------------------- +// 2. Word-boundary matching (no false positives for substrings) +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn word_boundary_no_false_positives() { + let tool = shell_tool().await; + // "lsblk" must NOT match "ls" (Low-risk prefix) + assert_eq!(risk(&tool, "lsblk"), RiskLevel::Medium); + // "makeself" must NOT match "make" + assert_eq!(risk(&tool, "makeself output.run"), RiskLevel::Medium); + // "git statusbar" must NOT match "git status" + assert_eq!(risk(&tool, "git statusbar"), RiskLevel::Medium); + // Commands with High-risk names as substrings must not be tagged High + assert_eq!(risk(&tool, "makeshutdownscript --help"), RiskLevel::Medium); + assert_eq!(risk(&tool, "nftables-config"), RiskLevel::Medium); + assert_eq!(risk(&tool, "passwdqc-check"), RiskLevel::Medium); +} + +#[tokio::test] +async fn word_boundary_correct_positive_matches() { + let tool = shell_tool().await; + assert_eq!(risk(&tool, "ls -la"), RiskLevel::Low); + assert_eq!(risk(&tool, "make install"), RiskLevel::Medium); + assert_eq!(risk(&tool, "git status"), RiskLevel::Low); +} + +// --------------------------------------------------------------------------- +// 3. Pipeline aggregation +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn pipeline_takes_max_risk() { + let tool = shell_tool().await; + // High-risk segment → whole pipeline is High + assert_eq!(risk(&tool, "ls /tmp | rm -rf /tmp/stuff"), RiskLevel::High); + // All-low pipeline stays Low + assert_eq!(risk(&tool, "ls -la | grep foo"), RiskLevel::Low); + // Low + Medium → max is Medium + assert_eq!(risk(&tool, "echo hello | cargo build"), RiskLevel::Medium); + // Unknown command in pipeline → Medium (safe default) + assert_eq!( + risk(&tool, "cat file.txt | my-custom-tool"), + RiskLevel::Medium + ); +} + +// --------------------------------------------------------------------------- +// 4. Redirect bypass regression (Low → UnlessAutoApproved, not Never) +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn low_risk_command_with_redirect_is_unless_auto_approved() { + let tool = shell_tool().await; + let cases = [ + "echo secret_data > /etc/passwd", + "cat /etc/shadow > /tmp/exfil.txt", + "printf '%s' value > /tmp/leak", + "ls -la >> /tmp/log.txt", + ]; + for cmd in &cases { + let result = approval(&tool, cmd); + assert_eq!( + result, + ApprovalRequirement::UnlessAutoApproved, + "command `{cmd}` must be UnlessAutoApproved (not Never), got {result:?}" + ); + } +} + +// --------------------------------------------------------------------------- +// 5. git push regressions +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn git_push_classifies_as_medium_risk() { + let tool = shell_tool().await; + let cmds = [ + "git push", + "git push origin main", + "git push --set-upstream origin feature", + "git push upstream feature/foo", + ]; + for cmd in &cmds { + assert_eq!(risk(&tool, cmd), RiskLevel::Medium, "command `{cmd}`"); + } +} + +#[tokio::test] +async fn git_push_force_remains_high_risk() { + let tool = shell_tool().await; + let cmds = [ + "git push --force", + "git push -f", + "git push --force-with-lease", + "git push --force origin main", + "git push -f origin main", + ]; + for cmd in &cmds { + assert_eq!(risk(&tool, cmd), RiskLevel::High, "command `{cmd}`"); + } +} + +#[tokio::test] +async fn git_push_non_force_is_unless_auto_approved() { + let tool = shell_tool().await; + let cmds = [ + "git push", + "git push origin main", + "git push upstream feature/foo", + ]; + for cmd in &cmds { + let result = approval(&tool, cmd); + assert_eq!( + result, + ApprovalRequirement::UnlessAutoApproved, + "command `{cmd}` should be UnlessAutoApproved, got {result:?}" + ); + } +} + +#[tokio::test] +async fn git_push_force_requires_always_approval() { + let tool = shell_tool().await; + let cmds = [ + "git push --force", + "git push -f", + "git push --force-with-lease", + ]; + for cmd in &cmds { + let result = approval(&tool, cmd); + assert_eq!( + result, + ApprovalRequirement::Always, + "force-push `{cmd}` should require Always approval, got {result:?}" + ); + } +} + +// --------------------------------------------------------------------------- +// 6. risk_level_for trait method +// --------------------------------------------------------------------------- + +#[tokio::test] +async fn risk_level_for_via_tool_trait() { + let tool = shell_tool().await; + assert_eq!(risk(&tool, "ls -la"), RiskLevel::Low); + assert_eq!(risk(&tool, "cargo build"), RiskLevel::Medium); + assert_eq!(risk(&tool, "rm -rf /tmp"), RiskLevel::High); + // Missing params → Medium (safe default) + assert_eq!( + tool.risk_level_for(&serde_json::json!({})), + RiskLevel::Medium + ); +}