From 8affe205bd6cf960a6dbf206c489f317bf77bb3d Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Wed, 23 Jul 2025 12:17:02 -0300 Subject: [PATCH] More progress --- crates/acp_thread/src/acp_thread.rs | 312 +++++++++++++++++++------ crates/agent_ui/src/acp/thread_view.rs | 2 +- crates/agent_ui/src/agent_diff.rs | 4 +- 3 files changed, 241 insertions(+), 77 deletions(-) diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 799cf4fa96da353c51628134ece5ebb788fac8ea..2b9bad80832fde16a39e05cde1c916fa45e7f8df 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -1,7 +1,7 @@ mod connection; pub use connection::*; -use agent_client_protocol::{self as acp}; +use agent_client_protocol as acp; use agentic_coding_protocol as acp_old; use anyhow::{Context as _, Result}; use assistant_tool::ActionLog; @@ -16,16 +16,18 @@ use language::{ }; use markdown::Markdown; use project::{AgentLocation, Project}; +use std::cell::RefCell; use std::collections::HashMap; use std::error::Error; use std::fmt::Formatter; +use std::rc::Rc; use std::{ fmt::Display, mem, path::{Path, PathBuf}, sync::Arc, }; -use ui::{App, IconName}; +use ui::App; use util::ResultExt; #[derive(Debug)] @@ -146,13 +148,10 @@ impl AgentThreadEntry { } } - pub fn diff(&self) -> Option<&Diff> { - if let AgentThreadEntry::ToolCall(ToolCall { - content: Some(ToolCallContent::Diff { diff }), - .. - }) = self - { - Some(&diff) + // todo! return all diffs? + pub fn first_diff(&self) -> Option<&Diff> { + if let AgentThreadEntry::ToolCall(call) = self { + call.first_diff() } else { None } @@ -172,8 +171,7 @@ pub struct ToolCall { pub id: acp::ToolCallId, pub label: Entity, pub kind: acp::ToolKind, - // todo! Should this be a vec? - pub content: Option, + pub content: Vec, pub status: ToolCallStatus, pub locations: Vec, } @@ -196,21 +194,30 @@ impl ToolCall { ) }), kind: tool_call.kind, - // todo! Do we assume there is either a coalesced content OR diff? - content: ToolCallContent::from_acp_contents(tool_call.content, language_registry, cx) + content: tool_call + .content .into_iter() - .next(), + .map(|content| ToolCallContent::from_acp(content, language_registry.clone(), cx)) + .collect(), locations: tool_call.locations, status, } } + + pub fn first_diff(&self) -> Option<&Diff> { + self.content.iter().find_map(|content| match content { + ToolCallContent::ContentBlock { .. } => None, + ToolCallContent::Diff { diff } => Some(diff), + }) + } + fn to_markdown(&self, cx: &App) -> String { let mut markdown = format!( "**Tool Call: {}**\nStatus: {}\n\n", self.label.read(cx).source(), self.status ); - if let Some(content) = &self.content { + for content in &self.content { markdown.push_str(content.to_markdown(cx).as_str()); markdown.push_str("\n\n"); } @@ -269,12 +276,12 @@ impl ContentBlock { pub fn new_combined( blocks: impl IntoIterator, - language_registry: &Arc, + language_registry: Arc, cx: &mut App, ) -> Self { let mut this = Self::Empty; for block in blocks { - this.append(block, language_registry, cx); + this.append(block, &language_registry, cx); } this } @@ -647,15 +654,7 @@ impl AcpThread { for entry in self.entries.iter().rev() { match entry { AgentThreadEntry::UserMessage(_) => return false, - AgentThreadEntry::ToolCall(ToolCall { - status: - ToolCallStatus::Allowed { - status: acp::ToolCallStatus::InProgress, - .. - }, - content: Some(ToolCallContent::Diff { .. }), - .. - }) => return true, + AgentThreadEntry::ToolCall(call) if call.first_diff().is_some() => return true, AgentThreadEntry::ToolCall(_) | AgentThreadEntry::AssistantMessage(_) => {} } } @@ -749,7 +748,25 @@ impl AcpThread { Ok(()) } + fn tool_call(&mut self, id: &acp::ToolCallId) -> Option<(usize, &ToolCall)> { + // todo! use map + self.entries + .iter() + .enumerate() + .rev() + .find_map(|(index, tool_call)| { + if let AgentThreadEntry::ToolCall(tool_call) = tool_call + && &tool_call.id == id + { + Some((index, tool_call)) + } else { + None + } + }) + } + fn tool_call_mut(&mut self, id: &acp::ToolCallId) -> Option<(usize, &mut ToolCall)> { + // todo! use map self.entries .iter_mut() .enumerate() @@ -908,14 +925,8 @@ impl AcpThread { false } - pub fn initialize(&self) -> impl use<> + Future> { - self.request(acp_old::InitializeParams { - protocol_version: acp_old::ProtocolVersion::latest(), - }) - } - pub fn authenticate(&self) -> impl use<> + Future> { - self.request(acp_old::AuthenticateParams) + self.connection.authenticate() } #[cfg(any(test, feature = "test-support"))] @@ -1220,8 +1231,17 @@ impl OldAcpClientDelegate { let content = self .cx .update(|cx| { - self.thread - .update(cx, |thread, cx| thread.read_text_file(request, true, cx)) + self.thread.update(cx, |thread, cx| { + thread.read_text_file( + acp::ReadTextFileArguments { + path: request.path, + line: request.line, + limit: request.limit, + }, + true, + cx, + ) + }) })? .context("Failed to update thread")? .await?; @@ -1238,8 +1258,13 @@ impl acp_old::Client for OldAcpClientDelegate { cx.update(|cx| { self.thread - .update(cx, |thread, cx| { - thread.push_assistant_chunk(params.chunk, cx) + .update(cx, |thread, cx| match params.chunk { + acp_old::AssistantMessageChunk::Text { text } => { + thread.push_assistant_chunk(text.into(), false, cx) + } + acp_old::AssistantMessageChunk::Thought { thought } => { + thread.push_assistant_chunk(thought.into(), true, cx) + } }) .ok(); })?; @@ -1271,29 +1296,22 @@ impl acp_old::Client for OldAcpClientDelegate { ) -> Result { let cx = &mut self.cx.clone(); - let new_id = acp::ToolCallId( - util::post_inc(self.next_tool_call_id.borrow_mut()) - .to_string() - .into(), - ); + let old_acp_id = *self.next_tool_call_id.borrow() + 1; + self.next_tool_call_id.replace(old_acp_id); - let new_tool_call = acp::ToolCall { - id: new_id, - label: request.label, - kind: acp_kind_from_icon(request.icon), - status: acp::ToolCallStatus::InProgress, - content: into_new_acp_content(request.content), - locations: request.locations.into_iter().map(into_new_acp_location).collect(), - }; - - let id = cx - .update(|cx| { - self.thread - .update(cx, |thread, cx| thread.update_tool_call(request, cx)) - })? - .context("Failed to update thread")?; + cx.update(|cx| { + self.thread.update(cx, |thread, cx| { + thread.update_tool_call( + into_new_tool_call(acp::ToolCallId(old_acp_id.to_string().into()), request), + cx, + ) + }) + })? + .context("Failed to update thread")?; - Ok(acp_old::PushToolCallResponse { id }) + Ok(acp_old::PushToolCallResponse { + id: acp_old::ToolCallId(old_acp_id), + }) } async fn update_tool_call( @@ -1304,7 +1322,31 @@ impl acp_old::Client for OldAcpClientDelegate { cx.update(|cx| { self.thread.update(cx, |thread, cx| { - thread.update_tool_call(request.tool_call_id, request.status, request.content, cx) + let languages = thread.project.read(cx).languages().clone(); + + if let Some((ix, tool_call)) = thread + .tool_call_mut(&acp::ToolCallId(request.tool_call_id.0.to_string().into())) + { + tool_call.status = ToolCallStatus::Allowed { + status: into_new_tool_call_status(request.status), + }; + tool_call.content = request + .content + .into_iter() + .map(|content| { + ToolCallContent::from_acp( + into_new_tool_call_content(content), + languages.clone(), + cx, + ) + }) + .collect(); + + cx.emit(AcpThreadEvent::EntryUpdated(ix)); + anyhow::Ok(()) + } else { + anyhow::bail!("Tool call not found") + } }) })? .context("Failed to update thread")??; @@ -1316,8 +1358,18 @@ impl acp_old::Client for OldAcpClientDelegate { let cx = &mut self.cx.clone(); cx.update(|cx| { - self.thread - .update(cx, |thread, cx| thread.update_plan(request, cx)) + self.thread.update(cx, |thread, cx| { + thread.update_plan( + acp::Plan { + entries: request + .entries + .into_iter() + .map(into_new_plan_entry) + .collect(), + }, + cx, + ) + }) })? .context("Failed to update thread")?; @@ -1331,8 +1383,17 @@ impl acp_old::Client for OldAcpClientDelegate { let content = self .cx .update(|cx| { - self.thread - .update(cx, |thread, cx| thread.read_text_file(request, false, cx)) + self.thread.update(cx, |thread, cx| { + thread.read_text_file( + acp::ReadTextFileArguments { + path: request.path, + line: request.line, + limit: request.limit, + }, + false, + cx, + ) + }) })? .context("Failed to update thread")? .await?; @@ -1346,7 +1407,13 @@ impl acp_old::Client for OldAcpClientDelegate { self.cx .update(|cx| { self.thread.update(cx, |thread, cx| { - thread.write_text_file(request.path, request.content, cx) + thread.write_text_file( + acp::WriteTextFileToolArguments { + path: request.path, + content: request.content, + }, + cx, + ) }) })? .context("Failed to update thread")? @@ -1356,16 +1423,106 @@ impl acp_old::Client for OldAcpClientDelegate { } } -fn acp_icon_to_ui_icon(icon: acp_old::Icon) -> IconName { +fn into_new_tool_call(id: acp::ToolCallId, request: acp_old::PushToolCallParams) -> acp::ToolCall { + acp::ToolCall { + id: id, + label: request.label, + kind: acp_kind_from_old_icon(request.icon), + status: acp::ToolCallStatus::InProgress, + content: request + .content + .into_iter() + .map(into_new_tool_call_content) + .collect(), + locations: request + .locations + .into_iter() + .map(into_new_tool_call_location) + .collect(), + } +} + +fn acp_kind_from_old_icon(icon: acp_old::Icon) -> acp::ToolKind { match icon { - acp_old::Icon::FileSearch => IconName::ToolSearch, - acp_old::Icon::Folder => IconName::ToolFolder, - acp_old::Icon::Globe => IconName::ToolWeb, - acp_old::Icon::Hammer => IconName::ToolHammer, - acp_old::Icon::LightBulb => IconName::ToolBulb, - acp_old::Icon::Pencil => IconName::ToolPencil, - acp_old::Icon::Regex => IconName::ToolRegex, - acp_old::Icon::Terminal => IconName::ToolTerminal, + acp_old::Icon::FileSearch => acp::ToolKind::Search, + acp_old::Icon::Folder => acp::ToolKind::Search, + acp_old::Icon::Globe => acp::ToolKind::Search, + acp_old::Icon::Hammer => acp::ToolKind::Other, + acp_old::Icon::LightBulb => acp::ToolKind::Think, + acp_old::Icon::Pencil => acp::ToolKind::Edit, + acp_old::Icon::Regex => acp::ToolKind::Search, + acp_old::Icon::Terminal => acp::ToolKind::Execute, + } +} + +fn into_new_tool_call_status(status: acp_old::ToolCallStatus) -> acp::ToolCallStatus { + match status { + acp_old::ToolCallStatus::Running => acp::ToolCallStatus::InProgress, + acp_old::ToolCallStatus::Finished => acp::ToolCallStatus::Completed, + acp_old::ToolCallStatus::Error => acp::ToolCallStatus::Failed, + } +} + +fn into_new_tool_call_content(content: acp_old::ToolCallContent) -> acp::ToolCallContent { + match content { + acp_old::ToolCallContent::Markdown { markdown } => acp::ToolCallContent::ContentBlock { + content: acp::ContentBlock::Text(acp::TextContent { + annotations: None, + text: markdown, + }), + }, + acp_old::ToolCallContent::Diff { diff } => acp::ToolCallContent::Diff { + diff: into_new_diff(diff), + }, + } +} + +fn into_new_diff(diff: acp_old::Diff) -> acp::Diff { + acp::Diff { + path: diff.path, + old_text: diff.old_text, + new_text: diff.new_text, + } +} + +fn into_new_tool_call_location(location: acp_old::ToolCallLocation) -> acp::ToolCallLocation { + acp::ToolCallLocation { + path: location.path, + line: location.line, + } +} + +fn into_new_plan(request: acp_old::UpdatePlanParams) -> acp::Plan { + acp::Plan { + entries: request + .entries + .into_iter() + .map(into_new_plan_entry) + .collect(), + } +} + +fn into_new_plan_entry(entry: acp_old::PlanEntry) -> acp::PlanEntry { + acp::PlanEntry { + content: entry.content, + priority: into_new_plan_priority(entry.priority), + status: into_new_plan_status(entry.status), + } +} + +fn into_new_plan_priority(priority: acp_old::PlanEntryPriority) -> acp::PlanEntryPriority { + match priority { + acp_old::PlanEntryPriority::Low => acp::PlanEntryPriority::Low, + acp_old::PlanEntryPriority::Medium => acp::PlanEntryPriority::Medium, + acp_old::PlanEntryPriority::High => acp::PlanEntryPriority::High, + } +} + +fn into_new_plan_status(status: acp_old::PlanEntryStatus) -> acp::PlanEntryStatus { + match status { + acp_old::PlanEntryStatus::Pending => acp::PlanEntryStatus::Pending, + acp_old::PlanEntryStatus::InProgress => acp::PlanEntryStatus::InProgress, + acp_old::PlanEntryStatus::Completed => acp::PlanEntryStatus::Completed, } } @@ -1678,7 +1835,14 @@ mod tests { Ok(()) } }); - AcpThread::new(connection, "Test".into(), Some(io_task), project, cx) + AcpThread::new( + connection, + "Test".into(), + Some(io_task), + project, + acp::SessionId("test".into()), + cx, + ) }); let agent = cx.update(|cx| cx.new(|cx| FakeAcpServer::new(stdin_rx, stdout_tx, cx))); (thread, agent) diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index a06295d883daadce93848226566a616f3d9800f7..0b4f3325fc1e6f4cda2428a7c1e4eabceb108d87 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -646,7 +646,7 @@ impl AcpThreadView { fn entry_diff_multibuffer(&self, entry_ix: usize, cx: &App) -> Option> { let entry = self.thread()?.read(cx).entries().get(entry_ix)?; - entry.diff().map(|diff| diff.multibuffer.clone()) + entry.first_diff().map(|diff| diff.multibuffer.clone()) } fn authenticate(&mut self, window: &mut Window, cx: &mut Context) { diff --git a/crates/agent_ui/src/agent_diff.rs b/crates/agent_ui/src/agent_diff.rs index e69664ce882df70915a84d2ba38cf7ec8521fd4b..3f1ea41f491db4e0efebf86d6df8350368a21336 100644 --- a/crates/agent_ui/src/agent_diff.rs +++ b/crates/agent_ui/src/agent_diff.rs @@ -1506,7 +1506,7 @@ impl AgentDiff { .read(cx) .entries() .last() - .and_then(|entry| entry.diff()) + .and_then(|entry| entry.first_diff()) .is_some() { self.update_reviewing_editors(workspace, window, cx); @@ -1517,7 +1517,7 @@ impl AgentDiff { .read(cx) .entries() .get(*ix) - .and_then(|entry| entry.diff()) + .and_then(|entry| entry.first_diff()) .is_some() { self.update_reviewing_editors(workspace, window, cx);