From 36873662cfb02ac495ceea9db2fc82a5031066fc Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Mon, 19 Jan 2026 14:47:04 +0100 Subject: [PATCH] agent: Make sure ACP still gets classic tool permission UI (#47142) This makes sure all of the new granular permission logic and ui only applies to the zed agent and doesn't affect the UI of external agents. Release Notes: - N/A --- crates/acp_thread/src/acp_thread.rs | 12 +- crates/acp_thread/src/connection.rs | 54 +++- crates/agent/src/tests/mod.rs | 131 ++++++++- crates/agent/src/thread.rs | 118 ++++++--- crates/agent_servers/src/acp.rs | 2 +- crates/agent_ui/src/acp/thread_view.rs | 354 +++++++++++-------------- 6 files changed, 419 insertions(+), 252 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 5d95563e14831880ec34db5a413143740cbe9963..389b9ca3f8fd97a8372c9855364b9a24007a9126 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -477,7 +477,7 @@ pub enum ToolCallStatus { Pending, /// The tool call is waiting for confirmation from the user. WaitingForConfirmation { - options: Vec, + options: PermissionOptions, respond_tx: oneshot::Sender, }, /// The tool call is currently running. @@ -1717,7 +1717,7 @@ impl AcpThread { pub fn request_tool_call_authorization( &mut self, tool_call: acp::ToolCallUpdate, - options: Vec, + options: PermissionOptions, respect_always_allow_setting: bool, cx: &mut Context, ) -> Result> { @@ -1726,13 +1726,7 @@ impl AcpThread { if respect_always_allow_setting && AgentSettings::get_global(cx).always_allow_tool_actions { // Don't use AllowAlways, because then if you were to turn off always_allow_tool_actions, // some tools would (incorrectly) continue to auto-accept. - if let Some(allow_once_option) = options.iter().find_map(|option| { - if matches!(option.kind, acp::PermissionOptionKind::AllowOnce) { - Some(option.option_id.clone()) - } else { - None - } - }) { + if let Some(allow_once_option) = options.allow_once_option_id() { self.upsert_tool_call_inner(tool_call, ToolCallStatus::Pending, cx)?; return Ok(async { acp::RequestPermissionOutcome::Selected(acp::SelectedPermissionOutcome::new( diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 46380f94b0e3ce121d32a485f955288285898443..cd8b40e30361a26721428af417fd309193b43d18 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -387,6 +387,56 @@ impl AgentModelList { } } +#[derive(Debug, Clone)] +pub struct PermissionOptionChoice { + pub allow: acp::PermissionOption, + pub deny: acp::PermissionOption, +} + +impl PermissionOptionChoice { + pub fn label(&self) -> SharedString { + self.allow.name.clone().into() + } +} + +#[derive(Debug, Clone)] +pub enum PermissionOptions { + Flat(Vec), + Dropdown(Vec), +} + +impl PermissionOptions { + pub fn is_empty(&self) -> bool { + match self { + PermissionOptions::Flat(options) => options.is_empty(), + PermissionOptions::Dropdown(options) => options.is_empty(), + } + } + + pub fn first_option_of_kind( + &self, + kind: acp::PermissionOptionKind, + ) -> Option<&acp::PermissionOption> { + match self { + PermissionOptions::Flat(options) => options.iter().find(|option| option.kind == kind), + PermissionOptions::Dropdown(options) => options.iter().find_map(|choice| { + if choice.allow.kind == kind { + Some(&choice.allow) + } else if choice.deny.kind == kind { + Some(&choice.deny) + } else { + None + } + }), + } + } + + pub fn allow_once_option_id(&self) -> Option { + self.first_option_of_kind(acp::PermissionOptionKind::AllowOnce) + .map(|option| option.option_id.clone()) + } +} + #[cfg(feature = "test-support")] mod test_support { //! Test-only stubs and helpers for acp_thread. @@ -435,7 +485,7 @@ mod test_support { #[derive(Clone, Default)] pub struct StubAgentConnection { sessions: Arc>>, - permission_requests: HashMap>, + permission_requests: HashMap, next_prompt_updates: Arc>>, } @@ -459,7 +509,7 @@ mod test_support { pub fn with_permission_requests( mut self, - permission_requests: HashMap>, + permission_requests: HashMap, ) -> Self { self.permission_requests = permission_requests; self diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index bd26c10cae39432c5d989fef9567a26681921750..e3ff61bb6a39535e32b5596d25235c0dfcd7344e 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -1,5 +1,7 @@ use super::*; -use acp_thread::{AgentConnection, AgentModelGroupName, AgentModelList, UserMessageId}; +use acp_thread::{ + AgentConnection, AgentModelGroupName, AgentModelList, PermissionOptions, UserMessageId, +}; use agent_client_protocol::{self as acp}; use agent_settings::AgentProfileId; use anyhow::Result; @@ -947,23 +949,132 @@ async fn next_tool_call_authorization( if let ThreadEvent::ToolCallAuthorization(tool_call_authorization) = event { let permission_kinds = tool_call_authorization .options - .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 + .first_option_of_kind(acp::PermissionOptionKind::AllowAlways) + .map(|option| option.kind); + let allow_once = tool_call_authorization + .options + .first_option_of_kind(acp::PermissionOptionKind::AllowOnce) + .map(|option| option.kind); + assert_eq!( permission_kinds, - vec![ - acp::PermissionOptionKind::AllowAlways, - acp::PermissionOptionKind::AllowOnce, - ] + Some(acp::PermissionOptionKind::AllowAlways) ); + assert_eq!(allow_once, Some(acp::PermissionOptionKind::AllowOnce)); return tool_call_authorization; } } } +#[test] +fn test_permission_options_terminal_with_pattern() { + let permission_options = + ToolPermissionContext::new("terminal", "cargo build --release").build_permission_options(); + + let PermissionOptions::Dropdown(choices) = permission_options else { + panic!("Expected dropdown permission options"); + }; + + assert_eq!(choices.len(), 3); + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); + assert!(labels.contains(&"Always for terminal")); + assert!(labels.contains(&"Always for `cargo` commands")); + assert!(labels.contains(&"Only this time")); +} + +#[test] +fn test_permission_options_edit_file_with_path_pattern() { + let permission_options = + ToolPermissionContext::new("edit_file", "src/main.rs").build_permission_options(); + + let PermissionOptions::Dropdown(choices) = permission_options else { + panic!("Expected dropdown permission options"); + }; + + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); + assert!(labels.contains(&"Always for edit file")); + assert!(labels.contains(&"Always for `src/`")); +} + +#[test] +fn test_permission_options_fetch_with_domain_pattern() { + let permission_options = + ToolPermissionContext::new("fetch", "https://docs.rs/gpui").build_permission_options(); + + let PermissionOptions::Dropdown(choices) = permission_options else { + panic!("Expected dropdown permission options"); + }; + + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); + assert!(labels.contains(&"Always for fetch")); + assert!(labels.contains(&"Always for `docs.rs`")); +} + +#[test] +fn test_permission_options_without_pattern() { + let permission_options = ToolPermissionContext::new("terminal", "./deploy.sh --production") + .build_permission_options(); + + let PermissionOptions::Dropdown(choices) = permission_options else { + panic!("Expected dropdown permission options"); + }; + + 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")); + assert!(!labels.iter().any(|label| label.contains("commands"))); +} + +#[test] +fn test_permission_option_ids_for_terminal() { + let permission_options = + ToolPermissionContext::new("terminal", "cargo build --release").build_permission_options(); + + let PermissionOptions::Dropdown(choices) = permission_options else { + 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(); + + 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:")), + "Missing allow pattern option" + ); + + 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:")), + "Missing deny pattern option" + ); +} + #[gpui::test] #[cfg_attr(not(feature = "e2e"), ignore)] async fn test_concurrent_tool_calls(cx: &mut TestAppContext) { diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 444a06589676554bbb5d9e39f6aedec2ab153954..c403258514febc147a11b25e1f82e80ad81c50bf 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -612,7 +612,7 @@ impl ToolPermissionContext { /// /// 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 { + pub fn build_permission_options(&self) -> acp_thread::PermissionOptions { use crate::pattern_extraction::*; let tool_name = &self.tool_name; @@ -634,11 +634,30 @@ impl ToolPermissionContext { _ => (None, None), }; - let mut options = vec![acp::PermissionOption::new( - acp::PermissionOptionId::new(format!("always:{}", tool_name)), + 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, + ), + }); + }; + + push_choice( format!("Always for {}", tool_name.replace('_', " ")), + format!("always_allow:{}", tool_name), + format!("always_deny:{}", tool_name), acp::PermissionOptionKind::AllowAlways, - )]; + acp::PermissionOptionKind::RejectAlways, + ); if let (Some(pattern), Some(display)) = (pattern, pattern_display) { let button_text = match tool_name.as_str() { @@ -646,27 +665,31 @@ impl ToolPermissionContext { "fetch" => format!("Always for `{}`", display), _ => format!("Always for `{}`", display), }; - options.push(acp::PermissionOption::new( - acp::PermissionOptionId::new(format!("always_pattern:{}:{}", tool_name, pattern)), + push_choice( button_text, + format!("always_allow_pattern:{}:{}", tool_name, pattern), + format!("always_deny_pattern:{}:{}", tool_name, pattern), acp::PermissionOptionKind::AllowAlways, - )); + acp::PermissionOptionKind::RejectAlways, + ); } - options.push(acp::PermissionOption::new( - acp::PermissionOptionId::new("once"), - "Only this time", + push_choice( + "Only this time".to_string(), + "allow".to_string(), + "deny".to_string(), acp::PermissionOptionKind::AllowOnce, - )); + acp::PermissionOptionKind::RejectOnce, + ); - options + acp_thread::PermissionOptions::Dropdown(choices) } } #[derive(Debug)] pub struct ToolCallAuthorization { pub tool_call: acp::ToolCallUpdate, - pub options: Vec, + pub options: acp_thread::PermissionOptions, pub response: oneshot::Sender, pub context: Option, } @@ -2937,10 +2960,9 @@ impl ToolCallEventStream { /// 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 + /// Uses the dropdown authorization flow with two granularities: + /// - "Always for MCP tool" → sets `tools..default_mode = "allow"` or "deny" + /// - "Only this time" → allow/deny once pub fn authorize_third_party_tool( &self, title: impl Into, @@ -2967,23 +2989,38 @@ impl ToolCallEventStream { self.tool_use_id.to_string(), acp::ToolCallUpdateFields::new().title(title.into()), ), - options: vec![ - acp::PermissionOption::new( - 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 once", - acp::PermissionOptionKind::AllowOnce, - ), - acp::PermissionOption::new( - acp::PermissionOptionId::new("deny"), - "Deny", - acp::PermissionOptionKind::RejectOnce, - ), - ], + options: acp_thread::PermissionOptions::Dropdown(vec![ + acp_thread::PermissionOptionChoice { + allow: acp::PermissionOption::new( + acp::PermissionOptionId::new(format!( + "always_allow_mcp:{}", + tool_id + )), + format!("Always for {} MCP tool", display_name), + acp::PermissionOptionKind::AllowAlways, + ), + deny: acp::PermissionOption::new( + acp::PermissionOptionId::new(format!( + "always_deny_mcp:{}", + tool_id + )), + format!("Always for {} MCP tool", display_name), + acp::PermissionOptionKind::RejectAlways, + ), + }, + 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, + ), + }, + ]), response: response_tx, context: None, }, @@ -3007,6 +3044,19 @@ impl ToolCallEventStream { } 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_mode(&tool_id, ToolPermissionMode::Deny); + }); + }); + } + return Err(anyhow!("Permission to run tool denied by user")); + } if response_str == "allow" { return Ok(()); diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index 5b82737daf06c14fa4e4612b661236b1fd603760..f2a87879bffa3ac4f76582d310a90cfdcc9fb529 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -1070,7 +1070,7 @@ impl acp::Client for ClientDelegate { let task = thread.update(cx, |thread, cx| { thread.request_tool_call_authorization( arguments.tool_call, - arguments.options, + acp_thread::PermissionOptions::Flat(arguments.options), respect_always_allow_setting, cx, ) diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index d83ce21608e78fe260dde336f393c6bf180541fa..2d168d6d7ce1d7abf8ed693c8abd72d95feae95c 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -1,7 +1,8 @@ use acp_thread::{ AcpThread, AcpThreadEvent, AgentSessionInfo, AgentThreadEntry, AssistantMessage, - AssistantMessageChunk, AuthRequired, LoadError, MentionUri, RetryStatus, ThreadStatus, - ToolCall, ToolCallContent, ToolCallStatus, UserMessageId, + AssistantMessageChunk, AuthRequired, LoadError, MentionUri, PermissionOptionChoice, + PermissionOptions, RetryStatus, ThreadStatus, ToolCall, ToolCallContent, ToolCallStatus, + UserMessageId, }; use acp_thread::{AgentConnection, Plan}; use action_log::{ActionLog, ActionLogTelemetry}; @@ -3121,7 +3122,6 @@ impl AcpThreadView { ) }) .child(self.render_permission_buttons( - tool_call.kind, options, entry_ix, tool_call.id.clone(), @@ -3907,8 +3907,24 @@ impl AcpThreadView { fn render_permission_buttons( &self, - kind: acp::ToolKind, - options: &[acp::PermissionOption], + options: &PermissionOptions, + entry_ix: usize, + tool_call_id: acp::ToolCallId, + cx: &Context, + ) -> Div { + match options { + PermissionOptions::Flat(options) => { + self.render_permission_buttons_flat(options, entry_ix, tool_call_id, cx) + } + PermissionOptions::Dropdown(options) => { + self.render_permission_buttons_dropdown(options, entry_ix, tool_call_id, cx) + } + } + } + + fn render_permission_buttons_dropdown( + &self, + choices: &[PermissionOptionChoice], entry_ix: usize, tool_call_id: acp::ToolCallId, cx: &Context, @@ -3920,77 +3936,26 @@ impl AcpThreadView { .is_some_and(|call| call.id == tool_call_id) }); - // 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); - } - - 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)); + .unwrap_or_else(|| choices.len().saturating_sub(1)); - let selected_option = granularity_options - .get(selected_index) - .or(granularity_options.last()) - .copied(); + let selected_choice = choices.get(selected_index).or(choices.last()); - let dropdown_label: SharedString = selected_option - .map(|o| o.name.clone().into()) + let dropdown_label: SharedString = selected_choice + .map(|choice| choice.label()) .unwrap_or_else(|| "Only this time".into()); 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") - }; - - 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, - }; - + if let Some(choice) = selected_choice { ( - acp::PermissionOptionId::new(allow_id), - allow_kind, - acp::PermissionOptionId::new(deny_id), - deny_kind, + choice.allow.option_id.clone(), + choice.allow.kind, + choice.deny.option_id.clone(), + choice.deny.kind, ) } else { ( @@ -4077,7 +4042,7 @@ impl AcpThreadView { ), ) .child(self.render_permission_granularity_dropdown( - &granularity_options, + choices, dropdown_label, entry_ix, tool_call_id, @@ -4089,7 +4054,7 @@ impl AcpThreadView { fn render_permission_granularity_dropdown( &self, - granularity_options: &[&acp::PermissionOption], + choices: &[PermissionOptionChoice], current_label: SharedString, entry_ix: usize, tool_call_id: acp::ToolCallId, @@ -4097,10 +4062,10 @@ impl AcpThreadView { is_first: bool, cx: &Context, ) -> impl IntoElement { - let menu_options: Vec<(usize, SharedString)> = granularity_options + let menu_options: Vec<(usize, SharedString)> = choices .iter() .enumerate() - .map(|(i, o)| (i, o.name.clone().into())) + .map(|(i, choice)| (i, choice.label())) .collect(); PopoverMenu::new(("permission-granularity", entry_ix)) @@ -4156,7 +4121,7 @@ impl AcpThreadView { }) } - fn render_permission_buttons_legacy( + fn render_permission_buttons_flat( &self, options: &[acp::PermissionOption], entry_ix: usize, @@ -6286,62 +6251,37 @@ impl AcpThreadView { }; 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(); + 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); + }; // 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) + .unwrap_or_else(|| choices.len().saturating_sub(1)); + + let selected_choice = choices.get(selected_index).or(choices.last())?; + + let selected_option = if is_allow { + &selected_choice.allow } 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) + &selected_choice.deny }; - self.authorize_tool_call(tool_call_id, final_option_id, final_option_kind, window, cx); + self.authorize_tool_call( + tool_call_id, + selected_option.option_id.clone(), + selected_option.kind, + window, + cx, + ); Some(()) } @@ -6397,7 +6337,7 @@ impl AcpThreadView { let ToolCallStatus::WaitingForConfirmation { options, .. } = &tool_call.status else { return None; }; - let option = options.iter().find(|o| o.kind == kind)?; + let option = options.first_option_of_kind(kind)?; self.authorize_tool_call( tool_call.id.clone(), @@ -8463,11 +8403,11 @@ pub(crate) mod tests { let connection = StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( tool_call_id, - vec![acp::PermissionOption::new( + PermissionOptions::Flat(vec![acp::PermissionOption::new( "1", "Allow", acp::PermissionOptionKind::AllowOnce, - )], + )]), )])); connection.set_next_prompt_updates(vec![acp::SessionUpdate::ToolCall(tool_call)]); @@ -9866,14 +9806,21 @@ pub(crate) mod tests { if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = &tool_call.status { + let PermissionOptions::Dropdown(choices) = options else { + panic!("Expected dropdown permission options"); + }; + assert_eq!( - options.len(), + choices.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(); + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); assert!( labels.contains(&"Always for terminal"), "Missing 'Always for terminal' option" @@ -9952,7 +9899,14 @@ pub(crate) mod tests { if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = &tool_call.status { - let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + let PermissionOptions::Dropdown(choices) = options else { + panic!("Expected dropdown permission options"); + }; + + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); assert!( labels.contains(&"Always for edit file"), "Missing 'Always for edit file' option" @@ -10029,7 +9983,14 @@ pub(crate) mod tests { if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = &tool_call.status { - let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + let PermissionOptions::Dropdown(choices) = options else { + panic!("Expected dropdown permission options"); + }; + + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); assert!( labels.contains(&"Always for fetch"), "Missing 'Always for fetch' option" @@ -10107,13 +10068,20 @@ pub(crate) mod tests { if let acp_thread::ToolCallStatus::WaitingForConfirmation { options, .. } = &tool_call.status { + let PermissionOptions::Dropdown(choices) = options else { + panic!("Expected dropdown permission options"); + }; + assert_eq!( - options.len(), + choices.len(), 2, "Expected 2 permission options (no pattern option)" ); - let labels: Vec<&str> = options.iter().map(|o| o.name.as_ref()).collect(); + let labels: Vec<&str> = choices + .iter() + .map(|choice| choice.allow.name.as_ref()) + .collect(); assert!( labels.contains(&"Always for terminal"), "Missing 'Always for terminal' option" @@ -10258,10 +10226,20 @@ pub(crate) mod tests { 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"); + let pattern_option = match &permission_options { + PermissionOptions::Dropdown(choices) => choices + .iter() + .find(|choice| { + choice + .allow + .option_id + .0 + .starts_with("always_allow_pattern:") + }) + .map(|choice| &choice.allow) + .expect("Should have a pattern option for npm command"), + _ => panic!("Expected dropdown permission options"), + }; // Dispatch action with the pattern option (simulating "Always allow `npm` commands") thread_view.update_in(cx, |_, window, cx| { @@ -10379,20 +10357,26 @@ pub(crate) mod tests { ToolPermissionContext::new("terminal", "npm install").build_permission_options(); // Verify we have the expected options - assert_eq!(permission_options.len(), 3); + let PermissionOptions::Dropdown(choices) = &permission_options else { + panic!("Expected dropdown permission options"); + }; + + assert_eq!(choices.len(), 3); assert!( - permission_options[0] + choices[0] + .allow .option_id .0 - .contains("always:terminal") + .contains("always_allow:terminal") ); assert!( - permission_options[1] + choices[1] + .allow .option_id .0 - .contains("always_pattern:terminal") + .contains("always_allow_pattern:terminal") ); - assert_eq!(permission_options[2].option_id.0.as_ref(), "once"); + assert_eq!(choices[2].allow.option_id.0.as_ref(), "allow"); let connection = StubAgentConnection::new().with_permission_requests(HashMap::from_iter([( @@ -10525,71 +10509,49 @@ pub(crate) mod tests { #[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", - ), - ]; + let permission_options = ToolPermissionContext::new("terminal", "cargo build --release") + .build_permission_options(); - 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); - } + let PermissionOptions::Dropdown(choices) = permission_options else { + panic!("Expected dropdown permission options"); + }; + + let allow_ids: Vec = choices + .iter() + .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!( + allow_ids + .iter() + .any(|id| id.starts_with("always_allow_pattern:terminal:")), + "Missing allow pattern option" + ); } #[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", - ), - ]; + let permission_options = ToolPermissionContext::new("terminal", "cargo build --release") + .build_permission_options(); - 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); - } + let PermissionOptions::Dropdown(choices) = permission_options else { + panic!("Expected dropdown permission options"); + }; + + let deny_ids: Vec = choices + .iter() + .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!( + deny_ids + .iter() + .any(|id| id.starts_with("always_deny_pattern:terminal:")), + "Missing deny pattern option" + ); } }