From 7d8576a4642652ce643a4ccba23a8c3abebbba98 Mon Sep 17 00:00:00 2001 From: Nick Pismenkov <50764773+nickpismenkov@users.noreply.github.com> Date: Mon, 9 Mar 2026 18:03:39 -0700 Subject: [PATCH] fix: destructive actions from ambiguous user prompts (#782) * fix: destructive actions from ambiguous user prompts * review fixes * review fixes --- src/agent/dispatcher.rs | 90 ++++++++++++++++++++++++++++ src/tools/builtin/extension_tools.rs | 36 +++++++++-- src/tools/builtin/skill_tools.rs | 36 ++++++++++- 3 files changed, 155 insertions(+), 7 deletions(-) diff --git a/src/agent/dispatcher.rs b/src/agent/dispatcher.rs index f5306644..6118ec7d 100644 --- a/src/agent/dispatcher.rs +++ b/src/agent/dispatcher.rs @@ -1263,6 +1263,96 @@ mod tests { } } + #[test] + fn test_always_approval_requirement_bypasses_session_auto_approve() { + // Regression test: even if tool is auto-approved in session, + // ApprovalRequirement::Always must still trigger approval. + use crate::tools::ApprovalRequirement; + + let mut session = Session::new("user-1"); + let tool_name = "tool_remove"; + + // Manually auto-approve tool_remove in this session + session.auto_approve_tool(tool_name); + assert!( + session.is_tool_auto_approved(tool_name), + "tool should be auto-approved" + ); + + // However, ApprovalRequirement::Always should always require approval + // This is verified by the dispatcher logic: Always => true (ignores session state) + let always_req = ApprovalRequirement::Always; + let requires_approval = match always_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name), + ApprovalRequirement::Always => true, + }; + + assert!( + requires_approval, + "ApprovalRequirement::Always must require approval even when tool is auto-approved" + ); + } + + #[test] + fn test_always_approval_requirement_vs_unless_auto_approved() { + // Verify the two requirements behave differently + use crate::tools::ApprovalRequirement; + + let mut session = Session::new("user-2"); + let tool_name = "http"; + + // Scenario 1: Tool is auto-approved + session.auto_approve_tool(tool_name); + + // UnlessAutoApproved → doesn't require approval if auto-approved + let unless_req = ApprovalRequirement::UnlessAutoApproved; + let unless_needs = match unless_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name), + ApprovalRequirement::Always => true, + }; + assert!( + !unless_needs, + "UnlessAutoApproved should not need approval when auto-approved" + ); + + // Always → always requires approval + let always_req = ApprovalRequirement::Always; + let always_needs = match always_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(tool_name), + ApprovalRequirement::Always => true, + }; + assert!( + always_needs, + "Always must always require approval, even when auto-approved" + ); + + // Scenario 2: Tool is NOT auto-approved + let new_tool = "new_tool"; + assert!(!session.is_tool_auto_approved(new_tool)); + + // UnlessAutoApproved → requires approval + let unless_needs = match unless_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(new_tool), + ApprovalRequirement::Always => true, + }; + assert!( + unless_needs, + "UnlessAutoApproved should need approval when not auto-approved" + ); + + // Always → always requires approval + let always_needs = match always_req { + ApprovalRequirement::Never => false, + ApprovalRequirement::UnlessAutoApproved => !session.is_tool_auto_approved(new_tool), + ApprovalRequirement::Always => true, + }; + assert!(always_needs, "Always must always require approval"); + } + #[test] fn test_pending_approval_serialization_backcompat_without_deferred_calls() { // PendingApproval from before the deferred_tool_calls field was added diff --git a/src/tools/builtin/extension_tools.rs b/src/tools/builtin/extension_tools.rs index 7ba4ef0c..00c79548 100644 --- a/src/tools/builtin/extension_tools.rs +++ b/src/tools/builtin/extension_tools.rs @@ -451,8 +451,8 @@ impl Tool for ToolRemoveTool { } fn description(&self) -> &str { - "Remove an installed extension (channel, tool, or MCP server). \ - Unregisters tools and deletes configuration." + "Permanently remove an installed extension (channel, tool, or MCP server) from disk. \ + This action cannot be undone — the WASM binary and configuration files will be deleted." } fn parameters_schema(&self) -> serde_json::Value { @@ -492,7 +492,7 @@ impl Tool for ToolRemoveTool { } fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always } } @@ -701,10 +701,38 @@ mod tests { assert_eq!(tool.name(), "tool_remove"); assert_eq!( tool.requires_approval(&serde_json::json!({})), - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always ); } + #[test] + fn tool_remove_always_requires_approval_regardless_of_params() { + use crate::tools::tool::ApprovalRequirement; + let tool = ToolRemoveTool { + manager: test_manager_stub(), + }; + + let test_cases = vec![ + ("no params", serde_json::json!({})), + ("empty name", serde_json::json!({"name": ""})), + ("slack", serde_json::json!({"name": "slack"})), + ("github-cli", serde_json::json!({"name": "github-cli"})), + ( + "with extra fields", + serde_json::json!({"name": "tool", "extra": "field"}), + ), + ]; + + for (case_name, params) in test_cases { + assert_eq!( + tool.requires_approval(¶ms), + ApprovalRequirement::Always, + "tool_remove must always require approval for case: {}", + case_name + ); + } + } + #[test] fn test_tool_upgrade_schema() { use crate::tools::tool::ApprovalRequirement; diff --git a/src/tools/builtin/skill_tools.rs b/src/tools/builtin/skill_tools.rs index 84c889ae..a7581ac4 100644 --- a/src/tools/builtin/skill_tools.rs +++ b/src/tools/builtin/skill_tools.rs @@ -709,7 +709,8 @@ impl Tool for SkillRemoveTool { } fn description(&self) -> &str { - "Remove an installed skill by name. Only user-installed skills can be removed." + "Permanently remove an installed skill from disk. This action cannot be undone — \ + the skill files will be deleted." } fn parameters_schema(&self) -> serde_json::Value { @@ -770,7 +771,7 @@ impl Tool for SkillRemoveTool { } fn requires_approval(&self, _params: &serde_json::Value) -> ApprovalRequirement { - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always } } @@ -837,12 +838,41 @@ mod tests { assert_eq!(tool.name(), "skill_remove"); assert_eq!( tool.requires_approval(&serde_json::json!({})), - ApprovalRequirement::UnlessAutoApproved + ApprovalRequirement::Always ); let schema = tool.parameters_schema(); assert!(schema["properties"].get("name").is_some()); } + #[test] + fn skill_remove_always_requires_approval_regardless_of_params() { + use crate::tools::tool::ApprovalRequirement; + let tool = SkillRemoveTool::new(test_registry()); + + let test_cases = vec![ + ("no params", serde_json::json!({})), + ("empty name", serde_json::json!({"name": ""})), + ( + "deployment skill", + serde_json::json!({"name": "deployment"}), + ), + ("custom skill", serde_json::json!({"name": "custom-skill"})), + ( + "with extra fields", + serde_json::json!({"name": "skill", "extra": "field"}), + ), + ]; + + for (case_name, params) in test_cases { + assert_eq!( + tool.requires_approval(¶ms), + ApprovalRequirement::Always, + "skill_remove must always require approval for case: {}", + case_name + ); + } + } + #[test] fn test_validate_fetch_url_allows_https() { assert!(super::validate_fetch_url("https://clawhub.ai/api/v1/download?slug=foo").is_ok());