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();