refactor: consolidate tool approval into single param-aware method (#274)

* refactor: consolidate tool approval into single param-aware method

Replace the two confusing approval methods (requires_approval() and
requires_approval_for()) with a single requires_approval(&self, params)
returning a 3-variant ApprovalRequirement enum (Never, UnlessAutoApproved,
Always). This enables param-aware approval decisions: HTTP calls without
auth headers now skip approval entirely, while authenticated requests
always require it. Shell tool merges its destructive-command detection
into the same method.

Co-Authored-By: Claude Opus 4.6 <[email protected]>

* feat: add credential injection to built-in HTTP tool

Wire the WASM credential injection system into the built-in HTTP tool
so credentials are auto-injected at the boundary (zero-exposure model).

- Add SharedCredentialRegistry: thread-safe, append-only registry of
  credential mappings populated by WASM tools at registration time
- Add credential_detect module with broad auth detection for headers
  (12 exact + 5 substring matches), header values (7 auth scheme
  prefixes), and URL query params (17 exact + 5 substring matches)
- HttpTool now accepts optional credential registry + secrets store,
  auto-injects matching credentials in execute(), and uses broader
  auth detection in requires_approval()
- ToolRegistry passes credential registry to HttpTool at startup and
  populates it when WASM tools register
- Remove old hardcoded AUTH_HEADER_NAMES / has_auth_headers in favor
  of the new params_contain_manual_credentials()

Co-Authored-By: Claude Opus 4.6 <[email protected]>

* fix: address PR #274 review comments (query param injection, lock poisoning, visibility)

- Fix injected query params not being sent on outbound HTTP requests by
  also calling .query() on the RequestBuilder alongside parsed_url mutation
- Recover from poisoned RwLock in SharedCredentialRegistry instead of
  silently ignoring failures, with tracing::warn for visibility
- Narrow inject_credential and host_matches_pattern to pub(crate) to
  avoid committing to them as stable public API

Co-Authored-By: Claude Opus 4.6 <[email protected]>

---------

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

src/tools/builtin/extension_tools.rs

@@ -9,7 +9,7 @@ use async_trait::async_trait;
 
 use crate::context::JobContext;
 use crate::extensions::{ExtensionKind, ExtensionManager};
-use crate::tools::tool::{Tool, ToolError, ToolOutput, require_str};
+use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput, require_str};
 
 // ── tool_search ──────────────────────────────────────────────────────────
 
