From a790e514af4d6957aa1a14cc8190b2ff24a0484c Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Fri, 29 Aug 2025 14:58:54 -0300 Subject: [PATCH] Fix ACP permission request with new tool calls (#37182) Release Notes: - Gemini integration: Fixed a bug with permission requests when `always_allow_tool_calls` is enabled --- Cargo.lock | 1 + crates/acp_thread/Cargo.toml | 1 + crates/acp_thread/src/acp_thread.rs | 35 ++++++++++++++++++++++++++-- crates/acp_thread/src/connection.rs | 17 +++++++------- crates/agent2/src/agent.rs | 13 ++++------- crates/agent_servers/src/acp.rs | 36 ++++------------------------- 6 files changed, 53 insertions(+), 50 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index aa1bcab9a68294baa4264916ef5a35adbeb20802..e201b4af804b0be95f100c34f93652b6ecf6f8e6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8,6 +8,7 @@ version = "0.1.0" dependencies = [ "action_log", "agent-client-protocol", + "agent_settings", "anyhow", "buffer_diff", "collections", diff --git a/crates/acp_thread/Cargo.toml b/crates/acp_thread/Cargo.toml index eab756db51885b8b2e2797bbf0303937f19fefb9..196614f731c6e330328e46eb75ba58cf928cf6cc 100644 --- a/crates/acp_thread/Cargo.toml +++ b/crates/acp_thread/Cargo.toml @@ -19,6 +19,7 @@ test-support = ["gpui/test-support", "project/test-support", "dep:parking_lot"] action_log.workspace = true agent-client-protocol.workspace = true anyhow.workspace = true +agent_settings.workspace = true buffer_diff.workspace = true collections.workspace = true editor.workspace = true diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 04ff032ad40c600c80fed7cff9f48139b2307931..394619732a72c205b6c5c940cc8b2b7d3a6d3d38 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -3,6 +3,7 @@ mod diff; mod mention; mod terminal; +use agent_settings::AgentSettings; use collections::HashSet; pub use connection::*; pub use diff::*; @@ -10,6 +11,7 @@ use language::language_settings::FormatOnSave; pub use mention::*; use project::lsp_store::{FormatTrigger, LspFormatTarget}; use serde::{Deserialize, Serialize}; +use settings::Settings as _; pub use terminal::*; use action_log::ActionLog; @@ -1230,9 +1232,29 @@ impl AcpThread { tool_call: acp::ToolCallUpdate, options: Vec, cx: &mut Context, - ) -> Result, acp::Error> { + ) -> Result> { let (tx, rx) = oneshot::channel(); + if 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.id.clone()) + } else { + None + } + }) { + self.upsert_tool_call_inner(tool_call, ToolCallStatus::Pending, cx)?; + return Ok(async { + acp::RequestPermissionOutcome::Selected { + option_id: allow_once_option, + } + } + .boxed()); + } + } + let status = ToolCallStatus::WaitingForConfirmation { options, respond_tx: tx, @@ -1240,7 +1262,16 @@ impl AcpThread { self.upsert_tool_call_inner(tool_call, status, cx)?; cx.emit(AcpThreadEvent::ToolAuthorizationRequired); - Ok(rx) + + let fut = async { + match rx.await { + Ok(option) => acp::RequestPermissionOutcome::Selected { option_id: option }, + Err(oneshot::Canceled) => acp::RequestPermissionOutcome::Cancelled, + } + } + .boxed(); + + Ok(fut) } pub fn authorize_tool_call( diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index af229b7545651c2f19f361afc7ea0abadcb5cc76..96abd1d2b4cf92698e7046cd4b7e24e6043280ff 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -393,14 +393,15 @@ mod test_support { }; let task = cx.spawn(async move |cx| { if let Some((tool_call, options)) = permission_request { - let permission = thread.update(cx, |thread, cx| { - thread.request_tool_call_authorization( - tool_call.clone().into(), - options.clone(), - cx, - ) - })?; - permission?.await?; + thread + .update(cx, |thread, cx| { + thread.request_tool_call_authorization( + tool_call.clone().into(), + options.clone(), + cx, + ) + })?? + .await; } thread.update(cx, |thread, cx| { thread.handle_session_update(update.clone(), cx).unwrap(); diff --git a/crates/agent2/src/agent.rs b/crates/agent2/src/agent.rs index ea80df8fb52cffab80c8c64307b75de7f0954a56..bb6a3c097ca27d6103c1072986f6d3255bc6c69f 100644 --- a/crates/agent2/src/agent.rs +++ b/crates/agent2/src/agent.rs @@ -762,18 +762,15 @@ impl NativeAgentConnection { options, response, }) => { - let recv = acp_thread.update(cx, |thread, cx| { + let outcome_task = acp_thread.update(cx, |thread, cx| { thread.request_tool_call_authorization(tool_call, options, cx) - })?; + })??; cx.background_spawn(async move { - if let Some(recv) = recv.log_err() - && let Some(option) = recv - .await - .context("authorization sender was dropped") - .log_err() + if let acp::RequestPermissionOutcome::Selected { option_id } = + outcome_task.await { response - .send(option) + .send(option_id) .map(|_| anyhow!("authorization receiver was dropped")) .log_err(); } diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index d929d1fc501fb2093f47f8bdeb4d3695b7b87ebf..b1d4bea5c35c113277847690906dd2f21e12050c 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -3,15 +3,13 @@ use acp_thread::AgentConnection; use acp_tools::AcpConnectionRegistry; use action_log::ActionLog; use agent_client_protocol::{self as acp, Agent as _, ErrorCode}; -use agent_settings::AgentSettings; use anyhow::anyhow; use collections::HashMap; use futures::AsyncBufReadExt as _; -use futures::channel::oneshot; use futures::io::BufReader; use project::Project; use serde::Deserialize; -use settings::Settings as _; + use std::{any::Any, cell::RefCell}; use std::{path::Path, rc::Rc}; use thiserror::Error; @@ -345,28 +343,7 @@ impl acp::Client for ClientDelegate { ) -> Result { let cx = &mut self.cx.clone(); - // If always_allow_tool_actions is enabled, then auto-choose the first "Allow" button - if AgentSettings::try_read_global(cx, |settings| settings.always_allow_tool_actions) - .unwrap_or(false) - { - // 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) = arguments.options.iter().find_map(|option| { - if matches!(option.kind, acp::PermissionOptionKind::AllowOnce) { - Some(option.id.clone()) - } else { - None - } - }) { - return Ok(acp::RequestPermissionResponse { - outcome: acp::RequestPermissionOutcome::Selected { - option_id: allow_once_option, - }, - }); - } - } - - let rx = self + let task = self .sessions .borrow() .get(&arguments.session_id) @@ -374,14 +351,9 @@ impl acp::Client for ClientDelegate { .thread .update(cx, |thread, cx| { thread.request_tool_call_authorization(arguments.tool_call, arguments.options, cx) - })?; + })??; - let result = rx?.await; - - let outcome = match result { - Ok(option) => acp::RequestPermissionOutcome::Selected { option_id: option }, - Err(oneshot::Canceled) => acp::RequestPermissionOutcome::Cancelled, - }; + let outcome = task.await; Ok(acp::RequestPermissionResponse { outcome }) }