From c7158f0bd7ec3324fe299b4cf3e6b4e34a492436 Mon Sep 17 00:00:00 2001 From: Oleksiy Syvokon Date: Tue, 22 Jul 2025 14:23:50 +0300 Subject: [PATCH] Improve distinguishing user from agent edits (#34716) We no longer rely on the `author` field to tell if a change was made by the user or the agent. The `author` can be set to `User` in many situations that are not really user-made edits, such as saving a file, accepting a change, auto-formatting, and more. I started tracking and fixing some of these cases, but found that inspecting changes in `diff_base` is a more reliable method. Also, we no longer show empty diffs. For example, if the user adds a line and then removes the same line, the final diff is empty, even though the buffer is marked as user-changed. Now we won't show such edit. There are still some issues to address: - When a user edits within an unaccepted agent-written block, this change becomes a part of the agent's edit. Rejecting this block will lose user edits. It won't be displayed in project notifications, either. - Accepting an agent block counts as a user-made edit. - Agent start to call `project_notifications` tool after seeing enough auto-calls. Release Notes: - N/A --- crates/agent/src/thread.rs | 20 ++++--- crates/assistant_tool/src/action_log.rs | 56 +++++++++++--------- crates/assistant_tools/src/edit_file_tool.rs | 3 ++ 3 files changed, 42 insertions(+), 37 deletions(-) diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index e50763535a461bef6769b4f7c1aadeb2f219b904..f8a78276154f3eec89e30acdb808b7c41242a1b1 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -47,7 +47,7 @@ use std::{ time::{Duration, Instant}, }; use thiserror::Error; -use util::{ResultExt as _, debug_panic, post_inc}; +use util::{ResultExt as _, post_inc}; use uuid::Uuid; use zed_llm_client::{CompletionIntent, CompletionRequestStatus, UsageLimit}; @@ -1582,23 +1582,21 @@ impl Thread { model: Arc, cx: &mut App, ) -> Option { - let action_log = self.action_log.read(cx); - - if !action_log.has_unnotified_user_edits() { - return None; - } - // Represent notification as a simulated `project_notifications` tool call let tool_name = Arc::from("project_notifications"); - let Some(tool) = self.tools.read(cx).tool(&tool_name, cx) else { - debug_panic!("`project_notifications` tool not found"); - return None; - }; + let tool = self.tools.read(cx).tool(&tool_name, cx)?; if !self.profile.is_tool_enabled(tool.source(), tool.name(), cx) { return None; } + if self + .action_log + .update(cx, |log, cx| log.unnotified_user_edits(cx).is_none()) + { + return None; + } + let input = serde_json::json!({}); let request = Arc::new(LanguageModelRequest::default()); // unused let window = None; diff --git a/crates/assistant_tool/src/action_log.rs b/crates/assistant_tool/src/action_log.rs index ecbbcc785e730e3a7ca60f014f641d555e696b9e..672c048872c954da977ce9a1c64d50835f662150 100644 --- a/crates/assistant_tool/src/action_log.rs +++ b/crates/assistant_tool/src/action_log.rs @@ -51,23 +51,13 @@ impl ActionLog { Some(self.tracked_buffers.get(buffer)?.snapshot.clone()) } - pub fn has_unnotified_user_edits(&self) -> bool { - self.tracked_buffers - .values() - .any(|tracked| tracked.has_unnotified_user_edits) - } - /// Return a unified diff patch with user edits made since last read or notification pub fn unnotified_user_edits(&self, cx: &Context) -> Option { - if !self.has_unnotified_user_edits() { - return None; - } - - let unified_diff = self + let diffs = self .tracked_buffers .values() .filter_map(|tracked| { - if !tracked.has_unnotified_user_edits { + if !tracked.may_have_unnotified_user_edits { return None; } @@ -95,9 +85,13 @@ impl ActionLog { Some(result) }) - .collect::>() - .join("\n\n"); + .collect::>(); + + if diffs.is_empty() { + return None; + } + let unified_diff = diffs.join("\n\n"); Some(unified_diff) } @@ -106,7 +100,7 @@ impl ActionLog { pub fn flush_unnotified_user_edits(&mut self, cx: &Context) -> Option { let patch = self.unnotified_user_edits(cx); self.tracked_buffers.values_mut().for_each(|tracked| { - tracked.has_unnotified_user_edits = false; + tracked.may_have_unnotified_user_edits = false; tracked.last_seen_base = tracked.diff_base.clone(); }); patch @@ -185,7 +179,7 @@ impl ActionLog { version: buffer.read(cx).version(), diff, diff_update: diff_update_tx, - has_unnotified_user_edits: false, + may_have_unnotified_user_edits: false, _open_lsp_handle: open_lsp_handle, _maintain_diff: cx.spawn({ let buffer = buffer.clone(); @@ -337,27 +331,34 @@ impl ActionLog { let new_snapshot = buffer_snapshot.clone(); let unreviewed_edits = tracked_buffer.unreviewed_edits.clone(); let edits = diff_snapshots(&old_snapshot, &new_snapshot); - if let ChangeAuthor::User = author - && !edits.is_empty() - { - tracked_buffer.has_unnotified_user_edits = true; - } + let mut has_user_changes = false; async move { if let ChangeAuthor::User = author { - apply_non_conflicting_edits( + has_user_changes = apply_non_conflicting_edits( &unreviewed_edits, edits, &mut base_text, new_snapshot.as_rope(), ); } - (Arc::new(base_text.to_string()), base_text) + + (Arc::new(base_text.to_string()), base_text, has_user_changes) } }); anyhow::Ok(rebase) })??; - let (new_base_text, new_diff_base) = rebase.await; + let (new_base_text, new_diff_base, has_user_changes) = rebase.await; + + this.update(cx, |this, _| { + let tracked_buffer = this + .tracked_buffers + .get_mut(buffer) + .context("buffer not tracked") + .unwrap(); + tracked_buffer.may_have_unnotified_user_edits |= has_user_changes; + })?; + Self::update_diff( this, buffer, @@ -829,11 +830,12 @@ fn apply_non_conflicting_edits( edits: Vec>, old_text: &mut Rope, new_text: &Rope, -) { +) -> bool { let mut old_edits = patch.edits().iter().cloned().peekable(); let mut new_edits = edits.into_iter().peekable(); let mut applied_delta = 0i32; let mut rebased_delta = 0i32; + let mut has_made_changes = false; while let Some(mut new_edit) = new_edits.next() { let mut conflict = false; @@ -883,8 +885,10 @@ fn apply_non_conflicting_edits( &new_text.chunks_in_range(new_bytes).collect::(), ); applied_delta += new_edit.new_len() as i32 - new_edit.old_len() as i32; + has_made_changes = true; } } + has_made_changes } fn diff_snapshots( @@ -958,7 +962,7 @@ struct TrackedBuffer { diff: Entity, snapshot: text::BufferSnapshot, diff_update: mpsc::UnboundedSender<(ChangeAuthor, text::BufferSnapshot)>, - has_unnotified_user_edits: bool, + may_have_unnotified_user_edits: bool, _open_lsp_handle: OpenLspBufferHandle, _maintain_diff: Task<()>, _subscription: Subscription, diff --git a/crates/assistant_tools/src/edit_file_tool.rs b/crates/assistant_tools/src/edit_file_tool.rs index 0423f56145bc484108ff958419353e2378a3779a..6413677bd9178c40259ca56fa00491478efda30c 100644 --- a/crates/assistant_tools/src/edit_file_tool.rs +++ b/crates/assistant_tools/src/edit_file_tool.rs @@ -278,6 +278,9 @@ impl Tool for EditFileTool { .unwrap_or(false); 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()]),