From 0770ae2612751b9ea80466c58cfbe8524fc103ae Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Thu, 3 Jul 2025 11:50:30 +0200 Subject: [PATCH] ToolCallContent is always available on the ToolCall Co-authored-by: Antonio Scandurra --- crates/acp/src/acp.rs | 82 ++++--- crates/acp/src/server.rs | 4 +- crates/acp/src/thread_view.rs | 441 ++++++++++++++++++---------------- 3 files changed, 278 insertions(+), 249 deletions(-) diff --git a/crates/acp/src/acp.rs b/crates/acp/src/acp.rs index 66adb140536ef863f5624842f22d9ad2b9643ba8..c5dfbd9f771e76f664afab223e0fbc7b73bf9fcc 100644 --- a/crates/acp/src/acp.rs +++ b/crates/acp/src/acp.rs @@ -127,6 +127,7 @@ pub struct ToolCall { id: ToolCallId, label: Entity, icon: IconName, + content: Option, status: ToolCallStatus, } @@ -138,7 +139,6 @@ pub enum ToolCallStatus { }, Allowed { status: acp::ToolCallStatus, - content: Option, }, Rejected, } @@ -146,7 +146,6 @@ pub enum ToolCallStatus { #[derive(Debug)] pub enum ToolCallConfirmation { Edit { - diff: Diff, description: Option>, }, Execute { @@ -187,8 +186,7 @@ impl ToolCallConfirmation { }; match confirmation { - acp::ToolCallConfirmation::Edit { diff, description } => Self::Edit { - diff: Diff::from_acp(diff, language_registry.clone(), cx), + acp::ToolCallConfirmation::Edit { description } => Self::Edit { description: description.map(|description| to_md(description, cx)), }, acp::ToolCallConfirmation::Execute { @@ -228,6 +226,23 @@ pub enum ToolCallContent { Diff { diff: Diff }, } +impl ToolCallContent { + pub fn from_acp( + content: acp::ToolCallContent, + language_registry: Arc, + cx: &mut App, + ) -> Self { + match content { + acp::ToolCallContent::Markdown { markdown } => Self::Markdown { + markdown: cx.new(|cx| Markdown::new_text(markdown.into(), cx)), + }, + acp::ToolCallContent::Diff { diff } => Self::Diff { + diff: Diff::from_acp(diff, language_registry, cx), + }, + } + } +} + #[derive(Debug)] pub struct Diff { // todo! show path somewhere @@ -442,6 +457,7 @@ impl AcpThread { &mut self, label: String, icon: acp::Icon, + content: Option, confirmation: acp::ToolCallConfirmation, cx: &mut Context, ) -> ToolCallRequest { @@ -456,7 +472,7 @@ impl AcpThread { respond_tx: tx, }; - let id = self.insert_tool_call(label, status, icon, cx); + let id = self.insert_tool_call(label, status, icon, content, cx); ToolCallRequest { id, outcome: rx } } @@ -464,14 +480,14 @@ impl AcpThread { &mut self, label: String, icon: acp::Icon, + content: Option, cx: &mut Context, ) -> ToolCallId { let status = ToolCallStatus::Allowed { status: acp::ToolCallStatus::Running, - content: None, }; - self.insert_tool_call(label, status, icon, cx) + self.insert_tool_call(label, status, icon, content, cx) } fn insert_tool_call( @@ -479,6 +495,7 @@ impl AcpThread { label: String, status: ToolCallStatus, icon: acp::Icon, + content: Option, cx: &mut Context, ) -> ToolCallId { let language_registry = self.project.read(cx).languages().clone(); @@ -491,6 +508,8 @@ impl AcpThread { Markdown::new(label.into(), Some(language_registry.clone()), None, cx) }), icon: acp_icon_to_ui_icon(icon), + content: content + .map(|content| ToolCallContent::from_acp(content, language_registry, cx)), status, }), cx, @@ -519,7 +538,6 @@ impl AcpThread { } else { ToolCallStatus::Allowed { status: acp::ToolCallStatus::Running, - content: None, } }; @@ -545,32 +563,23 @@ impl AcpThread { let entry = self.entry_mut(id.0).context("Entry not found")?; match &mut entry.content { - AgentThreadEntryContent::ToolCall(call) => match &mut call.status { - ToolCallStatus::Allowed { content, status } => { - *content = new_content.map(|new_content| match new_content { - acp::ToolCallContent::Markdown { markdown } => ToolCallContent::Markdown { - markdown: cx.new(|cx| { - Markdown::new( - markdown.into(), - Some(language_registry.clone()), - None, - cx, - ) - }), - }, - acp::ToolCallContent::Diff { diff } => ToolCallContent::Diff { - diff: Diff::from_acp(diff, language_registry, cx), - }, - }); - *status = new_status; - } - ToolCallStatus::WaitingForConfirmation { .. } => { - anyhow::bail!("Tool call hasn't been authorized yet") - } - ToolCallStatus::Rejected => { - anyhow::bail!("Tool call was rejected and therefore can't be updated") + AgentThreadEntryContent::ToolCall(call) => { + call.content = new_content.map(|new_content| { + ToolCallContent::from_acp(new_content, language_registry, cx) + }); + + match &mut call.status { + ToolCallStatus::Allowed { status } => { + *status = new_status; + } + ToolCallStatus::WaitingForConfirmation { .. } => { + anyhow::bail!("Tool call hasn't been authorized yet") + } + ToolCallStatus::Rejected => { + anyhow::bail!("Tool call was rejected and therefore can't be updated") + } } - }, + } _ => anyhow::bail!("Entry is not a tool call"), } @@ -789,11 +798,8 @@ mod tests { thread.read_with(cx, |thread, cx| { let AgentThreadEntryContent::ToolCall(ToolCall { - status: - ToolCallStatus::Allowed { - content: Some(ToolCallContent::Markdown { markdown }), - .. - }, + content: Some(ToolCallContent::Markdown { markdown }), + status: ToolCallStatus::Allowed { .. }, .. }) = &thread.entries()[1].content else { diff --git a/crates/acp/src/server.rs b/crates/acp/src/server.rs index a0cff3c98e81f3ad0c6b79761708b5bab48b8c78..b98393295abb2793076a530042d508f2bea57607 100644 --- a/crates/acp/src/server.rs +++ b/crates/acp/src/server.rs @@ -79,7 +79,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.label, request.icon, request.confirmation, cx) + thread.request_tool_call(request.label, request.icon, request.content, request.confirmation, cx) }) })? .context("Failed to update thread")?; @@ -98,7 +98,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.label, request.icon, cx) + thread.push_tool_call(request.label, request.icon, request.content, cx) }) })? .context("Failed to update thread")?; diff --git a/crates/acp/src/thread_view.rs b/crates/acp/src/thread_view.rs index 2e32e678032d64c97f1e35b717d318f5f1cd2fe6..8814082258aacc55f5bdcd88e8af737e3df4b885 100644 --- a/crates/acp/src/thread_view.rs +++ b/crates/acp/src/thread_view.rs @@ -336,21 +336,12 @@ impl AcpThreadView { fn entry_diff_multibuffer(&self, entry_ix: usize, cx: &App) -> Option> { let entry = self.thread()?.read(cx).entries().get(entry_ix)?; - - if let AgentThreadEntryContent::ToolCall(ToolCall { status, .. }) = &entry.content { - if let ToolCallStatus::WaitingForConfirmation { - confirmation: ToolCallConfirmation::Edit { diff, .. }, - .. - } - | ToolCallStatus::Allowed { - content: Some(ToolCallContent::Diff { diff }), - .. - } = status - { - Some(diff.multibuffer.clone()) - } else { - None - } + if let AgentThreadEntryContent::ToolCall(ToolCall { + content: Some(ToolCallContent::Diff { diff }), + .. + }) = &entry.content + { + Some(diff.multibuffer.clone()) } else { None } @@ -492,24 +483,18 @@ impl AcpThreadView { entry_ix, tool_call.id, confirmation, + tool_call.content.as_ref(), window, cx, )) } - ToolCallStatus::Allowed { content, .. } => content.as_ref().map(|content| { + ToolCallStatus::Allowed { .. } => tool_call.content.as_ref().map(|content| { div() .border_color(cx.theme().colors().border) .border_t_1() .px_2() .py_1p5() - .child(match content { - ToolCallContent::Markdown { markdown } => MarkdownElement::new( - markdown.clone(), - default_markdown_style(window, cx), - ) - .into_any_element(), - ToolCallContent::Diff { .. } => self.render_diff_editor(entry_ix), - }) + .child(self.render_tool_call_content(entry_ix, content, window, cx)) .into_any_element() }), ToolCallStatus::Rejected => None, @@ -543,54 +528,54 @@ impl AcpThreadView { .children(content) } + fn render_tool_call_content( + &self, + entry_ix: usize, + content: &ToolCallContent, + window: &Window, + cx: &Context, + ) -> AnyElement { + match content { + ToolCallContent::Markdown { markdown } => { + MarkdownElement::new(markdown.clone(), default_markdown_style(window, cx)) + .into_any_element() + } + ToolCallContent::Diff { .. } => self.render_diff_editor(entry_ix), + } + } + fn render_tool_call_confirmation( &self, entry_ix: usize, tool_call_id: ToolCallId, confirmation: &ToolCallConfirmation, + content: Option<&ToolCallContent>, window: &Window, cx: &Context, ) -> AnyElement { match confirmation { - ToolCallConfirmation::Edit { - description, - diff: _, - } => v_flex() - .border_color(cx.theme().colors().border) - .border_t_1() - .px_2() - .py_1p5() - .child(self.render_diff_editor(entry_ix)) - .children(description.clone().map(|description| { - MarkdownElement::new(description, default_markdown_style(window, cx)) - })) - .child( - h_flex() - .justify_end() - .gap_1() - .child( - 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") - .icon(IconName::Check) + ToolCallConfirmation::Edit { description } => { + v_flex() + .border_color(cx.theme().colors().border) + .border_t_1() + .px_2() + .py_1p5() + .children(description.clone().map(|description| { + MarkdownElement::new(description, default_markdown_style(window, cx)) + })) + .children(content.map(|content| { + self.render_tool_call_content(entry_ix, content, window, cx) + })) + .child( + h_flex() + .justify_end() + .gap_1() + .child( + 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) @@ -599,72 +584,77 @@ impl AcpThreadView { move |this, _, _, cx| { this.authorize_tool_call( id, - acp::ToolCallConfirmationOutcome::Allow, - cx, - ); - } - })), - ) - .child( - Button::new(("reject", tool_call_id.as_u64()), "Reject") - .icon(IconName::X) - .icon_position(IconPosition::Start) - .icon_size(IconSize::Small) - .icon_color(Color::Error) - .on_click(cx.listener({ - let id = tool_call_id; - move |this, _, _, cx| { - this.authorize_tool_call( - id, - acp::ToolCallConfirmationOutcome::Reject, + acp::ToolCallConfirmationOutcome::AlwaysAllow, cx, ); } })), - ), - ) - .into_any(), + ) + .child( + Button::new(("allow", tool_call_id.as_u64()), "Allow") + .icon(IconName::Check) + .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::Allow, + cx, + ); + } + })), + ) + .child( + Button::new(("reject", tool_call_id.as_u64()), "Reject") + .icon(IconName::X) + .icon_position(IconPosition::Start) + .icon_size(IconSize::Small) + .icon_color(Color::Error) + .on_click(cx.listener({ + let id = tool_call_id; + move |this, _, _, cx| { + this.authorize_tool_call( + id, + acp::ToolCallConfirmationOutcome::Reject, + cx, + ); + } + })), + ), + ) + .into_any() + } ToolCallConfirmation::Execute { command, root_command, description, - } => v_flex() - .border_color(cx.theme().colors().border) - .border_t_1() - .px_2() - .py_1p5() - // todo! nicer rendering - .child(command.clone()) - .children(description.clone().map(|description| { - MarkdownElement::new(description, default_markdown_style(window, cx)) - })) - .child( - h_flex() - .justify_end() - .gap_1() - .child( - Button::new( - ("always_allow", tool_call_id.as_u64()), - format!("Always Allow {root_command}"), - ) - .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") - .icon(IconName::Check) + } => { + v_flex() + .border_color(cx.theme().colors().border) + .border_t_1() + .px_2() + .py_1p5() + // todo! nicer rendering + .child(command.clone()) + .children(description.clone().map(|description| { + MarkdownElement::new(description, default_markdown_style(window, cx)) + })) + .children(content.map(|content| { + self.render_tool_call_content(entry_ix, content, window, cx) + })) + .child( + h_flex() + .justify_end() + .gap_1() + .child( + Button::new( + ("always_allow", tool_call_id.as_u64()), + format!("Always Allow {root_command}"), + ) + .icon(IconName::CheckDouble) .icon_position(IconPosition::Start) .icon_size(IconSize::Small) .icon_color(Color::Success) @@ -673,93 +663,78 @@ impl AcpThreadView { move |this, _, _, cx| { this.authorize_tool_call( id, - acp::ToolCallConfirmationOutcome::Allow, - cx, - ); - } - })), - ) - .child( - Button::new(("reject", tool_call_id.as_u64()), "Reject") - .icon(IconName::X) - .icon_position(IconPosition::Start) - .icon_size(IconSize::Small) - .icon_color(Color::Error) - .on_click(cx.listener({ - let id = tool_call_id; - move |this, _, _, cx| { - this.authorize_tool_call( - id, - acp::ToolCallConfirmationOutcome::Reject, + acp::ToolCallConfirmationOutcome::AlwaysAllow, cx, ); } })), - ), - ) - .into_any(), + ) + .child( + Button::new(("allow", tool_call_id.as_u64()), "Allow") + .icon(IconName::Check) + .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::Allow, + cx, + ); + } + })), + ) + .child( + Button::new(("reject", tool_call_id.as_u64()), "Reject") + .icon(IconName::X) + .icon_position(IconPosition::Start) + .icon_size(IconSize::Small) + .icon_color(Color::Error) + .on_click(cx.listener({ + let id = tool_call_id; + move |this, _, _, cx| { + this.authorize_tool_call( + id, + acp::ToolCallConfirmationOutcome::Reject, + cx, + ); + } + })), + ), + ) + .into_any() + } ToolCallConfirmation::Mcp { server_name, tool_name: _, tool_display_name, description, - } => v_flex() - .border_color(cx.theme().colors().border) - .border_t_1() - .px_2() - .py_1p5() - // todo! nicer rendering - .child(format!("{server_name} - {tool_display_name}")) - .children(description.clone().map(|description| { - MarkdownElement::new(description, default_markdown_style(window, cx)) - })) - .child( - h_flex() - .justify_end() - .gap_1() - .child( - Button::new( - ("always_allow_server", tool_call_id.as_u64()), - format!("Always Allow {server_name}"), - ) - .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::AlwaysAllowMcpServer, - cx, - ); - } - })), - ) - .child( - Button::new( - ("always_allow_tool", tool_call_id.as_u64()), - format!("Always Allow {tool_display_name}"), - ) - .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::AlwaysAllowTool, - cx, - ); - } - })), - ) - .child( - Button::new(("allow", tool_call_id.as_u64()), "Allow") - .icon(IconName::Check) + } => { + v_flex() + .border_color(cx.theme().colors().border) + .border_t_1() + .px_2() + .py_1p5() + // todo! nicer rendering + .child(format!("{server_name} - {tool_display_name}")) + .children(description.clone().map(|description| { + MarkdownElement::new(description, default_markdown_style(window, cx)) + })) + .children(content.map(|content| { + self.render_tool_call_content(entry_ix, content, window, cx) + })) + .child( + h_flex() + .justify_end() + .gap_1() + .child( + Button::new( + ("always_allow_server", tool_call_id.as_u64()), + format!("Always Allow {server_name}"), + ) + .icon(IconName::CheckDouble) .icon_position(IconPosition::Start) .icon_size(IconSize::Small) .icon_color(Color::Success) @@ -768,31 +743,69 @@ impl AcpThreadView { move |this, _, _, cx| { this.authorize_tool_call( id, - acp::ToolCallConfirmationOutcome::Allow, + acp::ToolCallConfirmationOutcome::AlwaysAllowMcpServer, cx, ); } })), - ) - .child( - Button::new(("reject", tool_call_id.as_u64()), "Reject") - .icon(IconName::X) + ) + .child( + Button::new( + ("always_allow_tool", tool_call_id.as_u64()), + format!("Always Allow {tool_display_name}"), + ) + .icon(IconName::CheckDouble) .icon_position(IconPosition::Start) .icon_size(IconSize::Small) - .icon_color(Color::Error) + .icon_color(Color::Success) .on_click(cx.listener({ let id = tool_call_id; move |this, _, _, cx| { this.authorize_tool_call( id, - acp::ToolCallConfirmationOutcome::Reject, + acp::ToolCallConfirmationOutcome::AlwaysAllowTool, cx, ); } })), - ), - ) - .into_any(), + ) + .child( + Button::new(("allow", tool_call_id.as_u64()), "Allow") + .icon(IconName::Check) + .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::Allow, + cx, + ); + } + })), + ) + .child( + Button::new(("reject", tool_call_id.as_u64()), "Reject") + .icon(IconName::X) + .icon_position(IconPosition::Start) + .icon_size(IconSize::Small) + .icon_color(Color::Error) + .on_click(cx.listener({ + let id = tool_call_id; + move |this, _, _, cx| { + this.authorize_tool_call( + id, + acp::ToolCallConfirmationOutcome::Reject, + cx, + ); + } + })), + ), + ) + .into_any() + } ToolCallConfirmation::Fetch { description, urls } => v_flex() .border_color(cx.theme().colors().border) .border_t_1() @@ -803,6 +816,11 @@ impl AcpThreadView { .children(description.clone().map(|description| { MarkdownElement::new(description, default_markdown_style(window, cx)) })) + .children( + content.map(|content| { + self.render_tool_call_content(entry_ix, content, window, cx) + }), + ) .child( h_flex() .justify_end() @@ -870,6 +888,11 @@ impl AcpThreadView { description.clone(), default_markdown_style(window, cx), )) + .children( + content.map(|content| { + self.render_tool_call_content(entry_ix, content, window, cx) + }), + ) .child( h_flex() .justify_end()