From 46d11d210edb4a942994f117a47cafc304ace283 Mon Sep 17 00:00:00 2001 From: Ben Brandt Date: Fri, 20 Feb 2026 16:28:22 +0100 Subject: [PATCH] agent: Allow tools to have structured output (#49722) Previously we only had the ability to return anyhow errors, which limited the amount of structured data we could send the model. Now tools can also return structured data in the error cases, which is super helpful for subagents in particular Release Notes: - N/A --- crates/agent/src/tests/mod.rs | 33 +- crates/agent/src/tests/test_tools.rs | 19 +- crates/agent/src/thread.rs | 102 ++-- .../src/tools/context_server_registry.rs | 16 +- crates/agent/src/tools/copy_path_tool.rs | 17 +- .../agent/src/tools/create_directory_tool.rs | 13 +- crates/agent/src/tools/delete_path_tool.rs | 21 +- crates/agent/src/tools/diagnostics_tool.rs | 13 +- crates/agent/src/tools/edit_file_tool.rs | 550 ++++++++++-------- crates/agent/src/tools/fetch_tool.rs | 10 +- crates/agent/src/tools/find_path_tool.rs | 65 ++- crates/agent/src/tools/grep_tool.rs | 29 +- crates/agent/src/tools/list_directory_tool.rs | 33 +- crates/agent/src/tools/move_path_tool.rs | 20 +- crates/agent/src/tools/now_tool.rs | 3 +- crates/agent/src/tools/open_tool.rs | 9 +- crates/agent/src/tools/read_file_tool.rs | 63 +- .../src/tools/restore_file_from_disk_tool.rs | 13 +- crates/agent/src/tools/save_file_tool.rs | 13 +- .../src/tools/streaming_edit_file_tool.rs | 496 +++++++++------- crates/agent/src/tools/subagent_tool.rs | 62 +- crates/agent/src/tools/terminal_tool.rs | 23 +- crates/agent/src/tools/web_search_tool.rs | 38 +- crates/zed/src/visual_test_runner.rs | 7 +- 24 files changed, 912 insertions(+), 756 deletions(-) diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index 6f8cd32dbcde72c6262b3c386926fced224043a7..b1c07c1cdb7d151c3b03676bf451c799fda8656d 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -331,7 +331,7 @@ async fn test_terminal_tool_timeout_kills_handle(cx: &mut TestAppContext) { "expected tool call update to include terminal content" ); - let mut task_future: Pin>>>> = Box::pin(task.fuse()); + let mut task_future: Pin>>>> = Box::pin(task.fuse()); let deadline = std::time::Instant::now() + Duration::from_millis(500); loop { @@ -4007,7 +4007,7 @@ async fn test_terminal_tool_permission_rules(cx: &mut TestAppContext) { result.is_err(), "expected command to be blocked by deny rule" ); - let err_msg = result.unwrap_err().to_string().to_lowercase(); + let err_msg = result.unwrap_err().to_lowercase(); assert!( err_msg.contains("blocked"), "error should mention the command was blocked" @@ -4165,7 +4165,7 @@ async fn test_terminal_tool_permission_rules(cx: &mut TestAppContext) { result.is_err(), "expected command to be blocked by tool-specific deny default" ); - let err_msg = result.unwrap_err().to_string().to_lowercase(); + let err_msg = result.unwrap_err().to_lowercase(); assert!( err_msg.contains("disabled"), "error should mention the tool is disabled, got: {err_msg}" @@ -4847,7 +4847,7 @@ async fn test_delete_path_tool_deny_rule_blocks_deletion(cx: &mut TestAppContext let result = task.await; assert!(result.is_err(), "expected deletion to be blocked"); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the deletion was blocked" ); } @@ -4903,7 +4903,7 @@ async fn test_move_path_tool_denies_if_destination_denied(cx: &mut TestAppContex "expected move to be blocked due to destination path" ); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the move was blocked" ); } @@ -4959,7 +4959,7 @@ async fn test_move_path_tool_denies_if_source_denied(cx: &mut TestAppContext) { "expected move to be blocked due to source path" ); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the move was blocked" ); } @@ -5014,7 +5014,7 @@ async fn test_copy_path_tool_deny_rule_blocks_copy(cx: &mut TestAppContext) { let result = task.await; assert!(result.is_err(), "expected copy to be blocked"); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the copy was blocked" ); } @@ -5074,7 +5074,7 @@ async fn test_save_file_tool_denies_if_any_path_denied(cx: &mut TestAppContext) "expected save to be blocked due to denied path" ); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the save was blocked" ); } @@ -5120,7 +5120,7 @@ async fn test_save_file_tool_respects_deny_rules(cx: &mut TestAppContext) { let result = task.await; assert!(result.is_err(), "expected save to be blocked"); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the save was blocked" ); } @@ -5157,10 +5157,15 @@ async fn test_web_search_tool_deny_rule_blocks_search(cx: &mut TestAppContext) { let result = task.await; assert!(result.is_err(), "expected search to be blocked"); - assert!( - result.unwrap_err().to_string().contains("blocked"), - "error should mention the search was blocked" - ); + match result.unwrap_err() { + crate::WebSearchToolOutput::Error { error } => { + assert!( + error.contains("blocked"), + "error should mention the search was blocked" + ); + } + other => panic!("expected Error variant, got: {other:?}"), + } } #[gpui::test] @@ -5332,7 +5337,7 @@ async fn test_fetch_tool_deny_rule_blocks_url(cx: &mut TestAppContext) { let result = task.await; assert!(result.is_err(), "expected fetch to be blocked"); assert!( - result.unwrap_err().to_string().contains("blocked"), + result.unwrap_err().contains("blocked"), "error should mention the fetch was blocked" ); } diff --git a/crates/agent/src/tests/test_tools.rs b/crates/agent/src/tests/test_tools.rs index dc48ae1719157e7db11c5d2812139d570bd0d166..0ed2eef90271538c575cc84b56a28df106e4bd41 100644 --- a/crates/agent/src/tests/test_tools.rs +++ b/crates/agent/src/tests/test_tools.rs @@ -1,6 +1,5 @@ use super::*; use agent_settings::AgentSettings; -use anyhow::Result; use gpui::{App, SharedString, Task}; use std::future; use std::sync::atomic::{AtomicBool, Ordering}; @@ -37,7 +36,7 @@ impl AgentTool for EchoTool { input: Self::Input, _event_stream: ToolCallEventStream, _cx: &mut App, - ) -> Task> { + ) -> Task> { Task::ready(Ok(input.text)) } } @@ -78,7 +77,7 @@ impl AgentTool for DelayTool { input: Self::Input, _event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> + ) -> Task> where Self: Sized, { @@ -118,14 +117,14 @@ impl AgentTool for ToolRequiringPermission { _input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx); let decision = decide_permission_from_settings(Self::NAME, &[String::new()], settings); let authorize = match decision { ToolPermissionDecision::Allow => None, ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); + return Task::ready(Err(reason)); } ToolPermissionDecision::Confirm => { let context = crate::ToolPermissionContext::new( @@ -138,7 +137,7 @@ impl AgentTool for ToolRequiringPermission { cx.foreground_executor().spawn(async move { if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } Ok("Allowed".to_string()) }) @@ -173,7 +172,7 @@ impl AgentTool for InfiniteTool { _input: Self::Input, _event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { cx.foreground_executor().spawn(async move { future::pending::<()>().await; unreachable!() @@ -225,12 +224,12 @@ impl AgentTool for CancellationAwareTool { _input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { cx.foreground_executor().spawn(async move { // Wait for cancellation - this tool does nothing but wait to be cancelled event_stream.cancelled_by_user().await; self.was_cancelled.store(true, Ordering::SeqCst); - anyhow::bail!("Tool cancelled by user"); + Err("Tool cancelled by user".to_string()) }) } } @@ -280,7 +279,7 @@ impl AgentTool for WordListTool { _input: Self::Input, _event_stream: ToolCallEventStream, _cx: &mut App, - ) -> Task> { + ) -> Task> { Task::ready(Ok("ok".to_string())) } } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 578f10554947567bd86a73d9abc02666daa06a84..93024a97073e6f2aa0f8bfe7a135c01633dc0b48 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -2084,32 +2084,28 @@ impl Thread { let tool_result = tool.run(tool_use.input, tool_event_stream, cx); log::debug!("Running tool {}", tool_use.name); Some(cx.foreground_executor().spawn(async move { - let tool_result = tool_result.await.and_then(|output| { - if let LanguageModelToolResultContent::Image(_) = &output.llm_output - && !supports_images - { - return Err(anyhow!( - "Attempted to read an image, but this model doesn't support it.", - )); + let (is_error, output) = match tool_result.await { + Ok(mut output) => { + if let LanguageModelToolResultContent::Image(_) = &output.llm_output + && !supports_images + { + output = AgentToolOutput::from_error( + "Attempted to read an image, but this model doesn't support it.", + ); + (true, output) + } else { + (false, output) + } } - Ok(output) - }); + Err(output) => (true, output), + }; - match tool_result { - Ok(output) => LanguageModelToolResult { - tool_use_id: tool_use.id, - tool_name: tool_use.name, - is_error: false, - content: output.llm_output, - output: Some(output.raw_output), - }, - Err(error) => LanguageModelToolResult { - tool_use_id: tool_use.id, - tool_name: tool_use.name, - is_error: true, - content: LanguageModelToolResultContent::Text(Arc::from(error.to_string())), - output: Some(error.to_string().into()), - }, + LanguageModelToolResult { + tool_use_id: tool_use.id, + tool_name: tool_use.name, + is_error, + content: output.llm_output, + output: Some(output.raw_output), } })) } @@ -2826,12 +2822,18 @@ where } /// Runs the tool with the provided input. + /// + /// Returns `Result` rather than `Result` + /// because tool errors are sent back to the model as tool results. This means error output must + /// be structured and readable by the agent — not an arbitrary `anyhow::Error`. Returning the + /// same `Output` type for both success and failure lets tools provide structured data while + /// still signaling whether the invocation succeeded or failed. fn run( self: Arc, input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task>; + ) -> Task>; /// Emits events for a previous execution of the tool. fn replay( @@ -2856,6 +2858,17 @@ pub struct AgentToolOutput { pub raw_output: serde_json::Value, } +impl AgentToolOutput { + pub fn from_error(message: impl Into) -> Self { + let message = message.into(); + let llm_output = LanguageModelToolResultContent::Text(Arc::from(message.as_str())); + Self { + raw_output: serde_json::Value::String(message), + llm_output, + } + } +} + pub trait AnyAgentTool { fn name(&self) -> SharedString; fn description(&self) -> SharedString; @@ -2865,12 +2878,13 @@ pub trait AnyAgentTool { fn supports_provider(&self, _provider: &LanguageModelProviderId) -> bool { true } + /// See [`AgentTool::run`] for why this returns `Result`. fn run( self: Arc, input: serde_json::Value, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task>; + ) -> Task>; fn replay( &self, input: serde_json::Value, @@ -2916,17 +2930,33 @@ where input: serde_json::Value, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { cx.spawn(async move |cx| { - let input = serde_json::from_value(input)?; - let output = cx - .update(|cx| self.0.clone().run(input, event_stream, cx)) - .await?; - let raw_output = serde_json::to_value(&output)?; - Ok(AgentToolOutput { - llm_output: output.into(), - raw_output, - }) + let input: T::Input = serde_json::from_value(input).map_err(|e| { + AgentToolOutput::from_error(format!("Failed to parse tool input: {e}")) + })?; + let task = cx.update(|cx| self.0.clone().run(input, event_stream, cx)); + match task.await { + Ok(output) => { + let raw_output = serde_json::to_value(&output).map_err(|e| { + AgentToolOutput::from_error(format!("Failed to serialize tool output: {e}")) + })?; + Ok(AgentToolOutput { + llm_output: output.into(), + raw_output, + }) + } + Err(error_output) => { + let raw_output = serde_json::to_value(&error_output).unwrap_or_else(|e| { + log::error!("Failed to serialize tool error output: {e}"); + serde_json::Value::Null + }); + Err(AgentToolOutput { + llm_output: error_output.into(), + raw_output, + }) + } + } }) } diff --git a/crates/agent/src/tools/context_server_registry.rs b/crates/agent/src/tools/context_server_registry.rs index c7aa697ed6c5dc9eb176e154243fbed61aa2eb3b..694e28750cd69facc49b7a0bf862203a00043b4c 100644 --- a/crates/agent/src/tools/context_server_registry.rs +++ b/crates/agent/src/tools/context_server_registry.rs @@ -1,6 +1,6 @@ use crate::{AgentToolOutput, AnyAgentTool, ToolCallEventStream}; use agent_client_protocol::ToolKind; -use anyhow::{Result, anyhow}; +use anyhow::Result; use collections::{BTreeMap, HashMap}; use context_server::{ContextServerId, client::NotificationSubscription}; use futures::FutureExt as _; @@ -332,9 +332,9 @@ impl AnyAgentTool for ContextServerTool { input: serde_json::Value, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let Some(server) = self.store.read(cx).get_running_server(&self.server_id) else { - return Task::ready(Err(anyhow!("Context server not found"))); + return Task::ready(Err(AgentToolOutput::from_error("Context server not found"))); }; let tool_name = self.tool.name.clone(); let tool_id = mcp_tool_id(&self.server_id.0, &self.tool.name); @@ -347,10 +347,10 @@ impl AnyAgentTool for ContextServerTool { ); cx.spawn(async move |_cx| { - authorize.await?; + authorize.await.map_err(|e| AgentToolOutput::from_error(e.to_string()))?; let Some(protocol) = server.client() else { - anyhow::bail!("Context server not initialized"); + return Err(AgentToolOutput::from_error("Context server not initialized")); }; let arguments = if let serde_json::Value::Object(map) = input { @@ -374,16 +374,16 @@ impl AnyAgentTool for ContextServerTool { ); let response = futures::select! { - response = request.fuse() => response?, + response = request.fuse() => response.map_err(|e| AgentToolOutput::from_error(e.to_string()))?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("MCP tool cancelled by user"); + return Err(AgentToolOutput::from_error("MCP tool cancelled by user")); } }; if response.is_error == Some(true) { let error_message: String = response.content.iter().filter_map(|c| c.text()).collect(); - anyhow::bail!(error_message); + return Err(AgentToolOutput::from_error(error_message)); } let mut result = String::new(); diff --git a/crates/agent/src/tools/copy_path_tool.rs b/crates/agent/src/tools/copy_path_tool.rs index 22da12b2e2a33577e50e6d0804f5580ff4e2b56c..c82d9e930e1987d389ece84347c1a0f43c601182 100644 --- a/crates/agent/src/tools/copy_path_tool.rs +++ b/crates/agent/src/tools/copy_path_tool.rs @@ -5,7 +5,6 @@ use super::tool_permissions::{ use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_paths}; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; -use anyhow::{Context as _, Result, anyhow}; use futures::FutureExt as _; use gpui::{App, Entity, Task}; use project::Project; @@ -83,12 +82,12 @@ impl AgentTool for CopyPathTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx); let paths = vec![input.source_path.clone(), input.destination_path.clone()]; let decision = decide_permission_for_paths(Self::NAME, &paths, settings); if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow!("{}", reason))); + return Task::ready(Err(reason)); } let project = self.project.clone(); @@ -147,7 +146,7 @@ impl AgentTool for CopyPathTool { }; if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let copy_task = project.update(cx, |project, cx| { @@ -157,12 +156,12 @@ impl AgentTool for CopyPathTool { { Some(entity) => match project.find_project_path(&input.destination_path, cx) { Some(project_path) => Ok(project.copy_entry(entity.id, project_path, cx)), - None => Err(anyhow!( + None => Err(format!( "Destination path {} was outside the project.", input.destination_path )), }, - None => Err(anyhow!( + None => Err(format!( "Source path {} was not found in the project.", input.source_path )), @@ -172,12 +171,12 @@ impl AgentTool for CopyPathTool { let result = futures::select! { result = copy_task.fuse() => result, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Copy cancelled by user"); + return Err("Copy cancelled by user".to_string()); } }; - result.with_context(|| { + result.map_err(|e| { format!( - "Copying {} to {}", + "Copying {} to {}: {e}", input.source_path, input.destination_path ) })?; diff --git a/crates/agent/src/tools/create_directory_tool.rs b/crates/agent/src/tools/create_directory_tool.rs index d19df105fa9053e1ba3be048db799bcd737bcb43..500b5f00289db245898d5918a79dc684a6f0f110 100644 --- a/crates/agent/src/tools/create_directory_tool.rs +++ b/crates/agent/src/tools/create_directory_tool.rs @@ -4,7 +4,6 @@ use super::tool_permissions::{ }; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; -use anyhow::{Context as _, Result, anyhow}; use futures::FutureExt as _; use gpui::{App, Entity, SharedString, Task}; use project::Project; @@ -72,12 +71,12 @@ impl AgentTool for CreateDirectoryTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx); let decision = decide_permission_for_path(Self::NAME, &input.path, settings); if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow!("{}", reason))); + return Task::ready(Err(reason)); } let destination_path: Arc = input.path.as_str().into(); @@ -136,22 +135,22 @@ impl AgentTool for CreateDirectoryTool { }; if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let create_entry = project.update(cx, |project, cx| { match project.find_project_path(&input.path, cx) { Some(project_path) => Ok(project.create_entry(project_path, true, cx)), - None => Err(anyhow!("Path to create was outside the project")), + None => Err("Path to create was outside the project".to_string()), } })?; futures::select! { result = create_entry.fuse() => { - result.with_context(|| format!("Creating directory {destination_path}"))?; + result.map_err(|e| format!("Creating directory {destination_path}: {e}"))?; } _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Create directory cancelled by user"); + return Err("Create directory cancelled by user".to_string()); } } diff --git a/crates/agent/src/tools/delete_path_tool.rs b/crates/agent/src/tools/delete_path_tool.rs index bcbb841fb7d0f1a416be17c04fac9542838ede1c..048f4bd8292077874b49bd74b09cbea38b4fafc5 100644 --- a/crates/agent/src/tools/delete_path_tool.rs +++ b/crates/agent/src/tools/delete_path_tool.rs @@ -6,7 +6,6 @@ use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permi use action_log::ActionLog; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; -use anyhow::{Context as _, Result, anyhow}; use futures::{FutureExt as _, SinkExt, StreamExt, channel::mpsc}; use gpui::{App, AppContext, Entity, SharedString, Task}; use project::{Project, ProjectPath}; @@ -75,14 +74,14 @@ impl AgentTool for DeletePathTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let path = input.path; let settings = AgentSettings::get_global(cx); let decision = decide_permission_for_path(Self::NAME, &path, settings); if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow!("{}", reason))); + return Task::ready(Err(reason)); } let project = self.project.clone(); @@ -140,20 +139,20 @@ impl AgentTool for DeletePathTool { }; if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let (project_path, worktree_snapshot) = project.read_with(cx, |project, cx| { let project_path = project.find_project_path(&path, cx).ok_or_else(|| { - anyhow!("Couldn't delete {path} because that path isn't in this project.") + format!("Couldn't delete {path} because that path isn't in this project.") })?; let worktree = project .worktree_for_id(project_path.worktree_id, cx) .ok_or_else(|| { - anyhow!("Couldn't delete {path} because that path isn't in this project.") + format!("Couldn't delete {path} because that path isn't in this project.") })?; let worktree_snapshot = worktree.read(cx).snapshot(); - anyhow::Ok((project_path, worktree_snapshot)) + Result::<_, String>::Ok((project_path, worktree_snapshot)) })?; let (mut paths_tx, mut paths_rx) = mpsc::channel(256); @@ -182,7 +181,7 @@ impl AgentTool for DeletePathTool { let path_result = futures::select! { path = paths_rx.next().fuse() => path, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Delete cancelled by user"); + return Err("Delete cancelled by user".to_string()); } }; let Some(path) = path_result else { @@ -202,16 +201,16 @@ impl AgentTool for DeletePathTool { .update(cx, |project, cx| { project.delete_file(project_path, false, cx) }) - .with_context(|| { + .ok_or_else(|| { format!("Couldn't delete {path} because that path isn't in this project.") })?; futures::select! { result = deletion_task.fuse() => { - result.with_context(|| format!("Deleting {path}"))?; + result.map_err(|e| format!("Deleting {path}: {e}"))?; } _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Delete cancelled by user"); + return Err("Delete cancelled by user".to_string()); } } Ok(format!("Deleted {path}")) diff --git a/crates/agent/src/tools/diagnostics_tool.rs b/crates/agent/src/tools/diagnostics_tool.rs index 8479bbbf3dbff7eef51be9a0d85bd2ff79b91935..fea16d531ed5f4212e6b1374aee04cee67b2fc0b 100644 --- a/crates/agent/src/tools/diagnostics_tool.rs +++ b/crates/agent/src/tools/diagnostics_tool.rs @@ -1,6 +1,6 @@ use crate::{AgentTool, ToolCallEventStream}; use agent_client_protocol as acp; -use anyhow::{Result, anyhow}; +use anyhow::Result; use futures::FutureExt as _; use gpui::{App, Entity, Task}; use language::{DiagnosticSeverity, OffsetRangeExt}; @@ -90,11 +90,11 @@ impl AgentTool for DiagnosticsTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { match input.path { Some(path) if !path.is_empty() => { let Some(project_path) = self.project.read(cx).find_project_path(&path, cx) else { - return Task::ready(Err(anyhow!("Could not find path {path} in project",))); + return Task::ready(Err(format!("Could not find path {path} in project"))); }; let open_buffer_task = self @@ -103,9 +103,9 @@ impl AgentTool for DiagnosticsTool { cx.spawn(async move |cx| { let buffer = futures::select! { - result = open_buffer_task.fuse() => result?, + result = open_buffer_task.fuse() => result.map_err(|e| e.to_string())?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Diagnostics cancelled by user"); + return Err("Diagnostics cancelled by user".to_string()); } }; let mut output = String::new(); @@ -126,7 +126,8 @@ impl AgentTool for DiagnosticsTool { severity, range.start.row + 1, entry.diagnostic.message - )?; + ) + .ok(); } if output.is_empty() { diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 697ab3022312f10d53f46df9c874554d2d16aa5e..7259ce21623d37d25f3a7cebeddf02a261061175 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -7,7 +7,7 @@ use crate::{ }; use acp_thread::Diff; use agent_client_protocol::{self as acp, ToolCallLocation, ToolCallUpdateFields}; -use anyhow::{Context as _, Result, anyhow}; +use anyhow::{Context as _, Result}; use cloud_llm_client::CompletionIntent; use collections::HashSet; use futures::{FutureExt as _, StreamExt as _}; @@ -95,29 +95,47 @@ pub enum EditFileMode { } #[derive(Debug, Serialize, Deserialize)] -pub struct EditFileToolOutput { - #[serde(alias = "original_path")] - input_path: PathBuf, - new_text: String, - old_text: Arc, - #[serde(default)] - diff: String, - #[serde(alias = "raw_output")] - edit_agent_output: EditAgentOutput, +#[serde(untagged)] +pub enum EditFileToolOutput { + Success { + #[serde(alias = "original_path")] + input_path: PathBuf, + new_text: String, + old_text: Arc, + #[serde(default)] + diff: String, + #[serde(alias = "raw_output")] + edit_agent_output: EditAgentOutput, + }, + Error { + error: String, + }, +} + +impl std::fmt::Display for EditFileToolOutput { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + EditFileToolOutput::Success { + diff, input_path, .. + } => { + if diff.is_empty() { + write!(f, "No edits were made.") + } else { + write!( + f, + "Edited {}:\n\n```diff\n{diff}\n```", + input_path.display() + ) + } + } + EditFileToolOutput::Error { error } => write!(f, "{error}"), + } + } } impl From for LanguageModelToolResultContent { fn from(output: EditFileToolOutput) -> Self { - if output.diff.is_empty() { - "No edits were made.".into() - } else { - format!( - "Edited {}:\n\n```diff\n{}\n```", - output.input_path.display(), - output.diff - ) - .into() - } + output.to_string().into() } } @@ -222,16 +240,22 @@ impl AgentTool for EditFileTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let Ok(project) = self .thread .read_with(cx, |thread, _cx| thread.project().clone()) else { - return Task::ready(Err(anyhow!("thread was dropped"))); + return Task::ready(Err(EditFileToolOutput::Error { + error: "thread was dropped".to_string(), + })); }; let project_path = match resolve_path(&input, project.clone(), cx) { Ok(path) => path, - Err(err) => return Task::ready(Err(anyhow!(err))), + Err(err) => { + return Task::ready(Err(EditFileToolOutput::Error { + error: err.to_string(), + })); + } }; let abs_path = project.read(cx).absolute_path(&project_path, cx); if let Some(abs_path) = abs_path.clone() { @@ -242,255 +266,259 @@ impl AgentTool for EditFileTool { let authorize = self.authorize(&input, &event_stream, cx); cx.spawn(async move |cx: &mut AsyncApp| { - authorize.await?; - - let (request, model, action_log) = self.thread.update(cx, |thread, cx| { - let request = thread.build_completion_request(CompletionIntent::ToolResults, cx); - (request, thread.model().cloned(), thread.action_log().clone()) - })?; - let request = request?; - let model = model.context("No language model configured")?; - - let edit_format = EditFormat::from_model(model.clone())?; - let edit_agent = EditAgent::new( - model, - project.clone(), - action_log.clone(), - self.templates.clone(), - edit_format, - ); + let result: anyhow::Result = async { + authorize.await?; - let buffer = project - .update(cx, |project, cx| { - project.open_buffer(project_path.clone(), cx) - }) - .await?; - - // Check if the file has been modified since the agent last read it - if let Some(abs_path) = abs_path.as_ref() { - let (last_read_mtime, current_mtime, is_dirty, has_save_tool, has_restore_tool) = self.thread.update(cx, |thread, cx| { - let last_read = thread.file_read_times.get(abs_path).copied(); - let current = buffer.read(cx).file().and_then(|file| file.disk_state().mtime()); - let dirty = buffer.read(cx).is_dirty(); - let has_save = thread.has_tool(SaveFileTool::NAME); - let has_restore = thread.has_tool(RestoreFileFromDiskTool::NAME); - (last_read, current, dirty, has_save, has_restore) + let (request, model, action_log) = self.thread.update(cx, |thread, cx| { + let request = thread.build_completion_request(CompletionIntent::ToolResults, cx); + (request, thread.model().cloned(), thread.action_log().clone()) })?; + let request = request?; + let model = model.context("No language model configured")?; - // Check for unsaved changes first - these indicate modifications we don't know about - if is_dirty { - let message = match (has_save_tool, has_restore_tool) { - (true, true) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ - If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ - If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." - } - (true, false) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ - If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ - If they want to discard them, ask the user to manually revert the file, then inform you when it's ok to proceed." - } - (false, true) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ - If they want to keep them, ask the user to manually save the file, then inform you when it's ok to proceed. \ - If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." - } - (false, false) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes, \ - then ask them to save or revert the file manually and inform you when it's ok to proceed." - } - }; - anyhow::bail!("{}", message); - } + let edit_format = EditFormat::from_model(model.clone())?; + let edit_agent = EditAgent::new( + model, + project.clone(), + action_log.clone(), + self.templates.clone(), + edit_format, + ); - // Check if the file was modified on disk since we last read it - if let (Some(last_read), Some(current)) = (last_read_mtime, current_mtime) { - // MTime can be unreliable for comparisons, so our newtype intentionally - // doesn't support comparing them. If the mtime at all different - // (which could be because of a modification or because e.g. system clock changed), - // we pessimistically assume it was modified. - if current != last_read { - anyhow::bail!( - "The file {} has been modified since you last read it. \ - Please read the file again to get the current state before editing it.", - input.path.display() - ); - } - } - } + let buffer = project + .update(cx, |project, cx| { + project.open_buffer(project_path.clone(), cx) + }) + .await?; + + // Check if the file has been modified since the agent last read it + if let Some(abs_path) = abs_path.as_ref() { + let (last_read_mtime, current_mtime, is_dirty, has_save_tool, has_restore_tool) = self.thread.update(cx, |thread, cx| { + let last_read = thread.file_read_times.get(abs_path).copied(); + let current = buffer.read(cx).file().and_then(|file| file.disk_state().mtime()); + let dirty = buffer.read(cx).is_dirty(); + let has_save = thread.has_tool(SaveFileTool::NAME); + let has_restore = thread.has_tool(RestoreFileFromDiskTool::NAME); + (last_read, current, dirty, has_save, has_restore) + })?; - let diff = cx.new(|cx| Diff::new(buffer.clone(), cx)); - event_stream.update_diff(diff.clone()); - let _finalize_diff = util::defer({ - let diff = diff.downgrade(); - let mut cx = cx.clone(); - move || { - diff.update(&mut cx, |diff, cx| diff.finalize(cx)).ok(); - } - }); + // Check for unsaved changes first - these indicate modifications we don't know about + if is_dirty { + let message = match (has_save_tool, has_restore_tool) { + (true, true) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ + If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ + If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." + } + (true, false) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ + If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ + If they want to discard them, ask the user to manually revert the file, then inform you when it's ok to proceed." + } + (false, true) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ + If they want to keep them, ask the user to manually save the file, then inform you when it's ok to proceed. \ + If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." + } + (false, false) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes, \ + then ask them to save or revert the file manually and inform you when it's ok to proceed." + } + }; + anyhow::bail!("{}", message); + } - let old_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); - let old_text = cx - .background_spawn({ - let old_snapshot = old_snapshot.clone(); - async move { Arc::new(old_snapshot.text()) } - }) - .await; + // Check if the file was modified on disk since we last read it + if let (Some(last_read), Some(current)) = (last_read_mtime, current_mtime) { + // MTime can be unreliable for comparisons, so our newtype intentionally + // doesn't support comparing them. If the mtime at all different + // (which could be because of a modification or because e.g. system clock changed), + // we pessimistically assume it was modified. + if current != last_read { + anyhow::bail!( + "The file {} has been modified since you last read it. \ + Please read the file again to get the current state before editing it.", + input.path.display() + ); + } + } + } - let (output, mut events) = if matches!(input.mode, EditFileMode::Edit) { - edit_agent.edit( - buffer.clone(), - input.display_description.clone(), - &request, - cx, - ) - } else { - edit_agent.overwrite( - buffer.clone(), - input.display_description.clone(), - &request, - cx, - ) - }; - - let mut hallucinated_old_text = false; - let mut ambiguous_ranges = Vec::new(); - let mut emitted_location = false; - loop { - let event = futures::select! { - event = events.next().fuse() => match event { - Some(event) => event, - None => break, - }, - _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Edit cancelled by user"); + let diff = cx.new(|cx| Diff::new(buffer.clone(), cx)); + event_stream.update_diff(diff.clone()); + let _finalize_diff = util::defer({ + let diff = diff.downgrade(); + let mut cx = cx.clone(); + move || { + diff.update(&mut cx, |diff, cx| diff.finalize(cx)).ok(); } + }); + + let old_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); + let old_text = cx + .background_spawn({ + let old_snapshot = old_snapshot.clone(); + async move { Arc::new(old_snapshot.text()) } + }) + .await; + + let (output, mut events) = if matches!(input.mode, EditFileMode::Edit) { + edit_agent.edit( + buffer.clone(), + input.display_description.clone(), + &request, + cx, + ) + } else { + edit_agent.overwrite( + buffer.clone(), + input.display_description.clone(), + &request, + cx, + ) }; - match event { - EditAgentOutputEvent::Edited(range) => { - if !emitted_location { - let line = Some(buffer.update(cx, |buffer, _cx| { - range.start.to_point(&buffer.snapshot()).row - })); - if let Some(abs_path) = abs_path.clone() { - event_stream.update_fields(ToolCallUpdateFields::new().locations(vec![ToolCallLocation::new(abs_path).line(line)])); + + let mut hallucinated_old_text = false; + let mut ambiguous_ranges = Vec::new(); + let mut emitted_location = false; + loop { + let event = futures::select! { + event = events.next().fuse() => match event { + Some(event) => event, + None => break, + }, + _ = event_stream.cancelled_by_user().fuse() => { + anyhow::bail!("Edit cancelled by user"); + } + }; + match event { + EditAgentOutputEvent::Edited(range) => { + if !emitted_location { + let line = Some(buffer.update(cx, |buffer, _cx| { + range.start.to_point(&buffer.snapshot()).row + })); + if let Some(abs_path) = abs_path.clone() { + event_stream.update_fields(ToolCallUpdateFields::new().locations(vec![ToolCallLocation::new(abs_path).line(line)])); + } + emitted_location = true; } - emitted_location = true; + }, + EditAgentOutputEvent::UnresolvedEditRange => hallucinated_old_text = true, + EditAgentOutputEvent::AmbiguousEditRange(ranges) => ambiguous_ranges = ranges, + EditAgentOutputEvent::ResolvingEditRange(range) => { + diff.update(cx, |card, cx| card.reveal_range(range.clone(), cx)); + // if !emitted_location { + // let line = buffer.update(cx, |buffer, _cx| { + // range.start.to_point(&buffer.snapshot()).row + // }).ok(); + // if let Some(abs_path) = abs_path.clone() { + // event_stream.update_fields(ToolCallUpdateFields { + // locations: Some(vec![ToolCallLocation { path: abs_path, line }]), + // ..Default::default() + // }); + // } + // } } - }, - EditAgentOutputEvent::UnresolvedEditRange => hallucinated_old_text = true, - EditAgentOutputEvent::AmbiguousEditRange(ranges) => ambiguous_ranges = ranges, - EditAgentOutputEvent::ResolvingEditRange(range) => { - diff.update(cx, |card, cx| card.reveal_range(range.clone(), cx)); - // if !emitted_location { - // let line = buffer.update(cx, |buffer, _cx| { - // range.start.to_point(&buffer.snapshot()).row - // }).ok(); - // if let Some(abs_path) = abs_path.clone() { - // event_stream.update_fields(ToolCallUpdateFields { - // locations: Some(vec![ToolCallLocation { path: abs_path, line }]), - // ..Default::default() - // }); - // } - // } } } - } - - let edit_agent_output = output.await?; - let format_on_save_enabled = buffer.read_with(cx, |buffer, cx| { - let settings = language_settings::language_settings( - buffer.language().map(|l| l.name()), - buffer.file(), - cx, - ); - settings.format_on_save != FormatOnSave::Off - }); - - if format_on_save_enabled { - action_log.update(cx, |log, cx| { - log.buffer_edited(buffer.clone(), cx); - }); + let edit_agent_output = output.await?; - let format_task = project.update(cx, |project, cx| { - project.format( - HashSet::from_iter([buffer.clone()]), - LspFormatTarget::Buffers, - false, // Don't push to history since the tool did it. - FormatTrigger::Save, + let format_on_save_enabled = buffer.read_with(cx, |buffer, cx| { + let settings = language_settings::language_settings( + buffer.language().map(|l| l.name()), + buffer.file(), cx, - ) + ); + settings.format_on_save != FormatOnSave::Off }); - format_task.await.log_err(); - } - project - .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)) - .await?; + if format_on_save_enabled { + action_log.update(cx, |log, cx| { + log.buffer_edited(buffer.clone(), cx); + }); + + let format_task = project.update(cx, |project, cx| { + project.format( + HashSet::from_iter([buffer.clone()]), + LspFormatTarget::Buffers, + false, // Don't push to history since the tool did it. + FormatTrigger::Save, + cx, + ) + }); + format_task.await.log_err(); + } - action_log.update(cx, |log, cx| { - log.buffer_edited(buffer.clone(), cx); - }); + project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)) + .await?; - // Update the recorded read time after a successful edit so consecutive edits work - if let Some(abs_path) = abs_path.as_ref() { - if let Some(new_mtime) = buffer.read_with(cx, |buffer, _| { - buffer.file().and_then(|file| file.disk_state().mtime()) - }) { - self.thread.update(cx, |thread, _| { - thread.file_read_times.insert(abs_path.to_path_buf(), new_mtime); - })?; - } - } + action_log.update(cx, |log, cx| { + log.buffer_edited(buffer.clone(), cx); + }); - let new_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); - let (new_text, unified_diff) = cx - .background_spawn({ - let new_snapshot = new_snapshot.clone(); - let old_text = old_text.clone(); - async move { - let new_text = new_snapshot.text(); - let diff = language::unified_diff(&old_text, &new_text); - (new_text, diff) + // Update the recorded read time after a successful edit so consecutive edits work + if let Some(abs_path) = abs_path.as_ref() { + if let Some(new_mtime) = buffer.read_with(cx, |buffer, _| { + buffer.file().and_then(|file| file.disk_state().mtime()) + }) { + self.thread.update(cx, |thread, _| { + thread.file_read_times.insert(abs_path.to_path_buf(), new_mtime); + })?; } - }) - .await; + } - let input_path = input.path.display(); - if unified_diff.is_empty() { - anyhow::ensure!( - !hallucinated_old_text, - formatdoc! {" - Some edits were produced but none of them could be applied. - Read the relevant sections of {input_path} again so that - I can perform the requested edits. - "} - ); - anyhow::ensure!( - ambiguous_ranges.is_empty(), - { - let line_numbers = ambiguous_ranges - .iter() - .map(|range| range.start.to_string()) - .collect::>() - .join(", "); + let new_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); + let (new_text, unified_diff) = cx + .background_spawn({ + let new_snapshot = new_snapshot.clone(); + let old_text = old_text.clone(); + async move { + let new_text = new_snapshot.text(); + let diff = language::unified_diff(&old_text, &new_text); + (new_text, diff) + } + }) + .await; + + let input_path = input.path.display(); + if unified_diff.is_empty() { + anyhow::ensure!( + !hallucinated_old_text, formatdoc! {" - matches more than one position in the file (lines: {line_numbers}). Read the - relevant sections of {input_path} again and extend so - that I can perform the requested edits. + Some edits were produced but none of them could be applied. + Read the relevant sections of {input_path} again so that + I can perform the requested edits. "} - } - ); - } + ); + anyhow::ensure!( + ambiguous_ranges.is_empty(), + { + let line_numbers = ambiguous_ranges + .iter() + .map(|range| range.start.to_string()) + .collect::>() + .join(", "); + formatdoc! {" + matches more than one position in the file (lines: {line_numbers}). Read the + relevant sections of {input_path} again and extend so + that I can perform the requested edits. + "} + } + ); + } - Ok(EditFileToolOutput { - input_path: input.path, - new_text, - old_text, - diff: unified_diff, - edit_agent_output, - }) + anyhow::Ok(EditFileToolOutput::Success { + input_path: input.path, + new_text, + old_text, + diff: unified_diff, + edit_agent_output, + }) + }.await; + result + .map_err(|e| EditFileToolOutput::Error { error: e.to_string() }) }) } @@ -501,16 +529,26 @@ impl AgentTool for EditFileTool { event_stream: ToolCallEventStream, cx: &mut App, ) -> Result<()> { - event_stream.update_diff(cx.new(|cx| { - Diff::finalized( - output.input_path.to_string_lossy().into_owned(), - Some(output.old_text.to_string()), - output.new_text, - self.language_registry.clone(), - cx, - ) - })); - Ok(()) + match output { + EditFileToolOutput::Success { + input_path, + old_text, + new_text, + .. + } => { + event_stream.update_diff(cx.new(|cx| { + Diff::finalized( + input_path.to_string_lossy().into_owned(), + Some(old_text.to_string()), + new_text, + self.language_registry.clone(), + cx, + ) + })); + Ok(()) + } + EditFileToolOutput::Error { .. } => Ok(()), + } } } diff --git a/crates/agent/src/tools/fetch_tool.rs b/crates/agent/src/tools/fetch_tool.rs index 5f65bcd6f1d12b6a7f3e7473f624c319068c80ce..e573c2202b09d1283d75c3eda20b65be1bcd82a7 100644 --- a/crates/agent/src/tools/fetch_tool.rs +++ b/crates/agent/src/tools/fetch_tool.rs @@ -144,7 +144,7 @@ impl AgentTool for FetchTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx); let decision = decide_permission_from_settings(Self::NAME, std::slice::from_ref(&input.url), settings); @@ -152,7 +152,7 @@ impl AgentTool for FetchTool { let authorize = match decision { ToolPermissionDecision::Allow => None, ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); + return Task::ready(Err(reason)); } ToolPermissionDecision::Confirm => { let context = @@ -177,13 +177,13 @@ impl AgentTool for FetchTool { cx.foreground_executor().spawn(async move { let text = futures::select! { - result = fetch_task.fuse() => result?, + result = fetch_task.fuse() => result.map_err(|e| e.to_string())?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Fetch cancelled by user"); + return Err("Fetch cancelled by user".to_string()); } }; if text.trim().is_empty() { - bail!("no textual content found"); + return Err("no textual content found".to_string()); } Ok(text) }) diff --git a/crates/agent/src/tools/find_path_tool.rs b/crates/agent/src/tools/find_path_tool.rs index 202d33b22d7d49e5e582ff58b6f866cc68f7ced3..4ba60c61063c08ac002dc7dc16fa11b987cbab74 100644 --- a/crates/agent/src/tools/find_path_tool.rs +++ b/crates/agent/src/tools/find_path_tool.rs @@ -39,33 +39,48 @@ pub struct FindPathToolInput { } #[derive(Debug, Serialize, Deserialize)] -pub struct FindPathToolOutput { - offset: usize, - current_matches_page: Vec, - all_matches_len: usize, +#[serde(untagged)] +pub enum FindPathToolOutput { + Success { + offset: usize, + current_matches_page: Vec, + all_matches_len: usize, + }, + Error { + error: String, + }, } impl From for LanguageModelToolResultContent { fn from(output: FindPathToolOutput) -> Self { - if output.current_matches_page.is_empty() { - "No matches found".into() - } else { - let mut llm_output = format!("Found {} total matches.", output.all_matches_len); - if output.all_matches_len > RESULTS_PER_PAGE { - write!( - &mut llm_output, - "\nShowing results {}-{} (provide 'offset' parameter for more results):", - output.offset + 1, - output.offset + output.current_matches_page.len() - ) - .unwrap(); - } + match output { + FindPathToolOutput::Success { + offset, + current_matches_page, + all_matches_len, + } => { + if current_matches_page.is_empty() { + "No matches found".into() + } else { + let mut llm_output = format!("Found {} total matches.", all_matches_len); + if all_matches_len > RESULTS_PER_PAGE { + write!( + &mut llm_output, + "\nShowing results {}-{} (provide 'offset' parameter for more results):", + offset + 1, + offset + current_matches_page.len() + ) + .ok(); + } - for mat in output.current_matches_page { - write!(&mut llm_output, "\n{}", mat.display()).unwrap(); - } + for mat in current_matches_page { + write!(&mut llm_output, "\n{}", mat.display()).ok(); + } - llm_output.into() + llm_output.into() + } + } + FindPathToolOutput::Error { error } => error.into(), } } } @@ -109,14 +124,14 @@ impl AgentTool for FindPathTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let search_paths_task = search_paths(&input.glob, self.project.clone(), cx); cx.background_spawn(async move { let matches = futures::select! { - result = search_paths_task.fuse() => result?, + result = search_paths_task.fuse() => result.map_err(|e| FindPathToolOutput::Error { error: e.to_string() })?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Path search cancelled by user"); + return Err(FindPathToolOutput::Error { error: "Path search cancelled by user".to_string() }); } }; let paginated_matches: &[PathBuf] = &matches[cmp::min(input.offset, matches.len()) @@ -146,7 +161,7 @@ impl AgentTool for FindPathTool { ), ); - Ok(FindPathToolOutput { + Ok(FindPathToolOutput::Success { offset: input.offset, current_matches_page: paginated_matches.to_vec(), all_matches_len: matches.len(), diff --git a/crates/agent/src/tools/grep_tool.rs b/crates/agent/src/tools/grep_tool.rs index 09e9c9e4fef73575cb8974a6cbea2f327a50034f..16162107dff84ab40117b7783e04b633d144a214 100644 --- a/crates/agent/src/tools/grep_tool.rs +++ b/crates/agent/src/tools/grep_tool.rs @@ -1,6 +1,6 @@ use crate::{AgentTool, ToolCallEventStream}; use agent_client_protocol as acp; -use anyhow::{Result, anyhow}; +use anyhow::Result; use futures::{FutureExt as _, StreamExt}; use gpui::{App, Entity, SharedString, Task}; use language::{OffsetRangeExt, ParseStatus, Point}; @@ -117,7 +117,7 @@ impl AgentTool for GrepTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { const CONTEXT_LINES: u32 = 2; const MAX_ANCESTOR_LINES: u32 = 10; @@ -133,7 +133,7 @@ impl AgentTool for GrepTool { ) { Ok(matcher) => matcher, Err(error) => { - return Task::ready(Err(anyhow!("invalid include glob pattern: {error}"))); + return Task::ready(Err(format!("invalid include glob pattern: {error}"))); } }; @@ -148,7 +148,7 @@ impl AgentTool for GrepTool { match PathMatcher::new(exclude_patterns, path_style) { Ok(matcher) => matcher, Err(error) => { - return Task::ready(Err(anyhow!("invalid exclude pattern: {error}"))); + return Task::ready(Err(format!("invalid exclude pattern: {error}"))); } } }; @@ -165,7 +165,7 @@ impl AgentTool for GrepTool { None, ) { Ok(query) => query, - Err(error) => return Task::ready(Err(error)), + Err(error) => return Task::ready(Err(error.to_string())), }; let results = self @@ -188,7 +188,7 @@ impl AgentTool for GrepTool { let search_result = futures::select! { result = rx.next().fuse() => result, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Search cancelled by user"); + return Err("Search cancelled by user".to_string()); } }; let Some(SearchResult::Buffer { buffer, ranges }) = search_result else { @@ -218,7 +218,7 @@ impl AgentTool for GrepTool { } while *parse_status.borrow() != ParseStatus::Idle { - parse_status.changed().await?; + parse_status.changed().await.map_err(|e| e.to_string())?; } let snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); @@ -280,7 +280,8 @@ impl AgentTool for GrepTool { } if !file_header_written { - writeln!(output, "\n## Matches in {}", path.display())?; + writeln!(output, "\n## Matches in {}", path.display()) + .ok(); file_header_written = true; } @@ -288,13 +289,16 @@ impl AgentTool for GrepTool { output.push_str("\n### "); for symbol in parent_symbols { - write!(output, "{} › ", symbol.text)?; + write!(output, "{} › ", symbol.text) + .ok(); } if range.start.row == end_row { - writeln!(output, "L{}", range.start.row + 1)?; + writeln!(output, "L{}", range.start.row + 1) + .ok(); } else { - writeln!(output, "L{}-{}", range.start.row + 1, end_row + 1)?; + writeln!(output, "L{}-{}", range.start.row + 1, end_row + 1) + .ok(); } output.push_str("```\n"); @@ -304,7 +308,8 @@ impl AgentTool for GrepTool { if let Some(ancestor_range) = ancestor_range && end_row < ancestor_range.end.row { let remaining_lines = ancestor_range.end.row - end_row; - writeln!(output, "\n{} lines remaining in ancestor node. Read the file to see all.", remaining_lines)?; + writeln!(output, "\n{} lines remaining in ancestor node. Read the file to see all.", remaining_lines) + .ok(); } matches_found += 1; diff --git a/crates/agent/src/tools/list_directory_tool.rs b/crates/agent/src/tools/list_directory_tool.rs index 16cc61c4e5d8f3ea7155608dfb048f6f3f994d6c..5dddee94904283ccb9198ce56aa4005250b5908a 100644 --- a/crates/agent/src/tools/list_directory_tool.rs +++ b/crates/agent/src/tools/list_directory_tool.rs @@ -149,7 +149,7 @@ impl AgentTool for ListDirectoryTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { // Sometimes models will return these even though we tell it to give a path and not a glob. // When this happens, just list the root worktree directories. if matches!(input.path.as_str(), "." | "" | "./" | "*") { @@ -187,7 +187,7 @@ impl AgentTool for ListDirectoryTool { canonical_target, } => (project_path, Some(canonical_target)), }) - })?; + }).map_err(|e| e.to_string())?; // Check settings exclusions synchronously project.read_with(cx, |project, cx| { @@ -236,7 +236,7 @@ impl AgentTool for ListDirectoryTool { } anyhow::Ok(()) - })?; + }).map_err(|e| e.to_string())?; if let Some(canonical_target) = &symlink_canonical_target { let authorize = cx.update(|cx| { @@ -248,13 +248,13 @@ impl AgentTool for ListDirectoryTool { cx, ) }); - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let list_path = input.path; cx.update(|cx| { Self::build_directory_output(&project, &project_path, &list_path, cx) - }) + }).map_err(|e| e.to_string()) }) } } @@ -422,7 +422,7 @@ mod tests { let output = cx .update(|cx| tool.clone().run(input, ToolCallEventStream::test().0, cx)) .await; - assert!(output.unwrap_err().to_string().contains("Path not found")); + assert!(output.unwrap_err().contains("Path not found")); // Test trying to list a file instead of directory let input = ListDirectoryToolInput { @@ -431,12 +431,7 @@ mod tests { let output = cx .update(|cx| tool.run(input, ToolCallEventStream::test().0, cx)) .await; - assert!( - output - .unwrap_err() - .to_string() - .contains("is not a directory") - ); + assert!(output.unwrap_err().contains("is not a directory")); } #[gpui::test] @@ -528,10 +523,7 @@ mod tests { .update(|cx| tool.clone().run(input, ToolCallEventStream::test().0, cx)) .await; assert!( - output - .unwrap_err() - .to_string() - .contains("file_scan_exclusions"), + output.unwrap_err().contains("file_scan_exclusions"), "Error should mention file_scan_exclusions" ); @@ -711,12 +703,7 @@ mod tests { let output = cx .update(|cx| tool.clone().run(input, ToolCallEventStream::test().0, cx)) .await; - assert!( - output - .unwrap_err() - .to_string() - .contains("Cannot list directory"), - ); + assert!(output.unwrap_err().contains("Cannot list directory"),); } #[gpui::test] @@ -897,7 +884,7 @@ mod tests { result.is_err(), "Expected list_directory to fail on private path" ); - let error = result.unwrap_err().to_string(); + let error = result.unwrap_err(); assert!( error.contains("private"), "Expected private path validation error, got: {error}" diff --git a/crates/agent/src/tools/move_path_tool.rs b/crates/agent/src/tools/move_path_tool.rs index 887cfd18d361b5d296017274b5600c5d3cb30716..4c337d0ec2827ad7c63c87ef206f0e74dc63091f 100644 --- a/crates/agent/src/tools/move_path_tool.rs +++ b/crates/agent/src/tools/move_path_tool.rs @@ -5,7 +5,6 @@ use super::tool_permissions::{ use crate::{AgentTool, ToolCallEventStream, ToolPermissionDecision, decide_permission_for_paths}; use agent_client_protocol::ToolKind; use agent_settings::AgentSettings; -use anyhow::{Context as _, Result, anyhow}; use futures::FutureExt as _; use gpui::{App, Entity, SharedString, Task}; use project::Project; @@ -96,12 +95,12 @@ impl AgentTool for MovePathTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx); let paths = vec![input.source_path.clone(), input.destination_path.clone()]; let decision = decide_permission_for_paths(Self::NAME, &paths, settings); if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow!("{}", reason))); + return Task::ready(Err(reason)); } let project = self.project.clone(); @@ -160,7 +159,7 @@ impl AgentTool for MovePathTool { }; if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let rename_task = project.update(cx, |project, cx| { @@ -170,27 +169,24 @@ impl AgentTool for MovePathTool { { Some(entity) => match project.find_project_path(&input.destination_path, cx) { Some(project_path) => Ok(project.rename_entry(entity.id, project_path, cx)), - None => Err(anyhow!( + None => Err(format!( "Destination path {} was outside the project.", input.destination_path )), }, - None => Err(anyhow!( + None => Err(format!( "Source path {} was not found in the project.", input.source_path )), } })?; - let result = futures::select! { - result = rename_task.fuse() => result, + futures::select! { + result = rename_task.fuse() => result.map_err(|e| format!("Moving {} to {}: {e}", input.source_path, input.destination_path))?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Move cancelled by user"); + return Err("Move cancelled by user".to_string()); } }; - result.with_context(|| { - format!("Moving {} to {}", input.source_path, input.destination_path) - })?; Ok(format!( "Moved {} to {}", input.source_path, input.destination_path diff --git a/crates/agent/src/tools/now_tool.rs b/crates/agent/src/tools/now_tool.rs index cb1a05f412340b5a31096851bd973892a51a9775..689d70ff20d15cbc56fcc0e14a3b46408647f737 100644 --- a/crates/agent/src/tools/now_tool.rs +++ b/crates/agent/src/tools/now_tool.rs @@ -1,7 +1,6 @@ use std::sync::Arc; use agent_client_protocol as acp; -use anyhow::Result; use chrono::{Local, Utc}; use gpui::{App, SharedString, Task}; use schemars::JsonSchema; @@ -52,7 +51,7 @@ impl AgentTool for NowTool { input: Self::Input, _event_stream: ToolCallEventStream, _cx: &mut App, - ) -> Task> { + ) -> Task> { let now = match input.timezone { Timezone::Utc => Utc::now().to_rfc3339(), Timezone::Local => Local::now().to_rfc3339(), diff --git a/crates/agent/src/tools/open_tool.rs b/crates/agent/src/tools/open_tool.rs index 4028d3998b237b196f9c15756cfe7495d1cda3fa..c0b24efbec6418c437e9e3d14ffb5d40b45c91b0 100644 --- a/crates/agent/src/tools/open_tool.rs +++ b/crates/agent/src/tools/open_tool.rs @@ -4,7 +4,6 @@ use super::tool_permissions::{ }; use crate::AgentTool; use agent_client_protocol::ToolKind; -use anyhow::{Context as _, Result}; use futures::FutureExt as _; use gpui::{App, AppContext as _, Entity, SharedString, Task}; use project::Project; @@ -65,7 +64,7 @@ impl AgentTool for OpenTool { input: Self::Input, event_stream: crate::ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { // If path_or_url turns out to be a path in the project, make it absolute. let abs_path = to_absolute_path(&input.path_or_url, self.project.clone(), cx); let initial_title = self.initial_title(Ok(input.clone()), cx); @@ -114,9 +113,9 @@ impl AgentTool for OpenTool { }; futures::select! { - result = authorize.fuse() => result?, + result = authorize.fuse() => result.map_err(|e| e.to_string())?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Open cancelled by user"); + return Err("Open cancelled by user".to_string()); } } @@ -126,7 +125,7 @@ impl AgentTool for OpenTool { Some(path) => open::that(path), None => open::that(path_or_url), } - .context("Failed to open URL or file path") + .map_err(|e| format!("Failed to open URL or file path: {e}")) }) .await?; diff --git a/crates/agent/src/tools/read_file_tool.rs b/crates/agent/src/tools/read_file_tool.rs index dcdb6dc914a7a3f135583e795671d2b03a2913bf..efd33fe5caece4cee4fc02aab8c1a0ebee92f94e 100644 --- a/crates/agent/src/tools/read_file_tool.rs +++ b/crates/agent/src/tools/read_file_tool.rs @@ -13,6 +13,10 @@ use settings::Settings; use std::sync::Arc; use util::markdown::MarkdownCodeBlock; +fn tool_content_err(e: impl std::fmt::Display) -> LanguageModelToolResultContent { + LanguageModelToolResultContent::from(e.to_string()) +} + use super::tool_permissions::{ ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, resolve_project_path, @@ -113,7 +117,7 @@ impl AgentTool for ReadFileTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let project = self.project.clone(); let thread = self.thread.clone(); let action_log = self.action_log.clone(); @@ -132,7 +136,7 @@ impl AgentTool for ReadFileTool { canonical_target, } => (project_path, Some(canonical_target)), }) - })?; + }).map_err(tool_content_err)?; let abs_path = project .read_with(cx, |project, cx| { @@ -140,7 +144,7 @@ impl AgentTool for ReadFileTool { }) .ok_or_else(|| { anyhow!("Failed to convert {} to absolute path", &input.path) - })?; + }).map_err(tool_content_err)?; // Check settings exclusions synchronously project.read_with(cx, |_project, cx| { @@ -175,7 +179,7 @@ impl AgentTool for ReadFileTool { } anyhow::Ok(()) - })?; + }).map_err(tool_content_err)?; if let Some(canonical_target) = &symlink_canonical_target { let authorize = cx.update(|cx| { @@ -187,7 +191,7 @@ impl AgentTool for ReadFileTool { cx, ) }); - authorize.await?; + authorize.await.map_err(tool_content_err)?; } let file_path = input.path.clone(); @@ -211,7 +215,7 @@ impl AgentTool for ReadFileTool { project.open_image(project_path.clone(), cx) }) }) - .await?; + .await.map_err(tool_content_err)?; let image = image_entity.read_with(cx, |image_item, _| Arc::clone(&image_item.image)); @@ -219,7 +223,8 @@ impl AgentTool for ReadFileTool { let language_model_image = cx .update(|cx| LanguageModelImage::from_image(image, cx)) .await - .context("processing image")?; + .context("processing image") + .map_err(tool_content_err)?; event_stream.update_fields(ToolCallUpdateFields::new().content(vec![ acp::ToolCallContent::Content(acp::Content::new(acp::ContentBlock::Image( @@ -235,9 +240,9 @@ impl AgentTool for ReadFileTool { }); let buffer = futures::select! { - result = open_buffer_task.fuse() => result?, + result = open_buffer_task.fuse() => result.map_err(tool_content_err)?, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("File read cancelled by user"); + return Err(tool_content_err("File read cancelled by user")); } }; if buffer.read_with(cx, |buffer, _| { @@ -246,7 +251,7 @@ impl AgentTool for ReadFileTool { .as_ref() .is_none_or(|file| !file.disk_state().exists()) }) { - anyhow::bail!("{file_path} not found"); + return Err(tool_content_err(format!("{file_path} not found"))); } // Record the file read time and mtime @@ -294,7 +299,7 @@ impl AgentTool for ReadFileTool { Some(&abs_path.to_string_lossy()), cx, ) - .await?; + .await.map_err(tool_content_err)?; action_log.update(cx, |log, cx| { log.buffer_read(buffer.clone(), cx); @@ -397,7 +402,7 @@ mod test { }) .await; assert_eq!( - result.unwrap_err().to_string(), + error_text(result.unwrap_err()), "root/nonexistent_file.txt not found" ); } @@ -640,6 +645,13 @@ mod test { assert_eq!(result.unwrap(), "Line 3\n".into()); } + fn error_text(content: LanguageModelToolResultContent) -> String { + match content { + LanguageModelToolResultContent::Text(text) => text.to_string(), + other => panic!("Expected text error, got: {other:?}"), + } + } + fn init_test(cx: &mut TestAppContext) { cx.update(|cx| { let settings_store = SettingsStore::test(cx); @@ -1051,10 +1063,7 @@ mod test { assert!(result.is_err()); assert!( - result - .unwrap_err() - .to_string() - .contains("worktree `private_files` setting"), + error_text(result.unwrap_err()).contains("worktree `private_files` setting"), "Error should mention worktree private_files setting" ); @@ -1072,10 +1081,7 @@ mod test { assert!(result.is_err()); assert!( - result - .unwrap_err() - .to_string() - .contains("worktree `file_scan_exclusions` setting"), + error_text(result.unwrap_err()).contains("worktree `file_scan_exclusions` setting"), "Error should mention worktree file_scan_exclusions setting" ); @@ -1111,10 +1117,7 @@ mod test { assert!(result.is_err()); assert!( - result - .unwrap_err() - .to_string() - .contains("worktree `private_files` setting"), + error_text(result.unwrap_err()).contains("worktree `private_files` setting"), "Error should mention worktree private_files setting" ); @@ -1132,10 +1135,7 @@ mod test { assert!(result.is_err()); assert!( - result - .unwrap_err() - .to_string() - .contains("worktree `file_scan_exclusions` setting"), + error_text(result.unwrap_err()).contains("worktree `file_scan_exclusions` setting"), "Error should mention worktree file_scan_exclusions setting" ); @@ -1154,10 +1154,7 @@ mod test { assert!(result.is_err()); assert!( - result - .unwrap_err() - .to_string() - .contains("worktree `private_files` setting"), + error_text(result.unwrap_err()).contains("worktree `private_files` setting"), "Config.toml should be blocked by worktree1's private_files setting" ); } @@ -1385,7 +1382,7 @@ mod test { result.is_err(), "Expected read_file to fail on private path" ); - let error = result.unwrap_err().to_string(); + let error = error_text(result.unwrap_err()); assert!( error.contains("private_files"), "Expected private-files validation error, got: {error}" diff --git a/crates/agent/src/tools/restore_file_from_disk_tool.rs b/crates/agent/src/tools/restore_file_from_disk_tool.rs index e07b7d52b0bdfcf41f4a07f56dda317573f5f716..304e0d1180fe626482206bfdc2dfa6d53f529816 100644 --- a/crates/agent/src/tools/restore_file_from_disk_tool.rs +++ b/crates/agent/src/tools/restore_file_from_disk_tool.rs @@ -5,7 +5,6 @@ use super::tool_permissions::{ }; use agent_client_protocol as acp; use agent_settings::AgentSettings; -use anyhow::Result; use collections::FxHashSet; use futures::FutureExt as _; use gpui::{App, Entity, SharedString, Task}; @@ -70,7 +69,7 @@ impl AgentTool for RestoreFileFromDiskTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx).clone(); // Check for any immediate deny before spawning async work. @@ -78,7 +77,7 @@ impl AgentTool for RestoreFileFromDiskTool { let path_str = path.to_string_lossy(); let decision = decide_permission_for_path(Self::NAME, &path_str, &settings); if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); + return Task::ready(Err(reason)); } } @@ -112,7 +111,7 @@ impl AgentTool for RestoreFileFromDiskTool { } } ToolPermissionDecision::Deny(reason) => { - return Err(anyhow::anyhow!("{}", reason)); + return Err(reason); } ToolPermissionDecision::Confirm => { if !symlink_escape { @@ -159,7 +158,7 @@ impl AgentTool for RestoreFileFromDiskTool { }; let context = crate::ToolPermissionContext::new(Self::NAME, confirmation_paths); let authorize = cx.update(|cx| event_stream.authorize(title, context, cx)); - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let mut buffers_to_reload: FxHashSet> = FxHashSet::default(); @@ -221,7 +220,7 @@ impl AgentTool for RestoreFileFromDiskTool { } } _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Restore cancelled by user"); + return Err("Restore cancelled by user".to_string()); } }; @@ -243,7 +242,7 @@ impl AgentTool for RestoreFileFromDiskTool { let result = futures::select! { result = reload_task.fuse() => result, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Restore cancelled by user"); + return Err("Restore cancelled by user".to_string()); } }; if let Err(error) = result { diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs index bb46e1018713baa44a187ae75aeda5f1a491f5c8..20140c77d113d96c741d5afbe672882f708870d6 100644 --- a/crates/agent/src/tools/save_file_tool.rs +++ b/crates/agent/src/tools/save_file_tool.rs @@ -1,6 +1,5 @@ use agent_client_protocol as acp; use agent_settings::AgentSettings; -use anyhow::Result; use collections::FxHashSet; use futures::FutureExt as _; use gpui::{App, Entity, SharedString, Task}; @@ -67,7 +66,7 @@ impl AgentTool for SaveFileTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx).clone(); // Check for any immediate deny before spawning async work. @@ -75,7 +74,7 @@ impl AgentTool for SaveFileTool { let path_str = path.to_string_lossy(); let decision = decide_permission_for_path(Self::NAME, &path_str, &settings); if let ToolPermissionDecision::Deny(reason) = decision { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); + return Task::ready(Err(reason)); } } @@ -109,7 +108,7 @@ impl AgentTool for SaveFileTool { } } ToolPermissionDecision::Deny(reason) => { - return Err(anyhow::anyhow!("{}", reason)); + return Err(reason); } ToolPermissionDecision::Confirm => { if !symlink_escape { @@ -154,7 +153,7 @@ impl AgentTool for SaveFileTool { let context = crate::ToolPermissionContext::new(Self::NAME, confirmation_paths.clone()); let authorize = cx.update(|cx| event_stream.authorize(title, context, cx)); - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let mut buffers_to_save: FxHashSet> = FxHashSet::default(); @@ -217,7 +216,7 @@ impl AgentTool for SaveFileTool { } } _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Save cancelled by user"); + return Err("Save cancelled by user".to_string()); } }; @@ -247,7 +246,7 @@ impl AgentTool for SaveFileTool { let save_result = futures::select! { result = save_task.fuse() => result, _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Save cancelled by user"); + return Err("Save cancelled by user".to_string()); } }; if let Err(error) = save_result { diff --git a/crates/agent/src/tools/streaming_edit_file_tool.rs b/crates/agent/src/tools/streaming_edit_file_tool.rs index 561a94be1bd34145498f22147c844874fee316d5..dd5445142a001fbd9106af548444165bc8331581 100644 --- a/crates/agent/src/tools/streaming_edit_file_tool.rs +++ b/crates/agent/src/tools/streaming_edit_file_tool.rs @@ -118,27 +118,45 @@ struct StreamingEditFileToolPartialInput { } #[derive(Debug, Serialize, Deserialize)] -pub struct StreamingEditFileToolOutput { - #[serde(alias = "original_path")] - input_path: PathBuf, - new_text: String, - old_text: Arc, - #[serde(default)] - diff: String, +#[serde(untagged)] +pub enum StreamingEditFileToolOutput { + Success { + #[serde(alias = "original_path")] + input_path: PathBuf, + new_text: String, + old_text: Arc, + #[serde(default)] + diff: String, + }, + Error { + error: String, + }, +} + +impl std::fmt::Display for StreamingEditFileToolOutput { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + StreamingEditFileToolOutput::Success { + diff, input_path, .. + } => { + if diff.is_empty() { + write!(f, "No edits were made.") + } else { + write!( + f, + "Edited {}:\n\n```diff\n{diff}\n```", + input_path.display() + ) + } + } + StreamingEditFileToolOutput::Error { error } => write!(f, "{error}"), + } + } } impl From for LanguageModelToolResultContent { fn from(output: StreamingEditFileToolOutput) -> Self { - if output.diff.is_empty() { - "No edits were made.".into() - } else { - format!( - "Edited {}:\n\n```diff\n{}\n```", - output.input_path.display(), - output.diff - ) - .into() - } + output.to_string().into() } } @@ -253,17 +271,23 @@ impl AgentTool for StreamingEditFileTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let Ok(project) = self .thread .read_with(cx, |thread, _cx| thread.project().clone()) else { - return Task::ready(Err(anyhow!("thread was dropped"))); + return Task::ready(Err(StreamingEditFileToolOutput::Error { + error: "thread was dropped".to_string(), + })); }; let project_path = match resolve_path(&input, project.clone(), cx) { Ok(path) => path, - Err(err) => return Task::ready(Err(anyhow!(err))), + Err(err) => { + return Task::ready(Err(StreamingEditFileToolOutput::Error { + error: err.to_string(), + })); + } }; let abs_path = project.read(cx).absolute_path(&project_path, cx); @@ -276,191 +300,195 @@ impl AgentTool for StreamingEditFileTool { let authorize = self.authorize(&input, &event_stream, cx); cx.spawn(async move |cx: &mut AsyncApp| { - authorize.await?; - - let buffer = project - .update(cx, |project, cx| { - project.open_buffer(project_path.clone(), cx) - }) - .await?; + let result: anyhow::Result = async { + authorize.await?; + + let buffer = project + .update(cx, |project, cx| { + project.open_buffer(project_path.clone(), cx) + }) + .await?; + + if let Some(abs_path) = abs_path.as_ref() { + let (last_read_mtime, current_mtime, is_dirty, has_save_tool, has_restore_tool) = + self.thread.update(cx, |thread, cx| { + let last_read = thread.file_read_times.get(abs_path).copied(); + let current = buffer + .read(cx) + .file() + .and_then(|file| file.disk_state().mtime()); + let dirty = buffer.read(cx).is_dirty(); + let has_save = thread.has_tool(SaveFileTool::NAME); + let has_restore = thread.has_tool(RestoreFileFromDiskTool::NAME); + (last_read, current, dirty, has_save, has_restore) + })?; + + if is_dirty { + let message = match (has_save_tool, has_restore_tool) { + (true, true) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ + If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ + If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." + } + (true, false) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ + If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ + If they want to discard them, ask the user to manually revert the file, then inform you when it's ok to proceed." + } + (false, true) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ + If they want to keep them, ask the user to manually save the file, then inform you when it's ok to proceed. \ + If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." + } + (false, false) => { + "This file has unsaved changes. Ask the user whether they want to keep or discard those changes, \ + then ask them to save or revert the file manually and inform you when it's ok to proceed." + } + }; + anyhow::bail!("{}", message); + } - if let Some(abs_path) = abs_path.as_ref() { - let (last_read_mtime, current_mtime, is_dirty, has_save_tool, has_restore_tool) = - self.thread.update(cx, |thread, cx| { - let last_read = thread.file_read_times.get(abs_path).copied(); - let current = buffer - .read(cx) - .file() - .and_then(|file| file.disk_state().mtime()); - let dirty = buffer.read(cx).is_dirty(); - let has_save = thread.has_tool(SaveFileTool::NAME); - let has_restore = thread.has_tool(RestoreFileFromDiskTool::NAME); - (last_read, current, dirty, has_save, has_restore) - })?; - - if is_dirty { - let message = match (has_save_tool, has_restore_tool) { - (true, true) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ - If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ - If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." - } - (true, false) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ - If they want to keep them, ask for confirmation then use the save_file tool to save the file, then retry this edit. \ - If they want to discard them, ask the user to manually revert the file, then inform you when it's ok to proceed." - } - (false, true) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes. \ - If they want to keep them, ask the user to manually save the file, then inform you when it's ok to proceed. \ - If they want to discard them, ask for confirmation then use the restore_file_from_disk tool to restore the on-disk contents, then retry this edit." - } - (false, false) => { - "This file has unsaved changes. Ask the user whether they want to keep or discard those changes, \ - then ask them to save or revert the file manually and inform you when it's ok to proceed." + if let (Some(last_read), Some(current)) = (last_read_mtime, current_mtime) { + if current != last_read { + anyhow::bail!( + "The file {} has been modified since you last read it. \ + Please read the file again to get the current state before editing it.", + input.path.display() + ); } - }; - anyhow::bail!("{}", message); - } - - if let (Some(last_read), Some(current)) = (last_read_mtime, current_mtime) { - if current != last_read { - anyhow::bail!( - "The file {} has been modified since you last read it. \ - Please read the file again to get the current state before editing it.", - input.path.display() - ); } } - } - - let diff = cx.new(|cx| Diff::new(buffer.clone(), cx)); - event_stream.update_diff(diff.clone()); - let _finalize_diff = util::defer({ - let diff = diff.downgrade(); - let mut cx = cx.clone(); - move || { - diff.update(&mut cx, |diff, cx| diff.finalize(cx)).ok(); - } - }); - - let old_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); - let old_text = cx - .background_spawn({ - let old_snapshot = old_snapshot.clone(); - async move { Arc::new(old_snapshot.text()) } - }) - .await; - let action_log = self.thread.read_with(cx, |thread, _cx| thread.action_log().clone())?; + let diff = cx.new(|cx| Diff::new(buffer.clone(), cx)); + event_stream.update_diff(diff.clone()); + let _finalize_diff = util::defer({ + let diff = diff.downgrade(); + let mut cx = cx.clone(); + move || { + diff.update(&mut cx, |diff, cx| diff.finalize(cx)).ok(); + } + }); - // Edit the buffer and report edits to the action log as part of the - // same effect cycle, otherwise the edit will be reported as if the - // user made it (due to the buffer subscription in action_log). - match input.mode { - StreamingEditFileMode::Create | StreamingEditFileMode::Overwrite => { - action_log.update(cx, |log, cx| { - log.buffer_created(buffer.clone(), cx); - }); - let content = input.content.ok_or_else(|| { - anyhow!("'content' field is required for create and overwrite modes") - })?; - cx.update(|cx| { - buffer.update(cx, |buffer, cx| { - buffer.edit([(0..buffer.len(), content.as_str())], None, cx); + let old_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); + let old_text = cx + .background_spawn({ + let old_snapshot = old_snapshot.clone(); + async move { Arc::new(old_snapshot.text()) } + }) + .await; + + let action_log = self.thread.read_with(cx, |thread, _cx| thread.action_log().clone())?; + + // Edit the buffer and report edits to the action log as part of the + // same effect cycle, otherwise the edit will be reported as if the + // user made it (due to the buffer subscription in action_log). + match input.mode { + StreamingEditFileMode::Create | StreamingEditFileMode::Overwrite => { + action_log.update(cx, |log, cx| { + log.buffer_created(buffer.clone(), cx); + }); + let content = input.content.ok_or_else(|| { + anyhow!("'content' field is required for create and overwrite modes") + })?; + cx.update(|cx| { + buffer.update(cx, |buffer, cx| { + buffer.edit([(0..buffer.len(), content.as_str())], None, cx); + }); + action_log.update(cx, |log, cx| { + log.buffer_edited(buffer.clone(), cx); + }); }); + } + StreamingEditFileMode::Edit => { action_log.update(cx, |log, cx| { - log.buffer_edited(buffer.clone(), cx); + log.buffer_read(buffer.clone(), cx); }); - }); + let edits = input.edits.ok_or_else(|| { + anyhow!("'edits' field is required for edit mode") + })?; + // apply_edits now handles buffer_edited internally in the same effect cycle + apply_edits(&buffer, &action_log, &edits, &diff, &event_stream, &abs_path, cx)?; + } } - StreamingEditFileMode::Edit => { + + let format_on_save_enabled = buffer.read_with(cx, |buffer, cx| { + let settings = language_settings::language_settings( + buffer.language().map(|l| l.name()), + buffer.file(), + cx, + ); + settings.format_on_save != FormatOnSave::Off + }); + + if format_on_save_enabled { action_log.update(cx, |log, cx| { - log.buffer_read(buffer.clone(), cx); + log.buffer_edited(buffer.clone(), cx); }); - let edits = input.edits.ok_or_else(|| { - anyhow!("'edits' field is required for edit mode") - })?; - // apply_edits now handles buffer_edited internally in the same effect cycle - apply_edits(&buffer, &action_log, &edits, &diff, &event_stream, &abs_path, cx)?; - } - } - let format_on_save_enabled = buffer.read_with(cx, |buffer, cx| { - let settings = language_settings::language_settings( - buffer.language().map(|l| l.name()), - buffer.file(), - cx, - ); - settings.format_on_save != FormatOnSave::Off - }); - - if format_on_save_enabled { - action_log.update(cx, |log, cx| { - log.buffer_edited(buffer.clone(), cx); - }); + let format_task = project.update(cx, |project, cx| { + project.format( + HashSet::from_iter([buffer.clone()]), + LspFormatTarget::Buffers, + false, + FormatTrigger::Save, + cx, + ) + }); + futures::select! { + result = format_task.fuse() => { result.log_err(); }, + _ = event_stream.cancelled_by_user().fuse() => { + anyhow::bail!("Edit cancelled by user"); + } + }; + } - let format_task = project.update(cx, |project, cx| { - project.format( - HashSet::from_iter([buffer.clone()]), - LspFormatTarget::Buffers, - false, - FormatTrigger::Save, - cx, - ) - }); + let save_task = project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)); futures::select! { - result = format_task.fuse() => { result.log_err(); }, + result = save_task.fuse() => { result?; }, _ = event_stream.cancelled_by_user().fuse() => { anyhow::bail!("Edit cancelled by user"); } }; - } - - let save_task = project - .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)); - futures::select! { - result = save_task.fuse() => { result?; }, - _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Edit cancelled by user"); - } - }; - - action_log.update(cx, |log, cx| { - log.buffer_edited(buffer.clone(), cx); - }); - if let Some(abs_path) = abs_path.as_ref() { - if let Some(new_mtime) = buffer.read_with(cx, |buffer, _| { - buffer.file().and_then(|file| file.disk_state().mtime()) - }) { - self.thread.update(cx, |thread, _| { - thread.file_read_times.insert(abs_path.to_path_buf(), new_mtime); - })?; - } - } + action_log.update(cx, |log, cx| { + log.buffer_edited(buffer.clone(), cx); + }); - let new_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); - let (new_text, unified_diff) = cx - .background_spawn({ - let new_snapshot = new_snapshot.clone(); - let old_text = old_text.clone(); - async move { - let new_text = new_snapshot.text(); - let diff = language::unified_diff(&old_text, &new_text); - (new_text, diff) + if let Some(abs_path) = abs_path.as_ref() { + if let Some(new_mtime) = buffer.read_with(cx, |buffer, _| { + buffer.file().and_then(|file| file.disk_state().mtime()) + }) { + self.thread.update(cx, |thread, _| { + thread.file_read_times.insert(abs_path.to_path_buf(), new_mtime); + })?; } - }) - .await; + } - let output = StreamingEditFileToolOutput { - input_path: input.path, - new_text, - old_text, - diff: unified_diff, - }; + let new_snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot()); + let (new_text, unified_diff) = cx + .background_spawn({ + let new_snapshot = new_snapshot.clone(); + let old_text = old_text.clone(); + async move { + let new_text = new_snapshot.text(); + let diff = language::unified_diff(&old_text, &new_text); + (new_text, diff) + } + }) + .await; + + let output = StreamingEditFileToolOutput::Success { + input_path: input.path, + new_text, + old_text, + diff: unified_diff, + }; - Ok(output) + Ok(output) + }.await; + result + .map_err(|e| StreamingEditFileToolOutput::Error { error: e.to_string() }) }) } @@ -471,16 +499,26 @@ impl AgentTool for StreamingEditFileTool { event_stream: ToolCallEventStream, cx: &mut App, ) -> Result<()> { - event_stream.update_diff(cx.new(|cx| { - Diff::finalized( - output.input_path.to_string_lossy().into_owned(), - Some(output.old_text.to_string()), - output.new_text, - self.language_registry.clone(), - cx, - ) - })); - Ok(()) + match output { + StreamingEditFileToolOutput::Success { + input_path, + old_text, + new_text, + .. + } => { + event_stream.update_diff(cx.new(|cx| { + Diff::finalized( + input_path.to_string_lossy().into_owned(), + Some(old_text.to_string()), + new_text, + self.language_registry.clone(), + cx, + ) + })); + Ok(()) + } + StreamingEditFileToolOutput::Error { .. } => Ok(()), + } } } @@ -755,10 +793,11 @@ mod tests { }) .await; - assert!(result.is_ok()); - let output = result.unwrap(); - assert_eq!(output.new_text, "Hello, World!"); - assert!(!output.diff.is_empty()); + let StreamingEditFileToolOutput::Success { new_text, diff, .. } = result.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "Hello, World!"); + assert!(!diff.is_empty()); } #[gpui::test] @@ -803,10 +842,14 @@ mod tests { }) .await; - assert!(result.is_ok()); - let output = result.unwrap(); - assert_eq!(output.new_text, "new content"); - assert_eq!(*output.old_text, "old content"); + let StreamingEditFileToolOutput::Success { + new_text, old_text, .. + } = result.unwrap() + else { + panic!("expected success"); + }; + assert_eq!(new_text, "new content"); + assert_eq!(*old_text, "old content"); } #[gpui::test] @@ -859,9 +902,10 @@ mod tests { }) .await; - assert!(result.is_ok()); - let output = result.unwrap(); - assert_eq!(output.new_text, "line 1\nmodified line 2\nline 3\n"); + let StreamingEditFileToolOutput::Success { new_text, .. } = result.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "line 1\nmodified line 2\nline 3\n"); } #[gpui::test] @@ -920,10 +964,11 @@ mod tests { }) .await; - assert!(result.is_ok()); - let output = result.unwrap(); + let StreamingEditFileToolOutput::Success { new_text, .. } = result.unwrap() else { + panic!("expected success"); + }; assert_eq!( - output.new_text, + new_text, "modified line 1\nline 2\nline 3\nline 4\nmodified line 5\n" ); } @@ -984,10 +1029,11 @@ mod tests { }) .await; - assert!(result.is_ok()); - let output = result.unwrap(); + let StreamingEditFileToolOutput::Success { new_text, .. } = result.unwrap() else { + panic!("expected success"); + }; assert_eq!( - output.new_text, + new_text, "line 1\nmodified line 2\nmodified line 3\nline 4\nline 5\n" ); } @@ -1048,10 +1094,11 @@ mod tests { }) .await; - assert!(result.is_ok()); - let output = result.unwrap(); + let StreamingEditFileToolOutput::Success { new_text, .. } = result.unwrap() else { + panic!("expected success"); + }; assert_eq!( - output.new_text, + new_text, "modified line 1\nline 2\nline 3\nline 4\nmodified line 5\n" ); } @@ -1100,10 +1147,10 @@ mod tests { }) .await; - assert_eq!( - result.unwrap_err().to_string(), - "Can't edit file: path not found" - ); + let StreamingEditFileToolOutput::Error { error } = result.unwrap_err() else { + panic!("expected error"); + }; + assert_eq!(error, "Can't edit file: path not found"); } #[gpui::test] @@ -1151,12 +1198,12 @@ mod tests { }) .await; - assert!(result.is_err()); + let StreamingEditFileToolOutput::Error { error } = result.unwrap_err() else { + panic!("expected error"); + }; assert!( - result - .unwrap_err() - .to_string() - .contains("Could not find matching text") + error.contains("Could not find matching text"), + "Expected error containing 'Could not find matching text' but got: {error}" ); } @@ -1221,11 +1268,12 @@ mod tests { }) .await; - let error = result.unwrap_err(); - let error_message = error.to_string(); + let StreamingEditFileToolOutput::Error { error } = result.unwrap_err() else { + panic!("expected error"); + }; assert!( - error_message.contains("Conflicting edit ranges detected"), - "Expected 'Conflicting edit ranges detected' but got: {error_message}" + error.contains("Conflicting edit ranges detected"), + "Expected 'Conflicting edit ranges detected' but got: {error}" ); } diff --git a/crates/agent/src/tools/subagent_tool.rs b/crates/agent/src/tools/subagent_tool.rs index 29fb870d5db20ad6d75ade8120137ed3560533ee..4dd3b54c0e16abd924c647b0b8dd2b5e71485641 100644 --- a/crates/agent/src/tools/subagent_tool.rs +++ b/crates/agent/src/tools/subagent_tool.rs @@ -1,6 +1,6 @@ use acp_thread::SUBAGENT_SESSION_ID_META_KEY; use agent_client_protocol as acp; -use anyhow::{Result, anyhow}; +use anyhow::Result; use gpui::{App, SharedString, Task, WeakEntity}; use language_model::LanguageModelToolResultContent; use schemars::JsonSchema; @@ -36,16 +36,25 @@ pub struct SubagentToolInput { } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -pub struct SubagentToolOutput { - pub session_id: acp::SessionId, - pub output: String, +#[serde(untagged)] +pub enum SubagentToolOutput { + Success { + session_id: acp::SessionId, + output: String, + }, + Error { + error: String, + }, } impl From for LanguageModelToolResultContent { fn from(output: SubagentToolOutput) -> Self { - serde_json::to_string(&output) - .expect("Failed to serialize SubagentToolOutput") - .into() + match output { + output @ SubagentToolOutput::Success { .. } => serde_json::to_string(&output) + .unwrap_or_else(|e| format!("Failed to serialize subagent output: {e}")) + .into(), + SubagentToolOutput::Error { error } => error.into(), + } } } @@ -89,9 +98,11 @@ impl AgentTool for SubagentTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let Some(parent_thread_entity) = self.parent_thread.upgrade() else { - return Task::ready(Err(anyhow!("Parent thread no longer exists"))); + return Task::ready(Err(SubagentToolOutput::Error { + error: "Parent thread no longer exists".to_string(), + })); }; let subagent = match self.environment.create_subagent( @@ -102,7 +113,11 @@ impl AgentTool for SubagentTool { cx, ) { Ok(subagent) => subagent, - Err(err) => return Task::ready(Err(err)), + Err(err) => { + return Task::ready(Err(SubagentToolOutput::Error { + error: err.to_string(), + })); + } }; let subagent_session_id = subagent.id(); @@ -115,8 +130,14 @@ impl AgentTool for SubagentTool { event_stream.update_fields_with_meta(acp::ToolCallUpdateFields::new(), Some(meta)); cx.spawn(async move |cx| { - let output = subagent.wait_for_output(cx).await?; - Ok(SubagentToolOutput { + let output = + subagent + .wait_for_output(cx) + .await + .map_err(|e| SubagentToolOutput::Error { + error: e.to_string(), + })?; + Ok(SubagentToolOutput::Success { session_id: subagent_session_id, output, }) @@ -130,12 +151,17 @@ impl AgentTool for SubagentTool { event_stream: ToolCallEventStream, _cx: &mut App, ) -> Result<()> { - event_stream.subagent_spawned(output.session_id.clone()); - let meta = acp::Meta::from_iter([( - SUBAGENT_SESSION_ID_META_KEY.into(), - output.session_id.to_string().into(), - )]); - event_stream.update_fields_with_meta(acp::ToolCallUpdateFields::new(), Some(meta)); + match output { + SubagentToolOutput::Success { session_id, .. } => { + event_stream.subagent_spawned(session_id.clone()); + let meta = acp::Meta::from_iter([( + SUBAGENT_SESSION_ID_META_KEY.into(), + session_id.to_string().into(), + )]); + event_stream.update_fields_with_meta(acp::ToolCallUpdateFields::new(), Some(meta)); + } + SubagentToolOutput::Error { .. } => {} + } Ok(()) } } diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index ddb8ee1975a6d4a3fc9838a133482f65dc3b00b6..57b3278da256c01408f704a8e2f6f7e075057597 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -88,10 +88,10 @@ impl AgentTool for TerminalTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let working_dir = match working_dir(&input, &self.project, cx) { Ok(dir) => dir, - Err(err) => return Task::ready(Err(err)), + Err(err) => return Task::ready(Err(err.to_string())), }; let settings = AgentSettings::get_global(cx); @@ -104,7 +104,7 @@ impl AgentTool for TerminalTool { let authorize = match decision { ToolPermissionDecision::Allow => None, ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow::anyhow!("{}", reason))); + return Task::ready(Err(reason)); } ToolPermissionDecision::Confirm => { let context = @@ -114,7 +114,7 @@ impl AgentTool for TerminalTool { }; cx.spawn(async move |cx| { if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| e.to_string())?; } let terminal = self @@ -125,9 +125,10 @@ impl AgentTool for TerminalTool { Some(COMMAND_OUTPUT_LIMIT), cx, ) - .await?; + .await + .map_err(|e| e.to_string())?; - let terminal_id = terminal.id(cx)?; + let terminal_id = terminal.id(cx).map_err(|e| e.to_string())?; event_stream.update_fields(acp::ToolCallUpdateFields::new().content(vec![ acp::ToolCallContent::Terminal(acp::Terminal::new(terminal_id)), ])); @@ -136,7 +137,7 @@ impl AgentTool for TerminalTool { let mut timed_out = false; let mut user_stopped_via_signal = false; - let wait_for_exit = terminal.wait_for_exit(cx)?; + let wait_for_exit = terminal.wait_for_exit(cx).map_err(|e| e.to_string())?; match timeout { Some(timeout) => { @@ -146,12 +147,12 @@ impl AgentTool for TerminalTool { _ = wait_for_exit.clone().fuse() => {}, _ = timeout_task.fuse() => { timed_out = true; - terminal.kill(cx)?; + terminal.kill(cx).map_err(|e| e.to_string())?; wait_for_exit.await; } _ = event_stream.cancelled_by_user().fuse() => { user_stopped_via_signal = true; - terminal.kill(cx)?; + terminal.kill(cx).map_err(|e| e.to_string())?; wait_for_exit.await; } } @@ -161,7 +162,7 @@ impl AgentTool for TerminalTool { _ = wait_for_exit.clone().fuse() => {}, _ = event_stream.cancelled_by_user().fuse() => { user_stopped_via_signal = true; - terminal.kill(cx)?; + terminal.kill(cx).map_err(|e| e.to_string())?; wait_for_exit.await; } } @@ -178,7 +179,7 @@ impl AgentTool for TerminalTool { let user_stopped_via_terminal = terminal.was_stopped_by_user(cx).unwrap_or(false); let user_stopped = user_stopped_via_signal || user_stopped_via_terminal; - let output = terminal.current_output(cx)?; + let output = terminal.current_output(cx).map_err(|e| e.to_string())?; Ok(process_content( output, diff --git a/crates/agent/src/tools/web_search_tool.rs b/crates/agent/src/tools/web_search_tool.rs index 2c92bc0b34b9dba252a8cd6884558622553eccb3..c536f45ba65c109d3068b0722db1ffb1cad8b87c 100644 --- a/crates/agent/src/tools/web_search_tool.rs +++ b/crates/agent/src/tools/web_search_tool.rs @@ -5,7 +5,7 @@ use crate::{ }; use agent_client_protocol as acp; use agent_settings::AgentSettings; -use anyhow::{Result, anyhow}; +use anyhow::Result; use cloud_llm_client::WebSearchResponse; use futures::FutureExt as _; use gpui::{App, AppContext, Task}; @@ -29,14 +29,20 @@ pub struct WebSearchToolInput { } #[derive(Debug, Serialize, Deserialize)] -#[serde(transparent)] -pub struct WebSearchToolOutput(WebSearchResponse); +#[serde(untagged)] +pub enum WebSearchToolOutput { + Success(WebSearchResponse), + Error { error: String }, +} impl From for LanguageModelToolResultContent { fn from(value: WebSearchToolOutput) -> Self { - serde_json::to_string(&value.0) - .expect("Failed to serialize WebSearchResponse") - .into() + match value { + WebSearchToolOutput::Success(response) => serde_json::to_string(&response) + .unwrap_or_else(|e| format!("Failed to serialize web search response: {e}")) + .into(), + WebSearchToolOutput::Error { error } => error.into(), + } } } @@ -70,7 +76,7 @@ impl AgentTool for WebSearchTool { input: Self::Input, event_stream: ToolCallEventStream, cx: &mut App, - ) -> Task> { + ) -> Task> { let settings = AgentSettings::get_global(cx); let decision = decide_permission_from_settings( Self::NAME, @@ -81,7 +87,7 @@ impl AgentTool for WebSearchTool { let authorize = match decision { ToolPermissionDecision::Allow => None, ToolPermissionDecision::Deny(reason) => { - return Task::ready(Err(anyhow!("{}", reason))); + return Task::ready(Err(WebSearchToolOutput::Error { error: reason })); } ToolPermissionDecision::Confirm => { let context = @@ -95,13 +101,15 @@ impl AgentTool for WebSearchTool { }; let Some(provider) = WebSearchRegistry::read_global(cx).active_provider() else { - return Task::ready(Err(anyhow!("Web search is not available."))); + return Task::ready(Err(WebSearchToolOutput::Error { + error: "Web search is not available.".to_string(), + })); }; let search_task = provider.search(input.query, cx); cx.background_spawn(async move { if let Some(authorize) = authorize { - authorize.await?; + authorize.await.map_err(|e| WebSearchToolOutput::Error { error: e.to_string() })?; } let response = futures::select! { @@ -111,17 +119,17 @@ impl AgentTool for WebSearchTool { Err(err) => { event_stream .update_fields(acp::ToolCallUpdateFields::new().title("Web Search Failed")); - return Err(err); + return Err(WebSearchToolOutput::Error { error: err.to_string() }); } } } _ = event_stream.cancelled_by_user().fuse() => { - anyhow::bail!("Web search cancelled by user"); + return Err(WebSearchToolOutput::Error { error: "Web search cancelled by user".to_string() }); } }; emit_update(&response, &event_stream); - Ok(WebSearchToolOutput(response)) + Ok(WebSearchToolOutput::Success(response)) }) } @@ -132,7 +140,9 @@ impl AgentTool for WebSearchTool { event_stream: ToolCallEventStream, _cx: &mut App, ) -> Result<()> { - emit_update(&output.0, &event_stream); + if let WebSearchToolOutput::Success(response) = &output { + emit_update(response, &event_stream); + } Ok(()) } } diff --git a/crates/zed/src/visual_test_runner.rs b/crates/zed/src/visual_test_runner.rs index 36d23ec114e6b52048fbc595917beef20a0dae69..89e3e5d3b05010931ba78d263dae543519f9b77a 100644 --- a/crates/zed/src/visual_test_runner.rs +++ b/crates/zed/src/visual_test_runner.rs @@ -2052,7 +2052,12 @@ fn run_agent_thread_view_test( cx.background_executor.allow_parking(); let run_result = cx.foreground_executor.block_test(run_task); cx.background_executor.forbid_parking(); - run_result.context("ReadFileTool failed")?; + run_result.map_err(|e| match e { + language_model::LanguageModelToolResultContent::Text(text) => { + anyhow::anyhow!("ReadFileTool failed: {text}") + } + other => anyhow::anyhow!("ReadFileTool failed: {other:?}"), + })?; cx.run_until_parked();