Cargo.lock 🔗
@@ -8,6 +8,7 @@ version = "0.1.0"
dependencies = [
"action_log",
"agent-client-protocol",
+ "agent_settings",
"anyhow",
"buffer_diff",
"collections",
Fix ACP permission request with new tool calls (#37182)
Agus Zubiaga created
Release Notes:
- Gemini integration: Fixed a bug with permission requests when
`always_allow_tool_calls` is enabledChange summary
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(-)
Detailed changes
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
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<acp::PermissionOption>,
cx: &mut Context<Self>,
- ) -> Result<oneshot::Receiver<acp::PermissionOptionId>, acp::Error> {
+ ) -> Result<BoxFuture<'static, acp::RequestPermissionOutcome>> {
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(
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();
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();
}
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<acp::RequestPermissionResponse, acp::Error> {
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 })
}