fix: relax approval requirements for low-risk tools (#922)

* fix: relax approval requirements for low-risk tools

Remove unnecessary UnlessAutoApproved friction from list_dir, image_gen,
image_analyze, image_edit, tool_install, tool_auth, tool_upgrade, and
build_tool — these operate on trusted inputs or are low-risk operations
so they now use the trait default (Never).

For the http tool, GET requests without credentials now return Never
instead of UnlessAutoApproved, while credential-bearing requests and
non-GET methods retain their existing approval levels.

[skip-regression-check]

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

* style: apply cargo fmt

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

* fix: address review feedback on approval changes

Rename test_requires_approval_returns_unless_auto_approved to
test_requires_approval_returns_never to match the asserted behavior.

In http requires_approval(), treat missing method as unknown (falls
through to UnlessAutoApproved) instead of defaulting to GET, since
the schema requires method. Updated comment to reflect this.

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

* fix: make http method optional, default to GET

Make method optional in schema (only url is required) and default to
GET in both execute() and requires_approval(). This aligns approval
logic with execution and reduces friction for simple GET requests.

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

* fix: restore UnlessAutoApproved for build_tool, tool_install, tool_upgrade

Address review feedback: these tools modify the system's trust boundary
(shell execution, WASM installation, version mutation) and should retain
approval gating. tool_auth kept as Never per owner decision.

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

---------

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

src/tools/builtin/file.rs

@@ -397,10 +397,6 @@ impl Tool for ListDirTool {
         false // Directory listings are safe
     }
 
-    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
-        ApprovalRequirement::UnlessAutoApproved
-    }
-
     fn domain(&self) -> ToolDomain {
         ToolDomain::Container
     }

src/tools/builtin/http.rs

@@ -398,7 +398,7 @@ impl Tool for HttpTool {
                 "method": {
                     "type": "string",
                     "enum": ["GET", "POST", "PUT", "DELETE", "PATCH"],
-                    "description": "HTTP method"
+                    "description": "HTTP method (default: GET)"
                 },
                 "url": {
                     "type": "string",
@@ -429,7 +429,7 @@ impl Tool for HttpTool {
                     "description": "Save response body as raw bytes to this file path instead of returning it. Use for binary downloads (images, PDFs, etc.). The path must be under /tmp/."
                 }
             },
-            "required": ["method", "url"]
+            "required": ["url"]
         })
     }
 
@@ -440,7 +440,7 @@ impl Tool for HttpTool {
     ) -> Result<ToolOutput, ToolError> {
         let start = std::time::Instant::now();
 
-        let method = require_str(&params, "method")?;
+        let method = params["method"].as_str().unwrap_or("GET");
         let method_upper = method.to_uppercase();
 
         let url = require_str(&params, "url")?;
@@ -829,18 +829,22 @@ impl Tool for HttpTool {
     }
 
     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) {
+        let has_credentials = crate::safety::params_contain_manual_credentials(params)
+            || (self.credential_registry.as_ref().is_some_and(|registry| {
+                extract_host_from_params(params)
+                    .is_some_and(|host| registry.has_credentials_for_host(&host))
+            }));
+
+        if has_credentials {
             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;
+
+        // GET requests (or missing method, since GET is the default) are low-risk
+        let method = params["method"].as_str().unwrap_or("GET");
+        if method.eq_ignore_ascii_case("GET") {
+            return ApprovalRequirement::Never;
         }
-        // Default: outbound HTTP still needs approval unless auto-approved
+
         ApprovalRequirement::UnlessAutoApproved
     }
 
@@ -1063,12 +1067,22 @@ mod tests {
     // ── Approval requirement tests ──────────────────────────────────────
 
     #[test]
-    fn test_no_auth_headers_returns_unless_auto_approved() {
+    fn test_get_no_auth_headers_returns_never() {
         let tool = HttpTool::new();
         let params = serde_json::json!({
             "method": "GET",
             "url": "https://api.example.com/data"
         });
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Never);
+    }
+
+    #[test]
+    fn test_post_no_auth_headers_returns_unless_auto_approved() {
+        let tool = HttpTool::new();
+        let params = serde_json::json!({
+            "method": "POST",
+            "url": "https://api.example.com/data"
+        });
         assert_eq!(
             tool.requires_approval(&params),
             ApprovalRequirement::UnlessAutoApproved
@@ -1152,21 +1166,18 @@ mod tests {
     }
 
     #[test]
-    fn test_non_auth_headers_return_unless_auto_approved() {
+    fn test_get_non_auth_headers_return_never() {
         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
-        );
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Never);
     }
 
     #[test]
-    fn test_empty_headers_return_unless_auto_approved() {
+    fn test_get_empty_headers_return_never() {
         let tool = HttpTool::new();
 
         // Empty object
@@ -1175,10 +1186,7 @@ mod tests {
             "url": "https://example.com",
             "headers": {}
         });
-        assert_eq!(
-            tool.requires_approval(&params),
-            ApprovalRequirement::UnlessAutoApproved
-        );
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Never);
 
         // Empty array
         let params = serde_json::json!({
@@ -1186,10 +1194,7 @@ mod tests {
             "url": "https://example.com",
             "headers": []
         });
-        assert_eq!(
-            tool.requires_approval(&params),
-            ApprovalRequirement::UnlessAutoApproved
-        );
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Never);
     }
 
     // ── Credential registry approval tests ─────────────────────────────
@@ -1219,7 +1224,7 @@ mod tests {
     }
 
     #[test]
-    fn test_host_without_credential_mapping_returns_unless_auto_approved() {
+    fn test_get_host_without_credential_mapping_returns_never() {
         use crate::tools::wasm::SharedCredentialRegistry;
 
         let registry = Arc::new(SharedCredentialRegistry::new());
@@ -1231,10 +1236,7 @@ mod tests {
             "method": "GET",
             "url": "https://api.example.com/data"
         });
-        assert_eq!(
-            tool.requires_approval(&params),
-            ApprovalRequirement::UnlessAutoApproved
-        );
+        assert_eq!(tool.requires_approval(&params), ApprovalRequirement::Never);
     }
 
     #[test]

src/tools/builtin/image_analyze.rs

@@ -8,7 +8,7 @@ use secrecy::{ExposeSecret, SecretString};
 
 use crate::context::JobContext;
 use crate::tools::builtin::path_utils::validate_path;
-use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput};
+use crate::tools::tool::{Tool, ToolError, ToolOutput};
 
 /// Tool for analyzing images using a vision-capable model.
 pub struct ImageAnalyzeTool {
@@ -86,10 +86,6 @@ impl Tool for ImageAnalyzeTool {
         })
     }
 
