From e78ddcac8d7bb36c3f68835c506f38f4850adb7d Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Fri, 8 May 2026 13:26:03 +0200 Subject: [PATCH] agent: Improve UX when agent tries to edit unsaved buffer (#55655) Before: 1. Agent tries to edit unsaved file 2. Tool call fails with error telling the agent to ask the user to save or discard edits 3. User types save/restore 4. Agent uses save/restore tool https://github.com/user-attachments/assets/c94dd361-e8e0-48ee-be31-da8afe594419 After: 1. Agent tries to edit unsaved file 2. User is prompted to save/restore file 3. User accepts/rejects or saves/discards file manually https://github.com/user-attachments/assets/1d98a0c4-4420-4426-94f2-42355de230be Release Notes: - agent: Improved UX when agent tries to edit unsaved buffer --------- Co-authored-by: Ben Brandt --- assets/settings/default.json | 2 - crates/acp_thread/src/acp_thread.rs | 43 +- crates/acp_thread/src/connection.rs | 3 + crates/agent/src/agent.rs | 5 +- crates/agent/src/tests/mod.rs | 106 --- crates/agent/src/thread.rs | 60 +- crates/agent/src/tools.rs | 6 - crates/agent/src/tools/edit_file_tool.rs | 256 ++++-- crates/agent/src/tools/edit_session.rs | 149 +++- .../src/tools/restore_file_from_disk_tool.rs | 673 ---------------- crates/agent/src/tools/save_file_tool.rs | 756 ------------------ crates/agent/src/tools/tool_permissions.rs | 86 ++ crates/agent/src/tools/write_file_tool.rs | 206 +++++ crates/agent_servers/src/acp.rs | 1 + crates/agent_ui/src/conversation_view.rs | 1 + crates/settings_ui/src/pages.rs | 3 +- .../src/pages/tool_permissions_setup.rs | 19 - crates/sidebar/src/sidebar_tests.rs | 1 + 18 files changed, 712 insertions(+), 1664 deletions(-) delete mode 100644 crates/agent/src/tools/restore_file_from_disk_tool.rs delete mode 100644 crates/agent/src/tools/save_file_tool.rs diff --git a/assets/settings/default.json b/assets/settings/default.json index d6d6feac644161b398df49e1e6638557784b37d9..7c74715b1224efd1cc995a1f2e0fd8ef200e7ed2 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -1122,8 +1122,6 @@ "now": true, "rename_symbol": true, "read_file": true, - "restore_file_from_disk": true, - "save_file": true, "open": true, "grep": true, "spawn_agent": true, diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index 769131a8e0d276a5ba00e16602f9d388498b2396..598e3e9683d3a509d7a7037ce6302a417df97240 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -570,6 +570,22 @@ impl From for acp::RequestPermissionOutcome { } } +/// What a `WaitingForConfirmation` prompt represents semantically. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum AuthorizationKind { + /// The user is granting or denying permission for the tool call to + /// proceed. The selected `PermissionOptionKind` determines whether the + /// tool call transitions to `InProgress` (allow) or `Rejected` (reject). + /// This is the default for tool authorization prompts. + PermissionGrant, + /// The user is choosing between actions for the tool to take next + /// (for example, "Save" vs "Discard" before editing a dirty buffer). + /// The tool call always transitions to `InProgress` regardless of the + /// selected `PermissionOptionKind`; the caller interprets the chosen + /// `option_id` to decide what to do. + ActionChoice, +} + #[derive(Debug)] pub enum ToolCallStatus { /// The tool call hasn't started running yet, but we start showing it to @@ -579,6 +595,7 @@ pub enum ToolCallStatus { WaitingForConfirmation { options: PermissionOptions, respond_tx: oneshot::Sender, + kind: AuthorizationKind, }, /// The tool call is currently running. InProgress, @@ -2080,6 +2097,7 @@ impl AcpThread { &mut self, tool_call: acp::ToolCallUpdate, options: PermissionOptions, + kind: AuthorizationKind, cx: &mut Context, ) -> Result> { let (tx, rx) = oneshot::channel(); @@ -2087,6 +2105,7 @@ impl AcpThread { let status = ToolCallStatus::WaitingForConfirmation { options, respond_tx: tx, + kind, }; let tool_call_id = tool_call.tool_call_id.clone(); @@ -2118,15 +2137,25 @@ impl AcpThread { return; }; - let new_status = match outcome.option_kind { - acp::PermissionOptionKind::RejectOnce | acp::PermissionOptionKind::RejectAlways => { - ToolCallStatus::Rejected + let is_action_choice = matches!( + call.status, + ToolCallStatus::WaitingForConfirmation { + kind: AuthorizationKind::ActionChoice, + .. } - acp::PermissionOptionKind::AllowOnce | acp::PermissionOptionKind::AllowAlways => { + ); + let new_status = + if is_action_choice { ToolCallStatus::InProgress - } - _ => ToolCallStatus::InProgress, - }; + } else { + match outcome.option_kind { + acp::PermissionOptionKind::RejectOnce + | acp::PermissionOptionKind::RejectAlways => ToolCallStatus::Rejected, + acp::PermissionOptionKind::AllowOnce + | acp::PermissionOptionKind::AllowAlways => ToolCallStatus::InProgress, + _ => ToolCallStatus::InProgress, + } + }; let curr_status = mem::replace(&mut call.status, new_status); diff --git a/crates/acp_thread/src/connection.rs b/crates/acp_thread/src/connection.rs index 41ca3e4c6a6bd1adbea6ded44bfd5ede5c7bb0fd..41cdd1250b3e693bdd0df5420463f8be259682ea 100644 --- a/crates/acp_thread/src/connection.rs +++ b/crates/acp_thread/src/connection.rs @@ -641,6 +641,8 @@ mod test_support { use gpui::{AppContext as _, WeakEntity}; use parking_lot::Mutex; + use crate::AuthorizationKind; + use super::*; /// Creates a PNG image encoded as base64 for testing. @@ -915,6 +917,7 @@ mod test_support { thread.request_tool_call_authorization( tool_call.clone().into(), options.clone(), + AuthorizationKind::PermissionGrant, cx, ) })?? diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index 95b79a0cc153bdf8f0caa51823fa13e4d4f79420..cd45c97f5826be01b39e7e1aa5f1c243367884e9 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -1298,9 +1298,12 @@ impl NativeAgentConnection { options, response, context: _, + kind, }) => { let outcome_task = acp_thread.update(cx, |thread, cx| { - thread.request_tool_call_authorization(tool_call, options, cx) + thread.request_tool_call_authorization( + tool_call, options, kind, cx, + ) })??; cx.background_spawn(async move { if let acp_thread::RequestPermissionOutcome::Selected(outcome) = diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index e4dd5a2425750141104c7ddc496879079fa3643a..511986ff0044338d5592e3bcf4084b886af6c905 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -6391,112 +6391,6 @@ async fn test_copy_path_tool_deny_rule_blocks_copy(cx: &mut TestAppContext) { ); } -#[gpui::test] -async fn test_save_file_tool_denies_if_any_path_denied(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - "/root", - json!({ - "normal.txt": "normal content", - "readonly": { - "config.txt": "readonly content" - } - }), - ) - .await; - let project = Project::test(fs.clone(), ["/root".as_ref()], cx).await; - - cx.update(|cx| { - let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); - settings.tool_permissions.tools.insert( - SaveFileTool::NAME.into(), - agent_settings::ToolRules { - default: Some(settings::ToolPermissionMode::Allow), - always_allow: vec![], - always_deny: vec![agent_settings::CompiledRegex::new(r"readonly", false).unwrap()], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - agent_settings::AgentSettings::override_global(settings, cx); - }); - - #[allow(clippy::arc_with_non_send_sync)] - let tool = Arc::new(crate::SaveFileTool::new(project)); - let (event_stream, _rx) = crate::ToolCallEventStream::test(); - - let task = cx.update(|cx| { - tool.run( - ToolInput::resolved(crate::SaveFileToolInput { - paths: vec![ - std::path::PathBuf::from("root/normal.txt"), - std::path::PathBuf::from("root/readonly/config.txt"), - ], - }), - event_stream, - cx, - ) - }); - - let result = task.await; - assert!( - result.is_err(), - "expected save to be blocked due to denied path" - ); - assert!( - result.unwrap_err().contains("blocked"), - "error should mention the save was blocked" - ); -} - -#[gpui::test] -async fn test_save_file_tool_respects_deny_rules(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree("/root", json!({"config.secret": "secret config"})) - .await; - let project = Project::test(fs.clone(), ["/root".as_ref()], cx).await; - - cx.update(|cx| { - let mut settings = agent_settings::AgentSettings::get_global(cx).clone(); - settings.tool_permissions.tools.insert( - SaveFileTool::NAME.into(), - agent_settings::ToolRules { - default: Some(settings::ToolPermissionMode::Allow), - always_allow: vec![], - always_deny: vec![agent_settings::CompiledRegex::new(r"\.secret$", false).unwrap()], - always_confirm: vec![], - invalid_patterns: vec![], - }, - ); - agent_settings::AgentSettings::override_global(settings, cx); - }); - - #[allow(clippy::arc_with_non_send_sync)] - let tool = Arc::new(crate::SaveFileTool::new(project)); - let (event_stream, _rx) = crate::ToolCallEventStream::test(); - - let task = cx.update(|cx| { - tool.run( - ToolInput::resolved(crate::SaveFileToolInput { - paths: vec![std::path::PathBuf::from("root/config.secret")], - }), - event_stream, - cx, - ) - }); - - let result = task.await; - assert!(result.is_err(), "expected save to be blocked"); - assert!( - result.unwrap_err().contains("blocked"), - "error should mention the save was blocked" - ); -} - #[gpui::test] async fn test_web_search_tool_deny_rule_blocks_search(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index ef03f47a8d37039f02bda12c6740f59f8dfbc1ff..8e5c9edd7f8e064c87dab7877d36490857daa0ee 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -3,8 +3,8 @@ use crate::{ DbLanguageModel, DbThread, DeletePathTool, DiagnosticsTool, EditFileTool, FetchTool, FindPathTool, FindReferencesTool, GetCodeActionsTool, GoToDefinitionTool, GrepTool, ListDirectoryTool, MovePathTool, NowTool, OpenTool, ProjectSnapshot, ReadFileTool, RenameTool, - RestoreFileFromDiskTool, SaveFileTool, SpawnAgentTool, SystemPromptTemplate, Template, - Templates, TerminalTool, ToolPermissionDecision, UpdatePlanTool, WebSearchTool, WriteFileTool, + SpawnAgentTool, SystemPromptTemplate, Template, Templates, TerminalTool, + ToolPermissionDecision, UpdatePlanTool, WebSearchTool, WriteFileTool, decide_permission_from_settings, }; use acp_thread::{MentionUri, UserMessageId}; @@ -825,7 +825,6 @@ impl ToolPermissionContext { || tool_name == WriteFileTool::NAME || tool_name == DeletePathTool::NAME || tool_name == CreateDirectoryTool::NAME - || tool_name == SaveFileTool::NAME { ( extract_path_pattern(value), @@ -925,6 +924,7 @@ pub struct ToolCallAuthorization { pub options: acp_thread::PermissionOptions, pub response: oneshot::Sender, pub context: Option, + pub kind: acp_thread::AuthorizationKind, } #[derive(Debug, thiserror::Error)] @@ -1571,8 +1571,6 @@ impl Thread { self.action_log.clone(), update_agent_location, )); - self.add_tool(SaveFileTool::new(self.project.clone())); - self.add_tool(RestoreFileFromDiskTool::new(self.project.clone())); self.add_tool(TerminalTool::new(self.project.clone(), environment.clone())); self.add_tool(WebSearchTool); @@ -3878,6 +3876,57 @@ impl ToolCallEventStream { self.run_authorization_loop(title, options, Some(context), None, cx) } + /// Prompts the user to choose between an explicit set of actions and + /// returns the chosen `option_id`. + /// + /// Unlike [`Self::authorize`] / [`Self::authorize_always_prompt`], this + /// does not interpret the user's choice as a permission grant — callers + /// are responsible for handling each `option_id` explicitly. Use this + /// when a tool needs the user to pick between several side-effecting + /// actions (for example, "Save" vs "Discard" for a dirty buffer). + pub fn prompt_for_decision( + &self, + title: Option, + message: Option, + options: Vec, + cx: &mut App, + ) -> Task> { + let options = acp_thread::PermissionOptions::Flat(options); + let stream = self.stream.clone(); + let tool_use_id = self.tool_use_id.clone(); + cx.spawn(async move |_cx| { + let mut fields = acp::ToolCallUpdateFields::new(); + if let Some(title) = title { + fields = fields.title(title); + } + if let Some(message) = message { + fields = fields.content(vec![acp::ToolCallContent::from(message)]); + } + + let (response_tx, response_rx) = oneshot::channel(); + if let Err(error) = stream + .0 + .unbounded_send(Ok(ThreadEvent::ToolCallAuthorization( + ToolCallAuthorization { + tool_call: acp::ToolCallUpdate::new(tool_use_id.to_string(), fields), + options, + response: response_tx, + context: None, + kind: acp_thread::AuthorizationKind::ActionChoice, + }, + ))) + { + log::error!("Failed to send tool call decision prompt: {error}"); + return Err(anyhow!("Failed to send tool call decision prompt: {error}")); + } + + let outcome = response_rx + .await + .map_err(|_| anyhow!("authorization channel closed"))?; + Ok(outcome.option_id) + }) + } + /// Prompts the user for authorization. /// /// When `check_settings` is `Some`, this gate is settings-driven: the @@ -3925,6 +3974,7 @@ impl ToolCallEventStream { options, response: response_tx, context, + kind: acp_thread::AuthorizationKind::PermissionGrant, }, ))) { diff --git a/crates/agent/src/tools.rs b/crates/agent/src/tools.rs index c52e0e4745e43853487cec9167e8a483763035b1..77b840a47ad626e777d20ccbec5814ebe7e67823 100644 --- a/crates/agent/src/tools.rs +++ b/crates/agent/src/tools.rs @@ -20,8 +20,6 @@ mod now_tool; mod open_tool; mod read_file_tool; mod rename_tool; -mod restore_file_from_disk_tool; -mod save_file_tool; mod spawn_agent_tool; mod symbol_locator; mod terminal_tool; @@ -79,8 +77,6 @@ pub use now_tool::*; pub use open_tool::*; pub use read_file_tool::*; pub use rename_tool::*; -pub use restore_file_from_disk_tool::*; -pub use save_file_tool::*; pub use spawn_agent_tool::*; pub use symbol_locator::*; pub use terminal_tool::*; @@ -176,8 +172,6 @@ tools! { OpenTool, ReadFileTool, RenameTool, - RestoreFileFromDiskTool, - SaveFileTool, SpawnAgentTool, TerminalTool, UpdatePlanTool, diff --git a/crates/agent/src/tools/edit_file_tool.rs b/crates/agent/src/tools/edit_file_tool.rs index 31eb788dfa3c6df3509a09f5ff40a2d16fe49d53..d439791970b4de8fe5d0e6ffbcf1dae416799fa3 100644 --- a/crates/agent/src/tools/edit_file_tool.rs +++ b/crates/agent/src/tools/edit_file_tool.rs @@ -1871,9 +1871,12 @@ mod tests { assert_eq!(input_path, Some(PathBuf::from("root/test.txt"))); } + /// When the buffer has unsaved changes and the user picks "Save", the + /// pending edits are flushed to disk and the agent's edit then proceeds + /// against the just-saved content. #[gpui::test] - async fn test_streaming_dirty_buffer_detected(cx: &mut TestAppContext) { - let (edit_tool, project, action_log, _fs, _thread) = + async fn test_streaming_dirty_buffer_save(cx: &mut TestAppContext) { + let (edit_tool, project, action_log, fs, _thread) = setup_test(cx, json!({"test.txt": "original content"})).await; let read_tool = Arc::new(crate::ReadFileTool::new( project.clone(), @@ -1881,7 +1884,6 @@ mod tests { true, )); - // Read the file first cx.update(|cx| { read_tool.clone().run( ToolInput::resolved(crate::ReadFileToolInput { @@ -1896,7 +1898,6 @@ mod tests { .await .unwrap(); - // Open the buffer and make it dirty let project_path = project .read_with(cx, |project, cx| { project.find_project_path("root/test.txt", cx) @@ -1909,54 +1910,219 @@ mod tests { buffer.update(cx, |buffer, cx| { let end_point = buffer.max_point(); - buffer.edit([(end_point..end_point, " added text")], None, cx); + buffer.edit([(end_point..end_point, " plus user edit")], None, cx); }); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); - let is_dirty = buffer.read_with(cx, |buffer, _| buffer.is_dirty()); - assert!(is_dirty, "Buffer should be dirty after in-memory edit"); - - // Try to edit - should fail because buffer has unsaved changes - let result = cx - .update(|cx| { - edit_tool.clone().run( - ToolInput::resolved(EditFileToolInput { - path: "root/test.txt".into(), - edits: vec![Edit { - old_text: "original content".into(), - new_text: "new content".into(), - }], - }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await; + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + edit_tool.clone().run( + ToolInput::resolved(EditFileToolInput { + path: "root/test.txt".into(), + edits: vec![Edit { + old_text: "original content plus user edit".into(), + new_text: "replaced content".into(), + }], + }), + stream_tx, + cx, + ) + }); - let EditFileToolOutput::Error { - error, - diff, - input_path, - } = result.unwrap_err() + let _update = stream_rx.expect_update_fields().await; + let auth = stream_rx.expect_authorization().await; + let content = auth.tool_call.fields.content.as_deref().unwrap_or(&[]); + let acp::ToolCallContent::Content(text) = content.first().expect("expected message body") else { - panic!("expected error"); + panic!("expected text body, got: {:?}", content.first()); + }; + let acp::ContentBlock::Text(text) = &text.content else { + panic!("expected text body, got: {:?}", text.content); }; assert!( - error.contains("This file has unsaved changes."), - "Error should mention unsaved changes, got: {}", - error - ); - assert!( - error.contains("keep or discard"), - "Error should ask whether to keep or discard changes, got: {}", - error - ); - assert!( - error.contains("save or revert the file manually"), - "Error should ask user to manually save or revert when tools aren't available, got: {}", - error + text.text.contains("unsaved changes") + && text.text.contains("save") + && text.text.contains("discard"), + "unexpected message body: {:?}", + text.text, ); - assert!(diff.is_empty()); - assert!(input_path.is_none()); + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("save"), + acp::PermissionOptionKind::AllowOnce, + )) + .unwrap(); + + let EditFileToolOutput::Success { new_text, .. } = task.await.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "replaced content"); + assert!(!buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + let on_disk = fs.load(path!("/root/test.txt").as_ref()).await.unwrap(); + assert_eq!(on_disk, "replaced content"); + } + + /// When the buffer has unsaved changes and the user picks "Discard", the + /// pending edits are reverted to match disk and the agent's edit then + /// proceeds against the on-disk content. + #[gpui::test] + async fn test_streaming_dirty_buffer_discard(cx: &mut TestAppContext) { + let (edit_tool, project, action_log, fs, _thread) = + setup_test(cx, json!({"test.txt": "original content"})).await; + let read_tool = Arc::new(crate::ReadFileTool::new( + project.clone(), + action_log.clone(), + true, + )); + + cx.update(|cx| { + read_tool.clone().run( + ToolInput::resolved(crate::ReadFileToolInput { + path: "root/test.txt".to_string(), + start_line: None, + end_line: None, + }), + ToolCallEventStream::test().0, + cx, + ) + }) + .await + .unwrap(); + + let project_path = project + .read_with(cx, |project, cx| { + project.find_project_path("root/test.txt", cx) + }) + .expect("Should find project path"); + let buffer = project + .update(cx, |project, cx| project.open_buffer(project_path, cx)) + .await + .unwrap(); + + buffer.update(cx, |buffer, cx| { + let end_point = buffer.max_point(); + buffer.edit([(end_point..end_point, " plus user edit")], None, cx); + }); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + edit_tool.clone().run( + ToolInput::resolved(EditFileToolInput { + path: "root/test.txt".into(), + // Match the on-disk content, not the dirty in-memory content. + edits: vec![Edit { + old_text: "original content".into(), + new_text: "replaced content".into(), + }], + }), + stream_tx, + cx, + ) + }); + + let _update = stream_rx.expect_update_fields().await; + let auth = stream_rx.expect_authorization().await; + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("discard"), + acp::PermissionOptionKind::RejectOnce, + )) + .unwrap(); + + let EditFileToolOutput::Success { new_text, .. } = task.await.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "replaced content"); + assert!(!buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + let on_disk = fs.load(path!("/root/test.txt").as_ref()).await.unwrap(); + assert_eq!(on_disk, "replaced content"); + } + + /// When the buffer is dirty and the user resolves it manually — e.g. + /// pressing `cmd-s` while the prompt is visible — the prompt is + /// dismissed automatically and the edit proceeds against the saved + /// content. The user shouldn't have to also click a button. + #[gpui::test] + async fn test_streaming_dirty_buffer_resolved_externally(cx: &mut TestAppContext) { + let (edit_tool, project, action_log, fs, _thread) = + setup_test(cx, json!({"test.txt": "original content"})).await; + let read_tool = Arc::new(crate::ReadFileTool::new( + project.clone(), + action_log.clone(), + true, + )); + + cx.update(|cx| { + read_tool.clone().run( + ToolInput::resolved(crate::ReadFileToolInput { + path: "root/test.txt".to_string(), + start_line: None, + end_line: None, + }), + ToolCallEventStream::test().0, + cx, + ) + }) + .await + .unwrap(); + + let project_path = project + .read_with(cx, |project, cx| { + project.find_project_path("root/test.txt", cx) + }) + .expect("Should find project path"); + let buffer = project + .update(cx, |project, cx| project.open_buffer(project_path, cx)) + .await + .unwrap(); + + buffer.update(cx, |buffer, cx| { + let end_point = buffer.max_point(); + buffer.edit([(end_point..end_point, " plus user edit")], None, cx); + }); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + edit_tool.clone().run( + ToolInput::resolved(EditFileToolInput { + path: "root/test.txt".into(), + edits: vec![Edit { + old_text: "original content plus user edit".into(), + new_text: "replaced content".into(), + }], + }), + stream_tx, + cx, + ) + }); + + let _update = stream_rx.expect_update_fields().await; + let auth = stream_rx.expect_authorization().await; + + // Simulate the user saving the buffer manually (e.g. cmd-s) while + // the prompt is visible. The tool should detect the buffer became + // clean and proceed without the user clicking anything. + project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)) + .await + .unwrap(); + + // The prompt's response channel should drop without a click; the + // tool dismisses the prompt by transitioning the tool call status + // to `InProgress`. + let dismiss = stream_rx.expect_update_fields().await; + assert_eq!(dismiss.status, Some(acp::ToolCallStatus::InProgress)); + drop(auth); + + let EditFileToolOutput::Success { new_text, .. } = task.await.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "replaced content"); + assert!(!buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + let on_disk = fs.load(path!("/root/test.txt").as_ref()).await.unwrap(); + assert_eq!(on_disk, "replaced content"); } #[gpui::test] diff --git a/crates/agent/src/tools/edit_session.rs b/crates/agent/src/tools/edit_session.rs index 1be22a579a0fdd201995ccbac9da98c653bc6e98..7955144f8eeeb7a5def775a1957d766032b96aa3 100644 --- a/crates/agent/src/tools/edit_session.rs +++ b/crates/agent/src/tools/edit_session.rs @@ -2,17 +2,16 @@ mod reindent; mod streaming_fuzzy_matcher; mod streaming_parser; -use super::restore_file_from_disk_tool::RestoreFileFromDiskTool; -use super::save_file_tool::SaveFileTool; -use crate::{AgentTool, Thread, ToolCallEventStream}; +use crate::{Thread, ToolCallEventStream}; use acp_thread::Diff; use action_log::ActionLog; -use agent_client_protocol::schema::{ToolCallLocation, ToolCallUpdateFields}; +use agent_client_protocol::schema::{self as acp, ToolCallLocation, ToolCallUpdateFields}; use anyhow::Result; use collections::HashSet; +use futures::{FutureExt, channel::oneshot}; use gpui::{App, AppContext, AsyncApp, Entity, Task, WeakEntity}; use language::language_settings::{self, FormatOnSave}; -use language::{Buffer, LanguageRegistry}; +use language::{Buffer, BufferEvent, LanguageRegistry}; use language_model::LanguageModelToolResultContent; use project::lsp_store::{FormatTrigger, LspFormatTarget}; use project::{AgentLocation, Project, ProjectPath}; @@ -665,7 +664,8 @@ impl EditSession { .await .map_err(|e| e.to_string())?; - let file_changed_since_last_read = ensure_buffer_saved(&buffer, &abs_path, &context, cx)?; + let file_changed_since_last_read = + ensure_buffer_saved(&buffer, &abs_path, mode, &context, event_stream, cx).await?; let diff = cx.new(|cx| Diff::new(buffer.clone(), cx)); event_stream.update_diff(diff.clone()); @@ -932,53 +932,25 @@ fn agent_edit_buffer( }); } -fn ensure_buffer_saved( +async fn ensure_buffer_saved( buffer: &Entity, abs_path: &PathBuf, + mode: EditSessionMode, context: &EditSessionContext, + event_stream: &ToolCallEventStream, cx: &mut AsyncApp, ) -> Result { let last_read_mtime = context .action_log .read_with(cx, |log, _| log.file_read_time(abs_path)); - let check_result = context.thread.read_with(cx, |thread, cx| { - 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); - (current, dirty, has_save, has_restore) + let (current_mtime, is_dirty) = buffer.read_with(cx, |buffer, _cx| { + let current = buffer.file().and_then(|file| file.disk_state().mtime()); + let dirty = buffer.is_dirty(); + (current, dirty) }); - let Ok((current_mtime, is_dirty, has_save_tool, has_restore_tool)) = check_result else { - return Ok(false); - }; - 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." - } - }; - return Err(message.to_string()); + resolve_dirty_buffer(buffer, mode, context, event_stream, cx).await?; } if let (Some(last_read), Some(current)) = (last_read_mtime, current_mtime) @@ -990,6 +962,99 @@ fn ensure_buffer_saved( Ok(false) } +/// Prompts the user about how to handle a dirty buffer that the agent +/// wants to edit (`EditSessionMode::Edit`) or overwrite +/// (`EditSessionMode::Write`), and performs the chosen action so the +/// edit session can proceed (or returns `Err` to cancel). +/// +/// If the user resolves the dirty state externally (e.g. cmd-s or +/// reload) while the prompt is visible, the prompt is dismissed +/// automatically. +async fn resolve_dirty_buffer( + buffer: &Entity, + mode: EditSessionMode, + context: &EditSessionContext, + event_stream: &ToolCallEventStream, + cx: &mut AsyncApp, +) -> Result<(), String> { + let (manual_resolve_tx, manual_resolve_rx) = oneshot::channel::<()>(); + let _buffer_subscription = cx.update(|cx| { + let mut tx = Some(manual_resolve_tx); + cx.subscribe(buffer, move |buffer, event: &BufferEvent, cx| { + if matches!( + event, + BufferEvent::Saved | BufferEvent::Reloaded | BufferEvent::DirtyChanged + ) && !buffer.read(cx).is_dirty() + && let Some(tx) = tx.take() + { + tx.send(()).ok(); + } + }) + }); + + let prompt_kind = match mode { + EditSessionMode::Edit => super::tool_permissions::DirtyBufferPromptKind::Edit, + EditSessionMode::Write => super::tool_permissions::DirtyBufferPromptKind::Overwrite, + }; + let prompt = cx.update(|cx| { + super::tool_permissions::authorize_dirty_buffer(prompt_kind, event_stream, cx) + }); + + let decision = futures::select_biased! { + _ = manual_resolve_rx.fuse() => { + None + } + decision = prompt.fuse() => { + Some(decision.map_err(|e| e.to_string())?) + } + }; + + let Some(decision) = decision else { + event_stream.update_fields( + acp::ToolCallUpdateFields::new().status(acp::ToolCallStatus::InProgress), + ); + return match mode { + EditSessionMode::Edit => Ok(()), + EditSessionMode::Write => Err( + "The user saved their unsaved changes while the prompt was visible; \ + the file overwrite was cancelled to preserve them. Ask the user how \ + they'd like to proceed before retrying." + .to_string(), + ), + }; + }; + + match decision { + super::tool_permissions::DirtyBufferDecision::Save => { + context + .project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)) + .await + .map_err(|e| format!("Failed to save buffer: {e}"))?; + } + super::tool_permissions::DirtyBufferDecision::Discard => { + context + .project + .update(cx, |project, cx| { + project.reload_buffers(HashSet::from_iter([buffer.clone()]), false, cx) + }) + .await + .map_err(|e| format!("Failed to discard unsaved changes: {e}"))?; + } + super::tool_permissions::DirtyBufferDecision::Keep => { + let error = "The user chose to keep their unsaved changes; the file overwrite \ + was cancelled. Ask the user how they'd like to proceed before \ + retrying." + .to_string(); + event_stream.update_fields( + acp::ToolCallUpdateFields::new().content(vec![error.clone().into()]), + ); + return Err(error); + } + } + Ok(()) +} + fn resolve_path( mode: EditSessionMode, path: &PathBuf, diff --git a/crates/agent/src/tools/restore_file_from_disk_tool.rs b/crates/agent/src/tools/restore_file_from_disk_tool.rs deleted file mode 100644 index 3b2c95596c3b2b56178a804f227a729e5d3faab1..0000000000000000000000000000000000000000 --- a/crates/agent/src/tools/restore_file_from_disk_tool.rs +++ /dev/null @@ -1,673 +0,0 @@ -use super::tool_permissions::{ - ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, - path_has_symlink_escape, resolve_project_path, sensitive_settings_kind, -}; -use agent_client_protocol::schema as acp; -use agent_settings::AgentSettings; -use collections::FxHashSet; -use futures::FutureExt as _; -use gpui::{App, Entity, SharedString, Task}; -use language::Buffer; -use project::Project; -use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; -use settings::Settings; -use std::path::{Path, PathBuf}; -use std::sync::Arc; -use util::markdown::MarkdownInlineCode; - -use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, - authorize_with_sensitive_settings, decide_permission_for_path, -}; - -/// Discards unsaved changes in open buffers by reloading file contents from disk. -/// -/// Use this tool when: -/// - You attempted to edit files but they have unsaved changes the user does not want to keep. -/// - You want to reset files to the on-disk state before retrying an edit. -/// -/// Only use this tool after asking the user for permission, because it will discard unsaved changes. -#[derive(Debug, Serialize, Deserialize, JsonSchema)] -pub struct RestoreFileFromDiskToolInput { - /// The paths of the files to restore from disk. - pub paths: Vec, -} - -pub struct RestoreFileFromDiskTool { - project: Entity, -} - -impl RestoreFileFromDiskTool { - pub fn new(project: Entity) -> Self { - Self { project } - } -} - -impl AgentTool for RestoreFileFromDiskTool { - type Input = RestoreFileFromDiskToolInput; - type Output = String; - - const NAME: &'static str = "restore_file_from_disk"; - - fn kind() -> acp::ToolKind { - acp::ToolKind::Other - } - - fn initial_title( - &self, - input: Result, - _cx: &mut App, - ) -> SharedString { - match input { - Ok(input) if input.paths.len() == 1 => "Restore file from disk".into(), - Ok(input) => format!("Restore {} files from disk", input.paths.len()).into(), - Err(_) => "Restore files from disk".into(), - } - } - - fn run( - self: Arc, - input: ToolInput, - event_stream: ToolCallEventStream, - cx: &mut App, - ) -> Task> { - let project = self.project.clone(); - - cx.spawn(async move |cx| { - let input = input.recv().await.map_err(|e| e.to_string())?; - - // Check for any immediate deny before doing async work. - for path in &input.paths { - let path_str = path.to_string_lossy(); - let decision = cx.update(|cx| { - decide_permission_for_path(Self::NAME, &path_str, AgentSettings::get_global(cx)) - }); - if let ToolPermissionDecision::Deny(reason) = decision { - return Err(reason); - } - } - - let input_paths = input.paths; - - let fs = project.read_with(cx, |project, _cx| project.fs().clone()); - let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; - - let mut confirmation_paths: Vec = Vec::new(); - - for path in &input_paths { - let path_str = path.to_string_lossy(); - let decision = cx.update(|cx| { - decide_permission_for_path(Self::NAME, &path_str, AgentSettings::get_global(cx)) - }); - let symlink_escape = project.read_with(cx, |project, cx| { - path_has_symlink_escape(project, path, &canonical_roots, cx) - }); - - match decision { - ToolPermissionDecision::Allow => { - if !symlink_escape { - let is_sensitive = super::tool_permissions::is_sensitive_settings_path( - Path::new(&*path_str), - fs.as_ref(), - ) - .await; - if is_sensitive { - confirmation_paths.push(path_str.to_string()); - } - } - } - ToolPermissionDecision::Deny(reason) => { - return Err(reason); - } - ToolPermissionDecision::Confirm => { - if !symlink_escape { - confirmation_paths.push(path_str.to_string()); - } - } - } - } - - if !confirmation_paths.is_empty() { - let title = if confirmation_paths.len() == 1 { - format!( - "Restore {} from disk", - MarkdownInlineCode(&confirmation_paths[0]) - ) - } else { - let paths: Vec<_> = confirmation_paths - .iter() - .take(3) - .map(|p| p.as_str()) - .collect(); - if confirmation_paths.len() > 3 { - format!( - "Restore {}, and {} more from disk", - paths.join(", "), - confirmation_paths.len() - 3 - ) - } else { - format!("Restore {} from disk", paths.join(", ")) - } - }; - - let mut settings_kind = None; - for p in &confirmation_paths { - if let Some(kind) = sensitive_settings_kind(Path::new(p), fs.as_ref()).await { - settings_kind = Some(kind); - break; - } - } - let context = crate::ToolPermissionContext::new(Self::NAME, confirmation_paths); - let authorize = cx.update(|cx| { - authorize_with_sensitive_settings( - settings_kind, - context, - &title, - &event_stream, - cx, - ) - }); - authorize.await.map_err(|e| e.to_string())?; - } - let mut buffers_to_reload: FxHashSet> = FxHashSet::default(); - - let mut restored_paths: Vec = Vec::new(); - let mut clean_paths: Vec = Vec::new(); - let mut not_found_paths: Vec = Vec::new(); - let mut open_errors: Vec<(PathBuf, String)> = Vec::new(); - let dirty_check_errors: Vec<(PathBuf, String)> = Vec::new(); - let mut reload_errors: Vec = Vec::new(); - - for path in input_paths { - let project_path = match project.read_with(cx, |project, cx| { - resolve_project_path(project, &path, &canonical_roots, cx) - }) { - Ok(resolved) => { - let (project_path, symlink_canonical_target) = match resolved { - ResolvedProjectPath::Safe(path) => (path, None), - ResolvedProjectPath::SymlinkEscape { - project_path, - canonical_target, - } => (project_path, Some(canonical_target)), - }; - if let Some(canonical_target) = &symlink_canonical_target { - let path_str = path.to_string_lossy(); - let authorize_task = cx.update(|cx| { - authorize_symlink_access( - Self::NAME, - &path_str, - canonical_target, - &event_stream, - cx, - ) - }); - let result = authorize_task.await; - if let Err(err) = result { - reload_errors.push(format!("{}: {}", path.to_string_lossy(), err)); - continue; - } - } - project_path - } - Err(_) => { - not_found_paths.push(path); - continue; - } - }; - - let open_buffer_task = - project.update(cx, |project, cx| project.open_buffer(project_path, cx)); - - let buffer = futures::select! { - result = open_buffer_task.fuse() => { - match result { - Ok(buffer) => buffer, - Err(error) => { - open_errors.push((path, error.to_string())); - continue; - } - } - } - _ = event_stream.cancelled_by_user().fuse() => { - return Err("Restore cancelled by user".to_string()); - } - }; - - let is_dirty = buffer.read_with(cx, |buffer, _| buffer.is_dirty()); - - if is_dirty { - buffers_to_reload.insert(buffer); - restored_paths.push(path); - } else { - clean_paths.push(path); - } - } - - if !buffers_to_reload.is_empty() { - let reload_task = project.update(cx, |project, cx| { - project.reload_buffers(buffers_to_reload, true, cx) - }); - - let result = futures::select! { - result = reload_task.fuse() => result, - _ = event_stream.cancelled_by_user().fuse() => { - return Err("Restore cancelled by user".to_string()); - } - }; - if let Err(error) = result { - reload_errors.push(error.to_string()); - } - } - - let mut lines: Vec = Vec::new(); - - if !restored_paths.is_empty() { - lines.push(format!("Restored {} file(s).", restored_paths.len())); - } - if !clean_paths.is_empty() { - lines.push(format!("{} clean.", clean_paths.len())); - } - - if !not_found_paths.is_empty() { - lines.push(format!("Not found ({}):", not_found_paths.len())); - for path in ¬_found_paths { - lines.push(format!("- {}", path.display())); - } - } - if !open_errors.is_empty() { - lines.push(format!("Open failed ({}):", open_errors.len())); - for (path, error) in &open_errors { - lines.push(format!("- {}: {}", path.display(), error)); - } - } - if !dirty_check_errors.is_empty() { - lines.push(format!( - "Dirty check failed ({}):", - dirty_check_errors.len() - )); - for (path, error) in &dirty_check_errors { - lines.push(format!("- {}: {}", path.display(), error)); - } - } - if !reload_errors.is_empty() { - lines.push(format!("Reload failed ({}):", reload_errors.len())); - for error in &reload_errors { - lines.push(format!("- {}", error)); - } - } - - if lines.is_empty() { - Ok("No paths provided.".to_string()) - } else { - Ok(lines.join("\n")) - } - }) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use fs::Fs as _; - use gpui::TestAppContext; - use language::LineEnding; - use project::FakeFs; - use serde_json::json; - use settings::SettingsStore; - use util::path; - - fn init_test(cx: &mut TestAppContext) { - cx.update(|cx| { - let settings_store = SettingsStore::test(cx); - cx.set_global(settings_store); - }); - cx.update(|cx| { - let mut settings = AgentSettings::get_global(cx).clone(); - settings.tool_permissions.default = settings::ToolPermissionMode::Allow; - AgentSettings::override_global(settings, cx); - }); - } - - #[gpui::test] - async fn test_restore_file_from_disk_output_and_effects(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - "/root", - json!({ - "dirty.txt": "on disk: dirty\n", - "clean.txt": "on disk: clean\n", - }), - ) - .await; - - let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; - let tool = Arc::new(RestoreFileFromDiskTool::new(project.clone())); - - // Make dirty.txt dirty in-memory by saving different content into the buffer without saving to disk. - let dirty_project_path = project.read_with(cx, |project, cx| { - project - .find_project_path("root/dirty.txt", cx) - .expect("dirty.txt should exist in project") - }); - - let dirty_buffer = project - .update(cx, |project, cx| { - project.open_buffer(dirty_project_path, cx) - }) - .await - .unwrap(); - dirty_buffer.update(cx, |buffer, cx| { - buffer.edit([(0..buffer.len(), "in memory: dirty\n")], None, cx); - }); - assert!( - dirty_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "dirty.txt buffer should be dirty before restore" - ); - - // Ensure clean.txt is opened but remains clean. - let clean_project_path = project.read_with(cx, |project, cx| { - project - .find_project_path("root/clean.txt", cx) - .expect("clean.txt should exist in project") - }); - - let clean_buffer = project - .update(cx, |project, cx| { - project.open_buffer(clean_project_path, cx) - }) - .await - .unwrap(); - assert!( - !clean_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "clean.txt buffer should start clean" - ); - - let output = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(RestoreFileFromDiskToolInput { - paths: vec![ - PathBuf::from("root/dirty.txt"), - PathBuf::from("root/clean.txt"), - ], - }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await - .unwrap(); - - // Output should mention restored + clean. - assert!( - output.contains("Restored 1 file(s)."), - "expected restored count line, got:\n{output}" - ); - assert!( - output.contains("1 clean."), - "expected clean count line, got:\n{output}" - ); - - // Effect: dirty buffer should be restored back to disk content and become clean. - let dirty_text = dirty_buffer.read_with(cx, |buffer, _| buffer.text()); - assert_eq!( - dirty_text, "on disk: dirty\n", - "dirty.txt buffer should be restored to disk contents" - ); - assert!( - !dirty_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "dirty.txt buffer should not be dirty after restore" - ); - - // Disk contents should be unchanged (restore-from-disk should not write). - let disk_dirty = fs.load(path!("/root/dirty.txt").as_ref()).await.unwrap(); - assert_eq!(disk_dirty, "on disk: dirty\n"); - - // Sanity: clean buffer should remain clean and unchanged. - let clean_text = clean_buffer.read_with(cx, |buffer, _| buffer.text()); - assert_eq!(clean_text, "on disk: clean\n"); - assert!( - !clean_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "clean.txt buffer should remain clean" - ); - - // Test empty paths case. - let output = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(RestoreFileFromDiskToolInput { paths: vec![] }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await - .unwrap(); - assert_eq!(output, "No paths provided."); - - // Test not-found path case (path outside the project root). - let output = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(RestoreFileFromDiskToolInput { - paths: vec![PathBuf::from("nonexistent/path.txt")], - }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await - .unwrap(); - assert!( - output.contains("Not found (1):"), - "expected not-found header line, got:\n{output}" - ); - assert!( - output.contains("- nonexistent/path.txt"), - "expected not-found path bullet, got:\n{output}" - ); - - let _ = LineEnding::Unix; // keep import used if the buffer edit API changes - } - - #[gpui::test] - async fn test_restore_file_symlink_escape_requests_authorization(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "src": {} - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let tool = Arc::new(RestoreFileFromDiskTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let task = cx.update(|cx| { - tool.clone().run( - ToolInput::resolved(RestoreFileFromDiskToolInput { - paths: vec![PathBuf::from("project/link.txt")], - }), - event_stream, - cx, - ) - }); - - cx.run_until_parked(); - - let auth = event_rx.expect_authorization().await; - let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); - assert!( - title.contains("points outside the project"), - "Expected symlink escape authorization, got: {title}", - ); - - auth.response - .send(acp_thread::SelectedPermissionOutcome::new( - acp::PermissionOptionId::new("allow"), - acp::PermissionOptionKind::AllowOnce, - )) - .unwrap(); - - let _result = task.await; - } - - #[gpui::test] - async fn test_restore_file_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { - init_test(cx); - cx.update(|cx| { - let mut settings = AgentSettings::get_global(cx).clone(); - settings.tool_permissions.tools.insert( - "restore_file_from_disk".into(), - agent_settings::ToolRules { - default: Some(settings::ToolPermissionMode::Deny), - ..Default::default() - }, - ); - AgentSettings::override_global(settings, cx); - }); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "src": {} - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let tool = Arc::new(RestoreFileFromDiskTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let result = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(RestoreFileFromDiskToolInput { - paths: vec![PathBuf::from("project/link.txt")], - }), - event_stream, - cx, - ) - }) - .await; - - assert!(result.is_err(), "Tool should fail when policy denies"); - assert!( - !matches!( - event_rx.try_recv(), - Ok(Ok(crate::ThreadEvent::ToolCallAuthorization(_))) - ), - "Deny policy should not emit symlink authorization prompt", - ); - } - - #[gpui::test] - async fn test_restore_file_symlink_escape_confirm_requires_single_approval( - cx: &mut TestAppContext, - ) { - init_test(cx); - cx.update(|cx| { - let mut settings = AgentSettings::get_global(cx).clone(); - settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; - AgentSettings::override_global(settings, cx); - }); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "src": {} - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let tool = Arc::new(RestoreFileFromDiskTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let task = cx.update(|cx| { - tool.clone().run( - ToolInput::resolved(RestoreFileFromDiskToolInput { - paths: vec![PathBuf::from("project/link.txt")], - }), - event_stream, - cx, - ) - }); - - cx.run_until_parked(); - - let auth = event_rx.expect_authorization().await; - let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); - assert!( - title.contains("points outside the project"), - "Expected symlink escape authorization, got: {title}", - ); - - auth.response - .send(acp_thread::SelectedPermissionOutcome::new( - acp::PermissionOptionId::new("allow"), - acp::PermissionOptionKind::AllowOnce, - )) - .unwrap(); - - assert!( - !matches!( - event_rx.try_recv(), - Ok(Ok(crate::ThreadEvent::ToolCallAuthorization(_))) - ), - "Expected a single authorization prompt", - ); - - let _result = task.await; - } -} diff --git a/crates/agent/src/tools/save_file_tool.rs b/crates/agent/src/tools/save_file_tool.rs deleted file mode 100644 index f70420984157fae438b746b146bd082c5cb23aff..0000000000000000000000000000000000000000 --- a/crates/agent/src/tools/save_file_tool.rs +++ /dev/null @@ -1,756 +0,0 @@ -use agent_client_protocol::schema as acp; -use agent_settings::AgentSettings; -use collections::FxHashSet; -use futures::FutureExt as _; -use gpui::{App, Entity, SharedString, Task}; -use language::Buffer; -use project::Project; -use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; -use settings::Settings; -use std::path::{Path, PathBuf}; -use std::sync::Arc; -use util::markdown::MarkdownInlineCode; - -use super::tool_permissions::{ - ResolvedProjectPath, authorize_symlink_access, canonicalize_worktree_roots, - path_has_symlink_escape, resolve_project_path, sensitive_settings_kind, -}; -use crate::{ - AgentTool, ToolCallEventStream, ToolInput, ToolPermissionDecision, - authorize_with_sensitive_settings, decide_permission_for_path, -}; - -/// Saves files that have unsaved changes. -/// -/// Use this tool when you need to edit files but they have unsaved changes that must be saved first. -/// Only use this tool after asking the user for permission to save their unsaved changes. -#[derive(Debug, Serialize, Deserialize, JsonSchema)] -pub struct SaveFileToolInput { - /// The paths of the files to save. - pub paths: Vec, -} - -pub struct SaveFileTool { - project: Entity, -} - -impl SaveFileTool { - pub fn new(project: Entity) -> Self { - Self { project } - } -} - -impl AgentTool for SaveFileTool { - type Input = SaveFileToolInput; - type Output = String; - - const NAME: &'static str = "save_file"; - - fn kind() -> acp::ToolKind { - acp::ToolKind::Other - } - - fn initial_title( - &self, - input: Result, - _cx: &mut App, - ) -> SharedString { - match input { - Ok(input) if input.paths.len() == 1 => "Save file".into(), - Ok(input) => format!("Save {} files", input.paths.len()).into(), - Err(_) => "Save files".into(), - } - } - - fn run( - self: Arc, - input: ToolInput, - event_stream: ToolCallEventStream, - cx: &mut App, - ) -> Task> { - let project = self.project.clone(); - - cx.spawn(async move |cx| { - let input = input.recv().await.map_err(|e| e.to_string())?; - - // Check for any immediate deny before doing async work. - for path in &input.paths { - let path_str = path.to_string_lossy(); - let decision = cx.update(|cx| { - decide_permission_for_path(Self::NAME, &path_str, AgentSettings::get_global(cx)) - }); - if let ToolPermissionDecision::Deny(reason) = decision { - return Err(reason); - } - } - - let input_paths = input.paths; - - let fs = project.read_with(cx, |project, _cx| project.fs().clone()); - let canonical_roots = canonicalize_worktree_roots(&project, &fs, cx).await; - - let mut confirmation_paths: Vec = Vec::new(); - - for path in &input_paths { - let path_str = path.to_string_lossy(); - let decision = cx.update(|cx| { - decide_permission_for_path(Self::NAME, &path_str, AgentSettings::get_global(cx)) - }); - let symlink_escape = project.read_with(cx, |project, cx| { - path_has_symlink_escape(project, path, &canonical_roots, cx) - }); - - match decision { - ToolPermissionDecision::Allow => { - if !symlink_escape { - let is_sensitive = super::tool_permissions::is_sensitive_settings_path( - Path::new(&*path_str), - fs.as_ref(), - ) - .await; - if is_sensitive { - confirmation_paths.push(path_str.to_string()); - } - } - } - ToolPermissionDecision::Deny(reason) => { - return Err(reason); - } - ToolPermissionDecision::Confirm => { - if !symlink_escape { - confirmation_paths.push(path_str.to_string()); - } - } - } - } - - if !confirmation_paths.is_empty() { - let title = if confirmation_paths.len() == 1 { - format!("Save {}", MarkdownInlineCode(&confirmation_paths[0])) - } else { - let paths: Vec<_> = confirmation_paths - .iter() - .take(3) - .map(|p| p.as_str()) - .collect(); - if confirmation_paths.len() > 3 { - format!( - "Save {}, and {} more", - paths.join(", "), - confirmation_paths.len() - 3 - ) - } else { - format!("Save {}", paths.join(", ")) - } - }; - - let mut settings_kind = None; - for p in &confirmation_paths { - if let Some(kind) = sensitive_settings_kind(Path::new(p), fs.as_ref()).await { - settings_kind = Some(kind); - break; - } - } - let context = - crate::ToolPermissionContext::new(Self::NAME, confirmation_paths.clone()); - let authorize = cx.update(|cx| { - authorize_with_sensitive_settings( - settings_kind, - context, - &title, - &event_stream, - cx, - ) - }); - authorize.await.map_err(|e| e.to_string())?; - } - - let mut buffers_to_save: FxHashSet> = FxHashSet::default(); - - let mut dirty_count: usize = 0; - let mut clean_paths: Vec = Vec::new(); - let mut not_found_paths: Vec = Vec::new(); - let mut open_errors: Vec<(PathBuf, String)> = Vec::new(); - let mut authorization_errors: Vec<(PathBuf, String)> = Vec::new(); - let mut save_errors: Vec<(String, String)> = Vec::new(); - - for path in input_paths { - let project_path = match project.read_with(cx, |project, cx| { - resolve_project_path(project, &path, &canonical_roots, cx) - }) { - Ok(resolved) => { - let (project_path, symlink_canonical_target) = match resolved { - ResolvedProjectPath::Safe(path) => (path, None), - ResolvedProjectPath::SymlinkEscape { - project_path, - canonical_target, - } => (project_path, Some(canonical_target)), - }; - if let Some(canonical_target) = &symlink_canonical_target { - let path_str = path.to_string_lossy(); - let authorize_task = cx.update(|cx| { - authorize_symlink_access( - Self::NAME, - &path_str, - canonical_target, - &event_stream, - cx, - ) - }); - let result = authorize_task.await; - if let Err(err) = result { - authorization_errors.push((path.clone(), err.to_string())); - continue; - } - } - project_path - } - Err(_) => { - not_found_paths.push(path); - continue; - } - }; - - let open_buffer_task = - project.update(cx, |project, cx| project.open_buffer(project_path, cx)); - - let buffer = futures::select! { - result = open_buffer_task.fuse() => { - match result { - Ok(buffer) => buffer, - Err(error) => { - open_errors.push((path, error.to_string())); - continue; - } - } - } - _ = event_stream.cancelled_by_user().fuse() => { - return Err("Save cancelled by user".to_string()); - } - }; - - let is_dirty = buffer.read_with(cx, |buffer, _| buffer.is_dirty()); - - if is_dirty { - buffers_to_save.insert(buffer); - dirty_count += 1; - } else { - clean_paths.push(path); - } - } - - // Save each buffer individually since there's no batch save API. - for buffer in buffers_to_save { - let path_for_buffer = buffer - .read_with(cx, |buffer, _| { - buffer - .file() - .map(|file| file.path().to_rel_path_buf()) - .map(|path| path.as_rel_path().as_unix_str().to_owned()) - }) - .unwrap_or_else(|| "".to_string()); - - let save_task = project.update(cx, |project, cx| project.save_buffer(buffer, cx)); - - let save_result = futures::select! { - result = save_task.fuse() => result, - _ = event_stream.cancelled_by_user().fuse() => { - return Err("Save cancelled by user".to_string()); - } - }; - if let Err(error) = save_result { - save_errors.push((path_for_buffer, error.to_string())); - } - } - - let mut lines: Vec = Vec::new(); - - let successful_saves = dirty_count.saturating_sub(save_errors.len()); - if successful_saves > 0 { - lines.push(format!("Saved {} file(s).", successful_saves)); - } - if !clean_paths.is_empty() { - lines.push(format!("{} clean.", clean_paths.len())); - } - - if !not_found_paths.is_empty() { - lines.push(format!("Not found ({}):", not_found_paths.len())); - for path in ¬_found_paths { - lines.push(format!("- {}", path.display())); - } - } - if !open_errors.is_empty() { - lines.push(format!("Open failed ({}):", open_errors.len())); - for (path, error) in &open_errors { - lines.push(format!("- {}: {}", path.display(), error)); - } - } - if !authorization_errors.is_empty() { - lines.push(format!( - "Authorization failed ({}):", - authorization_errors.len() - )); - for (path, error) in &authorization_errors { - lines.push(format!("- {}: {}", path.display(), error)); - } - } - if !save_errors.is_empty() { - lines.push(format!("Save failed ({}):", save_errors.len())); - for (path, error) in &save_errors { - lines.push(format!("- {}: {}", path, error)); - } - } - - if lines.is_empty() { - Ok("No paths provided.".to_string()) - } else { - Ok(lines.join("\n")) - } - }) - } -} - -#[cfg(test)] -mod tests { - use super::*; - use fs::Fs as _; - use gpui::TestAppContext; - use project::FakeFs; - use serde_json::json; - use settings::SettingsStore; - use util::path; - - fn init_test(cx: &mut TestAppContext) { - cx.update(|cx| { - let settings_store = SettingsStore::test(cx); - cx.set_global(settings_store); - }); - cx.update(|cx| { - let mut settings = AgentSettings::get_global(cx).clone(); - settings.tool_permissions.default = settings::ToolPermissionMode::Allow; - AgentSettings::override_global(settings, cx); - }); - } - - #[gpui::test] - async fn test_save_file_output_and_effects(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - "/root", - json!({ - "dirty.txt": "on disk: dirty\n", - "clean.txt": "on disk: clean\n", - }), - ) - .await; - - let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await; - let tool = Arc::new(SaveFileTool::new(project.clone())); - - // Make dirty.txt dirty in-memory. - let dirty_project_path = project.read_with(cx, |project, cx| { - project - .find_project_path("root/dirty.txt", cx) - .expect("dirty.txt should exist in project") - }); - - let dirty_buffer = project - .update(cx, |project, cx| { - project.open_buffer(dirty_project_path, cx) - }) - .await - .unwrap(); - dirty_buffer.update(cx, |buffer, cx| { - buffer.edit([(0..buffer.len(), "in memory: dirty\n")], None, cx); - }); - assert!( - dirty_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "dirty.txt buffer should be dirty before save" - ); - - // Ensure clean.txt is opened but remains clean. - let clean_project_path = project.read_with(cx, |project, cx| { - project - .find_project_path("root/clean.txt", cx) - .expect("clean.txt should exist in project") - }); - - let clean_buffer = project - .update(cx, |project, cx| { - project.open_buffer(clean_project_path, cx) - }) - .await - .unwrap(); - assert!( - !clean_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "clean.txt buffer should start clean" - ); - - let output = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { - paths: vec![ - PathBuf::from("root/dirty.txt"), - PathBuf::from("root/clean.txt"), - ], - }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await - .unwrap(); - - // Output should mention saved + clean. - assert!( - output.contains("Saved 1 file(s)."), - "expected saved count line, got:\n{output}" - ); - assert!( - output.contains("1 clean."), - "expected clean count line, got:\n{output}" - ); - - // Effect: dirty buffer should now be clean and disk should have new content. - assert!( - !dirty_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "dirty.txt buffer should not be dirty after save" - ); - - let disk_dirty = fs.load(path!("/root/dirty.txt").as_ref()).await.unwrap(); - assert_eq!( - disk_dirty, "in memory: dirty\n", - "dirty.txt disk content should be updated" - ); - - // Sanity: clean buffer should remain clean and disk unchanged. - let disk_clean = fs.load(path!("/root/clean.txt").as_ref()).await.unwrap(); - assert_eq!(disk_clean, "on disk: clean\n"); - - // Test empty paths case. - let output = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { paths: vec![] }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await - .unwrap(); - assert_eq!(output, "No paths provided."); - - // Test not-found path case. - let output = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { - paths: vec![PathBuf::from("nonexistent/path.txt")], - }), - ToolCallEventStream::test().0, - cx, - ) - }) - .await - .unwrap(); - assert!( - output.contains("Not found (1):"), - "expected not-found header line, got:\n{output}" - ); - assert!( - output.contains("- nonexistent/path.txt"), - "expected not-found path bullet, got:\n{output}" - ); - } - - #[gpui::test] - async fn test_save_file_symlink_escape_requests_authorization(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "src": {} - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let tool = Arc::new(SaveFileTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let task = cx.update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { - paths: vec![PathBuf::from("project/link.txt")], - }), - event_stream, - cx, - ) - }); - - cx.run_until_parked(); - - let auth = event_rx.expect_authorization().await; - let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); - assert!( - title.contains("points outside the project"), - "Expected symlink escape authorization, got: {title}", - ); - - auth.response - .send(acp_thread::SelectedPermissionOutcome::new( - acp::PermissionOptionId::new("allow"), - acp::PermissionOptionKind::AllowOnce, - )) - .unwrap(); - - let _result = task.await; - } - - #[gpui::test] - async fn test_save_file_symlink_escape_honors_deny_policy(cx: &mut TestAppContext) { - init_test(cx); - cx.update(|cx| { - let mut settings = AgentSettings::get_global(cx).clone(); - settings.tool_permissions.tools.insert( - "save_file".into(), - agent_settings::ToolRules { - default: Some(settings::ToolPermissionMode::Deny), - ..Default::default() - }, - ); - AgentSettings::override_global(settings, cx); - }); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "src": {} - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let tool = Arc::new(SaveFileTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let result = cx - .update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { - paths: vec![PathBuf::from("project/link.txt")], - }), - event_stream, - cx, - ) - }) - .await; - - assert!(result.is_err(), "Tool should fail when policy denies"); - assert!( - !matches!( - event_rx.try_recv(), - Ok(Ok(crate::ThreadEvent::ToolCallAuthorization(_))) - ), - "Deny policy should not emit symlink authorization prompt", - ); - } - - #[gpui::test] - async fn test_save_file_symlink_escape_confirm_requires_single_approval( - cx: &mut TestAppContext, - ) { - init_test(cx); - cx.update(|cx| { - let mut settings = AgentSettings::get_global(cx).clone(); - settings.tool_permissions.default = settings::ToolPermissionMode::Confirm; - AgentSettings::override_global(settings, cx); - }); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "src": {} - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let tool = Arc::new(SaveFileTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let task = cx.update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { - paths: vec![PathBuf::from("project/link.txt")], - }), - event_stream, - cx, - ) - }); - - cx.run_until_parked(); - - let auth = event_rx.expect_authorization().await; - let title = auth.tool_call.fields.title.as_deref().unwrap_or(""); - assert!( - title.contains("points outside the project"), - "Expected symlink escape authorization, got: {title}", - ); - - auth.response - .send(acp_thread::SelectedPermissionOutcome::new( - acp::PermissionOptionId::new("allow"), - acp::PermissionOptionKind::AllowOnce, - )) - .unwrap(); - - assert!( - !matches!( - event_rx.try_recv(), - Ok(Ok(crate::ThreadEvent::ToolCallAuthorization(_))) - ), - "Expected a single authorization prompt", - ); - - let _result = task.await; - } - - #[gpui::test] - async fn test_save_file_symlink_denial_does_not_reduce_success_count(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree( - path!("/root"), - json!({ - "project": { - "dirty.txt": "on disk value\n", - }, - "external": { - "secret.txt": "secret content" - } - }), - ) - .await; - - fs.create_symlink( - path!("/root/project/link.txt").as_ref(), - PathBuf::from("../external/secret.txt"), - ) - .await - .unwrap(); - - let project = Project::test(fs.clone(), [path!("/root/project").as_ref()], cx).await; - cx.executor().run_until_parked(); - - let dirty_project_path = project.read_with(cx, |project, cx| { - project - .find_project_path("project/dirty.txt", cx) - .expect("dirty.txt should exist in project") - }); - let dirty_buffer = project - .update(cx, |project, cx| { - project.open_buffer(dirty_project_path, cx) - }) - .await - .unwrap(); - dirty_buffer.update(cx, |buffer, cx| { - buffer.edit([(0..buffer.len(), "in memory value\n")], None, cx); - }); - assert!( - dirty_buffer.read_with(cx, |buffer, _| buffer.is_dirty()), - "dirty.txt should be dirty before save" - ); - - let tool = Arc::new(SaveFileTool::new(project)); - - let (event_stream, mut event_rx) = ToolCallEventStream::test(); - let task = cx.update(|cx| { - tool.clone().run( - ToolInput::resolved(SaveFileToolInput { - paths: vec![ - PathBuf::from("project/dirty.txt"), - PathBuf::from("project/link.txt"), - ], - }), - event_stream, - cx, - ) - }); - - cx.run_until_parked(); - - let auth = event_rx.expect_authorization().await; - auth.response - .send(acp_thread::SelectedPermissionOutcome::new( - acp::PermissionOptionId::new("deny"), - acp::PermissionOptionKind::RejectOnce, - )) - .unwrap(); - - let output = task.await.unwrap(); - assert!( - output.contains("Saved 1 file(s)."), - "Expected successful save count to remain accurate, got:\n{output}", - ); - assert!( - output.contains("Authorization failed (1):"), - "Expected authorization failure section, got:\n{output}", - ); - assert!( - !output.contains("Save failed"), - "Authorization denials should not be counted as save failures, got:\n{output}", - ); - } -} diff --git a/crates/agent/src/tools/tool_permissions.rs b/crates/agent/src/tools/tool_permissions.rs index aa541c3e0ef5793a8b491dc6686d5b7c58bc05b3..5d59dd2eddbfdc6ec06a8289be046b8d5ac1a05a 100644 --- a/crates/agent/src/tools/tool_permissions.rs +++ b/crates/agent/src/tools/tool_permissions.rs @@ -2,6 +2,7 @@ use crate::{ Thread, ToolCallEventStream, ToolPermissionContext, ToolPermissionDecision, decide_permission_for_path, }; +use agent_client_protocol::schema as acp; use anyhow::{Result, anyhow}; use fs::Fs; use gpui::{App, Entity, Task, WeakEntity}; @@ -521,6 +522,91 @@ pub fn authorize_file_edit( }) } +/// The user's choice when prompted about how to handle unsaved changes +/// in a buffer that the agent wants to edit or overwrite. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DirtyBufferDecision { + /// Save the buffer's pending edits to disk, then proceed. + /// (Edit-mode prompt only.) + Save, + /// Discard the buffer's pending edits (reload from disk), then proceed. + Discard, + /// Keep the buffer's pending edits and cancel the agent's operation. + /// (Overwrite-mode prompt only.) + Keep, +} + +/// Which prompt to show when the agent encounters a dirty buffer. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DirtyBufferPromptKind { + /// The agent wants to apply targeted edits on top of the current + /// content. Offers Save (persist edits, then edit on top) vs Discard + /// (revert to disk, then edit). + Edit, + /// The agent wants to overwrite the file's entire contents. Offers + /// Keep (cancel the overwrite to preserve the user's work) vs + /// Discard (reload from disk and let the agent overwrite). + Overwrite, +} + +/// Prompts the user about how to handle a dirty buffer that the agent +/// wants to edit or overwrite. Returns the chosen action; the caller is +/// responsible for actually performing the corresponding side effect +/// (save / reload / cancel) before continuing. +pub fn authorize_dirty_buffer( + kind: DirtyBufferPromptKind, + event_stream: &ToolCallEventStream, + cx: &mut App, +) -> Task> { + let (message, options) = match kind { + DirtyBufferPromptKind::Edit => ( + "This file has unsaved changes. Do you want to save or discard them \ + before the agent continues editing?" + .to_string(), + vec![ + acp::PermissionOption::new( + acp::PermissionOptionId::new("save"), + "Save", + acp::PermissionOptionKind::AllowOnce, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("discard"), + "Discard", + acp::PermissionOptionKind::RejectOnce, + ), + ], + ), + DirtyBufferPromptKind::Overwrite => ( + "This file has unsaved changes and the agent wants to overwrite it.".to_string(), + vec![ + acp::PermissionOption::new( + acp::PermissionOptionId::new("discard"), + "Overwrite", + acp::PermissionOptionKind::AllowOnce, + ), + acp::PermissionOption::new( + acp::PermissionOptionId::new("keep"), + "Cancel", + acp::PermissionOptionKind::RejectOnce, + ), + ], + ), + }; + + let prompt = event_stream.prompt_for_decision(None, Some(message), options, cx); + cx.spawn(async move |_cx| { + let option_id = prompt.await?; + match option_id.0.as_ref() { + "save" => Ok(DirtyBufferDecision::Save), + "discard" => Ok(DirtyBufferDecision::Discard), + "keep" => Ok(DirtyBufferDecision::Keep), + other => Err(anyhow!( + "Unexpected dirty-buffer decision option_id: {other}" + )), + } + }) +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/agent/src/tools/write_file_tool.rs b/crates/agent/src/tools/write_file_tool.rs index c9cd548f316ed898fc983f77bd2fdb8c5f94db4e..bfd454aba9723f2733dcaeda5732225e76261782 100644 --- a/crates/agent/src/tools/write_file_tool.rs +++ b/crates/agent/src/tools/write_file_tool.rs @@ -1091,6 +1091,212 @@ mod tests { ); } + /// When the buffer has unsaved user edits and the user picks + /// "Discard my edits", the pending edits are reverted to match disk + /// and the agent's overwrite proceeds. + #[gpui::test] + async fn test_streaming_write_dirty_buffer_discard(cx: &mut TestAppContext) { + let (write_tool, project, _action_log, fs, _thread) = + setup_test(cx, json!({"file.txt": "on disk content"})).await; + + let project_path = project + .read_with(cx, |project, cx| { + project.find_project_path("root/file.txt", cx) + }) + .expect("Should find project path"); + let buffer = project + .update(cx, |project, cx| project.open_buffer(project_path, cx)) + .await + .unwrap(); + buffer.update(cx, |buffer, cx| { + let end_point = buffer.max_point(); + buffer.edit([(end_point..end_point, " plus user edit")], None, cx); + }); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + write_tool.clone().run( + ToolInput::resolved(WriteFileToolInput { + path: "root/file.txt".into(), + content: "agent overwrote it".into(), + }), + stream_tx, + cx, + ) + }); + + let _update = stream_rx.expect_update_fields().await; + let auth = stream_rx.expect_authorization().await; + + // Verify the prompt is the overwrite-mode prompt. + let content = auth.tool_call.fields.content.as_deref().unwrap_or(&[]); + let acp::ToolCallContent::Content(text) = content.first().expect("expected message body") + else { + panic!("expected text body, got: {:?}", content.first()); + }; + let acp::ContentBlock::Text(text) = &text.content else { + panic!("expected text body, got: {:?}", text.content); + }; + assert!( + text.text.contains("overwrite"), + "expected overwrite-mode prompt, got: {:?}", + text.text, + ); + + // Verify both option ids are present (option_id is the stable contract). + let option_ids: Vec<&str> = match &auth.options { + acp_thread::PermissionOptions::Flat(opts) => { + opts.iter().map(|o| o.option_id.0.as_ref()).collect() + } + other => panic!("expected flat options, got: {other:?}"), + }; + assert!(option_ids.contains(&"keep"), "options: {option_ids:?}"); + assert!(option_ids.contains(&"discard"), "options: {option_ids:?}"); + + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("discard"), + acp::PermissionOptionKind::AllowOnce, + )) + .unwrap(); + + let EditSessionOutput::Success { new_text, .. } = task.await.unwrap() else { + panic!("expected success"); + }; + assert_eq!(new_text, "agent overwrote it"); + assert!(!buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + let on_disk = fs.load(path!("/root/file.txt").as_ref()).await.unwrap(); + assert_eq!(on_disk, "agent overwrote it"); + } + + /// When the buffer has unsaved user edits and the user picks + /// "Keep my edits", the overwrite is cancelled with an error and the + /// user's pending edits are preserved. + #[gpui::test] + async fn test_streaming_write_dirty_buffer_keep(cx: &mut TestAppContext) { + let (write_tool, project, _action_log, fs, _thread) = + setup_test(cx, json!({"file.txt": "on disk content"})).await; + + let project_path = project + .read_with(cx, |project, cx| { + project.find_project_path("root/file.txt", cx) + }) + .expect("Should find project path"); + let buffer = project + .update(cx, |project, cx| project.open_buffer(project_path, cx)) + .await + .unwrap(); + buffer.update(cx, |buffer, cx| { + let end_point = buffer.max_point(); + buffer.edit([(end_point..end_point, " plus user edit")], None, cx); + }); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + write_tool.clone().run( + ToolInput::resolved(WriteFileToolInput { + path: "root/file.txt".into(), + content: "agent overwrote it".into(), + }), + stream_tx, + cx, + ) + }); + + let _update = stream_rx.expect_update_fields().await; + let auth = stream_rx.expect_authorization().await; + auth.response + .send(acp_thread::SelectedPermissionOutcome::new( + acp::PermissionOptionId::new("keep"), + acp::PermissionOptionKind::RejectOnce, + )) + .unwrap(); + + let EditSessionOutput::Error { error, .. } = task.await.unwrap_err() else { + panic!("expected error"); + }; + assert!( + error.contains("keep") || error.contains("cancelled"), + "expected cancel-style error message, got: {error:?}", + ); + + // The user's in-memory edits are preserved. + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + let buffer_text = buffer.read_with(cx, |buffer, _| buffer.text()); + assert_eq!(buffer_text, "on disk content plus user edit"); + + // The on-disk content is untouched. + let on_disk = fs.load(path!("/root/file.txt").as_ref()).await.unwrap(); + assert_eq!(on_disk, "on disk content"); + } + + /// When the user manually saves the buffer (e.g. cmd-s) while the + /// overwrite prompt is visible, that's treated as "Keep my edits": + /// the user just deliberately persisted their work, so we cancel the + /// agent's overwrite to avoid clobbering it. + #[gpui::test] + async fn test_streaming_write_dirty_buffer_resolved_externally(cx: &mut TestAppContext) { + let (write_tool, project, _action_log, fs, _thread) = + setup_test(cx, json!({"file.txt": "on disk content"})).await; + + let project_path = project + .read_with(cx, |project, cx| { + project.find_project_path("root/file.txt", cx) + }) + .expect("Should find project path"); + let buffer = project + .update(cx, |project, cx| project.open_buffer(project_path, cx)) + .await + .unwrap(); + buffer.update(cx, |buffer, cx| { + let end_point = buffer.max_point(); + buffer.edit([(end_point..end_point, " plus user edit")], None, cx); + }); + assert!(buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + + let (stream_tx, mut stream_rx) = ToolCallEventStream::test(); + let task = cx.update(|cx| { + write_tool.clone().run( + ToolInput::resolved(WriteFileToolInput { + path: "root/file.txt".into(), + content: "agent overwrote it".into(), + }), + stream_tx, + cx, + ) + }); + + let _update = stream_rx.expect_update_fields().await; + let auth = stream_rx.expect_authorization().await; + + // User saves manually while the prompt is up. + project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx)) + .await + .unwrap(); + + // The prompt is dismissed by transitioning to InProgress. + let dismiss = stream_rx.expect_update_fields().await; + assert_eq!(dismiss.status, Some(acp::ToolCallStatus::InProgress)); + drop(auth); + + // The overwrite is cancelled with an error. + let EditSessionOutput::Error { error, .. } = task.await.unwrap_err() else { + panic!("expected error"); + }; + assert!( + error.contains("saved") || error.contains("cancelled"), + "expected cancel-on-manual-save error, got: {error:?}", + ); + + // The user's edits were saved to disk and not clobbered. + assert!(!buffer.read_with(cx, |buffer, _| buffer.is_dirty())); + let on_disk = fs.load(path!("/root/file.txt").as_ref()).await.unwrap(); + assert_eq!(on_disk, "on disk content plus user edit"); + } + async fn setup_test_with_fs( cx: &mut TestAppContext, fs: Arc, diff --git a/crates/agent_servers/src/acp.rs b/crates/agent_servers/src/acp.rs index cc467eb8cd0762ddf15ce6fd7223d996cc1b27e4..4ea43aceb68da14d9cc2e134858cc082c9c3a0ef 100644 --- a/crates/agent_servers/src/acp.rs +++ b/crates/agent_servers/src/acp.rs @@ -3359,6 +3359,7 @@ fn handle_request_permission( thread.request_tool_call_authorization( args.tool_call, acp_thread::PermissionOptions::Flat(args.options), + acp_thread::AuthorizationKind::PermissionGrant, cx, ) }) diff --git a/crates/agent_ui/src/conversation_view.rs b/crates/agent_ui/src/conversation_view.rs index 773507e2af1d5986a1f22b25ac61ac62992f2a21..20f5212dbce530c1f898a1a411bca5a72757f4bd 100644 --- a/crates/agent_ui/src/conversation_view.rs +++ b/crates/agent_ui/src/conversation_view.rs @@ -7120,6 +7120,7 @@ pub(crate) mod tests { "Allow", acp::PermissionOptionKind::AllowOnce, )]), + acp_thread::AuthorizationKind::PermissionGrant, cx, ) .unwrap() diff --git a/crates/settings_ui/src/pages.rs b/crates/settings_ui/src/pages.rs index 63c3965d095a3ae769dbaa36d01a66281f48d868..4a69069148ecae60c732c80edc96355d4a7460f7 100644 --- a/crates/settings_ui/src/pages.rs +++ b/crates/settings_ui/src/pages.rs @@ -15,7 +15,6 @@ pub(crate) use tool_permissions_setup::render_tool_permissions_setup_page; pub use tool_permissions_setup::{ render_copy_path_tool_config, render_create_directory_tool_config, render_delete_path_tool_config, render_edit_file_tool_config, render_fetch_tool_config, - render_move_path_tool_config, render_restore_file_from_disk_tool_config, - render_save_file_tool_config, render_terminal_tool_config, render_web_search_tool_config, + render_move_path_tool_config, render_terminal_tool_config, render_web_search_tool_config, render_write_file_tool_config, }; diff --git a/crates/settings_ui/src/pages/tool_permissions_setup.rs b/crates/settings_ui/src/pages/tool_permissions_setup.rs index 05cd51e3f924826609501f994a1abcb0ddf73ef4..550b9a3adc6ae0d2e5fb8f65500dab7f25c72e67 100644 --- a/crates/settings_ui/src/pages/tool_permissions_setup.rs +++ b/crates/settings_ui/src/pages/tool_permissions_setup.rs @@ -62,12 +62,6 @@ const TOOLS: &[ToolInfo] = &[ description: "Directory creation", regex_explanation: "Patterns are matched against the directory path being created.", }, - ToolInfo { - id: "save_file", - name: "Save File", - description: "File saving operations", - regex_explanation: "Patterns are matched against the file path being saved.", - }, ToolInfo { id: "fetch", name: "Fetch", @@ -80,12 +74,6 @@ const TOOLS: &[ToolInfo] = &[ description: "Web search queries", regex_explanation: "Patterns are matched against the search query.", }, - ToolInfo { - id: "restore_file_from_disk", - name: "Restore File from Disk", - description: "Discards unsaved changes by reloading from disk", - regex_explanation: "Patterns are matched against the file path being restored.", - }, ]; pub(crate) struct ToolInfo { @@ -314,10 +302,8 @@ fn get_tool_render_fn( "copy_path" => render_copy_path_tool_config, "move_path" => render_move_path_tool_config, "create_directory" => render_create_directory_tool_config, - "save_file" => render_save_file_tool_config, "fetch" => render_fetch_tool_config, "search_web" => render_web_search_tool_config, - "restore_file_from_disk" => render_restore_file_from_disk_tool_config, _ => render_terminal_tool_config, // fallback } } @@ -1395,13 +1381,8 @@ tool_config_page_fn!(render_delete_path_tool_config, "delete_path"); tool_config_page_fn!(render_copy_path_tool_config, "copy_path"); tool_config_page_fn!(render_move_path_tool_config, "move_path"); tool_config_page_fn!(render_create_directory_tool_config, "create_directory"); -tool_config_page_fn!(render_save_file_tool_config, "save_file"); tool_config_page_fn!(render_fetch_tool_config, "fetch"); tool_config_page_fn!(render_web_search_tool_config, "search_web"); -tool_config_page_fn!( - render_restore_file_from_disk_tool_config, - "restore_file_from_disk" -); #[cfg(test)] mod tests { diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index dc806009a9fc05950e47c0e79307096986bf47af..1d553d7d5ad625134600476f5212b34e641dd998 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -444,6 +444,7 @@ fn request_test_tool_authorization( "Allow", acp::PermissionOptionKind::AllowOnce, )]), + acp_thread::AuthorizationKind::PermissionGrant, cx, ) .unwrap()