From f8b74acc0fdff0cbfb233f24088592db1cefba1f Mon Sep 17 00:00:00 2001 From: Eric Holk Date: Mon, 23 Mar 2026 09:13:58 -0700 Subject: [PATCH] Move permission outcome construction out of the UI layer (#52050) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the terminal permission params refactor, addressing @benbrandt's [review feedback](https://github.com/zed-industries/zed/pull/51782#pullrequestreview-2926899804) about string formatting leaking into the UI layer. ## Changes ### Outcome construction moved to acp_thread - Added `PermissionOptionChoice::build_outcome(is_allow)` — builds a `SelectedPermissionOutcome` from a choice, attaching `SelectedPermissionParams::Terminal` when the choice carries `sub_patterns`. - Added `PermissionOptions::build_outcome_for_checked_patterns(checked_indices, is_allow)` — handles the `DropdownWithPatterns` per-command checklist flow, returning `None` when zero patterns are checked (degrading to once). The UI's `authorize_with_granularity` no longer does any `format!("always_allow:{}",...)` string formatting or `SelectedPermissionParams` construction. ### `option_kind` folded into `SelectedPermissionOutcome` `SelectedPermissionOutcome` now carries `option_kind: acp::PermissionOptionKind`, eliminating the separate parameter that was threaded through the entire `authorize_tool_call` chain: ``` ThreadView::authorize_tool_call → Conversation::authorize_tool_call → AcpThread::authorize_tool_call ``` Every signature in this chain dropped one parameter. ### `SelectedPermissionParams` removed from UI imports The type is now only referenced by `acp_thread` (construction) and `agent` (consumption). The UI passes `SelectedPermissionOutcome` opaquely. Release Notes: - N/A --- crates/acp_thread/src/acp_thread.rs | 13 +-- crates/acp_thread/src/connection.rs | 69 ++++++++++++++++ crates/agent/src/tests/mod.rs | 15 +++- crates/agent/src/tools/copy_path_tool.rs | 10 ++- .../agent/src/tools/create_directory_tool.rs | 10 ++- crates/agent/src/tools/delete_path_tool.rs | 10 ++- crates/agent/src/tools/edit_file_tool.rs | 5 +- crates/agent/src/tools/list_directory_tool.rs | 5 +- crates/agent/src/tools/move_path_tool.rs | 10 ++- crates/agent/src/tools/read_file_tool.rs | 10 ++- .../src/tools/restore_file_from_disk_tool.rs | 10 ++- crates/agent/src/tools/save_file_tool.rs | 15 +++- .../src/tools/streaming_edit_file_tool.rs | 5 +- crates/agent_servers/src/e2e_tests.rs | 6 +- crates/agent_ui/src/conversation_view.rs | 31 +++---- .../src/conversation_view/thread_view.rs | 80 +++---------------- 16 files changed, 189 insertions(+), 115 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 5fb27bcd5289f60bcdfdb6adc9007c86c60fbad7..e11d86196ec6367ee6d2ded709c3ba9e100da514 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -502,13 +502,15 @@ pub enum SelectedPermissionParams { #[derive(Debug)] pub struct SelectedPermissionOutcome { pub option_id: acp::PermissionOptionId, + pub option_kind: acp::PermissionOptionKind, pub params: Option, } impl SelectedPermissionOutcome { - pub fn new(option_id: acp::PermissionOptionId) -> Self { + pub fn new(option_id: acp::PermissionOptionId, option_kind: acp::PermissionOptionKind) -> Self { Self { option_id, + option_kind, params: None, } } @@ -519,12 +521,6 @@ impl SelectedPermissionOutcome { } } -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) @@ -2013,14 +2009,13 @@ impl AcpThread { &mut self, id: acp::ToolCallId, outcome: SelectedPermissionOutcome, - option_kind: acp::PermissionOptionKind, cx: &mut Context, ) { let Some((ix, call)) = self.tool_call_mut(&id) else { return; }; - let new_status = match option_kind { + let new_status = match outcome.option_kind { acp::PermissionOptionKind::RejectOnce | acp::PermissionOptionKind::RejectAlways => { ToolCallStatus::Rejected } diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 1c6f2a49f18b4b6c664f49d1a732de9a53c75aab..58a8aa33830f12ffb713490c87c47133cc2ad96f 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -477,6 +477,24 @@ impl PermissionOptionChoice { pub fn label(&self) -> SharedString { self.allow.name.clone().into() } + + /// Build a `SelectedPermissionOutcome` for this choice. + /// + /// If the choice carries `sub_patterns`, they are attached as + /// `SelectedPermissionParams::Terminal`. + pub fn build_outcome(&self, is_allow: bool) -> crate::SelectedPermissionOutcome { + let option = if is_allow { &self.allow } else { &self.deny }; + + let params = if !self.sub_patterns.is_empty() { + Some(crate::SelectedPermissionParams::Terminal { + patterns: self.sub_patterns.clone(), + }) + } else { + None + }; + + crate::SelectedPermissionOutcome::new(option.option_id.clone(), option.kind).params(params) + } } /// Pairs a tool's permission pattern with its display name @@ -548,6 +566,57 @@ impl PermissionOptions { self.first_option_of_kind(acp::PermissionOptionKind::RejectOnce) .map(|option| option.option_id.clone()) } + + /// Build a `SelectedPermissionOutcome` for the `DropdownWithPatterns` + /// variant when the user has checked specific pattern indices. + /// + /// Returns `Some` with the always-allow/deny outcome when at least one + /// pattern is checked. Returns `None` when zero patterns are checked, + /// signaling that the caller should degrade to allow-once / deny-once. + /// + /// Panics (debug) or returns `None` (release) if called on a non- + /// `DropdownWithPatterns` variant. + pub fn build_outcome_for_checked_patterns( + &self, + checked_indices: &[usize], + is_allow: bool, + ) -> Option { + let PermissionOptions::DropdownWithPatterns { + choices, patterns, .. + } = self + else { + debug_assert!( + false, + "build_outcome_for_checked_patterns called on non-DropdownWithPatterns" + ); + return None; + }; + + let checked_patterns: Vec = patterns + .iter() + .enumerate() + .filter(|(index, _)| checked_indices.contains(index)) + .map(|(_, cp)| cp.pattern.clone()) + .collect(); + + if checked_patterns.is_empty() { + return None; + } + + // Use the first choice (the "Always" choice) as the base for the outcome. + let always_choice = choices.first()?; + let option = if is_allow { + &always_choice.allow + } else { + &always_choice.deny + }; + + let outcome = crate::SelectedPermissionOutcome::new(option.option_id.clone(), option.kind) + .params(Some(crate::SelectedPermissionParams::Terminal { + patterns: checked_patterns, + })); + Some(outcome) + } } #[cfg(feature = "test-support")] diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 5cb2c99bfae222b13fd978e1bc3eba2fe98ca1d6..8a291a89e2f2a18b6180d288179406a8ba527d25 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -841,14 +841,20 @@ 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").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .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").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("deny"), + acp::PermissionOptionKind::RejectOnce, + )) .unwrap(); cx.run_until_parked(); @@ -892,7 +898,10 @@ 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").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("always_allow:tool_requiring_permission"), + acp::PermissionOptionKind::AllowAlways, + )) .unwrap(); cx.run_until_parked(); let completion = fake_model.pending_completions().pop().unwrap(); diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index 7955a6cc0755514ba4341e43af980e9b93478134..95688f27dcd8ca04aef72358ce52144f95138e17 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -266,7 +266,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let result = task.await; @@ -372,7 +375,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); assert!( diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index 7052b5dfdc2c7d546f5e477430d6de1a0039b03d..d6c59bcce30ab26991edba0fa7181ec45d10e1b0 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -241,7 +241,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let result = task.await; @@ -359,7 +362,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); assert!( diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index 9b2c0a20b8a26b57ef77bb91004c079265fc80cf..7433975c7b782a145dd3e5a80ee59cd92945a989 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -301,7 +301,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let result = task.await; @@ -428,7 +431,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); assert!( diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 3325a612a0143070a3fc157976be93276f98cb5f..0b4d7ce5eb94b79ed8f822e14b76c191788afcf9 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -1374,7 +1374,10 @@ mod tests { event .response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .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 7769669222631f7a2a4bd9de1e0d81a68665a816..7abbe1ed4c488210b9079e59765dddc8d5208bed 100644 --- a/crates/agent/src/tools/list_directory_tool.rs +++ b/crates/agent/src/tools/list_directory_tool.rs @@ -848,7 +848,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .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 ab5637c26d250b5866ebdc015ab6fce294adc7e7..147947bb67ec646c38b51f37dd75779ed78ec85b 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -273,7 +273,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let result = task.await; @@ -379,7 +382,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); assert!( diff --git a/crates/agent/src/tools/read_file_tool.rs b/crates/agent/src/tools/read_file_tool.rs index ef33d7d5b9d0f04783849ebd681badd04b7df052..093a8580892cfc4cec0a061bcc10717b28c608f2 100644 --- a/crates/agent/src/tools/read_file_tool.rs +++ b/crates/agent/src/tools/read_file_tool.rs @@ -896,7 +896,10 @@ mod test { ); authorization .response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let result = read_task.await; @@ -1185,7 +1188,10 @@ mod test { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .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 ffe886b73c217e7afe4c5a8754b12d15be4b9b0d..9273ea5b8bb041e0ea53f3ea72b94b46e5a7e294 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,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let _result = task.await; @@ -651,7 +654,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); assert!( diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index 3f741d2eea7794111e039dcb981a5239d96b7b65..c6a1cd79db65127164fe66f966029b58a366da7f 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -518,7 +518,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); let _result = task.await; @@ -646,7 +649,10 @@ mod tests { ); auth.response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); assert!( @@ -727,7 +733,10 @@ mod tests { let auth = event_rx.expect_authorization().await; auth.response - .send(acp::PermissionOptionId::new("deny").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("deny"), + acp::PermissionOptionKind::RejectOnce, + )) .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 9e7a7bbf1f287a8791591f3ae80a8731802eda42..065ff49f8498025830a6491344a57c7ae6053f5e 100644 --- a/crates/agent/src/tools/streaming_edit_file_tool.rs +++ b/crates/agent/src/tools/streaming_edit_file_tool.rs @@ -2581,7 +2581,10 @@ mod tests { event .response - .send(acp::PermissionOptionId::new("allow").into()) + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow"), + acp::PermissionOptionKind::AllowOnce, + )) .unwrap(); authorize_task.await.unwrap(); } diff --git a/crates/agent_servers/src/e2e_tests.rs b/crates/agent_servers/src/e2e_tests.rs index 93c37b5e258c5342be2c02eb3762bd775e22d001..956d106df2a260bd2eb31c14f4f1f1705bf74cd6 100644 --- a/crates/agent_servers/src/e2e_tests.rs +++ b/crates/agent_servers/src/e2e_tests.rs @@ -208,8 +208,10 @@ pub async fn test_tool_call_with_permission( thread.update(cx, |thread, cx| { thread.authorize_tool_call( tool_call_id, - allow_option_id.into(), - acp::PermissionOptionKind::AllowOnce, + acp_thread::SelectedPermissionOutcome::new( + allow_option_id, + acp::PermissionOptionKind::AllowOnce, + ), cx, ); diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index 28c823430c4cf92e24075fb56a526a75244b45c9..5ed33487f212d1513f13f00a2f07b3e352cd5fc3 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -1,9 +1,8 @@ use acp_thread::{ AcpThread, AcpThreadEvent, AgentSessionInfo, AgentThreadEntry, AssistantMessage, AssistantMessageChunk, AuthRequired, LoadError, MentionUri, PermissionOptionChoice, - PermissionOptions, PermissionPattern, RetryStatus, SelectedPermissionOutcome, - SelectedPermissionParams, ThreadStatus, ToolCall, ToolCallContent, ToolCallStatus, - UserMessageId, + PermissionOptions, PermissionPattern, RetryStatus, SelectedPermissionOutcome, ThreadStatus, + ToolCall, ToolCallContent, ToolCallStatus, UserMessageId, }; use acp_thread::{AgentConnection, Plan}; use action_log::{ActionLog, ActionLogTelemetry, DiffStats}; @@ -249,8 +248,7 @@ impl Conversation { self.authorize_tool_call( session_id.clone(), tool_call_id, - option.option_id.clone().into(), - option.kind, + SelectedPermissionOutcome::new(option.option_id.clone(), option.kind), cx, ); Some(()) @@ -261,7 +259,6 @@ impl Conversation { session_id: acp::SessionId, tool_call_id: acp::ToolCallId, outcome: SelectedPermissionOutcome, - option_kind: acp::PermissionOptionKind, cx: &mut Context, ) { let Some(thread) = self.threads.get(&session_id) else { @@ -273,11 +270,11 @@ impl Conversation { "Agent Tool Call Authorized", agent = agent_telemetry_id, session = session_id, - option = option_kind + option = outcome.option_kind ); thread.update(cx, |thread, cx| { - thread.authorize_tool_call(tool_call_id, outcome, option_kind, cx); + thread.authorize_tool_call(tool_call_id, outcome, cx); }); cx.notify(); } @@ -6276,8 +6273,10 @@ pub(crate) mod tests { conversation.authorize_tool_call( acp::SessionId::new("session-1"), acp::ToolCallId::new("tc-1"), - acp::PermissionOptionId::new("allow-1").into(), - acp::PermissionOptionKind::AllowOnce, + SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow-1"), + acp::PermissionOptionKind::AllowOnce, + ), cx, ); }); @@ -6299,8 +6298,10 @@ pub(crate) mod tests { conversation.authorize_tool_call( acp::SessionId::new("session-1"), acp::ToolCallId::new("tc-2"), - acp::PermissionOptionId::new("allow-2").into(), - acp::PermissionOptionKind::AllowOnce, + SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow-2"), + acp::PermissionOptionKind::AllowOnce, + ), cx, ); }); @@ -6438,8 +6439,10 @@ pub(crate) mod tests { conversation.authorize_tool_call( acp::SessionId::new("thread-a"), acp::ToolCallId::new("tc-a"), - acp::PermissionOptionId::new("allow-a").into(), - acp::PermissionOptionKind::AllowOnce, + SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("allow-a"), + 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 cafc2b9f369e6854d887ff63073831e48bff6116..b1b7ca98923f5dda70e5b256caeed6c4372584f2 100644 --- a/crates/agent_ui/src/conversation_view/thread_view.rs +++ b/crates/agent_ui/src/conversation_view/thread_view.rs @@ -1580,12 +1580,11 @@ impl ThreadView { session_id: acp::SessionId, tool_call_id: acp::ToolCallId, 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, outcome, option_kind, cx); + conversation.authorize_tool_call(session_id, tool_call_id, outcome, cx); }); if self.should_be_following { self.workspace @@ -1648,8 +1647,7 @@ impl ThreadView { self.authorize_tool_call( self.id.clone(), tool_call_id, - option_id.into(), - option_kind, + SelectedPermissionOutcome::new(option_id, option_kind), window, cx, ); @@ -1740,16 +1738,9 @@ impl ThreadView { 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 choices = match options { + PermissionOptions::Dropdown(choices) => choices.as_slice(), + PermissionOptions::DropdownWithPatterns { choices, .. } => choices.as_slice(), _ => { let kind = if is_allow { acp::PermissionOptionKind::AllowOnce @@ -1763,34 +1754,9 @@ impl ThreadView { 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); + if let Some(PermissionSelection::SelectedPatterns(checked)) = selection { + if let Some(outcome) = options.build_outcome_for_checked_patterns(checked, is_allow) { + self.authorize_tool_call(session_id, tool_call_id, outcome, window, cx); return Some(()); } } @@ -1801,32 +1767,9 @@ impl ThreadView { .unwrap_or_else(|| choices.len().saturating_sub(1)); let selected_choice = choices.get(selected_index).or(choices.last())?; + let outcome = selected_choice.build_outcome(is_allow); - let selected_option = if is_allow { - &selected_choice.allow - } else { - &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, - outcome, - selected_option.kind, - window, - cx, - ); + self.authorize_tool_call(session_id, tool_call_id, outcome, window, cx); Some(()) } @@ -6442,8 +6385,7 @@ impl ThreadView { this.authorize_tool_call( session_id.clone(), tool_call_id.clone(), - option_id.clone().into(), - option_kind, + SelectedPermissionOutcome::new(option_id.clone(), option_kind), window, cx, );