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 <[email protected]>

* 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 <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 <[email protected]>

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::<serde_json::Value>(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::<String>()
+                                    tool = %tc.name,
+                                    "Tool requires explicit approval for these parameters despite auto-approve"
                                 );
                                 is_auto_approved = false;
                             }

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::<serde_json::Value>(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()

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(&params, "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"})));
+    }
 }