diff --git a/Cargo.lock b/Cargo.lock index 8813bc66a03771bc8a1ec06e4049cdd439e10aa5..b5fd29820fe315c5a7e8a89544020fed3130cfd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -217,6 +217,7 @@ dependencies = [ "tree-sitter-rust", "ui", "unindent", + "url", "util", "uuid", "watch", diff --git a/assets/keymaps/default-linux.json b/assets/keymaps/default-linux.json index be7a2ff8c1be9c2b91fd4ae6dabfdbc550784a91..88d89cda84d24b1954389c59f6d61c46da399c95 100644 --- a/assets/keymaps/default-linux.json +++ b/assets/keymaps/default-linux.json @@ -259,7 +259,7 @@ "super-ctrl-b": "agent::ToggleBurnMode", "alt-enter": "agent::ContinueWithBurnMode", "ctrl-y": "agent::AllowOnce", - "ctrl-alt-y": "agent::AllowAlways", + "ctrl-alt-a": "agent::OpenPermissionDropdown", "ctrl-alt-z": "agent::RejectOnce", }, }, diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json index a0bd957bdab86241ad6e5ef6d0af6394fea6463a..00573cb1da61973c9bad74480e4ec138b2f1eab5 100644 --- a/assets/keymaps/default-macos.json +++ b/assets/keymaps/default-macos.json @@ -300,7 +300,7 @@ "cmd-shift-enter": "agent::ContinueThread", "alt-enter": "agent::ContinueWithBurnMode", "cmd-y": "agent::AllowOnce", - "cmd-alt-y": "agent::AllowAlways", + "cmd-alt-a": "agent::OpenPermissionDropdown", "cmd-alt-z": "agent::RejectOnce", }, }, diff --git a/assets/keymaps/default-windows.json b/assets/keymaps/default-windows.json index 05a65626d0bd3afa74ab174a565ec2ec3e2f1020..50c655c1e2aee4c1da6c5112b26d9b4f7a4abff6 100644 --- a/assets/keymaps/default-windows.json +++ b/assets/keymaps/default-windows.json @@ -260,7 +260,7 @@ "super-ctrl-b": "agent::ToggleBurnMode", "alt-enter": "agent::ContinueWithBurnMode", "shift-alt-a": "agent::AllowOnce", - "ctrl-alt-y": "agent::AllowAlways", + "ctrl-alt-a": "agent::OpenPermissionDropdown", "shift-alt-z": "agent::RejectOnce", }, }, diff --git a/assets/settings/default.json b/assets/settings/default.json index 0c24b3b7d6930869f21b28df6231ffa40f0d54b4..b31014687252c435b9c0189251ca248e098b0daa 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -997,22 +997,6 @@ // Privileged commands { "pattern": "sudo\\s" }, ], - "always_allow": [ - // Build and test commands - { "pattern": "^cargo\\s+(build|test|check|clippy|run)" }, - { "pattern": "^npm\\s+(test|run|install)" }, - { "pattern": "^pnpm\\s+(test|run|install)" }, - { "pattern": "^yarn\\s+(test|run|install)" }, - // Safe read-only commands - { "pattern": "^ls(\\s|$)" }, - { "pattern": "^cat\\s" }, - { "pattern": "^head\\s" }, - { "pattern": "^tail\\s" }, - { "pattern": "^grep\\s" }, - { "pattern": "^find\\s" }, - // Safe git commands - { "pattern": "^git\\s+(status|log|diff|branch|show)" }, - ], }, "edit_file": { "default_mode": "confirm", @@ -1048,12 +1032,6 @@ }, "fetch": { "default_mode": "confirm", - "always_allow": [ - // Common documentation sites - { "pattern": "^https://(docs\\.|api\\.)?github\\.com" }, - { "pattern": "^https://docs\\.rs" }, - { "pattern": "^https://crates\\.io" }, - ], }, }, }, diff --git a/crates/agent/Cargo.toml b/crates/agent/Cargo.toml index 055f93161f984ba679b7f2c75ee1f746ca0c72e8..578dbe7869972f4eb9c43103ba9d492d524f7001 100644 --- a/crates/agent/Cargo.toml +++ b/crates/agent/Cargo.toml @@ -66,6 +66,7 @@ telemetry.workspace = true text.workspace = true thiserror.workspace = true ui.workspace = true +url.workspace = true util.workspace = true uuid.workspace = true watch.workspace = true diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index cedbd4acdddfbddcbcd2de352e4d411a99337611..c2927a9a4c6469dced798e3a7a2b17acf0efc7d2 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -3,6 +3,7 @@ mod edit_agent; mod legacy_thread; mod native_agent_server; pub mod outline; +mod pattern_extraction; mod templates; #[cfg(test)] mod tests; @@ -14,6 +15,7 @@ mod tools; use context_server::ContextServerId; pub use db::*; pub use native_agent_server::NativeAgentServer; +pub use pattern_extraction::*; pub use templates::*; pub use thread::*; pub use thread_store::*; @@ -994,6 +996,7 @@ impl NativeAgentConnection { tool_call, options, response, + context: _, }) => { let outcome_task = acp_thread.update(cx, |thread, cx| { thread.request_tool_call_authorization( diff --git a/crates/agent/src/pattern_extraction.rs b/crates/agent/src/pattern_extraction.rs new file mode 100644 index 0000000000000000000000000000000000000000..e3b3bc20d98c6d45a0b6962c28c8e1d0b33bf266 --- /dev/null +++ b/crates/agent/src/pattern_extraction.rs @@ -0,0 +1,166 @@ +use url::Url; + +/// Extracts a regex pattern from a terminal command based on the first token (command name). +/// +/// Returns `None` for commands starting with `./`, `/`, or other path-like prefixes. +/// This is a deliberate security decision: we only allow pattern-based "always allow" +/// rules for well-known command names (like `cargo`, `npm`, `git`), not for arbitrary +/// scripts or absolute paths which could be manipulated by an attacker. +pub fn extract_terminal_pattern(command: &str) -> Option { + let first_token = command.split_whitespace().next()?; + // Only allow alphanumeric commands with hyphens/underscores. + // Reject paths like "./script.sh" or "/usr/bin/python" to prevent + // users from accidentally allowing arbitrary script execution. + if first_token + .chars() + .all(|c| c.is_alphanumeric() || c == '-' || c == '_') + { + Some(format!("^{}\\s", regex::escape(first_token))) + } else { + None + } +} + +pub fn extract_terminal_pattern_display(command: &str) -> Option { + let first_token = command.split_whitespace().next()?; + if first_token + .chars() + .all(|c| c.is_alphanumeric() || c == '-' || c == '_') + { + Some(first_token.to_string()) + } else { + None + } +} + +pub fn extract_path_pattern(path: &str) -> Option { + let parent = std::path::Path::new(path).parent()?; + let parent_str = parent.to_str()?; + if parent_str.is_empty() || parent_str == "/" { + return None; + } + Some(format!("^{}/", regex::escape(parent_str))) +} + +pub fn extract_path_pattern_display(path: &str) -> Option { + let parent = std::path::Path::new(path).parent()?; + let parent_str = parent.to_str()?; + if parent_str.is_empty() || parent_str == "/" { + return None; + } + Some(format!("{}/", parent_str)) +} + +pub fn extract_url_pattern(url: &str) -> Option { + let parsed = Url::parse(url).ok()?; + let domain = parsed.host_str()?; + Some(format!("^https?://{}", regex::escape(domain))) +} + +pub fn extract_url_pattern_display(url: &str) -> Option { + let parsed = Url::parse(url).ok()?; + let domain = parsed.host_str()?; + Some(domain.to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_extract_terminal_pattern() { + assert_eq!( + extract_terminal_pattern("cargo build --release"), + Some("^cargo\\s".to_string()) + ); + assert_eq!( + extract_terminal_pattern("npm install"), + Some("^npm\\s".to_string()) + ); + assert_eq!( + extract_terminal_pattern("git-lfs pull"), + Some("^git\\-lfs\\s".to_string()) + ); + assert_eq!( + extract_terminal_pattern("my_script arg"), + Some("^my_script\\s".to_string()) + ); + assert_eq!(extract_terminal_pattern("./script.sh arg"), None); + assert_eq!(extract_terminal_pattern("/usr/bin/python arg"), None); + } + + #[test] + fn test_extract_terminal_pattern_display() { + assert_eq!( + extract_terminal_pattern_display("cargo build --release"), + Some("cargo".to_string()) + ); + assert_eq!( + extract_terminal_pattern_display("npm install"), + Some("npm".to_string()) + ); + } + + #[test] + fn test_extract_path_pattern() { + assert_eq!( + extract_path_pattern("/Users/alice/project/src/main.rs"), + Some("^/Users/alice/project/src/".to_string()) + ); + assert_eq!( + extract_path_pattern("src/lib.rs"), + Some("^src/".to_string()) + ); + assert_eq!(extract_path_pattern("file.txt"), None); + assert_eq!(extract_path_pattern("/file.txt"), None); + } + + #[test] + fn test_extract_path_pattern_display() { + assert_eq!( + extract_path_pattern_display("/Users/alice/project/src/main.rs"), + Some("/Users/alice/project/src/".to_string()) + ); + assert_eq!( + extract_path_pattern_display("src/lib.rs"), + Some("src/".to_string()) + ); + } + + #[test] + fn test_extract_url_pattern() { + assert_eq!( + extract_url_pattern("https://github.com/user/repo"), + Some("^https?://github\\.com".to_string()) + ); + assert_eq!( + extract_url_pattern("http://example.com/path?query=1"), + Some("^https?://example\\.com".to_string()) + ); + assert_eq!(extract_url_pattern("not a url"), None); + } + + #[test] + fn test_extract_url_pattern_display() { + assert_eq!( + extract_url_pattern_display("https://github.com/user/repo"), + Some("github.com".to_string()) + ); + assert_eq!( + extract_url_pattern_display("http://api.example.com/v1/users"), + Some("api.example.com".to_string()) + ); + } + + #[test] + fn test_special_chars_are_escaped() { + assert_eq!( + extract_path_pattern("/path/with (parens)/file.txt"), + Some("^/path/with \\(parens\\)/".to_string()) + ); + assert_eq!( + extract_url_pattern("https://test.example.com/path"), + Some("^https?://test\\.example\\.com".to_string()) + ); + } +} diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index b0bf39ba2e8d1b5a13060173837b8f68a78f7a0c..08723529b42516728942362d56772be91464f513 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -772,17 +772,17 @@ async fn test_tool_authorization(cx: &mut TestAppContext) { let tool_call_auth_1 = next_tool_call_authorization(&mut events).await; let tool_call_auth_2 = next_tool_call_authorization(&mut events).await; - // Approve the first + // Approve the first - send "allow" option_id (UI transforms "once" to "allow") tool_call_auth_1 .response - .send(tool_call_auth_1.options[1].option_id.clone()) + .send(acp::PermissionOptionId::new("allow")) .unwrap(); cx.run_until_parked(); - // Reject the second + // Reject the second - send "deny" option_id directly since Deny is now a button tool_call_auth_2 .response - .send(tool_call_auth_1.options[2].option_id.clone()) + .send(acp::PermissionOptionId::new("deny")) .unwrap(); cx.run_until_parked(); @@ -821,11 +821,14 @@ async fn test_tool_authorization(cx: &mut TestAppContext) { )); fake_model.end_last_completion_stream(); - // Respond by always allowing tools. + // Respond by always allowing tools - send transformed option_id + // (UI transforms "always:tool_requiring_permission" to "always_allow:tool_requiring_permission") let tool_call_auth_3 = next_tool_call_authorization(&mut events).await; tool_call_auth_3 .response - .send(tool_call_auth_3.options[0].option_id.clone()) + .send(acp::PermissionOptionId::new( + "always_allow:tool_requiring_permission", + )) .unwrap(); cx.run_until_parked(); let completion = fake_model.pending_completions().pop().unwrap(); @@ -1139,12 +1142,13 @@ async fn next_tool_call_authorization( .iter() .map(|o| o.kind) .collect::>(); + // Only 2 options now: AllowAlways (for tool) and AllowOnce (granularity only) + // Deny is handled by the UI buttons, not as a separate option assert_eq!( permission_kinds, vec![ acp::PermissionOptionKind::AllowAlways, acp::PermissionOptionKind::AllowOnce, - acp::PermissionOptionKind::RejectOnce, ] ); return tool_call_authorization; diff --git a/crates/agent/src/tests/test_tools.rs b/crates/agent/src/tests/test_tools.rs index 755149230f596ae6be6d84c454fff70589b2c11d..90974f4d5d66745c9d691959e9d2ba2a5addd9ca 100644 --- a/crates/agent/src/tests/test_tools.rs +++ b/crates/agent/src/tests/test_tools.rs @@ -1,4 +1,5 @@ use super::*; +use agent_settings::AgentSettings; use anyhow::Result; use gpui::{App, SharedString, Task}; use std::future; @@ -124,9 +125,27 @@ impl AgentTool for ToolRequiringPermission { event_stream: ToolCallEventStream, cx: &mut App, ) -> Task> { - let authorize = event_stream.authorize("Authorize?", cx); + let settings = AgentSettings::get_global(cx); + let decision = decide_permission_from_settings(Self::name(), "", settings); + + let authorize = match decision { + ToolPermissionDecision::Allow => None, + ToolPermissionDecision::Deny(reason) => { + return Task::ready(Err(anyhow::anyhow!("{}", reason))); + } + ToolPermissionDecision::Confirm => { + let context = crate::ToolPermissionContext { + tool_name: "tool_requiring_permission".to_string(), + input_value: String::new(), + }; + Some(event_stream.authorize("Authorize?", context, cx)) + } + }; + cx.foreground_executor().spawn(async move { - authorize.await?; + if let Some(authorize) = authorize { + authorize.await?; + } Ok("Allowed".to_string()) }) } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 01bad43fa6f09c6ebe42b2e5891f927ef59ccd67..b9d82754bf523ee0ba6e4a60cb0d5b1cce120554 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -3,7 +3,8 @@ use crate::{ DeletePathTool, DiagnosticsTool, EditFileTool, FetchTool, FindPathTool, GrepTool, ListDirectoryTool, MovePathTool, NowTool, OpenTool, ProjectSnapshot, ReadFileTool, RestoreFileFromDiskTool, SaveFileTool, SubagentTool, SystemPromptTemplate, Template, Templates, - TerminalTool, ThinkingTool, WebSearchTool, + TerminalTool, ThinkingTool, ToolPermissionDecision, WebSearchTool, + decide_permission_from_settings, }; use acp_thread::{MentionUri, UserMessageId}; use action_log::ActionLog; @@ -42,7 +43,7 @@ use project::Project; use prompt_store::ProjectContext; use schemars::{JsonSchema, Schema}; use serde::{Deserialize, Serialize}; -use settings::{LanguageModelSelection, Settings, update_settings_file}; +use settings::{LanguageModelSelection, Settings, ToolPermissionMode, update_settings_file}; use smol::stream::StreamExt; use std::{ collections::BTreeMap, @@ -593,11 +594,81 @@ pub struct NewTerminal { pub response: oneshot::Sender>>, } +#[derive(Debug, Clone)] +pub struct ToolPermissionContext { + pub tool_name: String, + pub input_value: String, +} + +impl ToolPermissionContext { + pub fn new(tool_name: impl Into, input_value: impl Into) -> Self { + Self { + tool_name: tool_name.into(), + input_value: input_value.into(), + } + } + + /// Builds the permission options for this tool context. + /// + /// This is the canonical source for permission option generation. + /// Tests should use this function rather than manually constructing options. + pub fn build_permission_options(&self) -> Vec { + use crate::pattern_extraction::*; + + let tool_name = &self.tool_name; + let input_value = &self.input_value; + + let (pattern, pattern_display) = match tool_name.as_str() { + "terminal" => ( + extract_terminal_pattern(input_value), + extract_terminal_pattern_display(input_value), + ), + "edit_file" | "delete_path" | "move_path" | "create_directory" | "save_file" => ( + extract_path_pattern(input_value), + extract_path_pattern_display(input_value), + ), + "fetch" => ( + extract_url_pattern(input_value), + extract_url_pattern_display(input_value), + ), + _ => (None, None), + }; + + let mut options = vec![acp::PermissionOption::new( + acp::PermissionOptionId::new(format!("always:{}", tool_name)), + format!("Always for {}", tool_name.replace('_', " ")), + acp::PermissionOptionKind::AllowAlways, + )]; + + if let (Some(pattern), Some(display)) = (pattern, pattern_display) { + let button_text = match tool_name.as_str() { + "terminal" => format!("Always for `{}` commands", display), + "fetch" => format!("Always for `{}`", display), + _ => format!("Always for `{}`", display), + }; + options.push(acp::PermissionOption::new( + acp::PermissionOptionId::new(format!("always_pattern:{}:{}", tool_name, pattern)), + button_text, + acp::PermissionOptionKind::AllowAlways, + )); + } + + options.push(acp::PermissionOption::new( + acp::PermissionOptionId::new("once"), + "Only this time", + acp::PermissionOptionKind::AllowOnce, + )); + + options + } +} + #[derive(Debug)] pub struct ToolCallAuthorization { pub tool_call: acp::ToolCallUpdate, pub options: Vec, pub response: oneshot::Sender, + pub context: Option, } #[derive(Debug, thiserror::Error)] @@ -2883,19 +2954,32 @@ impl ToolCallEventStream { .ok(); } - pub fn authorize(&self, title: impl Into, cx: &mut App) -> Task> { - if agent_settings::AgentSettings::get_global(cx).always_allow_tool_actions { - return Task::ready(Ok(())); - } + /// Authorize a third-party tool (e.g., MCP tool from a context server). + /// + /// Unlike built-in tools, third-party tools don't support pattern-based permissions. + /// They only support `default_mode` (allow/deny/confirm) per tool. + /// + /// Shows 3 buttons: + /// - "Always allow MCP tool" → sets `tools..default_mode = "allow"` + /// - "Allow" → approve once + /// - "Deny" → reject once + pub fn authorize_third_party_tool( + &self, + title: impl Into, + tool_id: String, + display_name: String, + cx: &mut App, + ) -> Task> { + let settings = agent_settings::AgentSettings::get_global(cx); - self.authorize_required(title, cx) - } + let decision = decide_permission_from_settings(&tool_id, "", &settings); + + match decision { + ToolPermissionDecision::Allow => return Task::ready(Ok(())), + ToolPermissionDecision::Deny(reason) => return Task::ready(Err(anyhow!(reason))), + ToolPermissionDecision::Confirm => {} + } - /// Like `authorize`, but always prompts for confirmation regardless of - /// the `always_allow_tool_actions` setting. Use this when tool-specific - /// permission rules (like `always_confirm` patterns) have already determined - /// that confirmation is required. - pub fn authorize_required(&self, title: impl Into, cx: &mut App) -> Task> { let (response_tx, response_rx) = oneshot::channel(); self.stream .0 @@ -2907,13 +2991,13 @@ impl ToolCallEventStream { ), options: vec![ acp::PermissionOption::new( - acp::PermissionOptionId::new("always_allow"), - "Always Allow", + acp::PermissionOptionId::new(format!("always_allow_mcp:{}", tool_id)), + format!("Always allow {} MCP tool", display_name), acp::PermissionOptionKind::AllowAlways, ), acp::PermissionOption::new( acp::PermissionOptionId::new("allow"), - "Allow", + "Allow once", acp::PermissionOptionKind::AllowOnce, ), acp::PermissionOption::new( @@ -2923,27 +3007,146 @@ impl ToolCallEventStream { ), ], response: response_tx, + context: None, + }, + ))) + .ok(); + + let fs = self.fs.clone(); + cx.spawn(async move |cx| { + let response_str = response_rx.await?.0.to_string(); + + if response_str == format!("always_allow_mcp:{}", tool_id) { + if let Some(fs) = fs.clone() { + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + settings + .agent + .get_or_insert_default() + .set_tool_default_mode(&tool_id, ToolPermissionMode::Allow); + }); + }); + } + return Ok(()); + } + + if response_str == "allow" { + return Ok(()); + } + + Err(anyhow!("Permission to run tool denied by user")) + }) + } + + pub fn authorize( + &self, + title: impl Into, + context: ToolPermissionContext, + cx: &mut App, + ) -> Task> { + use settings::ToolPermissionMode; + + let options = context.build_permission_options(); + + let (response_tx, response_rx) = oneshot::channel(); + self.stream + .0 + .unbounded_send(Ok(ThreadEvent::ToolCallAuthorization( + ToolCallAuthorization { + tool_call: acp::ToolCallUpdate::new( + self.tool_use_id.to_string(), + acp::ToolCallUpdateFields::new().title(title.into()), + ), + options, + response: response_tx, + context: Some(context), }, ))) .ok(); + let fs = self.fs.clone(); - cx.spawn(async move |cx| match response_rx.await?.0.as_ref() { - "always_allow" => { + cx.spawn(async move |cx| { + let response_str = response_rx.await?.0.to_string(); + + // Handle "always allow tool" - e.g., "always_allow:terminal" + if let Some(tool) = response_str.strip_prefix("always_allow:") { + if let Some(fs) = fs.clone() { + let tool = tool.to_string(); + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + settings + .agent + .get_or_insert_default() + .set_tool_default_mode(&tool, ToolPermissionMode::Allow); + }); + }); + } + return Ok(()); + } + + // Handle "always deny tool" - e.g., "always_deny:terminal" + if let Some(tool) = response_str.strip_prefix("always_deny:") { if let Some(fs) = fs.clone() { + let tool = tool.to_string(); cx.update(|cx| { - update_settings_file(fs, cx, |settings, _| { + update_settings_file(fs, cx, move |settings, _| { settings .agent .get_or_insert_default() - .set_always_allow_tool_actions(true); + .set_tool_default_mode(&tool, ToolPermissionMode::Deny); }); }); } + return Err(anyhow!("Permission to run tool denied by user")); + } + + // Handle "always allow pattern" - e.g., "always_allow_pattern:terminal:^cargo\s" + if response_str.starts_with("always_allow_pattern:") { + let parts: Vec<&str> = response_str.splitn(3, ':').collect(); + if parts.len() == 3 { + let pattern_tool_name = parts[1].to_string(); + let pattern = parts[2].to_string(); + if let Some(fs) = fs.clone() { + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + settings + .agent + .get_or_insert_default() + .add_tool_allow_pattern(&pattern_tool_name, pattern); + }); + }); + } + } + return Ok(()); + } + + // Handle "always deny pattern" - e.g., "always_deny_pattern:terminal:^cargo\s" + if response_str.starts_with("always_deny_pattern:") { + let parts: Vec<&str> = response_str.splitn(3, ':').collect(); + if parts.len() == 3 { + let pattern_tool_name = parts[1].to_string(); + let pattern = parts[2].to_string(); + if let Some(fs) = fs.clone() { + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + settings + .agent + .get_or_insert_default() + .add_tool_deny_pattern(&pattern_tool_name, pattern); + }); + }); + } + } + return Err(anyhow!("Permission to run tool denied by user")); + } - Ok(()) + // Handle simple "allow" (allow once) + if response_str == "allow" { + return Ok(()); } - "allow" => Ok(()), - _ => Err(anyhow!("Permission to run tool denied by user")), + + // Handle simple "deny" (deny once) + Err(anyhow!("Permission to run tool denied by user")) }) } } diff --git a/crates/agent/src/tool_permissions.rs b/crates/agent/src/tool_permissions.rs index 1102610de92a90e0511a8bac5925d65667b8985a..85d4aec0fe0083f1fb6231e6de6aff3707ee83ff 100644 --- a/crates/agent/src/tool_permissions.rs +++ b/crates/agent/src/tool_permissions.rs @@ -130,300 +130,396 @@ mod tests { use agent_settings::{CompiledRegex, InvalidRegexPattern, ToolRules}; use std::sync::Arc; - fn empty_permissions() -> ToolPermissions { - ToolPermissions { - tools: collections::HashMap::default(), + struct PermTest { + tool: &'static str, + input: &'static str, + mode: ToolPermissionMode, + allow: Vec<&'static str>, + deny: Vec<&'static str>, + confirm: Vec<&'static str>, + global: bool, + } + + impl PermTest { + fn new(input: &'static str) -> Self { + Self { + tool: "terminal", + input, + mode: ToolPermissionMode::Confirm, + allow: vec![], + deny: vec![], + confirm: vec![], + global: false, + } + } + + fn tool(mut self, t: &'static str) -> Self { + self.tool = t; + self + } + fn mode(mut self, m: ToolPermissionMode) -> Self { + self.mode = m; + self + } + fn allow(mut self, p: &[&'static str]) -> Self { + self.allow = p.to_vec(); + self + } + fn deny(mut self, p: &[&'static str]) -> Self { + self.deny = p.to_vec(); + self + } + fn confirm(mut self, p: &[&'static str]) -> Self { + self.confirm = p.to_vec(); + self + } + fn global(mut self, g: bool) -> Self { + self.global = g; + self + } + + fn is_allow(self) { + assert_eq!( + self.run(), + ToolPermissionDecision::Allow, + "expected Allow for '{}'", + self.input + ); + } + fn is_deny(self) { + assert!( + matches!(self.run(), ToolPermissionDecision::Deny(_)), + "expected Deny for '{}'", + self.input + ); + } + fn is_confirm(self) { + assert_eq!( + self.run(), + ToolPermissionDecision::Confirm, + "expected Confirm for '{}'", + self.input + ); + } + + fn run(&self) -> ToolPermissionDecision { + let mut tools = collections::HashMap::default(); + tools.insert( + Arc::from(self.tool), + ToolRules { + default_mode: self.mode, + always_allow: self + .allow + .iter() + .filter_map(|p| CompiledRegex::new(p, false)) + .collect(), + always_deny: self + .deny + .iter() + .filter_map(|p| CompiledRegex::new(p, false)) + .collect(), + always_confirm: self + .confirm + .iter() + .filter_map(|p| CompiledRegex::new(p, false)) + .collect(), + invalid_patterns: vec![], + }, + ); + decide_permission( + self.tool, + self.input, + &ToolPermissions { tools }, + self.global, + ) } } - fn terminal_rules_with_deny(patterns: &[&str]) -> ToolPermissions { - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Confirm, - always_allow: vec![], - always_deny: patterns - .iter() - .filter_map(|p| CompiledRegex::new(p, false)) - .collect(), - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - ToolPermissions { tools } + fn t(input: &'static str) -> PermTest { + PermTest::new(input) } - fn terminal_rules_with_allow(patterns: &[&str]) -> ToolPermissions { - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Confirm, - always_allow: patterns - .iter() - .filter_map(|p| CompiledRegex::new(p, false)) - .collect(), - always_deny: vec![], - always_confirm: vec![], - invalid_patterns: vec![], + fn no_rules(input: &str, global: bool) -> ToolPermissionDecision { + decide_permission( + "terminal", + input, + &ToolPermissions { + tools: collections::HashMap::default(), }, - ); - ToolPermissions { tools } + global, + ) } + // allow pattern matches #[test] - fn test_deny_takes_precedence_over_allow() { - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![CompiledRegex::new("dangerous", false).unwrap()], - always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; - - let decision = decide_permission("terminal", "run dangerous command", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + fn allow_exact_match() { + t("cargo test").allow(&["^cargo\\s"]).is_allow(); } - #[test] - fn test_deny_takes_precedence_over_confirm() { - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![], - always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()], - always_confirm: vec![CompiledRegex::new("dangerous", false).unwrap()], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; - - let decision = decide_permission("terminal", "run dangerous command", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + fn allow_with_args() { + t("cargo build --release").allow(&["^cargo\\s"]).is_allow(); } - #[test] - fn test_confirm_takes_precedence_over_allow() { - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![CompiledRegex::new("risky", false).unwrap()], - always_deny: vec![], - always_confirm: vec![CompiledRegex::new("risky", false).unwrap()], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; - - let decision = decide_permission("terminal", "do risky thing", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Confirm); + fn allow_one_of_many() { + t("npm install").allow(&["^cargo\\s", "^npm\\s"]).is_allow(); } - #[test] - fn test_no_tool_rules_uses_global_setting() { - let permissions = empty_permissions(); - - let decision = decide_permission("terminal", "any command", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Confirm); - - let decision = decide_permission("terminal", "any command", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); + fn allow_middle_pattern() { + t("run cargo now").allow(&["cargo"]).is_allow(); } - #[test] - fn test_default_mode_fallthrough() { - // default_mode: Allow - should allow regardless of global setting - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![], - always_deny: vec![], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; - let decision = decide_permission("terminal", "any command", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Allow); + fn allow_anchor_prevents_middle() { + t("run cargo now").allow(&["^cargo"]).is_confirm(); + } - // default_mode: Deny - should deny regardless of global setting - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Deny, - always_allow: vec![], - always_deny: vec![], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; - let decision = decide_permission("terminal", "any command", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + // allow pattern doesn't match -> falls through + #[test] + fn allow_no_match_confirms() { + t("python x.py").allow(&["^cargo\\s"]).is_confirm(); + } + #[test] + fn allow_no_match_global_allows() { + t("python x.py") + .allow(&["^cargo\\s"]) + .global(true) + .is_allow(); + } - // default_mode: Confirm - respects global always_allow_tool_actions - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Confirm, - always_allow: vec![], - always_deny: vec![], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; - let decision = decide_permission("terminal", "any command", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Confirm); - let decision = decide_permission("terminal", "any command", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); + // deny pattern matches + #[test] + fn deny_blocks() { + t("rm -rf /").deny(&["rm\\s+-rf"]).is_deny(); + } + #[test] + fn deny_blocks_with_global() { + t("rm -rf /").deny(&["rm\\s+-rf"]).global(true).is_deny(); + } + #[test] + fn deny_blocks_with_mode_allow() { + t("rm -rf /") + .deny(&["rm\\s+-rf"]) + .mode(ToolPermissionMode::Allow) + .is_deny(); + } + #[test] + fn deny_middle_match() { + t("echo rm -rf x").deny(&["rm\\s+-rf"]).is_deny(); + } + #[test] + fn deny_no_match_allows() { + t("ls -la").deny(&["rm\\s+-rf"]).global(true).is_allow(); } + // confirm pattern matches + #[test] + fn confirm_requires_confirm() { + t("sudo apt install").confirm(&["sudo\\s"]).is_confirm(); + } + #[test] + fn confirm_overrides_global() { + t("sudo reboot") + .confirm(&["sudo\\s"]) + .global(true) + .is_confirm(); + } #[test] - fn test_empty_input() { - let permissions = terminal_rules_with_deny(&["rm"]); + fn confirm_overrides_mode_allow() { + t("sudo x") + .confirm(&["sudo"]) + .mode(ToolPermissionMode::Allow) + .is_confirm(); + } - // Empty input doesn't match the deny pattern, so falls through to default_mode (Confirm) - let decision = decide_permission("terminal", "", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Confirm); + // confirm beats allow + #[test] + fn confirm_beats_allow() { + t("git push --force") + .allow(&["^git\\s"]) + .confirm(&["--force"]) + .is_confirm(); + } + #[test] + fn confirm_beats_allow_overlap() { + t("deploy prod") + .allow(&["deploy"]) + .confirm(&["prod"]) + .is_confirm(); + } + #[test] + fn allow_when_confirm_no_match() { + t("git status") + .allow(&["^git\\s"]) + .confirm(&["--force"]) + .is_allow(); + } - // With always_allow_tool_actions=true and default_mode=Confirm, it returns Allow - let decision = decide_permission("terminal", "", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); + // deny beats allow + #[test] + fn deny_beats_allow() { + t("rm -rf /tmp/x") + .allow(&["/tmp/"]) + .deny(&["rm\\s+-rf"]) + .is_deny(); + } + #[test] + fn deny_beats_allow_diff() { + t("bad deploy").allow(&["deploy"]).deny(&["bad"]).is_deny(); } + // deny beats confirm #[test] - fn test_multiple_patterns_any_match() { - // Multiple deny patterns - any match should deny - let permissions = terminal_rules_with_deny(&["rm", "dangerous", "delete"]); + fn deny_beats_confirm() { + t("sudo rm -rf /") + .confirm(&["sudo"]) + .deny(&["rm\\s+-rf"]) + .is_deny(); + } - let decision = decide_permission("terminal", "run dangerous command", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + // deny beats everything + #[test] + fn deny_beats_all() { + t("bad cmd") + .allow(&["cmd"]) + .confirm(&["cmd"]) + .deny(&["bad"]) + .is_deny(); + } - let decision = decide_permission("terminal", "delete file", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + // no patterns -> default_mode + #[test] + fn default_confirm() { + t("python x.py") + .mode(ToolPermissionMode::Confirm) + .is_confirm(); + } + #[test] + fn default_allow() { + t("python x.py").mode(ToolPermissionMode::Allow).is_allow(); + } + #[test] + fn default_deny() { + t("python x.py").mode(ToolPermissionMode::Deny).is_deny(); + } - // Multiple allow patterns - any match should allow - let permissions = terminal_rules_with_allow(&["^cargo", "^npm", "^git"]); + // default_mode confirm + global + #[test] + fn default_confirm_global_false() { + t("x") + .mode(ToolPermissionMode::Confirm) + .global(false) + .is_confirm(); + } + #[test] + fn default_confirm_global_true() { + t("x") + .mode(ToolPermissionMode::Confirm) + .global(true) + .is_allow(); + } - let decision = decide_permission("terminal", "cargo build", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Allow); + // no rules at all -> global setting + #[test] + fn no_rules_global_false() { + assert_eq!(no_rules("x", false), ToolPermissionDecision::Confirm); + } + #[test] + fn no_rules_global_true() { + assert_eq!(no_rules("x", true), ToolPermissionDecision::Allow); + } - let decision = decide_permission("terminal", "npm install", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Allow); + // empty input + #[test] + fn empty_input_no_match() { + t("").deny(&["rm"]).is_confirm(); + } + #[test] + fn empty_input_global() { + t("").deny(&["rm"]).global(true).is_allow(); + } - // No pattern matches - falls through to default - let decision = decide_permission("terminal", "rm file", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Confirm); + // multiple patterns - any match + #[test] + fn multi_deny_first() { + t("rm x").deny(&["rm", "del", "drop"]).is_deny(); + } + #[test] + fn multi_deny_last() { + t("drop x").deny(&["rm", "del", "drop"]).is_deny(); + } + #[test] + fn multi_allow_first() { + t("cargo x").allow(&["^cargo", "^npm", "^git"]).is_allow(); + } + #[test] + fn multi_allow_last() { + t("git x").allow(&["^cargo", "^npm", "^git"]).is_allow(); + } + #[test] + fn multi_none_match() { + t("python x") + .allow(&["^cargo", "^npm"]) + .deny(&["rm"]) + .is_confirm(); } + // tool isolation #[test] - fn test_case_insensitive_matching() { - // Case-insensitive by default (case_sensitive: false) + fn other_tool_not_affected() { let mut tools = collections::HashMap::default(); tools.insert( Arc::from("terminal"), ToolRules { - default_mode: ToolPermissionMode::Confirm, + default_mode: ToolPermissionMode::Deny, always_allow: vec![], - always_deny: vec![CompiledRegex::new(r"\brm\b", false).unwrap()], + always_deny: vec![], always_confirm: vec![], invalid_patterns: vec![], }, ); - let permissions = ToolPermissions { tools }; - - // Should match regardless of case - let decision = decide_permission("terminal", "RM file.txt", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); - - let decision = decide_permission("terminal", "Rm file.txt", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); - - let decision = decide_permission("terminal", "rm file.txt", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); - } - - #[test] - fn test_case_sensitive_matching() { - // Case-sensitive matching when explicitly enabled - let mut tools = collections::HashMap::default(); tools.insert( - Arc::from("terminal"), + Arc::from("edit_file"), ToolRules { - default_mode: ToolPermissionMode::Confirm, + default_mode: ToolPermissionMode::Allow, always_allow: vec![], - always_deny: vec![CompiledRegex::new("DROP TABLE", true).unwrap()], + always_deny: vec![], always_confirm: vec![], invalid_patterns: vec![], }, ); - let permissions = ToolPermissions { tools }; - - // Should only match exact case - let decision = decide_permission("terminal", "DROP TABLE users", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); - - // Should NOT match different case - let decision = decide_permission("terminal", "drop table users", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); + let p = ToolPermissions { tools }; + assert!(matches!( + decide_permission("terminal", "x", &p, true), + ToolPermissionDecision::Deny(_) + )); + assert_eq!( + decide_permission("edit_file", "x", &p, false), + ToolPermissionDecision::Allow + ); } #[test] - fn test_multi_tool_isolation() { - // Rules for terminal should not affect edit_file + fn partial_tool_name_no_match() { let mut tools = collections::HashMap::default(); tools.insert( - Arc::from("terminal"), + Arc::from("term"), ToolRules { default_mode: ToolPermissionMode::Deny, always_allow: vec![], - always_deny: vec![CompiledRegex::new("dangerous", false).unwrap()], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - tools.insert( - Arc::from("edit_file"), - ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![], always_deny: vec![], always_confirm: vec![], invalid_patterns: vec![], }, ); - let permissions = ToolPermissions { tools }; - - // Terminal with "dangerous" should be denied - let decision = decide_permission("terminal", "run dangerous command", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); - - // edit_file with "dangerous" should be allowed (no deny rules for edit_file) - let decision = decide_permission("edit_file", "dangerous_file.txt", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); - - // Terminal without "dangerous" should still be denied due to default_mode: Deny - let decision = decide_permission("terminal", "safe command", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + let p = ToolPermissions { tools }; + assert_eq!( + decide_permission("terminal", "x", &p, true), + ToolPermissionDecision::Allow + ); } + // invalid patterns block the tool #[test] - fn test_invalid_patterns_block_tool() { + fn invalid_pattern_blocks() { let mut tools = collections::HashMap::default(); tools.insert( Arc::from("terminal"), @@ -433,115 +529,96 @@ mod tests { always_deny: vec![], always_confirm: vec![], invalid_patterns: vec![InvalidRegexPattern { - pattern: "[invalid(regex".to_string(), - rule_type: "always_deny".to_string(), - error: "unclosed character class".to_string(), + pattern: "[bad".into(), + rule_type: "always_deny".into(), + error: "err".into(), }], }, ); - let permissions = ToolPermissions { tools }; - - // Even though "echo" matches always_allow, the tool should be blocked - // because there are invalid patterns - let decision = decide_permission("terminal", "echo hello", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); - - if let ToolPermissionDecision::Deny(msg) = decision { - assert!( - msg.contains("regex"), - "error message should mention regex: {}", - msg - ); - assert!( - msg.contains("settings"), - "error message should mention settings: {}", - msg - ); - assert!( - msg.contains("terminal"), - "error message should mention the tool name: {}", - msg - ); - } + let p = ToolPermissions { tools }; + assert!(matches!( + decide_permission("terminal", "echo hi", &p, true), + ToolPermissionDecision::Deny(_) + )); } + // user scenario: only echo allowed, git should confirm #[test] - fn test_same_pattern_in_deny_and_allow_deny_wins() { - // When the same pattern appears in both deny and allow lists, deny should win - let mut tools = collections::HashMap::default(); - tools.insert( - Arc::from("terminal"), - ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![CompiledRegex::new("deploy", false).unwrap()], - always_deny: vec![CompiledRegex::new("deploy", false).unwrap()], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - let permissions = ToolPermissions { tools }; + fn user_scenario_only_echo() { + t("echo hello").allow(&["^echo\\s"]).is_allow(); + } + #[test] + fn user_scenario_git_confirms() { + t("git status").allow(&["^echo\\s"]).is_confirm(); + } + #[test] + fn user_scenario_rm_confirms() { + t("rm -rf /").allow(&["^echo\\s"]).is_confirm(); + } - let decision = decide_permission("terminal", "deploy production", &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + // mcp tools + #[test] + fn mcp_allow() { + t("") + .tool("mcp:fs:read") + .mode(ToolPermissionMode::Allow) + .is_allow(); + } + #[test] + fn mcp_deny() { + t("") + .tool("mcp:bad:del") + .mode(ToolPermissionMode::Deny) + .is_deny(); + } + #[test] + fn mcp_confirm() { + t("") + .tool("mcp:gh:issue") + .mode(ToolPermissionMode::Confirm) + .is_confirm(); + } + #[test] + fn mcp_confirm_global() { + t("") + .tool("mcp:gh:issue") + .mode(ToolPermissionMode::Confirm) + .global(true) + .is_allow(); } + // mcp vs builtin isolation #[test] - fn test_same_pattern_in_confirm_and_allow_confirm_wins() { - // When the same pattern appears in both confirm and allow lists, confirm should win + fn mcp_doesnt_collide_with_builtin() { let mut tools = collections::HashMap::default(); tools.insert( Arc::from("terminal"), ToolRules { - default_mode: ToolPermissionMode::Allow, - always_allow: vec![CompiledRegex::new("deploy", false).unwrap()], + default_mode: ToolPermissionMode::Deny, + always_allow: vec![], always_deny: vec![], - always_confirm: vec![CompiledRegex::new("deploy", false).unwrap()], + always_confirm: vec![], invalid_patterns: vec![], }, ); - let permissions = ToolPermissions { tools }; - - let decision = decide_permission("terminal", "deploy production", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Confirm); - } - - #[test] - fn test_partial_tool_name_does_not_match() { - // Rules for "term" should not affect "terminal" - let mut tools = collections::HashMap::default(); tools.insert( - Arc::from("term"), + Arc::from("mcp:srv:terminal"), ToolRules { - default_mode: ToolPermissionMode::Deny, + default_mode: ToolPermissionMode::Allow, always_allow: vec![], always_deny: vec![], always_confirm: vec![], invalid_patterns: vec![], }, ); - let permissions = ToolPermissions { tools }; - - // "terminal" should not be affected by "term" rules, falls back to global setting - let decision = decide_permission("terminal", "echo hello", &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); - - let decision = decide_permission("terminal", "echo hello", &permissions, false); - assert_eq!(decision, ToolPermissionDecision::Confirm); - } - - #[test] - fn test_very_long_input() { - // Test that very long inputs are handled correctly - let permissions = terminal_rules_with_deny(&[r"\brm\b"]); - - // Long input without the pattern should not match - let long_safe_input = "echo ".to_string() + &"a".repeat(100_000); - let decision = decide_permission("terminal", &long_safe_input, &permissions, true); - assert_eq!(decision, ToolPermissionDecision::Allow); - - // Long input with the pattern should match - let long_dangerous_input = "a".repeat(50_000) + " rm " + &"b".repeat(50_000); - let decision = decide_permission("terminal", &long_dangerous_input, &permissions, true); - assert!(matches!(decision, ToolPermissionDecision::Deny(_))); + let p = ToolPermissions { tools }; + assert!(matches!( + decide_permission("terminal", "x", &p, false), + ToolPermissionDecision::Deny(_) + )); + assert_eq!( + decide_permission("mcp:srv:terminal", "x", &p, false), + ToolPermissionDecision::Allow + ); } } diff --git a/crates/agent/src/tools/context_server_registry.rs b/crates/agent/src/tools/context_server_registry.rs index d73e1d7ddae3573f7a980c0e8f66a39132d59bd5..c2c398c28e8bac6ff6de85cea6acfe4308515922 100644 --- a/crates/agent/src/tools/context_server_registry.rs +++ b/crates/agent/src/tools/context_server_registry.rs @@ -9,6 +9,13 @@ use project::context_server_store::{ContextServerStatus, ContextServerStore}; use std::sync::Arc; use util::ResultExt; +/// Generates a tool ID for an MCP tool that can be used in settings. +/// +/// The format is `mcp::` to avoid collisions with built-in tools. +pub fn mcp_tool_id(server_id: &str, tool_name: &str) -> String { + format!("mcp:{}:{}", server_id, tool_name) +} + pub struct ContextServerPrompt { pub server_id: ContextServerId, pub prompt: context_server::types::Prompt, @@ -332,7 +339,14 @@ impl AnyAgentTool for ContextServerTool { return Task::ready(Err(anyhow!("Context server not found"))); }; let tool_name = self.tool.name.clone(); - let authorize = event_stream.authorize(self.initial_title(input.clone(), cx), cx); + let tool_id = mcp_tool_id(&self.server_id.0, &self.tool.name); + let display_name = self.tool.name.clone(); + let authorize = event_stream.authorize_third_party_tool( + self.initial_title(input.clone(), cx), + tool_id, + display_name, + cx, + ); cx.spawn(async move |_cx| { authorize.await?; @@ -435,3 +449,29 @@ pub fn get_prompt( Ok(response) }) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_mcp_tool_id_format() { + assert_eq!( + mcp_tool_id("filesystem", "read_file"), + "mcp:filesystem:read_file" + ); + assert_eq!( + mcp_tool_id("github", "create_issue"), + "mcp:github:create_issue" + ); + assert_eq!( + mcp_tool_id("my-custom-server", "do_something"), + "mcp:my-custom-server:do_something" + ); + // Underscores in names + assert_eq!(mcp_tool_id("my_server", "my_tool"), "mcp:my_server:my_tool"); + } + + // Note: Tests for MCP tool ID collision with built-in tools and permission + // decisions are in crates/agent/src/tool_permissions.rs to avoid duplication. +} diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index 694d5d0582b9e6828286cf6207d6232a48bb0b29..33fdcf3631eed6815ae2d1b1cecd4932f0b728a7 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -103,7 +103,11 @@ impl AgentTool for CopyPathTool { let authorize = if needs_confirmation { let src = MarkdownInlineCode(&input.source_path); let dest = MarkdownInlineCode(&input.destination_path); - Some(event_stream.authorize(format!("Copy {src} to {dest}"), cx)) + let context = crate::ToolPermissionContext { + tool_name: "copy_path".to_string(), + input_value: input.source_path.clone(), + }; + Some(event_stream.authorize(format!("Copy {src} to {dest}"), context, cx)) } else { None }; diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index bc31bed42196ea4788c7f6d29154d4f546083bce..ba2adf2debe23f168d2c54b87d193f1d25c1dc47 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -80,10 +80,17 @@ impl AgentTool for CreateDirectoryTool { ToolPermissionDecision::Deny(reason) => { return Task::ready(Err(anyhow!("{}", reason))); } - ToolPermissionDecision::Confirm => Some(event_stream.authorize( - format!("Create directory {}", MarkdownInlineCode(&input.path)), - cx, - )), + ToolPermissionDecision::Confirm => { + let context = crate::ToolPermissionContext { + tool_name: "create_directory".to_string(), + input_value: input.path.clone(), + }; + Some(event_stream.authorize( + format!("Create directory {}", MarkdownInlineCode(&input.path)), + context, + cx, + )) + } }; let project_path = match self.project.read(cx).find_project_path(&input.path, cx) { diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index c3fc1cc72d8eae22969ae72dc1bd39f791cf0362..f92f5171eaecb229a962b296e0932d627e93ea6b 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -86,7 +86,15 @@ impl AgentTool for DeletePathTool { return Task::ready(Err(anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - Some(event_stream.authorize(format!("Delete {}", MarkdownInlineCode(&path)), cx)) + let context = crate::ToolPermissionContext { + tool_name: "delete_path".to_string(), + input_value: path.clone(), + }; + Some(event_stream.authorize( + format!("Delete {}", MarkdownInlineCode(&path)), + context, + cx, + )) } }; diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 68d5c0b6e121277e25f8ccfa36c42b30f8926296..4f8288c24382d373a41fd15b779970676ad09fae 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -169,8 +169,13 @@ impl EditFileTool { if path.components().any(|component| { component.as_os_str() == <_ as AsRef>::as_ref(&local_settings_folder) }) { + let context = crate::ToolPermissionContext { + tool_name: "edit_file".to_string(), + input_value: path_str.to_string(), + }; return event_stream.authorize( format!("{} (local settings)", input.display_description), + context, cx, ); } @@ -181,8 +186,13 @@ impl EditFileTool { if let Ok(canonical_path) = std::fs::canonicalize(&input.path) && canonical_path.starts_with(paths::config_dir()) { + let context = crate::ToolPermissionContext { + tool_name: "edit_file".to_string(), + input_value: path_str.to_string(), + }; return event_stream.authorize( format!("{} (global settings)", input.display_description), + context, cx, ); } @@ -200,7 +210,11 @@ impl EditFileTool { if project_path.is_some() { Task::ready(Ok(())) } else { - event_stream.authorize(&input.display_description, cx) + let context = crate::ToolPermissionContext { + tool_name: "edit_file".to_string(), + input_value: path_str.to_string(), + }; + event_stream.authorize(&input.display_description, context, cx) } } } diff --git a/crates/agent/src/tools/fetch_tool.rs b/crates/agent/src/tools/fetch_tool.rs index e52ae4bd241df3d8c252c9fbed1872505ac2c0e3..59d6b56063943a9c9db46844a5d8901de918c95a 100644 --- a/crates/agent/src/tools/fetch_tool.rs +++ b/crates/agent/src/tools/fetch_tool.rs @@ -155,9 +155,17 @@ impl AgentTool for FetchTool { ToolPermissionDecision::Deny(reason) => { return Task::ready(Err(anyhow::anyhow!("{}", reason))); } - ToolPermissionDecision::Confirm => Some( - event_stream.authorize(format!("Fetch {}", MarkdownInlineCode(&input.url)), cx), - ), + ToolPermissionDecision::Confirm => { + let context = crate::ToolPermissionContext { + tool_name: "fetch".to_string(), + input_value: input.url.clone(), + }; + Some(event_stream.authorize( + format!("Fetch {}", MarkdownInlineCode(&input.url)), + context, + cx, + )) + } }; let fetch_task = cx.background_spawn({ diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index 9ec91a5350dbe550596c5c6ec5f5e27408cb5b6b..7fc56af2ddde848bbc4bfb9a21ab1a4156dd033c 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -117,7 +117,11 @@ impl AgentTool for MovePathTool { let authorize = if needs_confirmation { let src = MarkdownInlineCode(&input.source_path); let dest = MarkdownInlineCode(&input.destination_path); - Some(event_stream.authorize(format!("Move {src} to {dest}"), cx)) + let context = crate::ToolPermissionContext { + tool_name: "move_path".to_string(), + input_value: input.source_path.clone(), + }; + Some(event_stream.authorize(format!("Move {src} to {dest}"), context, cx)) } else { None }; diff --git a/crates/agent/src/tools/open_tool.rs b/crates/agent/src/tools/open_tool.rs index 2a3d14fb3ad0129fad7032e798ac5ec58c59b7f3..ccbe29153e632e9a2a735f54868991d450d1cdfc 100644 --- a/crates/agent/src/tools/open_tool.rs +++ b/crates/agent/src/tools/open_tool.rs @@ -66,7 +66,12 @@ impl AgentTool for OpenTool { ) -> Task> { // If path_or_url turns out to be a path in the project, make it absolute. let abs_path = to_absolute_path(&input.path_or_url, self.project.clone(), cx); - let authorize = event_stream.authorize(self.initial_title(Ok(input.clone()), cx), cx); + let context = crate::ToolPermissionContext { + tool_name: "open".to_string(), + input_value: input.path_or_url.clone(), + }; + let authorize = + event_stream.authorize(self.initial_title(Ok(input.clone()), cx), context, cx); cx.background_spawn(async move { futures::select! { result = authorize.fuse() => result?, diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index 1265b2f5c28b007574cbe495ad5884a21faf6670..44f701941fc7b30825e4b131d6fb5206a7a4a4d7 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -107,7 +107,16 @@ impl AgentTool for SaveFileTool { format!("Save {}", paths.join(", ")) } }; - Some(event_stream.authorize(title, cx)) + let first_path = input + .paths + .first() + .map(|p| p.to_string_lossy().to_string()) + .unwrap_or_default(); + let context = crate::ToolPermissionContext { + tool_name: "save_file".to_string(), + input_value: first_path, + }; + Some(event_stream.authorize(title, context, cx)) } else { None }; diff --git a/crates/agent/src/tools/subagent_tool.rs b/crates/agent/src/tools/subagent_tool.rs index 08b4744baef7b6a9a05b4595bfee0df59c616c95..fd4174e463810071904f18fd7206bf2f2e9cff7c 100644 --- a/crates/agent/src/tools/subagent_tool.rs +++ b/crates/agent/src/tools/subagent_tool.rs @@ -531,6 +531,7 @@ fn forward_event_to_acp_thread( tool_call, options, response, + .. }) => { let outcome_task = acp_thread.update(cx, |thread, cx| { thread.request_tool_call_authorization(tool_call, options, true, cx) diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index 2d98bbbc59caedce202a85647d99d6f71aa92473..5e2b7ead1e237841e135757ecfb0f59309ffcb64 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -105,8 +105,11 @@ impl AgentTool for TerminalTool { return Task::ready(Err(anyhow::anyhow!("{}", reason))); } ToolPermissionDecision::Confirm => { - // Use authorize_required since permission rules already determined confirmation is needed - Some(event_stream.authorize_required(self.initial_title(Ok(input.clone()), cx), cx)) + let context = crate::ToolPermissionContext { + tool_name: "terminal".to_string(), + input_value: input.command.clone(), + }; + Some(event_stream.authorize(self.initial_title(Ok(input.clone()), cx), context, cx)) } }; cx.spawn(async move |cx| { diff --git a/crates/agent/src/tools/web_search_tool.rs b/crates/agent/src/tools/web_search_tool.rs index 26d073415d77dc396c8bd2a24b203a7a46099a79..7bc9602a074d7ffe799a7022d98e99ab4ff337c1 100644 --- a/crates/agent/src/tools/web_search_tool.rs +++ b/crates/agent/src/tools/web_search_tool.rs @@ -81,10 +81,17 @@ impl AgentTool for WebSearchTool { ToolPermissionDecision::Deny(reason) => { return Task::ready(Err(anyhow!("{}", reason))); } - ToolPermissionDecision::Confirm => Some(event_stream.authorize( - format!("Search the web for {}", MarkdownInlineCode(&input.query)), - cx, - )), + ToolPermissionDecision::Confirm => { + let context = crate::ToolPermissionContext { + tool_name: "web_search".to_string(), + input_value: input.query.clone(), + }; + Some(event_stream.authorize( + format!("Search the web for {}", MarkdownInlineCode(&input.query)), + context, + cx, + )) + } }; let Some(provider) = WebSearchRegistry::read_global(cx).active_provider() else { diff --git a/crates/agent_settings/src/agent_settings.rs b/crates/agent_settings/src/agent_settings.rs index eab9e59545daa6055f156b2626986895e8fe669a..f35afe365685647977a83d768c4a59992c7a8063 100644 --- a/crates/agent_settings/src/agent_settings.rs +++ b/crates/agent_settings/src/agent_settings.rs @@ -600,11 +600,6 @@ mod tests { !terminal.always_confirm.is_empty(), "terminal should have confirm rules" ); - assert!( - !terminal.always_allow.is_empty(), - "terminal should have allow rules" - ); - let edit_file = permissions .tools .get("edit_file") @@ -627,9 +622,10 @@ mod tests { .tools .get("fetch") .expect("fetch tool should be configured"); - assert!( - !fetch.always_allow.is_empty(), - "fetch should have allow rules" + assert_eq!( + fetch.default_mode, + settings::ToolPermissionMode::Confirm, + "fetch should have confirm as default mode" ); } @@ -665,40 +661,6 @@ mod tests { } } - #[test] - fn test_default_allow_rules_match_safe_commands() { - let default_json = include_str!("../../../assets/settings/default.json"); - let value: serde_json::Value = serde_json_lenient::from_str(default_json).unwrap(); - let tool_permissions = value["agent"]["tool_permissions"].clone(); - let content: ToolPermissionsContent = serde_json::from_value(tool_permissions).unwrap(); - let permissions = compile_tool_permissions(Some(content)); - - let terminal = permissions.tools.get("terminal").unwrap(); - - let safe_commands = [ - "cargo build", - "cargo test", - "cargo check", - "npm test", - "pnpm install", - "yarn run build", - "ls", - "ls -la", - "cat file.txt", - "git status", - "git log", - "git diff", - ]; - - for cmd in &safe_commands { - assert!( - terminal.always_allow.iter().any(|r| r.is_match(cmd)), - "Command '{}' should be allowed by allow rules", - cmd - ); - } - } - #[test] fn test_deny_takes_precedence_over_allow_and_confirm() { let json = json!({ @@ -822,4 +784,108 @@ mod tests { "Default deny rules should block the classic fork bomb" ); } + + #[test] + fn test_compiled_regex_stores_case_sensitivity() { + let case_sensitive = CompiledRegex::new("test", true).unwrap(); + let case_insensitive = CompiledRegex::new("test", false).unwrap(); + + assert!(case_sensitive.case_sensitive); + assert!(!case_insensitive.case_sensitive); + } + + #[test] + fn test_invalid_regex_is_skipped_not_fail() { + let json = json!({ + "tools": { + "terminal": { + "always_deny": [ + { "pattern": "[invalid(regex" }, + { "pattern": "valid_pattern" } + ] + } + } + }); + + let content: ToolPermissionsContent = serde_json::from_value(json).unwrap(); + let permissions = compile_tool_permissions(Some(content)); + + let terminal = permissions.tools.get("terminal").unwrap(); + assert_eq!(terminal.always_deny.len(), 1); + assert!(terminal.always_deny[0].is_match("valid_pattern")); + } + + #[test] + fn test_unconfigured_tool_not_in_permissions() { + let json = json!({ + "tools": { + "terminal": { + "default_mode": "allow" + } + } + }); + + let content: ToolPermissionsContent = serde_json::from_value(json).unwrap(); + let permissions = compile_tool_permissions(Some(content)); + + assert!(permissions.tools.contains_key("terminal")); + assert!(!permissions.tools.contains_key("edit_file")); + assert!(!permissions.tools.contains_key("fetch")); + } + + #[test] + fn test_always_allow_pattern_only_matches_specified_commands() { + // Reproduces user-reported bug: when always_allow has pattern "^echo\s", + // only "echo hello" should be allowed, not "git status". + // + // User config: + // always_allow_tool_actions: false + // tool_permissions.tools.terminal.always_allow: [{ pattern: "^echo\\s" }] + let json = json!({ + "tools": { + "terminal": { + "always_allow": [ + { "pattern": "^echo\\s" } + ] + } + } + }); + + let content: ToolPermissionsContent = serde_json::from_value(json).unwrap(); + let permissions = compile_tool_permissions(Some(content)); + + let terminal = permissions.tools.get("terminal").unwrap(); + + // Verify the pattern was compiled + assert_eq!( + terminal.always_allow.len(), + 1, + "Should have one always_allow pattern" + ); + + // Verify the pattern matches "echo hello" + assert!( + terminal.always_allow[0].is_match("echo hello"), + "Pattern ^echo\\s should match 'echo hello'" + ); + + // Verify the pattern does NOT match "git status" + assert!( + !terminal.always_allow[0].is_match("git status"), + "Pattern ^echo\\s should NOT match 'git status'" + ); + + // Verify the pattern does NOT match "echoHello" (no space) + assert!( + !terminal.always_allow[0].is_match("echoHello"), + "Pattern ^echo\\s should NOT match 'echoHello' (requires whitespace)" + ); + + // Verify default_mode is Confirm (the default) + assert_eq!( + terminal.default_mode, + settings::ToolPermissionMode::Confirm, + "default_mode should be Confirm when not specified" + ); + } } diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index 3f2da94d16b752a398a955d7df1dee52ace65f8f..bf698a8771f4665be035cabc9e9b908ee036a8b1 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -50,8 +50,8 @@ use theme::{AgentFontSize, ThemeSettings}; use ui::{ Callout, CommonAnimationExt, ContextMenu, ContextMenuEntry, CopyButton, DecoratedIcon, DiffStat, Disclosure, Divider, DividerColor, ElevationIndex, IconDecoration, - IconDecorationKind, KeyBinding, PopoverMenuHandle, SpinnerLabel, TintColor, Tooltip, - WithScrollbar, prelude::*, right_click_menu, + IconDecorationKind, KeyBinding, PopoverMenu, PopoverMenuHandle, SpinnerLabel, TintColor, + Tooltip, WithScrollbar, prelude::*, right_click_menu, }; use util::defer; use util::{ResultExt, size::format_file_size, time::duration_alt_display}; @@ -71,10 +71,11 @@ use crate::profile_selector::{ProfileProvider, ProfileSelector}; use crate::ui::{AgentNotification, AgentNotificationEvent, BurnModeTooltip, UsageCallout}; use crate::{ - AgentDiffPane, AgentPanel, AllowAlways, AllowOnce, ClearMessageQueue, ContinueThread, - ContinueWithBurnMode, CycleFavoriteModels, CycleModeSelector, ExpandMessageEditor, Follow, - KeepAll, NewThread, OpenAgentDiff, OpenHistory, RejectAll, RejectOnce, SendImmediately, - SendNextQueuedMessage, ToggleBurnMode, ToggleProfileSelector, + AgentDiffPane, AgentPanel, AllowAlways, AllowOnce, AuthorizeToolCall, ClearMessageQueue, + ContinueThread, ContinueWithBurnMode, CycleFavoriteModels, CycleModeSelector, + ExpandMessageEditor, Follow, KeepAll, NewThread, OpenAgentDiff, OpenHistory, RejectAll, + RejectOnce, SelectPermissionGranularity, SendImmediately, SendNextQueuedMessage, + ToggleBurnMode, ToggleProfileSelector, }; /// Maximum number of lines to show for a collapsed terminal command preview. @@ -313,6 +314,11 @@ pub struct AcpThreadView { workspace: WeakEntity, project: Entity, thread_state: ThreadState, + permission_dropdown_handle: PopoverMenuHandle, + /// Tracks the selected granularity index for each tool call's permission dropdown. + /// The index corresponds to the position in the allow_options list. + /// Default is the last option (index pointing to "Only this time"). + selected_permission_granularity: HashMap, login: Option, recent_history_entries: Vec, history: Entity, @@ -502,6 +508,8 @@ impl AcpThreadView { workspace: workspace.clone(), project: project.clone(), entry_view_state, + permission_dropdown_handle: PopoverMenuHandle::default(), + selected_permission_granularity: HashMap::default(), thread_state: Self::initial_state( agent.clone(), resume_thread.clone(), @@ -3874,20 +3882,308 @@ impl AcpThreadView { .first_tool_awaiting_confirmation() .is_some_and(|call| call.id == tool_call_id) }); - let mut seen_kinds: ArrayVec = ArrayVec::new(); + + // For SwitchMode, use the old layout with all buttons + if kind == acp::ToolKind::SwitchMode { + return self.render_permission_buttons_legacy(options, entry_ix, tool_call_id, cx); + } + + // Get granularity options (all options except the old deny option which we no longer generate) + let granularity_options: Vec<_> = options + .iter() + .filter(|o| { + matches!( + o.kind, + acp::PermissionOptionKind::AllowOnce | acp::PermissionOptionKind::AllowAlways + ) + }) + .collect(); + + // Get the selected granularity index, defaulting to the last option ("Only this time") + let selected_index = self + .selected_permission_granularity + .get(&tool_call_id) + .copied() + .unwrap_or_else(|| granularity_options.len().saturating_sub(1)); + + // Get the selected option + let selected_option = granularity_options + .get(selected_index) + .or(granularity_options.last()) + .copied(); + + // The dropdown label should match the selected option + let dropdown_label: SharedString = selected_option + .map(|o| o.name.clone().into()) + .unwrap_or_else(|| "Only this time".into()); + + // Prepare data for button click handlers + let (allow_option_id, allow_option_kind, deny_option_id, deny_option_kind) = + if let Some(option) = selected_option { + let option_id_str = option.option_id.0.to_string(); + + // Transform option_id for allow: "always:tool" -> "always_allow:tool", "once" -> "allow" + let allow_id = if option_id_str == "once" { + "allow".to_string() + } else if let Some(rest) = option_id_str.strip_prefix("always:") { + format!("always_allow:{}", rest) + } else if let Some(rest) = option_id_str.strip_prefix("always_pattern:") { + format!("always_allow_pattern:{}", rest) + } else { + option_id_str.clone() + }; + + // Transform option_id for deny: "always:tool" -> "always_deny:tool", "once" -> "deny" + let deny_id = if option_id_str == "once" { + "deny".to_string() + } else if let Some(rest) = option_id_str.strip_prefix("always:") { + format!("always_deny:{}", rest) + } else if let Some(rest) = option_id_str.strip_prefix("always_pattern:") { + format!("always_deny_pattern:{}", rest) + } else { + option_id_str.replace("allow", "deny") + }; + + // Determine the kinds + let allow_kind = option.kind; + let deny_kind = match option.kind { + acp::PermissionOptionKind::AllowOnce => acp::PermissionOptionKind::RejectOnce, + acp::PermissionOptionKind::AllowAlways => { + acp::PermissionOptionKind::RejectAlways + } + other => other, + }; + + ( + acp::PermissionOptionId::new(allow_id), + allow_kind, + acp::PermissionOptionId::new(deny_id), + deny_kind, + ) + } else { + ( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + acp::PermissionOptionId::new("deny"), + acp::PermissionOptionKind::RejectOnce, + ) + }; div() .p_1() .border_t_1() .border_color(self.tool_card_border_color(cx)) .w_full() - .map(|this| { - if kind == acp::ToolKind::SwitchMode { - this.v_flex() - } else { - this.h_flex().justify_end().flex_wrap() - } + .h_flex() + .items_center() + .justify_between() + .gap_2() + .child( + // Left side: Allow and Deny buttons + h_flex() + .gap_1() + .child( + Button::new(("allow-btn", entry_ix), "Allow") + .icon(IconName::Check) + .icon_color(Color::Success) + .icon_position(IconPosition::Start) + .icon_size(IconSize::XSmall) + .label_size(LabelSize::Small) + .map(|btn| { + if is_first { + btn.key_binding( + KeyBinding::for_action_in( + &AllowOnce as &dyn Action, + &self.focus_handle, + cx, + ) + .map(|kb| kb.size(rems_from_px(10.))), + ) + } else { + btn + } + }) + .on_click(cx.listener({ + let tool_call_id = tool_call_id.clone(); + let option_id = allow_option_id; + let option_kind = allow_option_kind; + move |this, _, window, cx| { + this.authorize_tool_call( + tool_call_id.clone(), + option_id.clone(), + option_kind, + window, + cx, + ); + } + })), + ) + .child( + Button::new(("deny-btn", entry_ix), "Deny") + .icon(IconName::Close) + .icon_color(Color::Error) + .icon_position(IconPosition::Start) + .icon_size(IconSize::XSmall) + .label_size(LabelSize::Small) + .map(|btn| { + if is_first { + btn.key_binding( + KeyBinding::for_action_in( + &RejectOnce as &dyn Action, + &self.focus_handle, + cx, + ) + .map(|kb| kb.size(rems_from_px(10.))), + ) + } else { + btn + } + }) + .on_click(cx.listener({ + let tool_call_id = tool_call_id.clone(); + let option_id = deny_option_id; + let option_kind = deny_option_kind; + move |this, _, window, cx| { + this.authorize_tool_call( + tool_call_id.clone(), + option_id.clone(), + option_kind, + window, + cx, + ); + } + })), + ), + ) + .child( + // Right side: Granularity dropdown + self.render_permission_granularity_dropdown( + &granularity_options, + dropdown_label, + entry_ix, + tool_call_id, + selected_index, + is_first, + cx, + ), + ) + } + + fn render_permission_granularity_dropdown( + &self, + granularity_options: &[&acp::PermissionOption], + current_label: SharedString, + entry_ix: usize, + tool_call_id: acp::ToolCallId, + selected_index: usize, + is_first: bool, + cx: &Context, + ) -> impl IntoElement { + // Collect option info for the menu builder closure + // Each item is (index, display_name) + let menu_options: Vec<(usize, SharedString)> = granularity_options + .iter() + .enumerate() + .map(|(i, o)| (i, o.name.clone().into())) + .collect(); + + PopoverMenu::new(("permission-granularity", entry_ix)) + .with_handle(self.permission_dropdown_handle.clone()) + .trigger( + Button::new(("granularity-trigger", entry_ix), current_label) + .icon(IconName::ChevronDown) + .icon_position(IconPosition::End) + .icon_size(IconSize::XSmall) + .label_size(LabelSize::Small) + .style(ButtonStyle::Subtle) + .map(|btn| { + if is_first { + btn.key_binding( + KeyBinding::for_action_in( + &crate::OpenPermissionDropdown as &dyn Action, + &self.focus_handle, + cx, + ) + .map(|kb| kb.size(rems_from_px(10.))), + ) + } else { + btn + } + }), + ) + .menu(move |window, cx| { + let tool_call_id = tool_call_id.clone(); + let options = menu_options.clone(); + + Some(ContextMenu::build(window, cx, move |mut menu, _, _| { + for (index, display_name) in options.iter() { + let display_name = display_name.clone(); + let index = *index; + let tool_call_id_for_entry = tool_call_id.clone(); + let is_selected = index == selected_index; + + menu = menu.custom_entry( + { + let display_name = display_name.clone(); + move |_window, _cx| { + h_flex() + .w_full() + .justify_between() + .child( + Label::new(display_name.clone()).size(LabelSize::Small), + ) + .when(is_selected, |this| { + this.child( + Icon::new(IconName::Check) + .size(IconSize::Small) + .color(Color::Accent), + ) + }) + .into_any_element() + } + }, + { + let tool_call_id = tool_call_id_for_entry.clone(); + move |window, cx| { + window.dispatch_action( + SelectPermissionGranularity { + tool_call_id: tool_call_id.0.to_string(), + index, + } + .boxed_clone(), + cx, + ); + } + }, + ); + } + + menu + })) }) + } + + fn render_permission_buttons_legacy( + &self, + options: &[acp::PermissionOption], + entry_ix: usize, + tool_call_id: acp::ToolCallId, + cx: &Context, + ) -> Div { + let is_first = self.thread().is_some_and(|thread| { + thread + .read(cx) + .first_tool_awaiting_confirmation() + .is_some_and(|call| call.id == tool_call_id) + }); + let mut seen_kinds: ArrayVec = ArrayVec::new(); + + div() + .p_1() + .border_t_1() + .border_color(self.tool_card_border_color(cx)) + .w_full() + .v_flex() .gap_0p5() .children(options.iter().map(move |option| { let option_id = SharedString::from(option.option_id.0.clone()); @@ -5848,11 +6144,124 @@ impl AcpThreadView { } fn allow_once(&mut self, _: &AllowOnce, window: &mut Window, cx: &mut Context) { - self.authorize_pending_tool_call(acp::PermissionOptionKind::AllowOnce, window, cx); + self.authorize_pending_with_granularity(true, window, cx); } fn reject_once(&mut self, _: &RejectOnce, window: &mut Window, cx: &mut Context) { - self.authorize_pending_tool_call(acp::PermissionOptionKind::RejectOnce, window, cx); + self.authorize_pending_with_granularity(false, window, cx); + } + + fn authorize_pending_with_granularity( + &mut self, + is_allow: bool, + window: &mut Window, + cx: &mut Context, + ) -> Option<()> { + let thread = self.thread()?.read(cx); + let tool_call = thread.first_tool_awaiting_confirmation()?; + let ToolCallStatus::WaitingForConfirmation { options, .. } = &tool_call.status else { + return None; + }; + let tool_call_id = tool_call.id.clone(); + + // Get granularity options (all options except old deny option) + let granularity_options: Vec<_> = options + .iter() + .filter(|o| { + matches!( + o.kind, + acp::PermissionOptionKind::AllowOnce | acp::PermissionOptionKind::AllowAlways + ) + }) + .collect(); + + // Get selected index, defaulting to last option ("Only this time") + let selected_index = self + .selected_permission_granularity + .get(&tool_call_id) + .copied() + .unwrap_or_else(|| granularity_options.len().saturating_sub(1)); + + let selected_option = granularity_options + .get(selected_index) + .or(granularity_options.last()) + .copied()?; + + let option_id_str = selected_option.option_id.0.to_string(); + + // Transform option_id based on allow/deny + let (final_option_id, final_option_kind) = if is_allow { + let allow_id = if option_id_str == "once" { + "allow".to_string() + } else if let Some(rest) = option_id_str.strip_prefix("always:") { + format!("always_allow:{}", rest) + } else if let Some(rest) = option_id_str.strip_prefix("always_pattern:") { + format!("always_allow_pattern:{}", rest) + } else { + option_id_str + }; + (acp::PermissionOptionId::new(allow_id), selected_option.kind) + } else { + let deny_id = if option_id_str == "once" { + "deny".to_string() + } else if let Some(rest) = option_id_str.strip_prefix("always:") { + format!("always_deny:{}", rest) + } else if let Some(rest) = option_id_str.strip_prefix("always_pattern:") { + format!("always_deny_pattern:{}", rest) + } else { + option_id_str.replace("allow", "deny") + }; + let deny_kind = match selected_option.kind { + acp::PermissionOptionKind::AllowOnce => acp::PermissionOptionKind::RejectOnce, + acp::PermissionOptionKind::AllowAlways => acp::PermissionOptionKind::RejectAlways, + other => other, + }; + (acp::PermissionOptionId::new(deny_id), deny_kind) + }; + + self.authorize_tool_call(tool_call_id, final_option_id, final_option_kind, window, cx); + + Some(()) + } + + fn open_permission_dropdown( + &mut self, + _: &crate::OpenPermissionDropdown, + window: &mut Window, + cx: &mut Context, + ) { + self.permission_dropdown_handle.toggle(window, cx); + } + + fn handle_select_permission_granularity( + &mut self, + action: &SelectPermissionGranularity, + _window: &mut Window, + cx: &mut Context, + ) { + let tool_call_id = acp::ToolCallId::new(action.tool_call_id.clone()); + self.selected_permission_granularity + .insert(tool_call_id, action.index); + cx.notify(); + } + + fn handle_authorize_tool_call( + &mut self, + action: &AuthorizeToolCall, + window: &mut Window, + cx: &mut Context, + ) { + let tool_call_id = acp::ToolCallId::new(action.tool_call_id.clone()); + let option_id = acp::PermissionOptionId::new(action.option_id.clone()); + let option_kind = match action.option_kind.as_str() { + "AllowOnce" => acp::PermissionOptionKind::AllowOnce, + "AllowAlways" => acp::PermissionOptionKind::AllowAlways, + "RejectOnce" => acp::PermissionOptionKind::RejectOnce, + "RejectAlways" => acp::PermissionOptionKind::RejectAlways, + _ => acp::PermissionOptionKind::AllowOnce, + }; + + self.authorize_tool_call(tool_call_id, option_id, option_kind, window, cx); } fn authorize_pending_tool_call( @@ -7590,6 +7999,9 @@ impl Render for AcpThreadView { .on_action(cx.listener(Self::allow_always)) .on_action(cx.listener(Self::allow_once)) .on_action(cx.listener(Self::reject_once)) + .on_action(cx.listener(Self::handle_authorize_tool_call)) + .on_action(cx.listener(Self::handle_select_permission_granularity)) + .on_action(cx.listener(Self::open_permission_dropdown)) .on_action(cx.listener(|this, _: &SendNextQueuedMessage, window, cx| { this.send_queued_message_at_index(0, true, window, cx); })) @@ -7927,6 +8339,7 @@ pub(crate) mod tests { AgentSessionList, AgentSessionListRequest, AgentSessionListResponse, StubAgentConnection, }; use action_log::ActionLog; + use agent::ToolPermissionContext; use agent_client_protocol::SessionId; use editor::MultiBufferOffset; use fs::FakeFs; @@ -9443,4 +9856,804 @@ pub(crate) mod tests { assert_eq!(text, expected_txt); }) } + + #[gpui::test] + async fn test_tool_permission_buttons_terminal_with_pattern(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("terminal-1"); + let tool_call = acp::ToolCall::new(tool_call_id.clone(), "Run `cargo build --release`") + .kind(acp::ToolKind::Edit); + + let permission_options = ToolPermissionContext::new("terminal", "cargo build --release") + .build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options, + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + + // Disable notifications to avoid popup windows + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Run cargo build", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Verify the tool call is in WaitingForConfirmation state with the expected options + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + + let tool_call = thread.entries().iter().find_map(|entry| { + if let acp_thread::AgentThreadEntry::ToolCall(call) = entry { + Some(call) + } else { + None + } + }); + + assert!(tool_call.is_some(), "Expected a tool call entry"); + let tool_call = tool_call.unwrap(); + + // Verify it's waiting for confirmation + assert!( + matches!( + tool_call.status, + acp_thread::ToolCallStatus::WaitingForConfirmation { .. } + ), + "Expected WaitingForConfirmation status, got {:?}", + tool_call.status + ); + + // Verify the options count (granularity options only, no separate Deny option) + if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = + &tool_call.status + { + assert_eq!( + options.len(), + 3, + "Expected 3 permission options (granularity only)" + ); + + // Verify specific button labels (now using neutral names) + let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + assert!( + labels.contains(&"Always for terminal"), + "Missing 'Always for terminal' option" + ); + assert!( + labels.contains(&"Always for `cargo` commands"), + "Missing pattern option" + ); + assert!( + labels.contains(&"Only this time"), + "Missing 'Only this time' option" + ); + } + }); + } + + #[gpui::test] + async fn test_tool_permission_buttons_edit_file_with_path_pattern(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("edit-file-1"); + let tool_call = acp::ToolCall::new(tool_call_id.clone(), "Edit `src/main.rs`") + .kind(acp::ToolKind::Edit); + + let permission_options = + ToolPermissionContext::new("edit_file", "src/main.rs").build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options, + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + + // Disable notifications + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Edit the main file", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Verify the options + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + + let tool_call = thread.entries().iter().find_map(|entry| { + if let acp_thread::AgentThreadEntry::ToolCall(call) = entry { + Some(call) + } else { + None + } + }); + + assert!(tool_call.is_some(), "Expected a tool call entry"); + let tool_call = tool_call.unwrap(); + + if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = + &tool_call.status + { + let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + assert!( + labels.contains(&"Always for edit file"), + "Missing 'Always for edit file' option" + ); + assert!( + labels.contains(&"Always for `src/`"), + "Missing path pattern option" + ); + } else { + panic!("Expected WaitingForConfirmation status"); + } + }); + } + + #[gpui::test] + async fn test_tool_permission_buttons_fetch_with_domain_pattern(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("fetch-1"); + let tool_call = acp::ToolCall::new(tool_call_id.clone(), "Fetch `https://docs.rs/gpui`") + .kind(acp::ToolKind::Fetch); + + let permission_options = + ToolPermissionContext::new("fetch", "https://docs.rs/gpui").build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options, + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + + // Disable notifications + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Fetch the docs", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Verify the options + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + + let tool_call = thread.entries().iter().find_map(|entry| { + if let acp_thread::AgentThreadEntry::ToolCall(call) = entry { + Some(call) + } else { + None + } + }); + + assert!(tool_call.is_some(), "Expected a tool call entry"); + let tool_call = tool_call.unwrap(); + + if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = + &tool_call.status + { + let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + assert!( + labels.contains(&"Always for fetch"), + "Missing 'Always for fetch' option" + ); + assert!( + labels.contains(&"Always for `docs.rs`"), + "Missing domain pattern option" + ); + } else { + panic!("Expected WaitingForConfirmation status"); + } + }); + } + + #[gpui::test] + async fn test_tool_permission_buttons_without_pattern(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("terminal-no-pattern-1"); + let tool_call = acp::ToolCall::new(tool_call_id.clone(), "Run `./deploy.sh --production`") + .kind(acp::ToolKind::Edit); + + // No pattern button since ./deploy.sh doesn't match the alphanumeric pattern + let permission_options = ToolPermissionContext::new("terminal", "./deploy.sh --production") + .build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options, + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + + // Disable notifications + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Run the deploy script", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Verify only 2 options (no pattern button when command doesn't match pattern) + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + + let tool_call = thread.entries().iter().find_map(|entry| { + if let acp_thread::AgentThreadEntry::ToolCall(call) = entry { + Some(call) + } else { + None + } + }); + + assert!(tool_call.is_some(), "Expected a tool call entry"); + let tool_call = tool_call.unwrap(); + + if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = + &tool_call.status + { + assert_eq!( + options.len(), + 2, + "Expected 2 permission options (no pattern option)" + ); + + let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + assert!( + labels.contains(&"Always for terminal"), + "Missing 'Always for terminal' option" + ); + assert!( + labels.contains(&"Only this time"), + "Missing 'Only this time' option" + ); + // Should NOT contain a pattern option + assert!( + !labels.iter().any(|l| l.contains("commands")), + "Should not have pattern option" + ); + } else { + panic!("Expected WaitingForConfirmation status"); + } + }); + } + + #[gpui::test] + async fn test_authorize_tool_call_action_triggers_authorization(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("action-test-1"); + let tool_call = + acp::ToolCall::new(tool_call_id.clone(), "Run `cargo test`").kind(acp::ToolKind::Edit); + + let permission_options = + ToolPermissionContext::new("terminal", "cargo test").build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options, + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + add_to_workspace(thread_view.clone(), cx); + + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Run tests", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Verify tool call is waiting for confirmation + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + let tool_call = thread.first_tool_awaiting_confirmation(); + assert!( + tool_call.is_some(), + "Expected a tool call waiting for confirmation" + ); + }); + + // Dispatch the AuthorizeToolCall action (simulating dropdown menu selection) + thread_view.update_in(cx, |_, window, cx| { + window.dispatch_action( + crate::AuthorizeToolCall { + tool_call_id: "action-test-1".to_string(), + option_id: "allow".to_string(), + option_kind: "AllowOnce".to_string(), + } + .boxed_clone(), + cx, + ); + }); + + cx.run_until_parked(); + + // Verify tool call is no longer waiting for confirmation (was authorized) + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + let tool_call = thread.first_tool_awaiting_confirmation(); + assert!( + tool_call.is_none(), + "Tool call should no longer be waiting for confirmation after AuthorizeToolCall action" + ); + }); + } + + #[gpui::test] + async fn test_authorize_tool_call_action_with_pattern_option(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("pattern-action-test-1"); + let tool_call = + acp::ToolCall::new(tool_call_id.clone(), "Run `npm install`").kind(acp::ToolKind::Edit); + + let permission_options = + ToolPermissionContext::new("terminal", "npm install").build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options.clone(), + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + add_to_workspace(thread_view.clone(), cx); + + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Install dependencies", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Find the pattern option ID + let pattern_option = permission_options + .iter() + .find(|o| o.option_id.0.starts_with("always_pattern:")) + .expect("Should have a pattern option for npm command"); + + // Dispatch action with the pattern option (simulating "Always allow `npm` commands") + thread_view.update_in(cx, |_, window, cx| { + window.dispatch_action( + crate::AuthorizeToolCall { + tool_call_id: "pattern-action-test-1".to_string(), + option_id: pattern_option.option_id.0.to_string(), + option_kind: "AllowAlways".to_string(), + } + .boxed_clone(), + cx, + ); + }); + + cx.run_until_parked(); + + // Verify tool call was authorized + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + let tool_call = thread.first_tool_awaiting_confirmation(); + assert!( + tool_call.is_none(), + "Tool call should be authorized after selecting pattern option" + ); + }); + } + + #[gpui::test] + async fn test_granularity_selection_updates_state(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("granularity-test-1"); + let tool_call = + acp::ToolCall::new(tool_call_id.clone(), "Run `cargo build`").kind(acp::ToolKind::Edit); + + let permission_options = + ToolPermissionContext::new("terminal", "cargo build").build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options.clone(), + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + add_to_workspace(thread_view.clone(), cx); + + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Build the project", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Verify default granularity is the last option (index 2 = "Only this time") + thread_view.read_with(cx, |thread_view, _cx| { + let selected = thread_view + .selected_permission_granularity + .get(&tool_call_id); + assert!( + selected.is_none(), + "Should have no selection initially (defaults to last)" + ); + }); + + // Select the first option (index 0 = "Always for terminal") + thread_view.update_in(cx, |_, window, cx| { + window.dispatch_action( + crate::SelectPermissionGranularity { + tool_call_id: "granularity-test-1".to_string(), + index: 0, + } + .boxed_clone(), + cx, + ); + }); + + cx.run_until_parked(); + + // Verify the selection was updated + thread_view.read_with(cx, |thread_view, _cx| { + let selected = thread_view + .selected_permission_granularity + .get(&tool_call_id); + assert_eq!(selected, Some(&0), "Should have selected index 0"); + }); + } + + #[gpui::test] + async fn test_allow_button_uses_selected_granularity(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("allow-granularity-test-1"); + let tool_call = + acp::ToolCall::new(tool_call_id.clone(), "Run `npm install`").kind(acp::ToolKind::Edit); + + let permission_options = + ToolPermissionContext::new("terminal", "npm install").build_permission_options(); + + // Verify we have the expected options + assert_eq!(permission_options.len(), 3); + assert!( + permission_options[0] + .option_id + .0 + .contains("always:terminal") + ); + assert!( + permission_options[1] + .option_id + .0 + .contains("always_pattern:terminal") + ); + assert_eq!(permission_options[2].option_id.0.as_ref(), "once"); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options.clone(), + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + add_to_workspace(thread_view.clone(), cx); + + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Install dependencies", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Select the pattern option (index 1 = "Always for `npm` commands") + thread_view.update_in(cx, |_, window, cx| { + window.dispatch_action( + crate::SelectPermissionGranularity { + tool_call_id: "allow-granularity-test-1".to_string(), + index: 1, + } + .boxed_clone(), + cx, + ); + }); + + cx.run_until_parked(); + + // Simulate clicking the Allow button by dispatching AllowOnce action + // which should use the selected granularity + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.allow_once(&AllowOnce, window, cx); + }); + + cx.run_until_parked(); + + // Verify tool call was authorized + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + let tool_call = thread.first_tool_awaiting_confirmation(); + assert!( + tool_call.is_none(), + "Tool call should be authorized after Allow with pattern granularity" + ); + }); + } + + #[gpui::test] + async fn test_deny_button_uses_selected_granularity(cx: &mut TestAppContext) { + init_test(cx); + + let tool_call_id = acp::ToolCallId::new("deny-granularity-test-1"); + let tool_call = + acp::ToolCall::new(tool_call_id.clone(), "Run `git push`").kind(acp::ToolKind::Edit); + + let permission_options = + ToolPermissionContext::new("terminal", "git push").build_permission_options(); + + let connection = + StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( + tool_call_id.clone(), + permission_options.clone(), + )])); + + connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); + + let (thread_view, cx) = setup_thread_view(StubAgentServer::new(connection), cx).await; + add_to_workspace(thread_view.clone(), cx); + + cx.update(|_window, cx| { + AgentSettings::override_global( + AgentSettings { + notify_when_agent_waiting: NotifyWhenAgentWaiting::Never, + ..AgentSettings::get_global(cx).clone() + }, + cx, + ); + }); + + let message_editor = cx.read(|cx| thread_view.read(cx).message_editor.clone()); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Push changes", window, cx); + }); + + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.send(window, cx); + }); + + cx.run_until_parked(); + + // Use default granularity (last option = "Only this time") + // Simulate clicking the Deny button + thread_view.update_in(cx, |thread_view, window, cx| { + thread_view.reject_once(&RejectOnce, window, cx); + }); + + cx.run_until_parked(); + + // Verify tool call was rejected (no longer waiting for confirmation) + thread_view.read_with(cx, |thread_view, cx| { + let thread = thread_view.thread().expect("Thread should exist"); + let thread = thread.read(cx); + let tool_call = thread.first_tool_awaiting_confirmation(); + assert!( + tool_call.is_none(), + "Tool call should be rejected after Deny" + ); + }); + } + + #[gpui::test] + async fn test_option_id_transformation_for_allow() { + // Test the option_id transformation logic directly + // "once" -> "allow" + // "always:terminal" -> "always_allow:terminal" + // "always_pattern:terminal:^cargo\s" -> "always_allow_pattern:terminal:^cargo\s" + + let test_cases = vec![ + ("once", "allow"), + ("always:terminal", "always_allow:terminal"), + ( + "always_pattern:terminal:^cargo\\s", + "always_allow_pattern:terminal:^cargo\\s", + ), + ("always:fetch", "always_allow:fetch"), + ( + "always_pattern:fetch:^https?://docs\\.rs", + "always_allow_pattern:fetch:^https?://docs\\.rs", + ), + ]; + + for (input, expected) in test_cases { + let result = if input == "once" { + "allow".to_string() + } else if let Some(rest) = input.strip_prefix("always:") { + format!("always_allow:{}", rest) + } else if let Some(rest) = input.strip_prefix("always_pattern:") { + format!("always_allow_pattern:{}", rest) + } else { + input.to_string() + }; + assert_eq!(result, expected, "Failed for input: {}", input); + } + } + + #[gpui::test] + async fn test_option_id_transformation_for_deny() { + // Test the option_id transformation logic for deny + // "once" -> "deny" + // "always:terminal" -> "always_deny:terminal" + // "always_pattern:terminal:^cargo\s" -> "always_deny_pattern:terminal:^cargo\s" + + let test_cases = vec![ + ("once", "deny"), + ("always:terminal", "always_deny:terminal"), + ( + "always_pattern:terminal:^cargo\\s", + "always_deny_pattern:terminal:^cargo\\s", + ), + ("always:fetch", "always_deny:fetch"), + ( + "always_pattern:fetch:^https?://docs\\.rs", + "always_deny_pattern:fetch:^https?://docs\\.rs", + ), + ]; + + for (input, expected) in test_cases { + let result = if input == "once" { + "deny".to_string() + } else if let Some(rest) = input.strip_prefix("always:") { + format!("always_deny:{}", rest) + } else if let Some(rest) = input.strip_prefix("always_pattern:") { + format!("always_deny_pattern:{}", rest) + } else { + input.replace("allow", "deny") + }; + assert_eq!(result, expected, "Failed for input: {}", input); + } + } } diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index d13257ce5fc0d58e96b8bfd04c81183514b498ce..d46e5b11d265fa0b2732638e3d141634c11934ab 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -129,9 +129,37 @@ actions!( SendNextQueuedMessage, /// Clears all messages from the queue. ClearMessageQueue, + /// Opens the permission granularity dropdown for the current tool call. + OpenPermissionDropdown, ] ); +/// Action to authorize a tool call with a specific permission option. +/// This is used by the permission granularity dropdown to authorize tool calls. +#[derive(Clone, PartialEq, Deserialize, JsonSchema, Action)] +#[action(namespace = agent)] +#[serde(deny_unknown_fields)] +pub struct AuthorizeToolCall { + /// The tool call ID to authorize. + pub tool_call_id: String, + /// The permission option ID to use. + pub option_id: String, + /// The kind of permission option (serialized as string). + pub option_kind: String, +} + +/// Action to select a permission granularity option from the dropdown. +/// This updates the selected granularity without triggering authorization. +#[derive(Clone, PartialEq, Deserialize, JsonSchema, Action)] +#[action(namespace = agent)] +#[serde(deny_unknown_fields)] +pub struct SelectPermissionGranularity { + /// The tool call ID for which to select the granularity. + pub tool_call_id: String, + /// The index of the selected granularity option. + pub index: usize, +} + /// Creates a new conversation thread, optionally based on an existing thread. #[derive(Default, Clone, PartialEq, Deserialize, JsonSchema, Action)] #[action(namespace = agent)] diff --git a/crates/settings/src/settings_content/agent.rs b/crates/settings/src/settings_content/agent.rs index c152e0545eac1af9bede1f63b4329176b672aadc..af9151672d5cd165c822a36cf20a85d6dd32e299 100644 --- a/crates/settings/src/settings_content/agent.rs +++ b/crates/settings/src/settings_content/agent.rs @@ -201,6 +201,45 @@ impl AgentSettingsContent { pub fn remove_favorite_model(&mut self, model: &LanguageModelSelection) { self.favorite_models.retain(|m| m != model); } + + pub fn set_tool_default_mode(&mut self, tool_id: &str, mode: ToolPermissionMode) { + let tool_permissions = self.tool_permissions.get_or_insert_default(); + let tool_rules = tool_permissions + .tools + .entry(Arc::from(tool_id)) + .or_default(); + tool_rules.default_mode = Some(mode); + } + + pub fn add_tool_allow_pattern(&mut self, tool_name: &str, pattern: String) { + let tool_permissions = self.tool_permissions.get_or_insert_default(); + let tool_rules = tool_permissions + .tools + .entry(Arc::from(tool_name)) + .or_default(); + let always_allow = tool_rules.always_allow.get_or_insert_default(); + if !always_allow.0.iter().any(|r| r.pattern == pattern) { + always_allow.0.push(ToolRegexRule { + pattern, + case_sensitive: None, + }); + } + } + + pub fn add_tool_deny_pattern(&mut self, tool_name: &str, pattern: String) { + let tool_permissions = self.tool_permissions.get_or_insert_default(); + let tool_rules = tool_permissions + .tools + .entry(Arc::from(tool_name)) + .or_default(); + let always_deny = tool_rules.always_deny.get_or_insert_default(); + if !always_deny.0.iter().any(|r| r.pattern == pattern) { + always_deny.0.push(ToolRegexRule { + pattern, + case_sensitive: None, + }); + } + } } #[with_fallible_options] @@ -513,9 +552,10 @@ pub struct ToolRulesContent { } #[with_fallible_options] -#[derive(Clone, Debug, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)] +#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize, JsonSchema, MergeFrom)] pub struct ToolRegexRule { /// The regex pattern to match. + #[serde(default)] pub pattern: String, /// Whether the regex is case-sensitive. @@ -536,3 +576,181 @@ pub enum ToolPermissionMode { #[default] Confirm, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_set_tool_default_mode_creates_structure() { + let mut settings = AgentSettingsContent::default(); + assert!(settings.tool_permissions.is_none()); + + settings.set_tool_default_mode("terminal", ToolPermissionMode::Allow); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + assert_eq!(terminal_rules.default_mode, Some(ToolPermissionMode::Allow)); + } + + #[test] + fn test_set_tool_default_mode_updates_existing() { + let mut settings = AgentSettingsContent::default(); + + settings.set_tool_default_mode("terminal", ToolPermissionMode::Confirm); + settings.set_tool_default_mode("terminal", ToolPermissionMode::Allow); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + assert_eq!(terminal_rules.default_mode, Some(ToolPermissionMode::Allow)); + } + + #[test] + fn test_set_tool_default_mode_for_mcp_tool() { + let mut settings = AgentSettingsContent::default(); + + settings.set_tool_default_mode("mcp:github:create_issue", ToolPermissionMode::Allow); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let mcp_rules = tool_permissions + .tools + .get("mcp:github:create_issue") + .unwrap(); + assert_eq!(mcp_rules.default_mode, Some(ToolPermissionMode::Allow)); + } + + #[test] + fn test_add_tool_allow_pattern_creates_structure() { + let mut settings = AgentSettingsContent::default(); + assert!(settings.tool_permissions.is_none()); + + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + let always_allow = terminal_rules.always_allow.as_ref().unwrap(); + assert_eq!(always_allow.0.len(), 1); + assert_eq!(always_allow.0[0].pattern, "^cargo\\s"); + } + + #[test] + fn test_add_tool_allow_pattern_appends_to_existing() { + let mut settings = AgentSettingsContent::default(); + + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + settings.add_tool_allow_pattern("terminal", "^npm\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + let always_allow = terminal_rules.always_allow.as_ref().unwrap(); + assert_eq!(always_allow.0.len(), 2); + assert_eq!(always_allow.0[0].pattern, "^cargo\\s"); + assert_eq!(always_allow.0[1].pattern, "^npm\\s"); + } + + #[test] + fn test_add_tool_allow_pattern_does_not_duplicate() { + let mut settings = AgentSettingsContent::default(); + + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + let always_allow = terminal_rules.always_allow.as_ref().unwrap(); + assert_eq!( + always_allow.0.len(), + 1, + "Duplicate patterns should not be added" + ); + } + + #[test] + fn test_add_tool_allow_pattern_for_different_tools() { + let mut settings = AgentSettingsContent::default(); + + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + settings.add_tool_allow_pattern("fetch", "^https?://github\\.com".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + assert_eq!( + terminal_rules.always_allow.as_ref().unwrap().0[0].pattern, + "^cargo\\s" + ); + + let fetch_rules = tool_permissions.tools.get("fetch").unwrap(); + assert_eq!( + fetch_rules.always_allow.as_ref().unwrap().0[0].pattern, + "^https?://github\\.com" + ); + } + + #[test] + fn test_add_tool_deny_pattern_creates_structure() { + let mut settings = AgentSettingsContent::default(); + assert!(settings.tool_permissions.is_none()); + + settings.add_tool_deny_pattern("terminal", "^rm\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + let always_deny = terminal_rules.always_deny.as_ref().unwrap(); + assert_eq!(always_deny.0.len(), 1); + assert_eq!(always_deny.0[0].pattern, "^rm\\s"); + } + + #[test] + fn test_add_tool_deny_pattern_appends_to_existing() { + let mut settings = AgentSettingsContent::default(); + + settings.add_tool_deny_pattern("terminal", "^rm\\s".to_string()); + settings.add_tool_deny_pattern("terminal", "^sudo\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + let always_deny = terminal_rules.always_deny.as_ref().unwrap(); + assert_eq!(always_deny.0.len(), 2); + assert_eq!(always_deny.0[0].pattern, "^rm\\s"); + assert_eq!(always_deny.0[1].pattern, "^sudo\\s"); + } + + #[test] + fn test_add_tool_deny_pattern_does_not_duplicate() { + let mut settings = AgentSettingsContent::default(); + + settings.add_tool_deny_pattern("terminal", "^rm\\s".to_string()); + settings.add_tool_deny_pattern("terminal", "^rm\\s".to_string()); + settings.add_tool_deny_pattern("terminal", "^rm\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + let always_deny = terminal_rules.always_deny.as_ref().unwrap(); + assert_eq!( + always_deny.0.len(), + 1, + "Duplicate patterns should not be added" + ); + } + + #[test] + fn test_add_tool_deny_and_allow_patterns_separate() { + let mut settings = AgentSettingsContent::default(); + + settings.add_tool_allow_pattern("terminal", "^cargo\\s".to_string()); + settings.add_tool_deny_pattern("terminal", "^rm\\s".to_string()); + + let tool_permissions = settings.tool_permissions.as_ref().unwrap(); + let terminal_rules = tool_permissions.tools.get("terminal").unwrap(); + + let always_allow = terminal_rules.always_allow.as_ref().unwrap(); + assert_eq!(always_allow.0.len(), 1); + assert_eq!(always_allow.0[0].pattern, "^cargo\\s"); + + let always_deny = terminal_rules.always_deny.as_ref().unwrap(); + assert_eq!(always_deny.0.len(), 1); + assert_eq!(always_deny.0[0].pattern, "^rm\\s"); + } +} diff --git a/crates/settings/src/settings_file.rs b/crates/settings/src/settings_file.rs index df6aa8bbb4de45ae8b3f037396ed38faf20a0873..504dd4da241fc246f54e077c461afc5dbcc92ebb 100644 --- a/crates/settings/src/settings_file.rs +++ b/crates/settings/src/settings_file.rs @@ -7,6 +7,32 @@ use std::{path::PathBuf, sync::Arc, time::Duration}; pub const EMPTY_THEME_NAME: &str = "empty-theme"; +/// Settings for visual tests that use proper fonts instead of Courier. +/// Uses Helvetica Neue for UI (sans-serif) and Menlo for code (monospace), +/// which are available on all macOS systems. +#[cfg(any(test, feature = "test-support"))] +pub fn visual_test_settings() -> String { + let mut value = + crate::parse_json_with_comments::(crate::default_settings().as_ref()) + .unwrap(); + util::merge_non_null_json_value_into( + serde_json::json!({ + "ui_font_family": ".SystemUIFont", + "ui_font_features": {}, + "ui_font_size": 14, + "ui_font_fallback": [], + "buffer_font_family": "Menlo", + "buffer_font_features": {}, + "buffer_font_size": 14, + "buffer_font_fallbacks": [], + "theme": EMPTY_THEME_NAME, + }), + &mut value, + ); + value.as_object_mut().unwrap().remove("languages"); + serde_json::to_string(&value).unwrap() +} + #[cfg(any(test, feature = "test-support"))] pub fn test_settings() -> String { let mut value =