-    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
-        ApprovalRequirement::UnlessAutoApproved
-    }
-
     fn requires_sanitization(&self) -> bool {
         true
     }
@@ -185,6 +181,7 @@ impl Tool for ImageAnalyzeTool {
 mod tests {
     use super::super::media_type_from_path;
     use super::*;
+    use crate::tools::tool::ApprovalRequirement;
     use tempfile::TempDir;
 
     #[test]
@@ -199,7 +196,7 @@ mod tests {
     }
 
     #[test]
-    fn test_requires_approval_returns_unless_auto_approved() {
+    fn test_requires_approval_returns_never() {
         let tool = ImageAnalyzeTool::new(
             "https://api.example.com".to_string(),
             "test-key".to_string(),
@@ -208,7 +205,7 @@ mod tests {
         );
         assert_eq!(
             tool.requires_approval(&serde_json::json!({})),
-            ApprovalRequirement::UnlessAutoApproved
+            ApprovalRequirement::Never
         );
     }
 

src/tools/builtin/image_edit.rs

@@ -7,7 +7,7 @@ use secrecy::{ExposeSecret, SecretString};
 
 use crate::context::JobContext;
 use crate::tools::builtin::path_utils::validate_path;
-use crate::tools::tool::{ApprovalRequirement, Tool, ToolError, ToolOutput};
+use crate::tools::tool::{Tool, ToolError, ToolOutput};
 
 /// Tool for editing images using an AI image editing API.
 pub struct ImageEditTool {
@@ -85,10 +85,6 @@ impl Tool for ImageEditTool {
         })
     }
 
-    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
-        ApprovalRequirement::UnlessAutoApproved
-    }
-
     fn requires_sanitization(&self) -> bool {
         false
     }
@@ -266,6 +262,7 @@ impl ImageEditTool {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::tools::tool::ApprovalRequirement;
     use tempfile::TempDir;
 
     #[test]
@@ -280,7 +277,7 @@ mod tests {
         assert!(!tool.requires_sanitization());
         assert_eq!(
             tool.requires_approval(&serde_json::json!({})),
-            ApprovalRequirement::UnlessAutoApproved
+            ApprovalRequirement::Never
         );
     }
 

src/tools/builtin/image_gen.rs

@@ -5,7 +5,6 @@ use secrecy::{ExposeSecret, SecretString};
 use serde::{Deserialize, Serialize};
 
 use crate::context::JobContext;
-use crate::tools::tool::ApprovalRequirement;
 use crate::tools::{Tool, ToolError, ToolOutput};
 
 /// Tool for generating images using FLUX or compatible image generation APIs.
@@ -87,10 +86,6 @@ impl Tool for ImageGenerateTool {
         })
     }
 
-    fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement {
-        ApprovalRequirement::UnlessAutoApproved
-    }
-
     fn requires_sanitization(&self) -> bool {
         false
     }
@@ -186,6 +181,7 @@ impl Tool for ImageGenerateTool {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::tools::tool::ApprovalRequirement;
 
     #[test]
     fn test_tool_metadata() {
@@ -197,7 +193,7 @@ mod tests {
         assert_eq!(tool.name(), "image_generate");
         assert_eq!(
             tool.requires_approval(&serde_json::json!({})),
-            ApprovalRequirement::UnlessAutoApproved
+            ApprovalRequirement::Never
         );
 
         let schema = tool.parameters_schema();