From 6783cba4e4af19e49c263da6d30636c55e8bbb6b Mon Sep 17 00:00:00 2001 From: Illia Polosukhin Date: Mon, 16 Feb 2026 22:42:13 -0800 Subject: [PATCH] feat: move per-invocation approval check into Tool trait (#119) * feat: move per-invocation approval check into Tool trait (#94) Move shell-specific destructive command detection out of agent_loop.rs into a new `requires_approval_for(params)` method on the Tool trait. ShellTool overrides it to check for destructive patterns (rm -rf, git push --force, etc.) while the default delegates to `requires_approval()`. This follows the project's tool architecture principle of keeping tool-specific logic out of the main agent codebase, and enables other tools to implement per-invocation gating without modifying the agent loop. Co-Authored-By: Claude Opus 4.6 * fix: requires_approval_for default should return false, not self.requires_approval() The previous default broke auto-approval for all tools: since requires_approval_for() delegated to requires_approval(), any auto-approved tool would have its auto-approval immediately overridden on every invocation. The correct semantic is: - requires_approval(): "Does this tool use the approval system?" - requires_approval_for(params): "Should this invocation override auto-approval?" The default for the latter must be false (allow auto-approval). ShellTool's fallback for safe commands is also changed to false. Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- src/agent/agent_loop.rs | 29 ++++------------------- src/tools/builtin/shell.rs | 48 ++++++++++++++++++++++++++++++++++++++ src/tools/tool.rs | 22 +++++++++++++++++ 3 files changed, 75 insertions(+), 24 deletions(-) diff --git a/src/agent/agent_loop.rs b/src/agent/agent_loop.rs index 4a9c2ab9..968f0ba9 100644 --- a/src/agent/agent_loop.rs +++ b/src/agent/agent_loop.rs @@ -1248,31 +1248,12 @@ impl Agent { sess.is_tool_auto_approved(&tc.name) }; - // For shell commands, override auto-approval for - // destructive patterns that should always require - // explicit per-invocation approval. - if is_auto_approved - && tc.name == "shell" - && let Some(cmd) = tc - .arguments - .get("command") - .and_then(|c| c.as_str().map(String::from)) - .or_else(|| { - tc.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)) - }) - }) - && crate::tools::builtin::shell::requires_explicit_approval(&cmd) - { + // Let the tool inspect the specific parameters and + // override auto-approval (e.g. destructive shell commands). + if is_auto_approved && tool.requires_approval_for(&tc.arguments) { tracing::info!( - "Shell command '{}' requires explicit approval despite auto-approve", - cmd.chars().take(80).collect::() + tool = %tc.name, + "Tool requires explicit approval for these parameters despite auto-approve" ); is_auto_approved = false; } diff --git a/src/tools/builtin/shell.rs b/src/tools/builtin/shell.rs index 0d883da2..147345ee 100644 --- a/src/tools/builtin/shell.rs +++ b/src/tools/builtin/shell.rs @@ -426,6 +426,26 @@ impl Tool for ShellTool { true // Shell commands should require approval } + fn requires_approval_for(&self, params: &serde_json::Value) -> bool { + 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 true; + } + + false + } + fn requires_sanitization(&self) -> bool { true // Shell output could contain anything } @@ -566,6 +586,34 @@ mod tests { assert!(requires_explicit_approval(cmd.as_deref().unwrap())); } + #[test] + fn test_requires_approval_for_destructive_command() { + let tool = ShellTool::new(); + // Destructive commands must return true even though shell already + // requires base approval -- the distinction matters for auto-approve override. + assert!(tool.requires_approval_for(&serde_json::json!({"command": "rm -rf /tmp"}))); + assert!(tool.requires_approval_for( + &serde_json::json!({"command": "git push --force origin main"}) + )); + assert!(tool.requires_approval_for(&serde_json::json!({"command": "DROP TABLE users;"}))); + } + + #[test] + fn test_requires_approval_for_safe_command() { + let tool = ShellTool::new(); + // Safe commands should not override auto-approval; only destructive ones do. + assert!(!tool.requires_approval_for(&serde_json::json!({"command": "cargo build"}))); + assert!(!tool.requires_approval_for(&serde_json::json!({"command": "echo hello"}))); + } + + #[test] + fn test_requires_approval_for_string_encoded_args() { + let tool = ShellTool::new(); + // When arguments are string-encoded JSON (rare LLM behavior). + let args = serde_json::Value::String(r#"{"command": "rm -rf /tmp/stuff"}"#.to_string()); + assert!(tool.requires_approval_for(&args)); + } + #[test] fn test_sandbox_policy_builder() { let tool = ShellTool::new() diff --git a/src/tools/tool.rs b/src/tools/tool.rs index ae049ead..e0e4f4c4 100644 --- a/src/tools/tool.rs +++ b/src/tools/tool.rs @@ -172,6 +172,21 @@ pub trait Tool: Send + Sync { false } + /// Whether this specific invocation should override auto-approval. + /// + /// This method is called after checking `requires_approval()` and finding that + /// the tool is auto-approved for this session. Return `true` to force approval + /// for this specific invocation despite auto-approval (for example, for + /// destructive operations like `rm -rf` or `git push --force`). + /// + /// Return `false` to allow auto-approval to proceed normally. + /// + /// The default returns `false`. Override only if you need parameter-aware + /// approval gating. + fn requires_approval_for(&self, _params: &serde_json::Value) -> bool { + false + } + /// Maximum time this tool is allowed to run before the caller kills it. /// Override for long-running tools like sandbox execution. /// Default: 60 seconds. @@ -330,4 +345,11 @@ mod tests { let err = require_param(¶ms, "data").unwrap_err(); assert!(err.to_string().contains("missing 'data'")); } + + #[test] + fn test_requires_approval_for_default() { + let tool = EchoTool; + // Default requires_approval_for() returns false, allowing auto-approval. + assert!(!tool.requires_approval_for(&serde_json::json!({"message": "hi"}))); + } }