Cargo.lock 🔗
@@ -77,6 +77,7 @@ dependencies = [
"ctor",
"fs",
"futures 0.3.32",
+ "git",
"gpui",
"language",
"log",
action_log: Fix race condition when committing changes (#53884)
Bennet Bo Fenner created
Sometimes the action log would not auto-accept agent edits when
commiting.
Gpt-5.4 identified this race condition:
This fixes a race where `keep_committed_edits` could run after
`head_commit` changed but before the new git base text had been applied,
leaving committed agent edits marked as unreviewed; `ActionLog` now
waits for an explicit `BufferDiffEvent::BaseTextChanged` instead of
inferring readiness from generic `DiffChanged` activity, so it only
accepts edits after the diff base itself is actually updated.
- `ReloadGitState` updates `head_commit` before `ReloadBufferDiffBases`
finishes loading and applying the new HEAD text.
- In that gap, an unrelated `DiffChanged` can fire from a normal diff
recalculation.
- The old logic treated that event as the commit signal and ran
`keep_committed_edits` too early.
- `keep_committed_edits` then read stale diff base text, so it failed to
match the committed agent edits.
- When the real base-text update arrived later, the HEAD had already
been overwritten (`old_head`), and the edits stayed unreviewed.
Self-Review Checklist:
- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable
Closes #ISSUE
Release Notes:
- Fixed an issue where committing agent written code would sometimes not
mark edits as acceptedChange summary
Cargo.lock | 1
crates/action_log/Cargo.toml | 1
crates/action_log/src/action_log.rs | 135 ++++++++++++++++++++++++----
crates/buffer_diff/src/buffer_diff.rs | 35 +++++--
4 files changed, 140 insertions(+), 32 deletions(-)
Detailed changes
crates/action_log/Cargo.toml 🔗
@@ -33,6 +33,7 @@ watch.workspace = true
[dev-dependencies]
buffer_diff = { workspace = true, features = ["test-support"] }
+git.workspace = true
collections = { workspace = true, features = ["test-support"] }
clock = { workspace = true, features = ["test-support"] }
ctor.workspace = true
crates/action_log/src/action_log.rs 🔗
@@ -274,7 +274,6 @@ impl ActionLog {
mut buffer_updates: mpsc::UnboundedReceiver<(ChangeAuthor, text::BufferSnapshot)>,
cx: &mut AsyncApp,
) -> Result<()> {
- let git_store = this.read_with(cx, |this, cx| this.project.read(cx).git_store().clone())?;
let git_diff = this
.update(cx, |this, cx| {
this.project.update(cx, |project, cx| {
@@ -283,28 +282,18 @@ impl ActionLog {
})?
.await
.ok();
- let buffer_repo = git_store.read_with(cx, |git_store, cx| {
- git_store.repository_and_path_for_buffer_id(buffer.read(cx).remote_id(), cx)
- });
-
let (mut git_diff_updates_tx, mut git_diff_updates_rx) = watch::channel(());
- let _repo_subscription =
- if let Some((git_diff, (buffer_repo, _))) = git_diff.as_ref().zip(buffer_repo) {
- cx.update(|cx| {
- let mut old_head = buffer_repo.read(cx).head_commit.clone();
- Some(cx.subscribe(git_diff, move |_, event, cx| {
- if let buffer_diff::BufferDiffEvent::DiffChanged { .. } = event {
- let new_head = buffer_repo.read(cx).head_commit.clone();
- if new_head != old_head {
- old_head = new_head;
- git_diff_updates_tx.send(()).ok();
- }
- }
- }))
- })
- } else {
- None
- };
+ let _diff_subscription = if let Some(git_diff) = git_diff.as_ref() {
+ cx.update(|cx| {
+ Some(cx.subscribe(git_diff, move |_, event, _cx| {
+ if matches!(event, buffer_diff::BufferDiffEvent::BaseTextChanged) {
+ git_diff_updates_tx.send(()).ok();
+ }
+ }))
+ })
+ } else {
+ None
+ };
loop {
futures::select_biased! {
@@ -2714,6 +2703,108 @@ mod tests {
assert_eq!(unreviewed_hunks(&action_log, cx), vec![]);
}
+ /// Regression test: when head_commit updates before the BufferDiff's base
+ /// text does, an intermediate DiffChanged (e.g. from a buffer-edit diff
+ /// recalculation) must NOT consume the commit signal. The subscription
+ /// should only fire once the base text itself has changed.
+ #[gpui::test]
+ async fn test_keep_edits_on_commit_with_stale_diff_changed(cx: &mut TestAppContext) {
+ init_test(cx);
+
+ let fs = FakeFs::new(cx.executor());
+ fs.insert_tree(
+ path!("/project"),
+ json!({
+ ".git": {},
+ "file.txt": "aaa\nbbb\nccc\nddd\neee",
+ }),
+ )
+ .await;
+ fs.set_head_for_repo(
+ path!("/project/.git").as_ref(),
+ &[("file.txt", "aaa\nbbb\nccc\nddd\neee".into())],
+ "0000000",
+ );
+ cx.run_until_parked();
+
+ let project = Project::test(fs.clone(), [path!("/project").as_ref()], cx).await;
+ let action_log = cx.new(|_| ActionLog::new(project.clone()));
+
+ let file_path = project
+ .read_with(cx, |project, cx| {
+ project.find_project_path(path!("/project/file.txt"), cx)
+ })
+ .unwrap();
+ let buffer = project
+ .update(cx, |project, cx| project.open_buffer(file_path, cx))
+ .await
+ .unwrap();
+
+ // Agent makes an edit: bbb -> BBB
+ cx.update(|cx| {
+ action_log.update(cx, |log, cx| log.buffer_read(buffer.clone(), cx));
+ buffer.update(cx, |buffer, cx| {
+ buffer.edit([(Point::new(1, 0)..Point::new(1, 3), "BBB")], None, cx);
+ });
+ action_log.update(cx, |log, cx| log.buffer_edited(buffer.clone(), cx));
+ });
+ cx.run_until_parked();
+
+ // Verify the edit is tracked
+ let hunks = unreviewed_hunks(&action_log, cx);
+ assert_eq!(hunks.len(), 1);
+ let hunk = &hunks[0].1;
+ assert_eq!(hunk.len(), 1);
+ assert_eq!(hunk[0].old_text, "bbb\n");
+
+ // Simulate the race condition: update only the HEAD SHA first,
+ // without changing the committed file contents. This is analogous
+ // to compute_snapshot updating head_commit before
+ // reload_buffer_diff_bases has loaded the new base text.
+ fs.with_git_state(path!("/project/.git").as_ref(), true, |state| {
+ state.refs.insert("HEAD".into(), "0000001".into());
+ })
+ .unwrap();
+ cx.run_until_parked();
+
+ // Make a user edit (on a different line) to trigger a buffer diff
+ // recalculation. This fires DiffChanged while the BufferDiff base
+ // text is still the OLD text. With the old head_commit-based
+ // subscription this would "consume" the commit detection.
+ cx.update(|cx| {
+ buffer.update(cx, |buffer, cx| {
+ buffer.edit([(Point::new(3, 0)..Point::new(3, 3), "DDD")], None, cx);
+ });
+ action_log.update(cx, |log, cx| log.buffer_edited(buffer.clone(), cx));
+ });
+ cx.run_until_parked();
+
+ // Now update the committed file contents to match the buffer
+ // (the agent edit was committed). Keep the same SHA so head_commit
+ // does NOT change again — this is the second half of the race.
+ {
+ use git::repository::repo_path;
+ fs.with_git_state(path!("/project/.git").as_ref(), true, |state| {
+ state
+ .head_contents
+ .insert(repo_path("file.txt"), "aaa\nBBB\nccc\nDDD\neee".into());
+ })
+ .unwrap();
+ }
+ cx.run_until_parked();
+
+ // The agent's edit (bbb -> BBB) should be accepted because the
+ // committed content now matches. Only the user edit (ddd -> DDD)
+ // should remain, but since the user edit is tracked as coming from
+ // the user (ChangeAuthor::User) it would have been rebased into
+ // the diff base already. So no unreviewed hunks should remain.
+ assert_eq!(
+ unreviewed_hunks(&action_log, cx),
+ vec![],
+ "agent edits should have been accepted after the base text update"
+ );
+ }
+
#[gpui::test]
async fn test_undo_last_reject(cx: &mut TestAppContext) {
init_test(cx);
crates/buffer_diff/src/buffer_diff.rs 🔗
@@ -1515,11 +1515,17 @@ pub struct DiffChanged {
#[derive(Clone, Debug)]
pub enum BufferDiffEvent {
+ BaseTextChanged,
DiffChanged(DiffChanged),
LanguageChanged,
HunksStagedOrUnstaged(Option<Rope>),
}
+struct SetSnapshotResult {
+ change: DiffChanged,
+ base_text_changed: bool,
+}
+
impl EventEmitter<BufferDiffEvent> for BufferDiff {}
impl BufferDiff {
@@ -1784,7 +1790,7 @@ impl BufferDiff {
secondary_diff_change: Option<Range<Anchor>>,
clear_pending_hunks: bool,
cx: &mut Context<Self>,
- ) -> impl Future<Output = DiffChanged> + use<> {
+ ) -> impl Future<Output = SetSnapshotResult> + use<> {
log::debug!("set snapshot with secondary {secondary_diff_change:?}");
let old_snapshot = self.snapshot(cx);
@@ -1904,10 +1910,13 @@ impl BufferDiff {
if let Some(parsing_idle) = parsing_idle {
parsing_idle.await;
}
- DiffChanged {
- changed_range,
- base_text_changed_range,
- extended_range,
+ SetSnapshotResult {
+ change: DiffChanged {
+ changed_range,
+ base_text_changed_range,
+ extended_range,
+ },
+ base_text_changed,
}
}
}
@@ -1938,12 +1947,15 @@ impl BufferDiff {
);
cx.spawn(async move |this, cx| {
- let change = fut.await;
+ let result = fut.await;
this.update(cx, |_, cx| {
- cx.emit(BufferDiffEvent::DiffChanged(change.clone()));
+ if result.base_text_changed {
+ cx.emit(BufferDiffEvent::BaseTextChanged);
+ }
+ cx.emit(BufferDiffEvent::DiffChanged(result.change.clone()));
})
.ok();
- change.changed_range
+ result.change.changed_range
})
}
@@ -2019,8 +2031,11 @@ impl BufferDiff {
let fg_executor = cx.foreground_executor().clone();
let snapshot = fg_executor.block_on(fut);
let fut = self.set_snapshot_with_secondary_inner(snapshot, buffer, None, false, cx);
- let change = fg_executor.block_on(fut);
- cx.emit(BufferDiffEvent::DiffChanged(change));
+ let result = fg_executor.block_on(fut);
+ if result.base_text_changed {
+ cx.emit(BufferDiffEvent::BaseTextChanged);
+ }
+ cx.emit(BufferDiffEvent::DiffChanged(result.change));
}
pub fn base_text_buffer(&self) -> &Entity<language::Buffer> {