@@ -158,8 +158,8 @@ impl Tool for ToolInstallTool {
         Ok(ToolOutput::success(output, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 }
 
@@ -253,8 +253,8 @@ impl Tool for ToolAuthTool {
         Ok(ToolOutput::success(output, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 }
 
@@ -478,8 +478,8 @@ impl Tool for ToolRemoveTool {
         Ok(ToolOutput::success(output, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 }
 
@@ -500,11 +500,15 @@ mod tests {
 
     #[test]
     fn test_tool_install_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = ToolInstallTool {
             manager: test_manager_stub(),
         };
         assert_eq!(tool.name(), "tool_install");
-        assert!(tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::UnlessAutoApproved
+        );
         let schema = tool.parameters_schema();
         assert!(schema["properties"].get("name").is_some());
         assert!(schema["properties"].get("url").is_some());
@@ -512,11 +516,15 @@ mod tests {
 
     #[test]
     fn test_tool_auth_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = ToolAuthTool {
             manager: test_manager_stub(),
         };
         assert_eq!(tool.name(), "tool_auth");
-        assert!(tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::UnlessAutoApproved
+        );
         let schema = tool.parameters_schema();
         assert!(schema["properties"].get("name").is_some());
         // token param must NOT be in schema (security: tokens never go through LLM)
@@ -528,31 +536,43 @@ mod tests {
 
     #[test]
     fn test_tool_activate_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = ToolActivateTool {
             manager: test_manager_stub(),
         };
         assert_eq!(tool.name(), "tool_activate");
-        assert!(!tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::Never
+        );
     }
 
     #[test]
     fn test_tool_list_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = ToolListTool {
             manager: test_manager_stub(),
         };
         assert_eq!(tool.name(), "tool_list");
-        assert!(!tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::Never
+        );
         let schema = tool.parameters_schema();
         assert!(schema["properties"].get("kind").is_some());
     }
 
     #[test]
     fn test_tool_remove_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = ToolRemoveTool {
             manager: test_manager_stub(),
         };
         assert_eq!(tool.name(), "tool_remove");
-        assert!(tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::UnlessAutoApproved
+        );
     }
 
     /// Create a stub manager for schema tests (these don't call execute).

src/tools/builtin/file.rs

@@ -11,7 +11,9 @@ use async_trait::async_trait;
 use tokio::fs;
 
 use crate::context::JobContext;
-use crate::tools::tool::{Tool, ToolDomain, ToolError, ToolOutput, require_str};
+use crate::tools::tool::{
+    ApprovalRequirement, Tool, ToolDomain, ToolError, ToolOutput, require_str,
+};
 use crate::workspace::paths as ws_paths;
 
 /// Well-known workspace filenames that must go through memory_write, not write_file.
@@ -265,8 +267,8 @@ impl Tool for ReadFileTool {
         true // File content could contain anything
     }
 
-    fn requires_approval(&self) -> bool {
-        true // Reading local files should require approval
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn domain(&self) -> ToolDomain {
@@ -372,8 +374,8 @@ impl Tool for WriteFileTool {
         Ok(ToolOutput::success(result, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true // File writes should require approval
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn requires_sanitization(&self) -> bool {
@@ -488,8 +490,8 @@ impl Tool for ListDirTool {
         false // Directory listings are safe
     }
 
-    fn requires_approval(&self) -> bool {
-        true // Directory listings can leak filesystem structure
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn domain(&self) -> ToolDomain {
@@ -697,8 +699,8 @@ impl Tool for ApplyPatchTool {
         Ok(ToolOutput::success(result, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true // File edits should require approval
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn requires_sanitization(&self) -> bool {

src/tools/builtin/http.rs

@@ -2,6 +2,7 @@
 
 use std::collections::HashMap;
 use std::net::{IpAddr, ToSocketAddrs};
+use std::sync::Arc;
 use std::time::Duration;
 
 use async_trait::async_trait;
@@ -10,7 +11,9 @@ use reqwest::Client;
 
 use crate::context::JobContext;
 use crate::safety::LeakDetector;
-use crate::tools::tool::{Tool, ToolError, ToolOutput, require_str};
+use crate::secrets::SecretsStore;
+use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput, require_str};
+use crate::tools::wasm::{InjectedCredentials, SharedCredentialRegistry, inject_credential};
 
 /// Maximum response body size (5 MB).
 ///
@@ -22,6 +25,8 @@ const MAX_RESPONSE_SIZE: usize = 5 * 1024 * 1024;
 /// Tool for making HTTP requests.
 pub struct HttpTool {
     client: Client,
+    credential_registry: Option<Arc<SharedCredentialRegistry>>,
+    secrets_store: Option<Arc<dyn SecretsStore + Send + Sync>>,
 }
 
 impl HttpTool {
@@ -33,7 +38,22 @@ impl HttpTool {
             .build()
             .expect("Failed to create HTTP client");
 
-        Self { client }
+        Self {
+            client,
+            credential_registry: None,
+            secrets_store: None,
+        }
+    }
+
+    /// Attach a credential registry and secrets store for auto-injection.
+    pub fn with_credentials(
+        mut self,
+        registry: Arc<SharedCredentialRegistry>,
+        secrets_store: Arc<dyn SecretsStore + Send + Sync>,
+    ) -> Self {
+        self.credential_registry = Some(registry);
+        self.secrets_store = Some(secrets_store);
+        self
     }
 }
 
@@ -146,6 +166,15 @@ fn parse_headers_param(
     }
 }
 
+/// Extract host from URL in params (for approval checks).
+fn extract_host_from_params(params: &serde_json::Value) -> Option<String> {
+    params
+        .get("url")
+        .and_then(|u| u.as_str())
+        .and_then(|u| reqwest::Url::parse(u).ok())
+        .and_then(|u| u.host_str().map(|h| h.to_string()))
+}
+
 impl Default for HttpTool {
     fn default() -> Self {
         Self::new()
@@ -211,10 +240,10 @@ impl Tool for HttpTool {
         let method = require_str(&params, "method")?;
 
         let url = require_str(&params, "url")?;
-        let parsed_url = validate_url(url)?;
+        let mut parsed_url = validate_url(url)?;
 
         // Parse headers
-        let headers_vec = parse_headers_param(params.get("headers"))?;
+        let mut headers_vec = parse_headers_param(params.get("headers"))?;
 
         // Build request
         let mut request = match method.to_uppercase().as_str() {
@@ -261,6 +290,41 @@ impl Tool for HttpTool {
             None
         };
 
+        // Credential injection from shared registry
+        if let (Some(registry), Some(store)) = (
+            self.credential_registry.as_ref(),
+            self.secrets_store.as_ref(),
+        ) {
+            let host = parsed_url.host_str().unwrap_or("");
+            let matched: Vec<crate::secrets::CredentialMapping> = registry.find_for_host(host);
+            for mapping in &matched {
+                match store
+                    .get_decrypted(&_ctx.user_id, &mapping.secret_name)
+                    .await
+                {
+                    Ok(secret) => {
+                        let mut injected = InjectedCredentials::empty();
+                        inject_credential(&mut injected, &mapping.location, &secret);
+                        for (name, value) in &injected.headers {
+                            request = request.header(name.as_str(), value.as_str());
+                            headers_vec.push((name.clone(), value.clone()));
+                        }
+                        for (name, value) in &injected.query_params {
+                            parsed_url.query_pairs_mut().append_pair(name, value);
+                            request = request.query(&[(name.as_str(), value.as_str())]);
+                        }
+                    }
+                    Err(e) => {
+                        tracing::warn!(
+                            secret = %mapping.secret_name,
+                            error = %e,
+                            "Failed to inject credential for HTTP tool"
+                        );
+                    }
+                }
+            }
+        }
+
         // Leak detection on outbound request (url/headers/body)
         let detector = LeakDetector::new();
         detector
@@ -352,8 +416,20 @@ impl Tool for HttpTool {
         true // External data always needs sanitization
     }
 
-    fn requires_approval(&self) -> bool {
-        true // HTTP requests go to external services, require user approval
+    fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement {
+        // 1. Manual auth headers/query params in LLM params
+        if crate::safety::params_contain_manual_credentials(params) {
+            return ApprovalRequirement::Always;
+        }
+        // 2. Target host has credential mappings (will be auto-injected)
+        if let Some(ref registry) = self.credential_registry
+            && let Some(host) = extract_host_from_params(params)
+            && registry.has_credentials_for_host(&host)
+        {
+            return ApprovalRequirement::Always;
+        }
+        // Default: outbound HTTP still needs approval unless auto-approved
+        ApprovalRequirement::UnlessAutoApproved
     }
 }
 
@@ -467,4 +543,232 @@ mod tests {
             "body schema must include a type for OpenAI-compatible tool validation"
         );
     }
+
+    // ── Approval requirement tests ──────────────────────────────────────
+
+    #[test]
+    fn test_no_auth_headers_returns_unless_auto_approved() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com/data"
+        });
+        assert_eq!(
+            tool.requires_approval(&params),
+            ApprovalRequirement::UnlessAutoApproved
+        );
+    }
+
+    #[test]
+    fn test_auth_header_object_format_returns_always() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com/data",
+            "headers": {"Authorization": "Bearer token123"}
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+    }
+
+    #[test]
+    fn test_auth_header_array_format_returns_always() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com/data",
+            "headers": [{"name": "Authorization", "value": "Bearer token123"}]
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+    }
+
+    #[test]
+    fn test_auth_header_case_insensitive() {
+        let tool = HttpTool::new();
+
+        // Object format with mixed case
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://example.com",
+            "headers": {"AUTHORIZATION": "Bearer x"}
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+
+        // Array format with mixed case
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://example.com",
+            "headers": [{"name": "X-Api-Key", "value": "key123"}]
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+    }
+
+    #[test]
+    fn test_all_auth_header_names_detected() {
+        let tool = HttpTool::new();
+        for header_name in [
+            "authorization",
+            "x-api-key",
+            "cookie",
+            "proxy-authorization",
+            "x-auth-token",
+            "api-key",
+            "x-token",
+            "x-access-token",
+            "x-session-token",
+            "x-csrf-token",
+            "x-secret",
+            "x-api-secret",
+        ] {
+            let mut headers = serde_json::Map::new();
+            headers.insert(header_name.to_string(), serde_json::json!("value"));
+            let params = serde_json::json!({
+                "method": "GET",
+                "url": "https://example.com",
+                "headers": headers
+            });
+            assert_eq!(
+                tool.requires_approval(&params),
+                ApprovalRequirement::Always,
+                "Header '{}' should trigger Always approval",
+                header_name
+            );
+        }
+    }
+
+    #[test]
+    fn test_non_auth_headers_return_unless_auto_approved() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://example.com",
+            "headers": {"Content-Type": "application/json", "Accept": "text/html"}
+        });
+        assert_eq!(
+            tool.requires_approval(&params),
+            ApprovalRequirement::UnlessAutoApproved
+        );
+    }
+
+    #[test]
+    fn test_empty_headers_return_unless_auto_approved() {
+        let tool = HttpTool::new();
+
+        // Empty object
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://example.com",
+            "headers": {}
+        });
+        assert_eq!(
+            tool.requires_approval(&params),
+            ApprovalRequirement::UnlessAutoApproved
+        );
+
+        // Empty array
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://example.com",
+            "headers": []
+        });
+        assert_eq!(
+            tool.requires_approval(&params),
+            ApprovalRequirement::UnlessAutoApproved
+        );
+    }
+
+    // ── Credential registry approval tests ─────────────────────────────
+
+    #[test]
+    fn test_host_with_credential_mapping_returns_always() {
+        use crate::secrets::CredentialMapping;
+        use crate::tools::wasm::SharedCredentialRegistry;
+
+        let registry = Arc::new(SharedCredentialRegistry::new());
+        registry.add_mappings(vec![CredentialMapping::bearer(
+            "openai_key",
+            "api.openai.com",
+        )]);
+
+        let tool = HttpTool::new().with_credentials(
+            registry,
+            // secrets_store is not used in requires_approval, just needs to be present
+            Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new(
+                crate::secrets::SecretsCrypto::new(secrecy::SecretString::from(
+                    "0123456789abcdef0123456789abcdef".to_string(),
+                ))
+                .unwrap(),
+            ))),
+        );
+
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.openai.com/v1/models"
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+    }
+
+    #[test]
+    fn test_host_without_credential_mapping_returns_unless_auto_approved() {
+        use crate::tools::wasm::SharedCredentialRegistry;
+
+        let registry = Arc::new(SharedCredentialRegistry::new());
+        // Empty registry - no credential mappings
+
+        let tool = HttpTool::new().with_credentials(
+            registry,
+            Arc::new(crate::secrets::InMemorySecretsStore::new(Arc::new(
+                crate::secrets::SecretsCrypto::new(secrecy::SecretString::from(
+                    "0123456789abcdef0123456789abcdef".to_string(),
+                ))
+                .unwrap(),
+            ))),
+        );
+
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com/data"
+        });
+        assert_eq!(
+            tool.requires_approval(&params),
+            ApprovalRequirement::UnlessAutoApproved
+        );
+    }
+
+    #[test]
+    fn test_url_query_param_credential_returns_always() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://api.example.com/data?api_key=secret123"
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+    }
+
+    #[test]
+    fn test_bearer_value_in_custom_header_returns_always() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "GET",
+            "url": "https://example.com",
+            "headers": {"X-Custom": "Bearer sk-test123"}
+        });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Always);
+    }
+
+    #[test]
+    fn test_extract_host_from_params_valid() {
+        let params = serde_json::json!({
+            "url": "https://api.example.com/path"
+        });
+        assert_eq!(
+            extract_host_from_params(&params),
+            Some("api.example.com".to_string())
+        );
+    }
+
+    #[test]
+    fn test_extract_host_from_params_missing_url() {
+        let params = serde_json::json!({"method": "GET"});
+        assert_eq!(extract_host_from_params(&params), None);
+    }
 }

