From 7d2f7cb70e68a7c71093873a921888059162078f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 2 Jul 2025 14:40:16 +0200 Subject: [PATCH 1/2] Replace title with display_name for tool calls Co-authored-by: Ben Brandt --- crates/acp/src/acp.rs | 89 +++++++++++------------------------ crates/acp/src/server.rs | 4 +- crates/acp/src/thread_view.rs | 2 +- 3 files changed, 31 insertions(+), 64 deletions(-) diff --git a/crates/acp/src/acp.rs b/crates/acp/src/acp.rs index b755db232a..0d2a9cec4f 100644 --- a/crates/acp/src/acp.rs +++ b/crates/acp/src/acp.rs @@ -123,7 +123,7 @@ pub enum AgentThreadEntryContent { #[derive(Debug)] pub struct ToolCall { id: ToolCallId, - tool_name: Entity, + display_name: Entity, status: ToolCallStatus, } @@ -271,7 +271,7 @@ impl AcpThread { pub fn request_tool_call( &mut self, - title: String, + display_name: String, confirmation: acp::ToolCallConfirmation, cx: &mut Context, ) -> ToolCallRequest { @@ -282,22 +282,22 @@ impl AcpThread { respond_tx: tx, }; - let id = self.insert_tool_call(title, status, cx); + let id = self.insert_tool_call(display_name, status, cx); ToolCallRequest { id, outcome: rx } } - pub fn push_tool_call(&mut self, title: String, cx: &mut Context) -> ToolCallId { + pub fn push_tool_call(&mut self, display_name: String, cx: &mut Context) -> ToolCallId { let status = ToolCallStatus::Allowed { status: acp::ToolCallStatus::Running, content: None, }; - self.insert_tool_call(title, status, cx) + self.insert_tool_call(display_name, status, cx) } fn insert_tool_call( &mut self, - title: String, + display_name: String, status: ToolCallStatus, cx: &mut Context, ) -> ToolCallId { @@ -307,8 +307,13 @@ impl AcpThread { AgentThreadEntryContent::ToolCall(ToolCall { // todo! clean up id creation id: ToolCallId(ThreadEntryId(self.entries.len() as u64)), - tool_name: cx.new(|cx| { - Markdown::new(title.into(), Some(language_registry.clone()), None, cx) + display_name: cx.new(|cx| { + Markdown::new( + display_name.into(), + Some(language_registry.clone()), + None, + cx, + ) }), status, }), @@ -509,27 +514,27 @@ mod tests { let project = Project::test(fs, [path!("/private/tmp").as_ref()], cx).await; let server = gemini_acp_server(project.clone(), cx.to_async()).unwrap(); let thread = server.create_thread(&mut cx.to_async()).await.unwrap(); - let full_turn = thread.update(cx, |thread, cx| { - thread.send( - "Read the '/private/tmp/foo' file and tell me what you see.", - cx, - ) - }); - - run_until_tool_call(&thread, cx).await; - - let tool_call_id = thread.read_with(cx, |thread, cx| { + thread + .update(cx, |thread, cx| { + thread.send( + "Read the '/private/tmp/foo' file and tell me what you see.", + cx, + ) + }) + .await + .unwrap(); + thread.read_with(cx, |thread, cx| { let AgentThreadEntryContent::ToolCall(ToolCall { id, - tool_name, - status: ToolCallStatus::Allowed { .. }, - }) = &thread.entries().last().unwrap().content + display_name, + status: ToolCallStatus::Allowed { content, .. }, + }) = &thread.entries()[1].content else { panic!(); }; - tool_name.read_with(cx, |md, _cx| { - assert_eq!(md.source(), "read_file"); + display_name.read_with(cx, |md, _cx| { + assert_eq!(md.source(), "ReadFile"); }); // todo! @@ -542,44 +547,6 @@ mod tests { // }); *id }); - - thread.update(cx, |thread, cx| { - thread.authorize_tool_call(tool_call_id, acp::ToolCallConfirmationOutcome::Allow, cx); - assert!(matches!( - thread.entries().last().unwrap().content, - AgentThreadEntryContent::ToolCall(ToolCall { - status: ToolCallStatus::Allowed { .. }, - .. - }) - )); - }); - - full_turn.await.unwrap(); - - thread.read_with(cx, |thread, _| { - assert!(thread.entries.len() >= 3, "{:?}", &thread.entries); - assert!(matches!( - thread.entries[0].content, - AgentThreadEntryContent::Message(Message { - role: Role::User, - .. - }) - )); - assert!(matches!( - thread.entries[1].content, - AgentThreadEntryContent::ToolCall(ToolCall { - status: ToolCallStatus::Allowed { .. }, - .. - }) - )); - assert!(matches!( - thread.entries[2].content, - AgentThreadEntryContent::Message(Message { - role: Role::Assistant, - .. - }) - )); - }); } async fn run_until_tool_call(thread: &Entity, cx: &mut TestAppContext) { diff --git a/crates/acp/src/server.rs b/crates/acp/src/server.rs index ed4032777f..98bb3c35e5 100644 --- a/crates/acp/src/server.rs +++ b/crates/acp/src/server.rs @@ -185,7 +185,7 @@ impl acp::Client for AcpClientDelegate { let ToolCallRequest { id, outcome } = cx .update(|cx| { self.update_thread(&request.thread_id.into(), cx, |thread, cx| { - thread.request_tool_call(request.title, request.confirmation, cx) + thread.request_tool_call(request.display_name, request.confirmation, cx) }) })? .context("Failed to update thread")?; @@ -204,7 +204,7 @@ impl acp::Client for AcpClientDelegate { let entry_id = cx .update(|cx| { self.update_thread(&request.thread_id.into(), cx, |thread, cx| { - thread.push_tool_call(request.title, cx) + thread.push_tool_call(request.display_name, cx) }) })? .context("Failed to update thread")?; diff --git a/crates/acp/src/thread_view.rs b/crates/acp/src/thread_view.rs index 1ca02dab32..183ff22253 100644 --- a/crates/acp/src/thread_view.rs +++ b/crates/acp/src/thread_view.rs @@ -330,7 +330,7 @@ impl AcpThreadView { .color(Color::Muted), ) .child(MarkdownElement::new( - tool_call.tool_name.clone(), + tool_call.display_name.clone(), default_markdown_style(window, cx), )) .child(div().w_full()) From 975a7e6f7f55dc096cd1c5fe25579b6ae060c6dc Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 2 Jul 2025 14:54:24 +0200 Subject: [PATCH 2/2] Fix clicking on tool confirmation buttons Co-authored-by: Ben Brandt --- crates/acp/src/acp.rs | 30 +------------------------ crates/acp/src/thread_view.rs | 42 ++++++++++++++++++----------------- 2 files changed, 23 insertions(+), 49 deletions(-) diff --git a/crates/acp/src/acp.rs b/crates/acp/src/acp.rs index 0d2a9cec4f..33fa793e84 100644 --- a/crates/acp/src/acp.rs +++ b/crates/acp/src/acp.rs @@ -446,13 +446,11 @@ pub struct ToolCallRequest { #[cfg(test)] mod tests { use super::*; - use futures::{FutureExt as _, channel::mpsc, select}; use gpui::{AsyncApp, TestAppContext}; use project::FakeFs; use serde_json::json; use settings::SettingsStore; - use smol::stream::StreamExt; - use std::{env, path::Path, process::Stdio, time::Duration}; + use std::{env, path::Path, process::Stdio}; use util::path; fn init_test(cx: &mut TestAppContext) { @@ -549,32 +547,6 @@ mod tests { }); } - async fn run_until_tool_call(thread: &Entity, cx: &mut TestAppContext) { - let (mut tx, mut rx) = mpsc::channel(1); - - let subscription = cx.update(|cx| { - cx.subscribe(thread, move |thread, _, cx| { - if thread - .read(cx) - .entries - .iter() - .any(|e| matches!(e.content, AgentThreadEntryContent::ToolCall(_))) - { - tx.try_send(()).unwrap(); - } - }) - }); - - select! { - _ = cx.executor().timer(Duration::from_secs(5)).fuse() => { - panic!("Timeout waiting for tool call") - } - _ = rx.next().fuse() => { - drop(subscription); - } - } - } - pub fn gemini_acp_server(project: Entity, mut cx: AsyncApp) -> Result> { let cli_path = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../../gemini-cli/packages/cli"); diff --git a/crates/acp/src/thread_view.rs b/crates/acp/src/thread_view.rs index 183ff22253..b311110c94 100644 --- a/crates/acp/src/thread_view.rs +++ b/crates/acp/src/thread_view.rs @@ -62,7 +62,6 @@ impl AcpThreadView { let child = util::command::new_smol_command("node") .arg(cli_path) .arg("--acp") - .args(["--model", "gemini-2.5-flash"]) .current_dir(root_dir) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) @@ -362,21 +361,24 @@ impl AcpThreadView { .justify_end() .gap_1() .child( - Button::new(("allow", tool_call_id.as_u64()), "Always Allow Edits") - .icon(IconName::CheckDouble) - .icon_position(IconPosition::Start) - .icon_size(IconSize::Small) - .icon_color(Color::Success) - .on_click(cx.listener({ - let id = tool_call_id; - move |this, _, _, cx| { - this.authorize_tool_call( - id, - acp::ToolCallConfirmationOutcome::AlwaysAllow, - cx, - ); - } - })), + Button::new( + ("always_allow", tool_call_id.as_u64()), + "Always Allow Edits", + ) + .icon(IconName::CheckDouble) + .icon_position(IconPosition::Start) + .icon_size(IconSize::Small) + .icon_color(Color::Success) + .on_click(cx.listener({ + let id = tool_call_id; + move |this, _, _, cx| { + this.authorize_tool_call( + id, + acp::ToolCallConfirmationOutcome::AlwaysAllow, + cx, + ); + } + })), ) .child( Button::new(("allow", tool_call_id.as_u64()), "Allow") @@ -430,7 +432,7 @@ impl AcpThreadView { .gap_1() .child( Button::new( - ("allow", tool_call_id.as_u64()), + ("always_allow", tool_call_id.as_u64()), format!("Always Allow {root_command}"), ) .icon(IconName::CheckDouble) @@ -501,7 +503,7 @@ impl AcpThreadView { .gap_1() .child( Button::new( - ("allow", tool_call_id.as_u64()), + ("always_allow_server", tool_call_id.as_u64()), format!("Always Allow {server_name}"), ) .icon(IconName::CheckDouble) @@ -521,7 +523,7 @@ impl AcpThreadView { ) .child( Button::new( - ("allow", tool_call_id.as_u64()), + ("always_allow_tool", tool_call_id.as_u64()), format!("Always Allow {tool_display_name}"), ) .icon(IconName::CheckDouble) @@ -587,7 +589,7 @@ impl AcpThreadView { .justify_end() .gap_1() .child( - Button::new(("allow", tool_call_id.as_u64()), "Always Allow") + Button::new(("always_allow", tool_call_id.as_u64()), "Always Allow") .icon(IconName::CheckDouble) .icon_position(IconPosition::Start) .icon_size(IconSize::Small)