diff --git a/src/tools/builtin/file.rs b/src/tools/builtin/file.rs index 72e0151c..724b5bae 100644 --- a/src/tools/builtin/file.rs +++ b/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 } diff --git a/src/tools/builtin/http.rs b/src/tools/builtin/http.rs index e8138a26..4ed2bb0b 100644 --- a/src/tools/builtin/http.rs +++ b/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 { let start = std::time::Instant::now(); - let method = require_str(¶ms, "method")?; + let method = params["method"].as_str().unwrap_or("GET"); let method_upper = method.to_uppercase(); let url = require_str(¶ms, "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(¶ms), 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(¶ms), 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(¶ms), - ApprovalRequirement::UnlessAutoApproved - ); + assert_eq!(tool.requires_approval(¶ms), 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(¶ms), - ApprovalRequirement::UnlessAutoApproved - ); + assert_eq!(tool.requires_approval(¶ms), 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(¶ms), - ApprovalRequirement::UnlessAutoApproved - ); + assert_eq!(tool.requires_approval(¶ms), 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(¶ms), - ApprovalRequirement::UnlessAutoApproved - ); + assert_eq!(tool.requires_approval(¶ms), ApprovalRequirement::Never); } #[test] diff --git a/src/tools/builtin/image_analyze.rs b/src/tools/builtin/image_analyze.rs index b1f8a62f..d6f8f264 100644 --- a/src/tools/builtin/image_analyze.rs +++ b/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 ); } diff --git a/src/tools/builtin/image_edit.rs b/src/tools/builtin/image_edit.rs index 818454cc..36c2d90d 100644 --- a/src/tools/builtin/image_edit.rs +++ b/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 ); } diff --git a/src/tools/builtin/image_gen.rs b/src/tools/builtin/image_gen.rs index c87b10d7..a9cc98e9 100644 --- a/src/tools/builtin/image_gen.rs +++ b/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();