src/tools/builtin/job.rs

@@ -22,7 +22,7 @@ use crate::history::SandboxJobRecord;
 use crate::orchestrator::auth::CredentialGrant;
 use crate::orchestrator::job_manager::{ContainerJobManager, JobMode};
 use crate::secrets::SecretsStore;
-use crate::tools::tool::{Tool, ToolError, ToolOutput, require_str};
+use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput, require_str};
 
 /// Resolve a job ID from a full UUID or a short prefix (like git short SHAs).
 ///
@@ -1005,8 +1005,8 @@ impl Tool for CancelJobTool {
         }
     }
 
-    fn requires_approval(&self) -> bool {
-        true // Canceling a job should require approval
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn requires_sanitization(&self) -> bool {
@@ -1268,8 +1268,8 @@ impl Tool for JobPromptTool {
         Ok(ToolOutput::success(result, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn requires_sanitization(&self) -> bool {
@@ -1611,10 +1611,14 @@ mod tests {
 
     #[tokio::test]
     async fn test_job_prompt_tool_requires_approval() {
+        use crate::tools::tool::ApprovalRequirement;
         let queue: PromptQueue =
             Arc::new(tokio::sync::Mutex::new(std::collections::HashMap::new()));
         let tool = test_prompt_tool(queue);
-        assert!(tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::UnlessAutoApproved
+        );
     }
 
     #[tokio::test]

src/tools/builtin/shell.rs

@@ -55,7 +55,9 @@ use tokio::process::Command;
 
 use crate::context::JobContext;
 use crate::sandbox::{SandboxManager, SandboxPolicy};
-use crate::tools::tool::{Tool, ToolDomain, ToolError, ToolOutput, require_str};
+use crate::tools::tool::{
+    ApprovalRequirement, Tool, ToolDomain, ToolError, ToolOutput, require_str,
+};
 
 /// Maximum output size before truncation (64KB).
 const MAX_OUTPUT_SIZE: usize = 64 * 1024;
@@ -696,11 +698,7 @@ impl Tool for ShellTool {
         Ok(ToolOutput::success(result, duration))
     }
 
-    fn requires_approval(&self) -> bool {
-        true // Shell commands should require approval
-    }
-
-    fn requires_approval_for(&self, params: &serde_json::Value) -> bool {
+    fn requires_approval(&self, params: &serde_json::Value) -> ApprovalRequirement {
         let cmd = params
             .get("command")
             .and_then(|c| c.as_str().map(String::from))
@@ -714,10 +712,10 @@ impl Tool for ShellTool {
         if let Some(ref cmd) = cmd
             && requires_explicit_approval(cmd)
         {
-            return true;
+            return ApprovalRequirement::Always;
         }
 
-        false
+        ApprovalRequirement::UnlessAutoApproved
     }
 
     fn requires_sanitization(&self) -> bool {
@@ -861,31 +859,46 @@ mod tests {
     }
 
     #[test]
-    fn test_requires_approval_for_destructive_command() {
+    fn test_requires_approval_destructive_command() {
+        use crate::tools::tool::ApprovalRequirement;
         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;"})));
+        // Destructive commands must return Always to bypass auto-approve.
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({"command": "rm -rf /tmp"})),
+            ApprovalRequirement::Always
+        );
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({"command": "git push --force origin main"})),
+            ApprovalRequirement::Always
+        );
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({"command": "DROP TABLE users;"})),
+            ApprovalRequirement::Always
+        );
     }
 
     #[test]
-    fn test_requires_approval_for_safe_command() {
+    fn test_requires_approval_safe_command() {
+        use crate::tools::tool::ApprovalRequirement;
         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"})));
+        // Safe 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
+        );
     }
 
     #[test]
-    fn test_requires_approval_for_string_encoded_args() {
+    fn test_requires_approval_string_encoded_args() {
+        use crate::tools::tool::ApprovalRequirement;
         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));
+        assert_eq!(tool.requires_approval(&args), ApprovalRequirement::Always);
     }
 
     #[test]

src/tools/builtin/skill_tools.rs

@@ -10,7 +10,7 @@ use async_trait::async_trait;
 use crate::context::JobContext;
 use crate::skills::catalog::SkillCatalog;
 use crate::skills::registry::SkillRegistry;
-use crate::tools::tool::{Tool, ToolError, ToolOutput, require_str};
+use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput, require_str};
 
 // ── skill_list ──────────────────────────────────────────────────────────
 
@@ -356,8 +356,8 @@ impl Tool for SkillInstallTool {
         Ok(ToolOutput::success(output, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 }
 
@@ -553,8 +553,8 @@ impl Tool for SkillRemoveTool {
         Ok(ToolOutput::success(output, start.elapsed()))
     }
 
-    fn requires_approval(&self) -> bool {
-        true
+    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
+        ApprovalRequirement::UnlessAutoApproved
     }
 }
 
@@ -575,27 +575,39 @@ mod tests {
 
     #[test]
     fn test_skill_list_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = SkillListTool::new(test_registry());
         assert_eq!(tool.name(), "skill_list");
-        assert!(!tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::Never
+        );
         let schema = tool.parameters_schema();
         assert!(schema.get("properties").is_some());
     }
 
     #[test]
     fn test_skill_search_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = SkillSearchTool::new(test_registry(), test_catalog());
         assert_eq!(tool.name(), "skill_search");
-        assert!(!tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::Never
+        );
         let schema = tool.parameters_schema();
         assert!(schema["properties"].get("query").is_some());
     }
 
     #[test]
     fn test_skill_install_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = SkillInstallTool::new(test_registry(), test_catalog());
         assert_eq!(tool.name(), "skill_install");
-        assert!(tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::UnlessAutoApproved
+        );
         let schema = tool.parameters_schema();
         assert!(schema["properties"].get("name").is_some());
         assert!(schema["properties"].get("url").is_some());
@@ -604,9 +616,13 @@ mod tests {
 
     #[test]
     fn test_skill_remove_schema() {
+        use crate::tools::tool::ApprovalRequirement;
         let tool = SkillRemoveTool::new(test_registry());
         assert_eq!(tool.name(), "skill_remove");
-        assert!(tool.requires_approval());
+        assert_eq!(
+            tool.requires_approval(&serde_json::json!({})),
+            ApprovalRequirement::UnlessAutoApproved
+        );
         let schema = tool.parameters_schema();
         assert!(schema["properties"].get("name").is_some());
     }