From f586129d2c25dc107ac1fdb86f613a2fdbc42ea9 Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Fri, 20 Mar 2026 11:55:49 -0700 Subject: [PATCH] Terminal permissions: Per-command pipeline UI (#49547) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds a new permission UI for terminal pipeline commands (e.g. `cargo test | tail`) that lets users selectively always-allow individual commands in the pipeline, rather than only offering a blanket always-allow for the first command. ## Screenshot Screenshot 2026-03-18 at 3 27 48 PM Release notes: - The terminal permissions UI now allows you to select individual subcommands independently. --------- Co-authored-by: Danilo Leal Co-authored-by: Ben Brandt --- crates/acp_thread/src/acp_thread.rs | 66 ++- crates/acp_thread/src/connection.rs | 29 + crates/agent/src/agent.rs | 7 +- crates/agent/src/pattern_extraction.rs | 76 ++- crates/agent/src/tests/mod.rs | 108 +++- crates/agent/src/thread.rs | 308 ++++++---- crates/agent/src/tools/copy_path_tool.rs | 4 +- .../agent/src/tools/create_directory_tool.rs | 4 +- crates/agent/src/tools/delete_path_tool.rs | 4 +- crates/agent/src/tools/edit_file_tool.rs | 2 +- crates/agent/src/tools/list_directory_tool.rs | 2 +- crates/agent/src/tools/move_path_tool.rs | 4 +- crates/agent/src/tools/read_file_tool.rs | 4 +- .../src/tools/restore_file_from_disk_tool.rs | 4 +- crates/agent/src/tools/save_file_tool.rs | 6 +- .../src/tools/streaming_edit_file_tool.rs | 2 +- crates/agent_servers/src/acp.rs | 2 +- crates/agent_servers/src/e2e_tests.rs | 2 +- crates/agent_ui/src/agent_ui.rs | 23 + crates/agent_ui/src/conversation_view.rs | 246 ++++++-- .../src/conversation_view/thread_view.rs | 560 +++++++++++++++--- 21 files changed, 1125 insertions(+), 338 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index f3ebcc72691d41c3a19d60eaa1ca44e21c3d94bc..a16a4a7895b28b281d5a1d8d883206252b33c412 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -494,6 +494,58 @@ impl From<&ResolvedLocation> for AgentLocation { } } +#[derive(Debug, Clone)] +pub enum SelectedPermissionParams { + Terminal { patterns: Vec }, +} + +#[derive(Debug)] +pub struct SelectedPermissionOutcome { + pub option_id: acp::PermissionOptionId, + pub params: Option, +} + +impl SelectedPermissionOutcome { + pub fn new(option_id: acp::PermissionOptionId) -> Self { + Self { + option_id, + params: None, + } + } + + pub fn params(mut self, params: Option) -> Self { + self.params = params; + self + } +} + +impl From for SelectedPermissionOutcome { + fn from(option_id: acp::PermissionOptionId) -> Self { + Self::new(option_id) + } +} + +impl From for acp::SelectedPermissionOutcome { + fn from(value: SelectedPermissionOutcome) -> Self { + Self::new(value.option_id) + } +} + +#[derive(Debug)] +pub enum RequestPermissionOutcome { + Cancelled, + Selected(SelectedPermissionOutcome), +} + +impl From for acp::RequestPermissionOutcome { + fn from(value: RequestPermissionOutcome) -> Self { + match value { + RequestPermissionOutcome::Cancelled => Self::Cancelled, + RequestPermissionOutcome::Selected(outcome) => Self::Selected(outcome.into()), + } + } +} + #[derive(Debug)] pub enum ToolCallStatus { /// The tool call hasn't started running yet, but we start showing it to @@ -502,7 +554,7 @@ pub enum ToolCallStatus { /// The tool call is waiting for confirmation from the user. WaitingForConfirmation { options: PermissionOptions, - respond_tx: oneshot::Sender, + respond_tx: oneshot::Sender, }, /// The tool call is currently running. InProgress, @@ -1929,7 +1981,7 @@ impl AcpThread { tool_call: acp::ToolCallUpdate, options: PermissionOptions, cx: &mut Context, - ) -> Result> { + ) -> Result> { let (tx, rx) = oneshot::channel(); let status = ToolCallStatus::WaitingForConfirmation { @@ -1945,10 +1997,8 @@ impl AcpThread { Ok(cx.spawn(async move |this, cx| { let outcome = match rx.await { - Ok(option) => acp::RequestPermissionOutcome::Selected( - acp::SelectedPermissionOutcome::new(option), - ), - Err(oneshot::Canceled) => acp::RequestPermissionOutcome::Cancelled, + Ok(outcome) => RequestPermissionOutcome::Selected(outcome), + Err(oneshot::Canceled) => RequestPermissionOutcome::Cancelled, }; this.update(cx, |_this, cx| { cx.emit(AcpThreadEvent::ToolAuthorizationReceived(tool_call_id)) @@ -1961,7 +2011,7 @@ impl AcpThread { pub fn authorize_tool_call( &mut self, id: acp::ToolCallId, - option_id: acp::PermissionOptionId, + outcome: SelectedPermissionOutcome, option_kind: acp::PermissionOptionKind, cx: &mut Context, ) { @@ -1982,7 +2032,7 @@ impl AcpThread { let curr_status = mem::replace(&mut call.status, new_status); if let ToolCallStatus::WaitingForConfirmation { respond_tx, .. } = curr_status { - respond_tx.send(option_id).log_err(); + respond_tx.send(outcome).log_err(); } else if cfg!(debug_assertions) { panic!("tried to authorize an already authorized tool call"); } diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 56b3531e6fd7281541c6cded995a6bc43376e393..fd47c77e7d6ff7245dd4e98f1b87cce80e1cbba6 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -470,6 +470,7 @@ impl AgentModelList { pub struct PermissionOptionChoice { pub allow: acp::PermissionOption, pub deny: acp::PermissionOption, + pub sub_patterns: Vec, } impl PermissionOptionChoice { @@ -478,10 +479,26 @@ impl PermissionOptionChoice { } } +/// Pairs a tool's permission pattern with its display name +/// +/// For example, a pattern of `^cargo\\s+build(\\s|$)` would display as `cargo +/// build`. It's handy to keep these together rather than trying to derive +/// one from the other. +#[derive(Debug, Clone, PartialEq)] +pub struct PermissionPattern { + pub pattern: String, + pub display_name: String, +} + #[derive(Debug, Clone)] pub enum PermissionOptions { Flat(Vec), Dropdown(Vec), + DropdownWithPatterns { + choices: Vec, + patterns: Vec, + tool_name: String, + }, } impl PermissionOptions { @@ -489,6 +506,7 @@ impl PermissionOptions { match self { PermissionOptions::Flat(options) => options.is_empty(), PermissionOptions::Dropdown(options) => options.is_empty(), + PermissionOptions::DropdownWithPatterns { choices, .. } => choices.is_empty(), } } @@ -507,6 +525,17 @@ impl PermissionOptions { None } }), + PermissionOptions::DropdownWithPatterns { choices, .. } => { + choices.iter().find_map(|choice| { + if choice.allow.kind == kind { + Some(&choice.allow) + } else if choice.deny.kind == kind { + Some(&choice.deny) + } else { + None + } + }) + } } } diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index 5a3b1710d18f16805d8ca10ff7cbd5708de88316..bf756d433f0ca3812b5c5419dfef82e4d9760093 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -1198,12 +1198,11 @@ impl NativeAgentConnection { thread.request_tool_call_authorization(tool_call, options, cx) })??; cx.background_spawn(async move { - if let acp::RequestPermissionOutcome::Selected( - acp::SelectedPermissionOutcome { option_id, .. }, - ) = outcome_task.await + if let acp_thread::RequestPermissionOutcome::Selected(outcome) = + outcome_task.await { response - .send(option_id) + .send(outcome) .map(|_| anyhow!("authorization receiver was dropped")) .log_err(); } diff --git a/crates/agent/src/pattern_extraction.rs b/crates/agent/src/pattern_extraction.rs index 19f00fea14156131cc0062921aa3ce334705e89a..7015d69827d7286a1564ce0528ce4627059c49fb 100644 --- a/crates/agent/src/pattern_extraction.rs +++ b/crates/agent/src/pattern_extraction.rs @@ -1,4 +1,5 @@ -use shell_command_parser::extract_terminal_command_prefix; +use acp_thread::PermissionPattern; +use shell_command_parser::{extract_commands, extract_terminal_command_prefix}; use std::path::{Path, PathBuf}; use url::Url; @@ -42,12 +43,21 @@ fn extract_command_prefix(command: &str) -> Option { }) } -/// Extracts a regex pattern from a terminal command based on the first token (command name). +/// Extracts a regex pattern and display name from a terminal command. /// /// 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_permission_pattern(command: &str) -> Option { + let pattern = extract_terminal_pattern(command)?; + let display_name = extract_terminal_pattern_display(command)?; + Some(PermissionPattern { + pattern, + display_name, + }) +} + pub fn extract_terminal_pattern(command: &str) -> Option { let prefix = extract_command_prefix(command)?; let tokens = prefix.normalized_tokens; @@ -71,6 +81,35 @@ pub fn extract_terminal_pattern_display(command: &str) -> Option { Some(prefix.display) } +/// Extracts patterns for ALL commands in a pipeline, not just the first one. +/// +/// For a command like `"cargo test 2>&1 | tail"`, this returns patterns for +/// both `cargo` and `tail`. Path-based commands (e.g. `./script.sh`) are +/// filtered out, and duplicate command names are deduplicated while preserving +/// order. +pub fn extract_all_terminal_patterns(command: &str) -> Vec { + let commands = match extract_commands(command) { + Some(commands) => commands, + None => return Vec::new(), + }; + + let mut results = Vec::new(); + + for cmd in &commands { + let Some(permission_pattern) = extract_terminal_permission_pattern(cmd) else { + continue; + }; + + if results.contains(&permission_pattern) { + continue; + } + + results.push(permission_pattern); + } + + results +} + pub fn extract_path_pattern(path: &str) -> Option { let parent = Path::new(path).parent()?; let parent_str = normalize_separators(parent.to_str()?); @@ -273,6 +312,39 @@ mod tests { ); } + #[test] + fn test_extract_all_terminal_patterns_pipeline() { + assert_eq!( + extract_all_terminal_patterns("cargo test 2>&1 | tail"), + vec![ + PermissionPattern { + pattern: "^cargo\\s+test(\\s|$)".to_string(), + display_name: "cargo test".to_string(), + }, + PermissionPattern { + pattern: "^tail\\b".to_string(), + display_name: "tail".to_string(), + }, + ] + ); + } + + #[test] + fn test_extract_all_terminal_patterns_with_path_commands() { + assert_eq!( + extract_all_terminal_patterns("./script.sh | grep foo"), + vec![PermissionPattern { + pattern: "^grep\\s+foo(\\s|$)".to_string(), + display_name: "grep foo".to_string(), + }] + ); + } + + #[test] + fn test_extract_all_terminal_patterns_all_paths() { + assert_eq!(extract_all_terminal_patterns("./a.sh | /usr/bin/b"), vec![]); + } + #[test] fn test_extract_path_pattern() { assert_eq!( diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 7482ffda5854525f78efb2fcd3fd8cc9f757e3be..31563729cfd361991832eeeb3428043c40b81e79 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -841,14 +841,14 @@ async fn test_tool_authorization(cx: &mut TestAppContext) { // Approve the first - send "allow" option_id (UI transforms "once" to "allow") tool_call_auth_1 .response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); cx.run_until_parked(); // Reject the second - send "deny" option_id directly since Deny is now a button tool_call_auth_2 .response - .send(acp::PermissionOptionId::new("deny")) + .send(acp::PermissionOptionId::new("deny").into()) .unwrap(); cx.run_until_parked(); @@ -892,9 +892,7 @@ async fn test_tool_authorization(cx: &mut TestAppContext) { let tool_call_auth_3 = next_tool_call_authorization(&mut events).await; tool_call_auth_3 .response - .send(acp::PermissionOptionId::new( - "always_allow:tool_requiring_permission", - )) + .send(acp::PermissionOptionId::new("always_allow:tool_requiring_permission").into()) .unwrap(); cx.run_until_parked(); let completion = fake_model.pending_completions().pop().unwrap(); @@ -1183,32 +1181,88 @@ fn test_permission_option_ids_for_terminal() { panic!("Expected dropdown permission options"); }; - let allow_ids: Vec = choices - .iter() - .map(|choice| choice.allow.option_id.0.to_string()) - .collect(); - let deny_ids: Vec = choices - .iter() - .map(|choice| choice.deny.option_id.0.to_string()) - .collect(); + // Expect 3 choices: always-tool, always-pattern, once + assert_eq!(choices.len(), 3); - assert!(allow_ids.contains(&"always_allow:terminal".to_string())); - assert!(allow_ids.contains(&"allow".to_string())); - assert!( - allow_ids - .iter() - .any(|id| id.starts_with("always_allow_pattern:terminal\n")), - "Missing allow pattern option" + // First two choices both use the tool-level option IDs + assert_eq!( + choices[0].allow.option_id.0.as_ref(), + "always_allow:terminal" ); + assert_eq!(choices[0].deny.option_id.0.as_ref(), "always_deny:terminal"); + assert!(choices[0].sub_patterns.is_empty()); - assert!(deny_ids.contains(&"always_deny:terminal".to_string())); - assert!(deny_ids.contains(&"deny".to_string())); - assert!( - deny_ids - .iter() - .any(|id| id.starts_with("always_deny_pattern:terminal\n")), - "Missing deny pattern option" + assert_eq!( + choices[1].allow.option_id.0.as_ref(), + "always_allow:terminal" ); + assert_eq!(choices[1].deny.option_id.0.as_ref(), "always_deny:terminal"); + assert_eq!(choices[1].sub_patterns, vec!["^cargo\\s+build(\\s|$)"]); + + // Third choice is the one-time allow/deny + assert_eq!(choices[2].allow.option_id.0.as_ref(), "allow"); + assert_eq!(choices[2].deny.option_id.0.as_ref(), "deny"); + assert!(choices[2].sub_patterns.is_empty()); +} + +#[test] +fn test_permission_options_terminal_pipeline_produces_dropdown_with_patterns() { + let permission_options = ToolPermissionContext::new( + TerminalTool::NAME, + vec!["cargo test 2>&1 | tail".to_string()], + ) + .build_permission_options(); + + let PermissionOptions::DropdownWithPatterns { + choices, + patterns, + tool_name, + } = permission_options + else { + panic!("Expected DropdownWithPatterns permission options for pipeline command"); + }; + + assert_eq!(tool_name, TerminalTool::NAME); + + // Should have "Always for terminal" and "Only this time" choices + assert_eq!(choices.len(), 2); + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); + assert!(labels.contains(&"Always for terminal")); + assert!(labels.contains(&"Only this time")); + + // Should have per-command patterns for "cargo test" and "tail" + assert_eq!(patterns.len(), 2); + let pattern_names: Vec<&str> = patterns.iter().map(|cp| cp.display_name.as_str()).collect(); + assert!(pattern_names.contains(&"cargo test")); + assert!(pattern_names.contains(&"tail")); + + // Verify patterns are valid regex patterns + let regex_patterns: Vec<&str> = patterns.iter().map(|cp| cp.pattern.as_str()).collect(); + assert!(regex_patterns.contains(&"^cargo\\s+test(\\s|$)")); + assert!(regex_patterns.contains(&"^tail\\b")); +} + +#[test] +fn test_permission_options_terminal_pipeline_with_chaining() { + let permission_options = ToolPermissionContext::new( + TerminalTool::NAME, + vec!["npm install && npm test | tail".to_string()], + ) + .build_permission_options(); + + let PermissionOptions::DropdownWithPatterns { patterns, .. } = permission_options else { + panic!("Expected DropdownWithPatterns for chained pipeline command"); + }; + + // With subcommand-aware patterns, "npm install" and "npm test" are distinct + assert_eq!(patterns.len(), 3); + let pattern_names: Vec<&str> = patterns.iter().map(|cp| cp.display_name.as_str()).collect(); + assert!(pattern_names.contains(&"npm install")); + assert!(pattern_names.contains(&"npm test")); + assert!(pattern_names.contains(&"tail")); } #[gpui::test] diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 2f6eee28b6ab7311bde461170e56704daa5b7b9f..bf4f923b343bf45c9f8a8bf45c1788f09e18c7f3 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -758,6 +758,48 @@ impl ToolPermissionContext { true }; + // For terminal commands with multiple pipeline commands, use DropdownWithPatterns + // to let users individually select which command patterns to always allow. + if tool_name == TerminalTool::NAME && shell_supports_always_allow { + if let Some(input) = input_values.first() { + let all_patterns = extract_all_terminal_patterns(input); + if all_patterns.len() > 1 { + let mut choices = Vec::new(); + choices.push(acp_thread::PermissionOptionChoice { + allow: acp::PermissionOption::new( + acp::PermissionOptionId::new(format!("always_allow:{}", tool_name)), + format!("Always for {}", tool_name.replace('_', " ")), + acp::PermissionOptionKind::AllowAlways, + ), + deny: acp::PermissionOption::new( + acp::PermissionOptionId::new(format!("always_deny:{}", tool_name)), + format!("Always for {}", tool_name.replace('_', " ")), + acp::PermissionOptionKind::RejectAlways, + ), + sub_patterns: vec![], + }); + choices.push(acp_thread::PermissionOptionChoice { + allow: acp::PermissionOption::new( + acp::PermissionOptionId::new("allow"), + "Only this time", + acp::PermissionOptionKind::AllowOnce, + ), + deny: acp::PermissionOption::new( + acp::PermissionOptionId::new("deny"), + "Only this time", + acp::PermissionOptionKind::RejectOnce, + ), + sub_patterns: vec![], + }); + return acp_thread::PermissionOptions::DropdownWithPatterns { + choices, + patterns: all_patterns, + tool_name: tool_name.clone(), + }; + } + } + } + let extract_for_value = |value: &str| -> (Option, Option) { if tool_name == TerminalTool::NAME { ( @@ -806,20 +848,22 @@ impl ToolPermissionContext { let mut choices = Vec::new(); - let mut push_choice = |label: String, allow_id, deny_id, allow_kind, deny_kind| { - choices.push(acp_thread::PermissionOptionChoice { - allow: acp::PermissionOption::new( - acp::PermissionOptionId::new(allow_id), - label.clone(), - allow_kind, - ), - deny: acp::PermissionOption::new( - acp::PermissionOptionId::new(deny_id), - label, - deny_kind, - ), - }); - }; + let mut push_choice = + |label: String, allow_id, deny_id, allow_kind, deny_kind, sub_patterns: Vec| { + choices.push(acp_thread::PermissionOptionChoice { + allow: acp::PermissionOption::new( + acp::PermissionOptionId::new(allow_id), + label.clone(), + allow_kind, + ), + deny: acp::PermissionOption::new( + acp::PermissionOptionId::new(deny_id), + label, + deny_kind, + ), + sub_patterns, + }); + }; if shell_supports_always_allow { push_choice( @@ -828,6 +872,7 @@ impl ToolPermissionContext { format!("always_deny:{}", tool_name), acp::PermissionOptionKind::AllowAlways, acp::PermissionOptionKind::RejectAlways, + vec![], ); if let (Some(pattern), Some(display)) = (pattern, pattern_display) { @@ -838,10 +883,11 @@ impl ToolPermissionContext { }; push_choice( button_text, - format!("always_allow_pattern:{}\n{}", tool_name, pattern), - format!("always_deny_pattern:{}\n{}", tool_name, pattern), + format!("always_allow:{}", tool_name), + format!("always_deny:{}", tool_name), acp::PermissionOptionKind::AllowAlways, acp::PermissionOptionKind::RejectAlways, + vec![pattern], ); } } @@ -852,6 +898,7 @@ impl ToolPermissionContext { "deny".to_string(), acp::PermissionOptionKind::AllowOnce, acp::PermissionOptionKind::RejectOnce, + vec![], ); acp_thread::PermissionOptions::Dropdown(choices) @@ -862,7 +909,7 @@ impl ToolPermissionContext { pub struct ToolCallAuthorization { pub tool_call: acp::ToolCallUpdate, pub options: acp_thread::PermissionOptions, - pub response: oneshot::Sender, + pub response: oneshot::Sender, pub context: Option, } @@ -3617,6 +3664,7 @@ impl ToolCallEventStream { format!("Always for {} MCP tool", display_name), acp::PermissionOptionKind::RejectAlways, ), + sub_patterns: vec![], }, acp_thread::PermissionOptionChoice { allow: acp::PermissionOption::new( @@ -3629,6 +3677,7 @@ impl ToolCallEventStream { "Only this time", acp::PermissionOptionKind::RejectOnce, ), + sub_patterns: vec![], }, ]), response: response_tx, @@ -3644,40 +3693,13 @@ impl ToolCallEventStream { 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_permission(&tool_id, ToolPermissionMode::Allow); - }); - }); - } - return Ok(()); - } - if response_str == format!("always_deny_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_permission(&tool_id, ToolPermissionMode::Deny); - }); - }); - } - return Err(anyhow!("Permission to run tool denied by user")); - } - - if response_str == "allow" { - return Ok(()); + let outcome = response_rx.await?; + let is_allow = Self::persist_permission_outcome(&outcome, fs, &cx); + if is_allow { + Ok(()) + } else { + Err(anyhow!("Permission to run tool denied by user")) } - - Err(anyhow!("Permission to run tool denied by user")) }) } @@ -3687,8 +3709,6 @@ impl ToolCallEventStream { context: ToolPermissionContext, cx: &mut App, ) -> Task> { - use settings::ToolPermissionMode; - let options = context.build_permission_options(); let (response_tx, response_rx) = oneshot::channel(); @@ -3715,90 +3735,118 @@ impl ToolCallEventStream { let fs = self.fs.clone(); 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_permission(&tool, ToolPermissionMode::Allow); - }); - }); - } - return Ok(()); + let outcome = response_rx.await?; + let is_allow = Self::persist_permission_outcome(&outcome, fs, &cx); + if is_allow { + Ok(()) + } else { + Err(anyhow!("Permission to run tool denied by user")) } + }) + } - // 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, move |settings, _| { - settings - .agent - .get_or_insert_default() - .set_tool_default_permission(&tool, ToolPermissionMode::Deny); - }); - }); - } - return Err(anyhow!("Permission to run tool denied by user")); - } + /// Interprets a `SelectedPermissionOutcome` and persists any settings changes. + /// Returns `true` if the tool call should be allowed, `false` if denied. + fn persist_permission_outcome( + outcome: &acp_thread::SelectedPermissionOutcome, + fs: Option>, + cx: &AsyncApp, + ) -> bool { + let option_id = outcome.option_id.0.as_ref(); + + let always_permission = option_id + .strip_prefix("always_allow:") + .map(|tool| (tool, ToolPermissionMode::Allow)) + .or_else(|| { + option_id + .strip_prefix("always_deny:") + .map(|tool| (tool, ToolPermissionMode::Deny)) + }) + .or_else(|| { + option_id + .strip_prefix("always_allow_mcp:") + .map(|tool| (tool, ToolPermissionMode::Allow)) + }) + .or_else(|| { + option_id + .strip_prefix("always_deny_mcp:") + .map(|tool| (tool, ToolPermissionMode::Deny)) + }); - // Handle "always allow pattern" - e.g., "always_allow_pattern:mcp:server:tool\n^cargo\s" - if let Some(rest) = response_str.strip_prefix("always_allow_pattern:") { - if let Some((pattern_tool_name, pattern)) = rest.split_once('\n') { - let pattern_tool_name = pattern_tool_name.to_string(); - let pattern = pattern.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); - }); - }); - } - } else { - log::error!("Failed to parse always allow pattern: missing newline separator in '{rest}'"); - } - return Ok(()); - } + if let Some((tool, mode)) = always_permission { + let params = outcome.params.as_ref(); + Self::persist_always_permission(tool, mode, params, fs, cx); + return mode == ToolPermissionMode::Allow; + } - // Handle "always deny pattern" - e.g., "always_deny_pattern:mcp:server:tool\n^cargo\s" - if let Some(rest) = response_str.strip_prefix("always_deny_pattern:") { - if let Some((pattern_tool_name, pattern)) = rest.split_once('\n') { - let pattern_tool_name = pattern_tool_name.to_string(); - let pattern = pattern.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); - }); - }); - } - } else { - log::error!("Failed to parse always deny pattern: missing newline separator in '{rest}'"); - } - return Err(anyhow!("Permission to run tool denied by user")); - } + // Handle simple "allow" / "deny" (once, no persistence) + if option_id == "allow" || option_id == "deny" { + debug_assert!( + outcome.params.is_none(), + "unexpected params for once-only permission" + ); + return option_id == "allow"; + } - // Handle simple "allow" (allow once) - if response_str == "allow" { - return Ok(()); - } + debug_assert!(false, "unexpected permission option_id: {option_id}"); + false + } - // Handle simple "deny" (deny once) - Err(anyhow!("Permission to run tool denied by user")) - }) + /// Persists an "always allow" or "always deny" permission, using sub_patterns + /// from params when present. + fn persist_always_permission( + tool: &str, + mode: ToolPermissionMode, + params: Option<&acp_thread::SelectedPermissionParams>, + fs: Option>, + cx: &AsyncApp, + ) { + let Some(fs) = fs else { + return; + }; + + match params { + Some(acp_thread::SelectedPermissionParams::Terminal { + patterns: sub_patterns, + }) => { + debug_assert!( + !sub_patterns.is_empty(), + "empty sub_patterns for tool {tool} — callers should pass None instead" + ); + let tool = tool.to_string(); + let sub_patterns = sub_patterns.clone(); + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + let agent = settings.agent.get_or_insert_default(); + for pattern in sub_patterns { + match mode { + ToolPermissionMode::Allow => { + agent.add_tool_allow_pattern(&tool, pattern); + } + ToolPermissionMode::Deny => { + agent.add_tool_deny_pattern(&tool, pattern); + } + // If there's no matching pattern this will + // default to confirm, so falling through is + // fine here. + ToolPermissionMode::Confirm => (), + } + } + }); + }); + } + None => { + let tool = tool.to_string(); + cx.update(|cx| { + update_settings_file(fs, cx, move |settings, _| { + settings + .agent + .get_or_insert_default() + .set_tool_default_permission(&tool, mode); + }); + }); + } + } } } diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index 7f53a5c36a7979a01de96535f19e421fa3119e16..7955a6cc0755514ba4341e43af980e9b93478134 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -266,7 +266,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = task.await; @@ -372,7 +372,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); assert!( diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index 5d8930f3c7400428d55cfe7d14bafc16d94be43a..7052b5dfdc2c7d546f5e477430d6de1a0039b03d 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -241,7 +241,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = task.await; @@ -359,7 +359,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); assert!( diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index 27ab68db667a4cf3223e6521682814dc1c245bb7..9b2c0a20b8a26b57ef77bb91004c079265fc80cf 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -301,7 +301,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = task.await; @@ -428,7 +428,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); assert!( diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 29b08ac09db4417123403fd3915b8575791b2a4e..3325a612a0143070a3fc157976be93276f98cb5f 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -1374,7 +1374,7 @@ mod tests { event .response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); authorize_task.await.unwrap(); } diff --git a/crates/agent/src/tools/list_directory_tool.rs b/crates/agent/src/tools/list_directory_tool.rs index 1a674aaa71fef5bf9c11688e82982a5dbcfee331..7769669222631f7a2a4bd9de1e0d81a68665a816 100644 --- a/crates/agent/src/tools/list_directory_tool.rs +++ b/crates/agent/src/tools/list_directory_tool.rs @@ -848,7 +848,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = task.await; diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index c246b3c5b0661546f4617bb5521766f9da3839fb..ab5637c26d250b5866ebdc015ab6fce294adc7e7 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -273,7 +273,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = task.await; @@ -379,7 +379,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); assert!( diff --git a/crates/agent/src/tools/read_file_tool.rs b/crates/agent/src/tools/read_file_tool.rs index f7a75bc63a1c461b65c3a2e6f74f2c70e0ca15f6..ef33d7d5b9d0f04783849ebd681badd04b7df052 100644 --- a/crates/agent/src/tools/read_file_tool.rs +++ b/crates/agent/src/tools/read_file_tool.rs @@ -896,7 +896,7 @@ mod test { ); authorization .response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = read_task.await; @@ -1185,7 +1185,7 @@ mod test { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let result = task.await; diff --git a/crates/agent/src/tools/restore_file_from_disk_tool.rs b/crates/agent/src/tools/restore_file_from_disk_tool.rs index c1aa8690a840ea6911dcb94c26c8cef3cb5f313d..ffe886b73c217e7afe4c5a8754b12d15be4b9b0d 100644 --- a/crates/agent/src/tools/restore_file_from_disk_tool.rs +++ b/crates/agent/src/tools/restore_file_from_disk_tool.rs @@ -523,7 +523,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let _result = task.await; @@ -651,7 +651,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); assert!( diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index 99e937b9dff2a1b4781dde16bd2bf6d64edd25ad..3f741d2eea7794111e039dcb981a5239d96b7b65 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -518,7 +518,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); let _result = task.await; @@ -646,7 +646,7 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); assert!( @@ -727,7 +727,7 @@ mod tests { let auth = event_rx.expect_authorization().await; auth.response - .send(acp::PermissionOptionId::new("deny")) + .send(acp::PermissionOptionId::new("deny").into()) .unwrap(); let output = task.await.unwrap(); diff --git a/crates/agent/src/tools/streaming_edit_file_tool.rs b/crates/agent/src/tools/streaming_edit_file_tool.rs index ccfde2dd5ba0772f51210526f35896c9bb53b559..9e7a7bbf1f287a8791591f3ae80a8731802eda42 100644 --- a/crates/agent/src/tools/streaming_edit_file_tool.rs +++ b/crates/agent/src/tools/streaming_edit_file_tool.rs @@ -2581,7 +2581,7 @@ mod tests { event .response - .send(acp::PermissionOptionId::new("allow")) + .send(acp::PermissionOptionId::new("allow").into()) .unwrap(); authorize_task.await.unwrap(); } diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index beb313445e7d4223fea96f2b68dea2e7dbf4e047..14b4628c764b76e58a2d2be1637f760a46d7bfad 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -1470,7 +1470,7 @@ impl acp::Client for ClientDelegate { let outcome = task.await; - Ok(acp::RequestPermissionResponse::new(outcome)) + Ok(acp::RequestPermissionResponse::new(outcome.into())) } async fn write_text_file( diff --git a/crates/agent_servers/src/e2e_tests.rs b/crates/agent_servers/src/e2e_tests.rs index c7f90ac6bc431c5bb0ca5b18bdacf2e31465bfba..93c37b5e258c5342be2c02eb3762bd775e22d001 100644 --- a/crates/agent_servers/src/e2e_tests.rs +++ b/crates/agent_servers/src/e2e_tests.rs @@ -208,7 +208,7 @@ pub async fn test_tool_call_with_permission( thread.update(cx, |thread, cx| { thread.authorize_tool_call( tool_call_id, - allow_option_id, + allow_option_id.into(), acp::PermissionOptionKind::AllowOnce, cx, ); diff --git a/crates/agent_ui/src/agent_ui.rs b/crates/agent_ui/src/agent_ui.rs index fed5a7b5618534c10a1a017302d77a0150e92d2a..f4b346d2ec86e65e75a64453421276ebcae8be38 100644 --- a/crates/agent_ui/src/agent_ui.rs +++ b/crates/agent_ui/src/agent_ui.rs @@ -194,6 +194,29 @@ pub struct AuthorizeToolCall { 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, +} + +/// Action to toggle a command pattern checkbox in the permission dropdown. +#[derive(Clone, PartialEq, Deserialize, JsonSchema, Action)] +#[action(namespace = agent)] +#[serde(deny_unknown_fields)] +pub struct ToggleCommandPattern { + /// The tool call ID for which to toggle the pattern. + pub tool_call_id: String, + /// The index of the command pattern to toggle. + pub pattern_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/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index 1dbd3984b51ed6b997cb9453c76fcfb32b84d287..2ba60eba96ed08bdc276164e01b7480731edf635 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -1,7 +1,8 @@ use acp_thread::{ AcpThread, AcpThreadEvent, AgentSessionInfo, AgentThreadEntry, AssistantMessage, AssistantMessageChunk, AuthRequired, LoadError, MentionUri, PermissionOptionChoice, - PermissionOptions, RetryStatus, ThreadStatus, ToolCall, ToolCallContent, ToolCallStatus, + PermissionOptions, PermissionPattern, RetryStatus, SelectedPermissionOutcome, + SelectedPermissionParams, ThreadStatus, ToolCall, ToolCallContent, ToolCallStatus, UserMessageId, }; use acp_thread::{AgentConnection, Plan}; @@ -164,9 +165,6 @@ pub(crate) struct Conversation { threads: HashMap>, permission_requests: IndexMap>, subscriptions: Vec, - /// Tracks the selected granularity index for each tool call's permission dropdown. - /// The index corresponds to the position in the allow_options list. - selected_permission_granularity: HashMap>, updated_at: Option, } @@ -212,29 +210,6 @@ impl Conversation { .insert(thread.read(cx).session_id().clone(), thread); } - pub fn selected_permission_granularity( - &self, - session_id: &acp::SessionId, - tool_call_id: &acp::ToolCallId, - ) -> Option { - self.selected_permission_granularity - .get(session_id) - .and_then(|map| map.get(tool_call_id)) - .copied() - } - - pub fn set_selected_permission_granularity( - &mut self, - session_id: acp::SessionId, - tool_call_id: acp::ToolCallId, - granularity: usize, - ) { - self.selected_permission_granularity - .entry(session_id) - .or_default() - .insert(tool_call_id, granularity); - } - pub fn pending_tool_call<'a>( &'a self, session_id: &acp::SessionId, @@ -274,7 +249,7 @@ impl Conversation { self.authorize_tool_call( session_id.clone(), tool_call_id, - option.option_id.clone(), + option.option_id.clone().into(), option.kind, cx, ); @@ -285,7 +260,7 @@ impl Conversation { &mut self, session_id: acp::SessionId, tool_call_id: acp::ToolCallId, - option_id: acp::PermissionOptionId, + outcome: SelectedPermissionOutcome, option_kind: acp::PermissionOptionKind, cx: &mut Context, ) { @@ -302,7 +277,7 @@ impl Conversation { ); thread.update(cx, |thread, cx| { - thread.authorize_tool_call(tool_call_id, option_id, option_kind, cx); + thread.authorize_tool_call(tool_call_id, outcome, option_kind, cx); }); cx.notify(); } @@ -5779,17 +5754,11 @@ pub(crate) mod tests { cx.run_until_parked(); - // Find the pattern option ID + // Find the pattern option ID (the choice with non-empty sub_patterns) let pattern_option = match &permission_options { PermissionOptions::Dropdown(choices) => choices .iter() - .find(|choice| { - choice - .allow - .option_id - .0 - .starts_with("always_allow_pattern:") - }) + .find(|choice| !choice.sub_patterns.is_empty()) .map(|choice| &choice.allow) .expect("Should have a pattern option for npm command"), _ => panic!("Expected dropdown permission options"), @@ -5820,6 +5789,181 @@ pub(crate) mod tests { }); } + #[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(TerminalTool::NAME, vec!["cargo build".to_string()]) + .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_conversation_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 = message_editor(&thread_view, cx); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Build the project", window, cx); + }); + + active_thread(&thread_view, cx).update_in(cx, |view, window, cx| 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 state = thread_view.active_thread().unwrap(); + let selected = state.read(cx).permission_selections.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 state = thread_view.active_thread().unwrap(); + let selected = state.read(cx).permission_selections.get(&tool_call_id); + assert_eq!( + selected.and_then(|s| s.choice_index()), + 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(TerminalTool::NAME, vec!["npm install".to_string()]) + .build_permission_options(); + + // Verify we have the expected options + let PermissionOptions::Dropdown(choices) = &permission_options else { + panic!("Expected dropdown permission options"); + }; + + assert_eq!(choices.len(), 3); + assert!( + choices[0] + .allow + .option_id + .0 + .contains("always_allow:terminal") + ); + assert!( + choices[1] + .allow + .option_id + .0 + .contains("always_allow:terminal") + ); + assert!(!choices[1].sub_patterns.is_empty()); + assert_eq!(choices[2].allow.option_id.0.as_ref(), "allow"); + + 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_conversation_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 = message_editor(&thread_view, cx); + message_editor.update_in(cx, |editor, window, cx| { + editor.set_text("Install dependencies", window, cx); + }); + + active_thread(&thread_view, cx).update_in(cx, |view, window, cx| 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 + active_thread(&thread_view, cx).update_in(cx, |view, window, cx| { + view.allow_once(&AllowOnce, window, cx) + }); + + cx.run_until_parked(); + + // Verify tool call was authorized + thread_view.read_with(cx, |thread_view, cx| { + let tool_call = thread_view.pending_tool_call(cx); + 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); @@ -5899,13 +6043,14 @@ pub(crate) mod tests { .map(|choice| choice.allow.option_id.0.to_string()) .collect(); - assert!(allow_ids.contains(&"always_allow:terminal".to_string())); assert!(allow_ids.contains(&"allow".to_string())); - assert!( + assert_eq!( allow_ids .iter() - .any(|id| id.starts_with("always_allow_pattern:terminal\n")), - "Missing allow pattern option" + .filter(|id| *id == "always_allow:terminal") + .count(), + 2, + "Expected two always_allow:terminal IDs (one whole-tool, one pattern with sub_patterns)" ); } @@ -5926,13 +6071,14 @@ pub(crate) mod tests { .map(|choice| choice.deny.option_id.0.to_string()) .collect(); - assert!(deny_ids.contains(&"always_deny:terminal".to_string())); assert!(deny_ids.contains(&"deny".to_string())); - assert!( + assert_eq!( deny_ids .iter() - .any(|id| id.starts_with("always_deny_pattern:terminal\n")), - "Missing deny pattern option" + .filter(|id| *id == "always_deny:terminal") + .count(), + 2, + "Expected two always_deny:terminal IDs (one whole-tool, one pattern with sub_patterns)" ); } @@ -6067,7 +6213,7 @@ pub(crate) mod tests { tool_call_id: &str, option_id: &str, cx: &mut TestAppContext, - ) -> Task { + ) -> Task { let tool_call_id = acp::ToolCallId::new(tool_call_id); let label = format!("Tool {tool_call_id}"); let option_id = acp::PermissionOptionId::new(option_id); @@ -6126,7 +6272,7 @@ pub(crate) mod tests { conversation.authorize_tool_call( acp::SessionId::new("session-1"), acp::ToolCallId::new("tc-1"), - acp::PermissionOptionId::new("allow-1"), + acp::PermissionOptionId::new("allow-1").into(), acp::PermissionOptionKind::AllowOnce, cx, ); @@ -6149,7 +6295,7 @@ pub(crate) mod tests { conversation.authorize_tool_call( acp::SessionId::new("session-1"), acp::ToolCallId::new("tc-2"), - acp::PermissionOptionId::new("allow-2"), + acp::PermissionOptionId::new("allow-2").into(), acp::PermissionOptionKind::AllowOnce, cx, ); @@ -6288,7 +6434,7 @@ pub(crate) mod tests { conversation.authorize_tool_call( acp::SessionId::new("thread-a"), acp::ToolCallId::new("tc-a"), - acp::PermissionOptionId::new("allow-a"), + acp::PermissionOptionId::new("allow-a").into(), acp::PermissionOptionKind::AllowOnce, cx, ); diff --git a/crates/agent_ui/src/conversation_view/thread_view.rs b/crates/agent_ui/src/conversation_view/thread_view.rs index 2f08070ed77aaa7e403e1fe131a6b82f1acb13e8..d086cb91f8204f51dbfa5eaaf2d011a2a9656309 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -1,3 +1,4 @@ +use crate::SelectPermissionGranularity; use std::cell::RefCell; use acp_thread::ContentBlock; @@ -165,6 +166,56 @@ pub enum AcpThreadViewEvent { impl EventEmitter for ThreadView {} +/// Tracks the user's permission dropdown selection state for a specific tool call. +/// +/// Default (no entry in the map) means the last dropdown choice is selected, +/// which is typically "Only this time". +#[derive(Clone)] +pub(crate) enum PermissionSelection { + /// A specific choice from the dropdown (e.g., "Always for terminal", "Only this time"). + /// The index corresponds to the position in the `choices` list from `PermissionOptions`. + Choice(usize), + /// "Select options…" mode where individual command patterns can be toggled. + /// Contains the indices of checked patterns in the `patterns` list. + /// All patterns start checked when this mode is first activated. + SelectedPatterns(Vec), +} + +impl PermissionSelection { + /// Returns the choice index if a specific dropdown choice is selected, + /// or `None` if in per-command pattern mode. + pub(crate) fn choice_index(&self) -> Option { + match self { + Self::Choice(index) => Some(*index), + Self::SelectedPatterns(_) => None, + } + } + + fn is_pattern_checked(&self, index: usize) -> bool { + match self { + Self::SelectedPatterns(checked) => checked.contains(&index), + _ => false, + } + } + + fn has_any_checked_patterns(&self) -> bool { + match self { + Self::SelectedPatterns(checked) => !checked.is_empty(), + _ => false, + } + } + + fn toggle_pattern(&mut self, index: usize) { + if let Self::SelectedPatterns(checked) = self { + if let Some(pos) = checked.iter().position(|&i| i == index) { + checked.swap_remove(pos); + } else { + checked.push(index); + } + } + } +} + pub struct ThreadView { pub id: acp::SessionId, pub parent_id: Option, @@ -213,6 +264,9 @@ pub struct ThreadView { pub is_loading_contents: bool, pub new_server_version_available: Option, pub resumed_without_history: bool, + pub(crate) permission_selections: + HashMap, + pub resume_thread_metadata: Option, pub _cancel_task: Option>, _save_task: Option>, _draft_resolve_task: Option>, @@ -446,6 +500,8 @@ impl ThreadView { discarded_partial_edits: HashSet::default(), is_loading_contents: false, new_server_version_available: None, + permission_selections: HashMap::default(), + resume_thread_metadata: None, _cancel_task: None, _save_task: None, _draft_resolve_task: None, @@ -1518,13 +1574,13 @@ impl ThreadView { &mut self, session_id: acp::SessionId, tool_call_id: acp::ToolCallId, - option_id: acp::PermissionOptionId, + outcome: SelectedPermissionOutcome, option_kind: acp::PermissionOptionKind, window: &mut Window, cx: &mut Context, ) { self.conversation.update(cx, |conversation, cx| { - conversation.authorize_tool_call(session_id, tool_call_id, option_id, option_kind, cx); + conversation.authorize_tool_call(session_id, tool_call_id, outcome, option_kind, cx); }); if self.should_be_following { self.workspace @@ -1587,13 +1643,77 @@ impl ThreadView { self.authorize_tool_call( self.id.clone(), tool_call_id, - option_id, + option_id.into(), option_kind, window, cx, ); } + pub 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.permission_selections + .insert(tool_call_id, PermissionSelection::Choice(action.index)); + + cx.notify(); + } + + pub fn handle_toggle_command_pattern( + &mut self, + action: &crate::ToggleCommandPattern, + _window: &mut Window, + cx: &mut Context, + ) { + let tool_call_id = acp::ToolCallId::new(action.tool_call_id.clone()); + + match self.permission_selections.get_mut(&tool_call_id) { + Some(PermissionSelection::SelectedPatterns(checked)) => { + // Already in pattern mode — toggle the individual pattern. + if let Some(pos) = checked.iter().position(|&i| i == action.pattern_index) { + checked.swap_remove(pos); + } else { + checked.push(action.pattern_index); + } + } + _ => { + // First click: activate "Select options" with all patterns checked. + let thread = self.thread.read(cx); + let pattern_count = thread + .entries() + .iter() + .find_map(|entry| { + if let AgentThreadEntry::ToolCall(call) = entry { + if call.id == tool_call_id { + if let ToolCallStatus::WaitingForConfirmation { options, .. } = + &call.status + { + if let PermissionOptions::DropdownWithPatterns { + patterns, + .. + } = options + { + return Some(patterns.len()); + } + } + } + } + None + }) + .unwrap_or(0); + self.permission_selections.insert( + tool_call_id, + PermissionSelection::SelectedPatterns((0..pattern_count).collect()), + ); + } + } + cx.notify(); + } + fn authorize_pending_with_granularity( &mut self, is_allow: bool, @@ -1602,20 +1722,77 @@ impl ThreadView { ) -> Option<()> { let (session_id, tool_call_id, options) = self.conversation.read(cx).pending_tool_call(&self.id, cx)?; - let PermissionOptions::Dropdown(choices) = options else { - let kind = if is_allow { - acp::PermissionOptionKind::AllowOnce - } else { - acp::PermissionOptionKind::RejectOnce - }; - return self.authorize_pending_tool_call(kind, window, cx); + let options = options.clone(); + self.authorize_with_granularity(session_id, tool_call_id, &options, is_allow, window, cx) + } + + fn authorize_with_granularity( + &mut self, + session_id: acp::SessionId, + tool_call_id: acp::ToolCallId, + options: &PermissionOptions, + is_allow: bool, + window: &mut Window, + cx: &mut Context, + ) -> Option<()> { + let (choices, dropdown_with_patterns) = match options { + PermissionOptions::Dropdown(choices) => (choices.as_slice(), None), + PermissionOptions::DropdownWithPatterns { + choices, + patterns, + tool_name, + } => ( + choices.as_slice(), + Some((patterns.as_slice(), tool_name.as_str())), + ), + _ => { + let kind = if is_allow { + acp::PermissionOptionKind::AllowOnce + } else { + acp::PermissionOptionKind::RejectOnce + }; + return self.authorize_pending_tool_call(kind, window, cx); + } }; - // Get selected index, defaulting to last option ("Only this time") - let selected_index = self - .conversation - .read(cx) - .selected_permission_granularity(&session_id, &tool_call_id) + let selection = self.permission_selections.get(&tool_call_id); + + // When in per-command pattern mode, use the checked patterns. + if let Some(PermissionSelection::SelectedPatterns(checked)) = selection + && let Some((patterns, tool_name)) = dropdown_with_patterns + { + let checked_patterns: Vec<_> = patterns + .iter() + .enumerate() + .filter(|(index, _)| checked.contains(index)) + .map(|(_, cp)| cp.pattern.clone()) + .collect(); + + if !checked_patterns.is_empty() { + let (option_id_str, kind) = if is_allow { + ( + format!("always_allow:{}", tool_name), + acp::PermissionOptionKind::AllowAlways, + ) + } else { + ( + format!("always_deny:{}", tool_name), + acp::PermissionOptionKind::RejectAlways, + ) + }; + let outcome = + SelectedPermissionOutcome::new(acp::PermissionOptionId::new(option_id_str)) + .params(Some(SelectedPermissionParams::Terminal { + patterns: checked_patterns, + })); + self.authorize_tool_call(session_id, tool_call_id, outcome, kind, window, cx); + return Some(()); + } + } + + // Use the selected granularity choice ("Always for terminal" or "Only this time") + let selected_index = selection + .and_then(|s| s.choice_index()) .unwrap_or_else(|| choices.len().saturating_sub(1)); let selected_choice = choices.get(selected_index).or(choices.last())?; @@ -1626,10 +1803,21 @@ impl ThreadView { &selected_choice.deny }; + let params = if !selected_choice.sub_patterns.is_empty() { + Some(SelectedPermissionParams::Terminal { + patterns: selected_choice.sub_patterns.clone(), + }) + } else { + None + }; + + let outcome = + SelectedPermissionOutcome::new(selected_option.option_id.clone()).params(params); + self.authorize_tool_call( session_id, tool_call_id, - selected_option.option_id.clone(), + outcome, selected_option.kind, window, cx, @@ -5772,10 +5960,23 @@ impl ThreadView { focus_handle, cx, ), - PermissionOptions::Dropdown(options) => self.render_permission_buttons_dropdown( - session_id, + PermissionOptions::Dropdown(choices) => self.render_permission_buttons_with_dropdown( is_first, - options, + choices, + None, + entry_ix, + tool_call_id, + focus_handle, + cx, + ), + PermissionOptions::DropdownWithPatterns { + choices, + patterns, + tool_name, + } => self.render_permission_buttons_with_dropdown( + is_first, + choices, + Some((patterns, tool_name)), entry_ix, tool_call_id, focus_handle, @@ -5784,46 +5985,56 @@ impl ThreadView { } } - fn render_permission_buttons_dropdown( + fn render_permission_buttons_with_dropdown( &self, - session_id: acp::SessionId, is_first: bool, choices: &[PermissionOptionChoice], + patterns: Option<(&[PermissionPattern], &str)>, entry_ix: usize, tool_call_id: acp::ToolCallId, focus_handle: &FocusHandle, cx: &Context, ) -> Div { - // Get the selected granularity index, defaulting to the last option ("Only this time") - let selected_index = self - .conversation - .read(cx) - .selected_permission_granularity(&session_id, &tool_call_id) - .unwrap_or_else(|| choices.len().saturating_sub(1)); + let selection = self.permission_selections.get(&tool_call_id); - let selected_choice = choices.get(selected_index).or(choices.last()); - - let dropdown_label: SharedString = selected_choice - .map(|choice| choice.label()) - .unwrap_or_else(|| "Only this time".into()); + let selected_index = selection + .and_then(|s| s.choice_index()) + .unwrap_or_else(|| choices.len().saturating_sub(1)); - let (allow_option_id, allow_option_kind, deny_option_id, deny_option_kind) = - if let Some(choice) = selected_choice { - ( - choice.allow.option_id.clone(), - choice.allow.kind, - choice.deny.option_id.clone(), - choice.deny.kind, - ) + let dropdown_label: SharedString = + if matches!(selection, Some(PermissionSelection::SelectedPatterns(_))) { + "Always for selected commands".into() } else { - ( - acp::PermissionOptionId::new("allow"), - acp::PermissionOptionKind::AllowOnce, - acp::PermissionOptionId::new("deny"), - acp::PermissionOptionKind::RejectOnce, - ) + choices + .get(selected_index) + .or(choices.last()) + .map(|choice| choice.label()) + .unwrap_or_else(|| "Only this time".into()) }; + let dropdown = if let Some((pattern_list, tool_name)) = patterns { + self.render_permission_granularity_dropdown_with_patterns( + choices, + pattern_list, + tool_name, + dropdown_label, + entry_ix, + tool_call_id.clone(), + is_first, + cx, + ) + } else { + self.render_permission_granularity_dropdown( + choices, + dropdown_label, + entry_ix, + tool_call_id.clone(), + selected_index, + is_first, + cx, + ) + }; + h_flex() .w_full() .p_1() @@ -5853,19 +6064,8 @@ impl ThreadView { ) }) .on_click(cx.listener({ - let session_id = session_id.clone(); - 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( - session_id.clone(), - tool_call_id.clone(), - option_id.clone(), - option_kind, - window, - cx, - ); + this.authorize_pending_with_granularity(true, window, cx); } })), ) @@ -5888,33 +6088,13 @@ impl ThreadView { ) }) .on_click(cx.listener({ - let session_id = session_id.clone(); - 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( - session_id.clone(), - tool_call_id.clone(), - option_id.clone(), - option_kind, - window, - cx, - ); + this.authorize_pending_with_granularity(false, window, cx); } })), ), ) - .child(self.render_permission_granularity_dropdown( - choices, - dropdown_label, - entry_ix, - session_id, - tool_call_id, - selected_index, - is_first, - cx, - )) + .child(dropdown) } fn render_permission_granularity_dropdown( @@ -5922,7 +6102,6 @@ impl ThreadView { choices: &[PermissionOptionChoice], current_label: SharedString, entry_ix: usize, - session_id: acp::SessionId, tool_call_id: acp::ToolCallId, selected_index: usize, is_first: bool, @@ -5936,8 +6115,6 @@ impl ThreadView { let permission_dropdown_handle = self.permission_dropdown_handle.clone(); - let conversation = self.conversation.clone(); - PopoverMenu::new(("permission-granularity", entry_ix)) .with_handle(permission_dropdown_handle) .trigger( @@ -5960,8 +6137,6 @@ impl ThreadView { }), ) .menu(move |window, cx| { - let session_id = session_id.clone(); - let conversation = conversation.clone(); let tool_call_id = tool_call_id.clone(); let options = menu_options.clone(); @@ -5969,23 +6144,22 @@ impl ThreadView { for (index, display_name) in options.iter() { let display_name = display_name.clone(); let index = *index; - let session_id = session_id.clone(); - let conversation = conversation.clone(); - let tool_call_id = tool_call_id.clone(); + let tool_call_id_for_entry = tool_call_id.clone(); let is_selected = index == selected_index; menu = menu.toggleable_entry( display_name, is_selected, IconPosition::End, None, - move |_window, cx| { - conversation.update(cx, |conversation, _cx| { - conversation.set_selected_permission_granularity( - session_id.clone(), - tool_call_id.clone(), + move |window, cx| { + window.dispatch_action( + SelectPermissionGranularity { + tool_call_id: tool_call_id_for_entry.0.to_string(), index, - ); - }); + } + .boxed_clone(), + cx, + ); }, ); } @@ -5996,6 +6170,193 @@ impl ThreadView { .into_any_element() } + fn render_permission_granularity_dropdown_with_patterns( + &self, + choices: &[PermissionOptionChoice], + patterns: &[PermissionPattern], + _tool_name: &str, + current_label: SharedString, + entry_ix: usize, + tool_call_id: acp::ToolCallId, + is_first: bool, + cx: &Context, + ) -> AnyElement { + let default_choice_index = choices.len().saturating_sub(1); + let menu_options: Vec<(usize, SharedString)> = choices + .iter() + .enumerate() + .map(|(i, choice)| (i, choice.label())) + .collect(); + + let pattern_options: Vec<(usize, SharedString)> = patterns + .iter() + .enumerate() + .map(|(i, cp)| { + ( + i, + SharedString::from(format!("Always for `{}` commands", cp.display_name)), + ) + }) + .collect(); + + let pattern_count = patterns.len(); + let permission_dropdown_handle = self.permission_dropdown_handle.clone(); + let view = cx.entity().downgrade(); + + PopoverMenu::new(("permission-granularity", entry_ix)) + .with_handle(permission_dropdown_handle.clone()) + .anchor(Corner::TopRight) + .attach(Corner::BottomRight) + .trigger( + Button::new(("granularity-trigger", entry_ix), current_label) + .end_icon( + Icon::new(IconName::ChevronDown) + .size(IconSize::XSmall) + .color(Color::Muted), + ) + .label_size(LabelSize::Small) + .when(is_first, |this| { + this.key_binding( + KeyBinding::for_action_in( + &crate::OpenPermissionDropdown as &dyn Action, + &self.focus_handle(cx), + cx, + ) + .map(|kb| kb.size(rems_from_px(10.))), + ) + }), + ) + .menu(move |window, cx| { + let tool_call_id = tool_call_id.clone(); + let options = menu_options.clone(); + let patterns = pattern_options.clone(); + let view = view.clone(); + let dropdown_handle = permission_dropdown_handle.clone(); + + Some(ContextMenu::build_persistent( + window, + cx, + move |menu, _window, cx| { + let mut menu = menu; + + // Read fresh selection state from the view on each rebuild. + let selection: Option = view.upgrade().and_then(|v| { + let view = v.read(cx); + view.permission_selections.get(&tool_call_id).cloned() + }); + + let is_pattern_mode = + matches!(selection, Some(PermissionSelection::SelectedPatterns(_))); + + // Granularity choices: "Always for terminal", "Only this time" + 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 = !is_pattern_mode + && selection + .as_ref() + .and_then(|s| s.choice_index()) + .map_or(index == default_choice_index, |ci| ci == index); + + let view = view.clone(); + menu = menu.toggleable_entry( + display_name, + is_selected, + IconPosition::End, + None, + move |_window, cx| { + view.update(cx, |this, cx| { + this.permission_selections.insert( + tool_call_id_for_entry.clone(), + PermissionSelection::Choice(index), + ); + cx.notify(); + }) + .log_err(); + }, + ); + } + + menu = menu.separator().header("Select Options…"); + + for (pattern_index, label) in patterns.iter() { + let label = label.clone(); + let pattern_index = *pattern_index; + let tool_call_id_for_pattern = tool_call_id.clone(); + let is_checked = selection + .as_ref() + .is_some_and(|s| s.is_pattern_checked(pattern_index)); + + let view = view.clone(); + menu = menu.toggleable_entry( + label, + is_checked, + IconPosition::End, + None, + move |_window, cx| { + view.update(cx, |this, cx| { + let selection = this + .permission_selections + .get_mut(&tool_call_id_for_pattern); + + match selection { + Some(PermissionSelection::SelectedPatterns(_)) => { + // Already in pattern mode — toggle. + this.permission_selections + .get_mut(&tool_call_id_for_pattern) + .expect("just matched above") + .toggle_pattern(pattern_index); + } + _ => { + // First click: activate pattern mode + // with all patterns checked. + this.permission_selections.insert( + tool_call_id_for_pattern.clone(), + PermissionSelection::SelectedPatterns( + (0..pattern_count).collect(), + ), + ); + } + } + cx.notify(); + }) + .log_err(); + }, + ); + } + + let any_patterns_checked = selection + .as_ref() + .is_some_and(|s| s.has_any_checked_patterns()); + let dropdown_handle = dropdown_handle.clone(); + menu = menu.custom_row(move |_window, _cx| { + div() + .py_1() + .w_full() + .child( + Button::new("apply-patterns", "Apply") + .full_width() + .style(ButtonStyle::Outlined) + .label_size(LabelSize::Small) + .disabled(!any_patterns_checked) + .on_click({ + let dropdown_handle = dropdown_handle.clone(); + move |_event, _window, cx| { + dropdown_handle.hide(cx); + } + }), + ) + .into_any_element() + }); + + menu + }, + )) + }) + .into_any_element() + } + fn render_permission_buttons_flat( &self, session_id: acp::SessionId, @@ -6073,7 +6434,7 @@ impl ThreadView { this.authorize_tool_call( session_id.clone(), tool_call_id.clone(), - option_id.clone(), + option_id.clone().into(), option_kind, window, cx, @@ -7677,7 +8038,10 @@ impl ThreadView { window: &mut Window, cx: &mut Context, ) { - self.permission_dropdown_handle.clone().toggle(window, cx); + let menu_handle = self.permission_dropdown_handle.clone(); + window.defer(cx, move |window, cx| { + menu_handle.toggle(window, cx); + }); } fn open_add_context_menu( @@ -7816,6 +8180,8 @@ impl Render for ThreadView { .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::handle_toggle_command_pattern)) .on_action(cx.listener(Self::open_permission_dropdown)) .on_action(cx.listener(Self::open_add_context_menu)) .on_action(cx.listener(|this, _: &ToggleFastMode, _window, cx| {