diff --git a/crates/acp_thread/src/acp_thread.rs b/crates/acp_thread/src/acp_thread.rs index ffffa79bd287672dccc1195061b8b7f4269d8636..9842a9d1203f94a57208383d9eced56bd96582dd 100644 --- a/crates/acp_thread/src/acp_thread.rs +++ b/crates/acp_thread/src/acp_thread.rs @@ -1013,6 +1013,7 @@ struct RunningTurn { send_task: Task<()>, } +#[derive(Clone)] struct InferredEditCandidateReady { nonce: u64, buffer: Entity, @@ -1021,13 +1022,21 @@ struct InferredEditCandidateReady { enum InferredEditCandidateState { Pending { nonce: u64 }, Ready(InferredEditCandidateReady), + Finalizing(InferredEditCandidateReady), } impl InferredEditCandidateState { fn nonce(&self) -> u64 { match self { Self::Pending { nonce } => *nonce, - Self::Ready(candidate) => candidate.nonce, + Self::Ready(candidate) | Self::Finalizing(candidate) => candidate.nonce, + } + } + + fn into_buffer_to_end(self) -> Option> { + match self { + Self::Ready(candidate) | Self::Finalizing(candidate) => Some(candidate.buffer), + Self::Pending { .. } => None, } } } @@ -1744,6 +1753,7 @@ impl AcpThread { fn should_infer_external_edits(tool_call: &ToolCall) -> bool { tool_call.tool_name.is_none() && tool_call.kind == acp::ToolKind::Edit + && tool_call.diffs().next().is_none() && !tool_call.locations.is_empty() } @@ -1758,6 +1768,49 @@ impl AcpThread { nonce } + fn end_expected_external_edits( + &mut self, + buffers: impl IntoIterator>, + cx: &mut Context, + ) { + for buffer in buffers { + self.action_log.update(cx, |action_log, cx| { + action_log.end_expected_external_edit(buffer, cx); + }); + } + } + + fn remove_inferred_edit_tool_call_if_empty(&mut self, tool_call_id: &acp::ToolCallId) { + let remove_tool_call = self + .inferred_edit_candidates + .get(tool_call_id) + .is_some_and(|candidates| candidates.is_empty()); + + if remove_tool_call { + self.inferred_edit_candidates.remove(tool_call_id); + self.finalizing_inferred_edit_tool_calls + .remove(tool_call_id); + } + } + + fn clear_inferred_edit_tool_call_tracking( + &mut self, + tool_call_id: &acp::ToolCallId, + ) -> Vec> { + let buffers_to_end = self + .inferred_edit_candidates + .remove(tool_call_id) + .into_iter() + .flat_map(|candidates| candidates.into_values()) + .filter_map(|candidate_state| candidate_state.into_buffer_to_end()) + .collect::>(); + + self.finalizing_inferred_edit_tool_calls + .remove(tool_call_id); + + buffers_to_end + } + fn remove_inferred_edit_candidate_if_matching( &mut self, tool_call_id: &acp::ToolCallId, @@ -1765,35 +1818,25 @@ impl AcpThread { nonce: u64, cx: &mut Context, ) { - let mut buffer_to_end = None; - let remove_tool_call = + let buffer_to_end = if let Some(candidates) = self.inferred_edit_candidates.get_mut(tool_call_id) { let should_remove = candidates .get(abs_path) .is_some_and(|candidate_state| candidate_state.nonce() == nonce); + if should_remove { - if let Some(InferredEditCandidateState::Ready(candidate)) = - candidates.remove(abs_path) - { - buffer_to_end = Some(candidate.buffer); - } + candidates + .remove(abs_path) + .and_then(|candidate_state| candidate_state.into_buffer_to_end()) + } else { + None } - candidates.is_empty() } else { - false + None }; - if let Some(buffer) = buffer_to_end { - self.action_log.update(cx, |action_log, cx| { - action_log.end_expected_external_edit(buffer, cx); - }); - } - - if remove_tool_call { - self.inferred_edit_candidates.remove(tool_call_id); - self.finalizing_inferred_edit_tool_calls - .remove(tool_call_id); - } + self.end_expected_external_edits(buffer_to_end, cx); + self.remove_inferred_edit_tool_call_if_empty(tool_call_id); } fn clear_inferred_edit_candidates_for_tool_calls( @@ -1804,22 +1847,10 @@ impl AcpThread { let mut buffers_to_end = Vec::new(); for tool_call_id in tool_call_ids { - if let Some(candidates) = self.inferred_edit_candidates.remove(&tool_call_id) { - for candidate_state in candidates.into_values() { - if let InferredEditCandidateState::Ready(candidate) = candidate_state { - buffers_to_end.push(candidate.buffer); - } - } - } - self.finalizing_inferred_edit_tool_calls - .remove(&tool_call_id); + buffers_to_end.extend(self.clear_inferred_edit_tool_call_tracking(&tool_call_id)); } - for buffer in buffers_to_end { - self.action_log.update(cx, |action_log, cx| { - action_log.end_expected_external_edit(buffer, cx); - }); - } + self.end_expected_external_edits(buffers_to_end, cx); } fn finalize_all_inferred_edit_tool_calls(&mut self, cx: &mut Context) { @@ -1850,37 +1881,28 @@ impl AcpThread { current_paths.insert(location.path.clone()); } - let mut buffers_to_end = Vec::new(); - let remove_tool_call = if let Some(candidates) = - self.inferred_edit_candidates.get_mut(tool_call_id) - { - let removed_paths = candidates - .keys() - .filter(|path| !current_paths.contains(*path)) - .cloned() - .collect::>(); - for path in removed_paths { - if let Some(InferredEditCandidateState::Ready(candidate)) = candidates.remove(&path) - { - buffers_to_end.push(candidate.buffer); - } - } - candidates.is_empty() - } else { - false - }; + let buffers_to_end = + if let Some(candidates) = self.inferred_edit_candidates.get_mut(tool_call_id) { + let removed_paths = candidates + .keys() + .filter(|path| !current_paths.contains(*path)) + .cloned() + .collect::>(); - for buffer in buffers_to_end { - self.action_log.update(cx, |action_log, cx| { - action_log.end_expected_external_edit(buffer, cx); - }); - } + removed_paths + .into_iter() + .filter_map(|path| { + candidates + .remove(&path) + .and_then(|candidate_state| candidate_state.into_buffer_to_end()) + }) + .collect::>() + } else { + Vec::new() + }; - if remove_tool_call { - self.inferred_edit_candidates.remove(tool_call_id); - self.finalizing_inferred_edit_tool_calls - .remove(tool_call_id); - } + self.end_expected_external_edits(buffers_to_end, cx); + self.remove_inferred_edit_tool_call_if_empty(tool_call_id); } fn register_inferred_edit_locations( @@ -1995,102 +2017,115 @@ impl AcpThread { buffer: Entity, cx: &mut Context, ) { - let Some(candidates) = self.inferred_edit_candidates.get_mut(&tool_call_id) else { - return; - }; - let Some(candidate_state) = candidates.get_mut(&abs_path) else { - return; - }; - if candidate_state.nonce() != nonce { - return; - } + let buffer_for_action_log = { + let Some(candidates) = self.inferred_edit_candidates.get_mut(&tool_call_id) else { + return; + }; + let Some(candidate_state) = candidates.get_mut(&abs_path) else { + return; + }; + if candidate_state.nonce() != nonce { + return; + } - let buffer_for_action_log = buffer.clone(); - *candidate_state = - InferredEditCandidateState::Ready(InferredEditCandidateReady { nonce, buffer }); + *candidate_state = InferredEditCandidateState::Ready(InferredEditCandidateReady { + nonce, + buffer: buffer.clone(), + }); + buffer + }; self.action_log.update(cx, |action_log, cx| { action_log.begin_expected_external_edit(buffer_for_action_log, cx); }); + + if self + .finalizing_inferred_edit_tool_calls + .contains(&tool_call_id) + { + self.start_finalizing_ready_inferred_edit_candidates(tool_call_id, cx); + } } - fn finalize_inferred_edit_tool_call( + fn start_finalizing_ready_inferred_edit_candidates( &mut self, tool_call_id: acp::ToolCallId, cx: &mut Context, ) { - let should_finalize = self.tool_call(&tool_call_id).is_some_and(|(_, tool_call)| { - Self::should_infer_external_edits(tool_call) - && Self::is_inferred_edit_terminal_status(&tool_call.status) - }); - if !should_finalize { - self.clear_inferred_edit_candidates_for_tool_calls([tool_call_id], cx); - return; - } - - if !self - .finalizing_inferred_edit_tool_calls - .insert(tool_call_id.clone()) - { - return; - } - - let project = self.project.clone(); - cx.spawn::<_, anyhow::Result<()>>(async move |this, cx| { - const MAX_ATTEMPTS: usize = 3; - const ATTEMPT_DELAY: Duration = Duration::from_millis(50); - - for attempt in 0..MAX_ATTEMPTS { - let (ready_buffers, has_pending) = this - .read_with(cx, |this, _| { - let Some(candidates) = this.inferred_edit_candidates.get(&tool_call_id) - else { - return (Vec::new(), false); - }; - - let mut ready_buffers = Vec::new(); - let mut has_pending = false; - - for candidate_state in candidates.values() { - match candidate_state { - InferredEditCandidateState::Pending { .. } => has_pending = true, - InferredEditCandidateState::Ready(candidate) => { - ready_buffers.push(candidate.buffer.clone()); - } - } + let ready_candidates = + if let Some(candidates) = self.inferred_edit_candidates.get_mut(&tool_call_id) { + let ready_candidates = candidates + .iter() + .filter_map(|(abs_path, candidate_state)| match candidate_state { + InferredEditCandidateState::Ready(candidate) => { + Some((abs_path.clone(), candidate.clone())) } - - (ready_buffers, has_pending) + InferredEditCandidateState::Pending { .. } + | InferredEditCandidateState::Finalizing(_) => None, }) - .unwrap_or((Vec::new(), false)); - - if ready_buffers.is_empty() && !has_pending { - break; - } + .collect::>(); - for buffer in ready_buffers { - let should_reload = buffer.read_with(cx, |buffer, _| !buffer.is_dirty()); - if !should_reload { + for (abs_path, candidate) in &ready_candidates { + let Some(candidate_state) = candidates.get_mut(abs_path) else { + continue; + }; + if candidate_state.nonce() != candidate.nonce { continue; } - let reload = project.update(cx, |project, cx| { - let mut buffers = HashSet::default(); - buffers.insert(buffer.clone()); - project.reload_buffers(buffers, false, cx) - }); - reload.await.log_err(); + *candidate_state = InferredEditCandidateState::Finalizing(candidate.clone()); } - if !has_pending || attempt + 1 == MAX_ATTEMPTS { - break; - } + ready_candidates + } else { + Vec::new() + }; - cx.background_executor().timer(ATTEMPT_DELAY).await; - } + for (abs_path, candidate) in ready_candidates { + self.finalize_inferred_edit_candidate(tool_call_id.clone(), abs_path, candidate, cx); + } + + if !self.inferred_edit_candidates.contains_key(&tool_call_id) { + self.finalizing_inferred_edit_tool_calls + .remove(&tool_call_id); + } + } + + fn finalize_inferred_edit_candidate( + &mut self, + tool_call_id: acp::ToolCallId, + abs_path: PathBuf, + candidate: InferredEditCandidateReady, + cx: &mut Context, + ) { + let nonce = candidate.nonce; + let buffer = candidate.buffer; + let should_reload = buffer.read_with(cx, |buffer, _| !buffer.is_dirty()); + if !should_reload { + self.remove_inferred_edit_candidate_if_matching(&tool_call_id, &abs_path, nonce, cx); + return; + } + + self.action_log.update(cx, |action_log, cx| { + action_log.arm_expected_external_reload(buffer.clone(), cx); + }); + + let project = self.project.clone(); + cx.spawn::<_, anyhow::Result<()>>(async move |this, cx| { + let reload = project.update(cx, |project, cx| { + let mut buffers = HashSet::default(); + buffers.insert(buffer.clone()); + project.reload_buffers(buffers, false, cx) + }); + reload.await.log_err(); this.update(cx, |this, cx| { - this.clear_inferred_edit_candidates_for_tool_calls([tool_call_id.clone()], cx); + this.remove_inferred_edit_candidate_if_matching( + &tool_call_id, + &abs_path, + nonce, + cx, + ); }) .ok(); @@ -2099,6 +2134,25 @@ impl AcpThread { .detach_and_log_err(cx); } + fn finalize_inferred_edit_tool_call( + &mut self, + tool_call_id: acp::ToolCallId, + cx: &mut Context, + ) { + let should_finalize = self.tool_call(&tool_call_id).is_some_and(|(_, tool_call)| { + Self::should_infer_external_edits(tool_call) + && Self::is_inferred_edit_terminal_status(&tool_call.status) + }); + if !should_finalize { + self.clear_inferred_edit_candidates_for_tool_calls([tool_call_id], cx); + return; + } + + self.finalizing_inferred_edit_tool_calls + .insert(tool_call_id.clone()); + self.start_finalizing_ready_inferred_edit_candidates(tool_call_id, cx); + } + fn refresh_inferred_edit_tool_call( &mut self, tool_call_id: acp::ToolCallId, @@ -4450,19 +4504,30 @@ mod tests { tool_call_id: &acp::ToolCallId, locations: Vec, cx: &mut TestAppContext, + ) { + start_external_edit_tool_call_with_meta(thread, tool_call_id, locations, None, cx); + } + + fn start_external_edit_tool_call_with_meta( + thread: &Entity, + tool_call_id: &acp::ToolCallId, + locations: Vec, + meta: Option, + cx: &mut TestAppContext, ) { let tool_call_id = tool_call_id.clone(); thread .update(cx, move |thread, cx| { - thread.handle_session_update( - acp::SessionUpdate::ToolCall( - acp::ToolCall::new(tool_call_id, "Label") - .kind(acp::ToolKind::Edit) - .status(acp::ToolCallStatus::InProgress) - .locations(locations), - ), - cx, - ) + let mut tool_call = acp::ToolCall::new(tool_call_id, "Label") + .kind(acp::ToolKind::Edit) + .status(acp::ToolCallStatus::InProgress) + .locations(locations); + + if let Some(meta) = meta { + tool_call = tool_call.meta(meta); + } + + thread.handle_session_update(acp::SessionUpdate::ToolCall(tool_call), cx) }) .unwrap(); } @@ -4521,6 +4586,18 @@ mod tests { }) } + fn inferred_edit_tool_call_is_finalizing( + thread: &Entity, + tool_call_id: &acp::ToolCallId, + cx: &TestAppContext, + ) -> bool { + thread.read_with(cx, |thread, _| { + thread + .finalizing_inferred_edit_tool_calls + .contains(tool_call_id) + }) + } + fn inferred_edit_candidate_is_ready( thread: &Entity, tool_call_id: &acp::ToolCallId, @@ -4808,6 +4885,213 @@ mod tests { assert_eq!(changed_buffer_count(&thread, cx), 0); } + #[gpui::test] + async fn test_terminal_external_edit_candidates_remain_active_until_late_readiness( + cx: &mut TestAppContext, + ) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree(path!("/test"), json!({"file.txt": "one\ntwo\n"})) + .await; + let project = Project::test(fs.clone(), [path!("/test").as_ref()], cx).await; + let connection = Rc::new(FakeAgentConnection::new()); + + let thread = cx + .update(|cx| { + connection.new_session( + project.clone(), + PathList::new(&[Path::new(path!("/test"))]), + cx, + ) + }) + .await + .unwrap(); + + let abs_path = PathBuf::from(path!("/test/file.txt")); + let buffer = open_test_buffer(&project, Path::new(path!("/test/file.txt")), cx).await; + + let tool_call_id = acp::ToolCallId::new("test"); + start_external_edit_tool_call(&thread, &tool_call_id, Vec::new(), cx); + + let nonce = thread.update(cx, |thread, _cx| { + let nonce = thread.allocate_inferred_edit_candidate_nonce(); + { + let (_, tool_call) = thread.tool_call_mut(&tool_call_id).unwrap(); + tool_call.locations = vec![acp::ToolCallLocation::new(abs_path.clone())]; + tool_call.resolved_locations = vec![None]; + } + thread + .inferred_edit_candidates + .entry(tool_call_id.clone()) + .or_default() + .insert( + abs_path.clone(), + InferredEditCandidateState::Pending { nonce }, + ); + nonce + }); + + complete_external_edit_tool_call(&thread, &tool_call_id, cx); + cx.executor().advance_clock(Duration::from_millis(200)); + cx.run_until_parked(); + + assert_eq!(inferred_edit_candidate_count(&thread, cx), 1); + assert!(inferred_edit_tool_call_is_finalizing( + &thread, + &tool_call_id, + cx + )); + + thread.update(cx, |thread, cx| { + thread.set_inferred_edit_candidate_ready( + tool_call_id.clone(), + abs_path.clone(), + nonce, + buffer.clone(), + cx, + ); + }); + cx.run_until_parked(); + + assert_eq!(changed_buffer_count(&thread, cx), 0); + assert_eq!(inferred_edit_candidate_count(&thread, cx), 0); + assert!(!inferred_edit_tool_call_is_finalizing( + &thread, + &tool_call_id, + cx + )); + } + + #[gpui::test] + async fn test_tool_name_disables_external_edit_inference_for_location_edit_calls( + cx: &mut TestAppContext, + ) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree(path!("/test"), json!({"file.txt": "one\ntwo\n"})) + .await; + let project = Project::test(fs.clone(), [path!("/test").as_ref()], cx).await; + let connection = Rc::new(FakeAgentConnection::new()); + + let thread = cx + .update(|cx| { + connection.new_session( + project.clone(), + PathList::new(&[Path::new(path!("/test"))]), + cx, + ) + }) + .await + .unwrap(); + + let tool_call_id = acp::ToolCallId::new("test"); + start_external_edit_tool_call_with_meta( + &thread, + &tool_call_id, + vec![acp::ToolCallLocation::new(PathBuf::from(path!( + "/test/file.txt" + )))], + Some(meta_with_tool_name("edit_file")), + cx, + ); + cx.run_until_parked(); + + assert_eq!(inferred_edit_candidate_count(&thread, cx), 0); + assert!(!cx.read(|cx| thread.read(cx).has_pending_edit_tool_calls())); + + fs.save( + path!("/test/file.txt").as_ref(), + &"one\ntwo\nthree\n".into(), + Default::default(), + ) + .await + .unwrap(); + cx.run_until_parked(); + + complete_external_edit_tool_call(&thread, &tool_call_id, cx); + cx.executor().advance_clock(Duration::from_millis(200)); + cx.run_until_parked(); + + assert_eq!(changed_buffer_count(&thread, cx), 0); + } + + #[gpui::test] + async fn test_explicit_diff_content_disables_external_edit_inference_for_location_edit_calls( + cx: &mut TestAppContext, + ) { + init_test(cx); + let fs = FakeFs::new(cx.background_executor.clone()); + fs.insert_tree(path!("/test"), json!({"file.txt": "one\ntwo\n"})) + .await; + let project = Project::test(fs.clone(), [path!("/test").as_ref()], cx).await; + let connection = Rc::new(FakeAgentConnection::new()); + + let thread = cx + .update(|cx| { + connection.new_session( + project.clone(), + PathList::new(&[Path::new(path!("/test"))]), + cx, + ) + }) + .await + .unwrap(); + + let abs_path = PathBuf::from(path!("/test/file.txt")); + let tool_call_id = acp::ToolCallId::new("test"); + start_external_edit_tool_call( + &thread, + &tool_call_id, + vec![acp::ToolCallLocation::new(abs_path.clone())], + cx, + ); + cx.run_until_parked(); + + assert_eq!(inferred_edit_candidate_count(&thread, cx), 1); + + let languages = project.read_with(cx, |project, _| project.languages().clone()); + let diff = cx.new(|cx| { + Diff::finalized( + path!("/test/file.txt").to_string(), + Some("one\ntwo\n".into()), + "one\ntwo\nthree\n".into(), + languages, + cx, + ) + }); + + thread + .update(cx, |thread, cx| { + thread.update_tool_call( + ToolCallUpdateDiff { + id: tool_call_id.clone(), + diff, + }, + cx, + ) + }) + .unwrap(); + cx.run_until_parked(); + + assert_eq!(inferred_edit_candidate_count(&thread, cx), 0); + assert!(cx.read(|cx| thread.read(cx).has_pending_edit_tool_calls())); + + fs.save( + path!("/test/file.txt").as_ref(), + &"one\ntwo\nthree\n".into(), + Default::default(), + ) + .await + .unwrap(); + cx.run_until_parked(); + + complete_external_edit_tool_call(&thread, &tool_call_id, cx); + cx.executor().advance_clock(Duration::from_millis(200)); + cx.run_until_parked(); + + assert_eq!(changed_buffer_count(&thread, cx), 0); + } + #[gpui::test(iterations = 10)] async fn test_checkpoints(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/action_log/src/action_log.rs b/crates/action_log/src/action_log.rs index 219598cfe0b58626ab72afb3a92dba1501ae1f7e..8fee58ea62725b71bb7f4d2bc3b12ea7abcc790b 100644 --- a/crates/action_log/src/action_log.rs +++ b/crates/action_log/src/action_log.rs @@ -280,15 +280,10 @@ impl ActionLog { event: &BufferEvent, cx: &mut Context, ) -> bool { - let Some((_, expected_external_edit)) = - self.tracked_buffers - .get(&buffer) - .and_then(|tracked_buffer| { - tracked_buffer - .expected_external_edit - .clone() - .map(|expected_external_edit| (tracked_buffer.mode, expected_external_edit)) - }) + let Some(expected_external_edit) = self + .tracked_buffers + .get(&buffer) + .and_then(|tracked_buffer| tracked_buffer.expected_external_edit.clone()) else { return false; }; @@ -299,7 +294,8 @@ impl ActionLog { match event { BufferEvent::Saved - if expected_external_edit.observed_external_file_change + if (expected_external_edit.observed_external_file_change + || expected_external_edit.armed_explicit_reload) && !expected_external_edit.has_attributed_change => { self.mark_expected_external_edit_disqualified(&buffer); @@ -322,7 +318,11 @@ impl ActionLog { } } + // Reload applies its text changes through ordinary local edit events before + // emitting `Reloaded`, so an explicitly armed reload must suppress those edits + // to preserve the pre-reload baseline for attribution. expected_external_edit.observed_external_file_change + || expected_external_edit.armed_explicit_reload } BufferEvent::FileHandleChanged => { let (is_deleted, is_empty, is_dirty) = buffer.read_with(cx, |buffer, _| { @@ -341,7 +341,14 @@ impl ActionLog { { if !is_dirty || is_deleted { expected_external_edit.observed_external_file_change = true; + expected_external_edit.armed_explicit_reload = false; } + // Non-delete external changes against dirty buffers stay unsupported for now. + // We do not mark them as observed here, so they are not automatically + // remembered for attribution once the buffer becomes clean. Later + // attribution only happens after a subsequent clean file change or an + // explicitly armed reload, which keeps conflicted reloads and local-save + // noise from becoming agent edits. expected_external_edit.pending_delete = is_deleted; } } @@ -360,7 +367,8 @@ impl ActionLog { true } BufferEvent::Reloaded - if expected_external_edit.observed_external_file_change + if (expected_external_edit.observed_external_file_change + || expected_external_edit.armed_explicit_reload) && !expected_external_edit.pending_delete => { if self.expected_external_edit_has_meaningful_change(&buffer, cx) { @@ -370,6 +378,7 @@ impl ActionLog { tracked_buffer.expected_external_edit.as_mut() { expected_external_edit.observed_external_file_change = false; + expected_external_edit.armed_explicit_reload = false; expected_external_edit.pending_delete = false; } } @@ -390,6 +399,7 @@ impl ActionLog { expected_external_edit.is_disqualified = true; expected_external_edit.observed_external_file_change = false; + expected_external_edit.armed_explicit_reload = false; expected_external_edit.pending_delete = false; } @@ -454,6 +464,7 @@ impl ActionLog { expected_external_edit.has_attributed_change = true; expected_external_edit.observed_external_file_change = false; + expected_external_edit.armed_explicit_reload = false; expected_external_edit.pending_delete = false; tracked_buffer.mode = TrackedBufferMode::Normal; @@ -506,6 +517,7 @@ impl ActionLog { expected_external_edit.has_attributed_change = true; expected_external_edit.observed_external_file_change = false; + expected_external_edit.armed_explicit_reload = false; expected_external_edit.pending_delete = false; tracked_buffer.mode = TrackedBufferMode::Normal; tracked_buffer.status = TrackedBufferStatus::Deleted; @@ -932,6 +944,7 @@ impl ActionLog { record_file_read_time_source_count: 0, initial_exists_on_disk, observed_external_file_change: false, + armed_explicit_reload: false, has_attributed_change, pending_delete: false, is_disqualified: false, @@ -942,6 +955,39 @@ impl ActionLog { } } + pub fn arm_expected_external_reload(&mut self, buffer: Entity, cx: &mut Context) { + self.arm_expected_external_reload_impl(buffer, true, cx); + } + + fn arm_expected_external_reload_impl( + &mut self, + buffer: Entity, + forward_to_linked_action_log: bool, + cx: &mut Context, + ) { + if forward_to_linked_action_log { + if let Some(linked_action_log) = &self.linked_action_log { + linked_action_log.update(cx, |log, cx| { + log.arm_expected_external_reload_impl(buffer.clone(), false, cx); + }); + } + } + + let Some(tracked_buffer) = self.tracked_buffers.get_mut(&buffer) else { + return; + }; + let Some(expected_external_edit) = tracked_buffer.expected_external_edit.as_mut() else { + return; + }; + if expected_external_edit.is_disqualified || expected_external_edit.pending_delete { + return; + } + + // Explicit reloads can observe on-disk contents before the worktree has delivered + // `FileHandleChanged`, so we arm the next reload for attribution ahead of time. + expected_external_edit.armed_explicit_reload = true; + } + pub fn end_expected_external_edit(&mut self, buffer: Entity, cx: &mut Context) { self.end_expected_external_edit_impl(buffer, true, cx); } @@ -1000,139 +1046,104 @@ impl ActionLog { baseline_snapshot: text::BufferSnapshot, cx: &mut Context, ) { - self.infer_buffer_created_impl(buffer, baseline_snapshot, true, cx); + self.infer_buffer_from_snapshot_impl( + buffer, + baseline_snapshot, + InferredSnapshotKind::Created, + true, + cx, + ); } - fn infer_buffer_created_impl( + pub fn infer_buffer_edited_from_snapshot( &mut self, buffer: Entity, baseline_snapshot: text::BufferSnapshot, - record_file_read_time: bool, cx: &mut Context, ) { - if let Some(linked_action_log) = &self.linked_action_log { - let linked_baseline_snapshot = baseline_snapshot.clone(); - if !linked_action_log.read(cx).has_changed_buffer(&buffer, cx) { - linked_action_log.update(cx, |log, cx| { - log.infer_buffer_created_impl( - buffer.clone(), - linked_baseline_snapshot, - false, - cx, - ); - }); - } - } - - if record_file_read_time { - self.update_file_read_time(&buffer, cx); - } - self.prime_tracked_buffer_from_snapshot( - buffer.clone(), + self.infer_buffer_from_snapshot_impl( + buffer, baseline_snapshot, - TrackedBufferStatus::Created { - existing_file_content: None, - }, + InferredSnapshotKind::Edited, + true, cx, ); - - if let Some(tracked_buffer) = self.tracked_buffers.get(&buffer) { - tracked_buffer.schedule_diff_update(ChangeAuthor::Agent, cx); - } } - pub fn infer_buffer_edited_from_snapshot( + pub fn infer_buffer_deleted_from_snapshot( &mut self, buffer: Entity, baseline_snapshot: text::BufferSnapshot, cx: &mut Context, ) { - self.infer_buffer_edited_from_snapshot_impl(buffer, baseline_snapshot, true, cx); + self.infer_buffer_from_snapshot_impl( + buffer, + baseline_snapshot, + InferredSnapshotKind::Deleted, + true, + cx, + ); } - fn infer_buffer_edited_from_snapshot_impl( + fn forward_inferred_snapshot_to_linked_action_log( &mut self, - buffer: Entity, - baseline_snapshot: text::BufferSnapshot, - record_file_read_time: bool, + buffer: &Entity, + baseline_snapshot: &text::BufferSnapshot, + kind: InferredSnapshotKind, cx: &mut Context, ) { if let Some(linked_action_log) = &self.linked_action_log { let linked_baseline_snapshot = baseline_snapshot.clone(); - if !linked_action_log.read(cx).has_changed_buffer(&buffer, cx) { + if !linked_action_log.read(cx).has_changed_buffer(buffer, cx) { linked_action_log.update(cx, |log, cx| { - log.infer_buffer_edited_from_snapshot_impl( + log.infer_buffer_from_snapshot_impl( buffer.clone(), linked_baseline_snapshot, + kind, false, cx, ); }); } } - - if record_file_read_time { - self.update_file_read_time(&buffer, cx); - } - self.prime_tracked_buffer_from_snapshot( - buffer.clone(), - baseline_snapshot, - TrackedBufferStatus::Modified, - cx, - ); - - if let Some(tracked_buffer) = self.tracked_buffers.get(&buffer) { - tracked_buffer.schedule_diff_update(ChangeAuthor::Agent, cx); - } } - pub fn infer_buffer_deleted_from_snapshot( - &mut self, - buffer: Entity, - baseline_snapshot: text::BufferSnapshot, - cx: &mut Context, - ) { - self.infer_buffer_deleted_from_snapshot_impl(buffer, baseline_snapshot, true, cx); - } - - fn infer_buffer_deleted_from_snapshot_impl( + fn infer_buffer_from_snapshot_impl( &mut self, buffer: Entity, baseline_snapshot: text::BufferSnapshot, + kind: InferredSnapshotKind, record_file_read_time: bool, cx: &mut Context, ) { - if let Some(linked_action_log) = &self.linked_action_log { - let linked_baseline_snapshot = baseline_snapshot.clone(); - if !linked_action_log.read(cx).has_changed_buffer(&buffer, cx) { - linked_action_log.update(cx, |log, cx| { - log.infer_buffer_deleted_from_snapshot_impl( - buffer.clone(), - linked_baseline_snapshot, - false, - cx, - ); - }); - } - } + self.forward_inferred_snapshot_to_linked_action_log(&buffer, &baseline_snapshot, kind, cx); if record_file_read_time { - self.remove_file_read_time(&buffer, cx); + match kind { + InferredSnapshotKind::Created | InferredSnapshotKind::Edited => { + self.update_file_read_time(&buffer, cx); + } + InferredSnapshotKind::Deleted => { + self.remove_file_read_time(&buffer, cx); + } + } } - let has_linked_action_log = self.linked_action_log.is_some(); + self.prime_tracked_buffer_from_snapshot( buffer.clone(), baseline_snapshot, - TrackedBufferStatus::Deleted, + kind.tracked_buffer_status(), cx, ); - if !has_linked_action_log { + if kind == InferredSnapshotKind::Deleted && self.linked_action_log.is_none() { buffer.update(cx, |buffer, cx| buffer.set_text("", cx)); } if let Some(tracked_buffer) = self.tracked_buffers.get_mut(&buffer) { - tracked_buffer.version = buffer.read(cx).version(); + if kind == InferredSnapshotKind::Deleted { + tracked_buffer.version = buffer.read(cx).version(); + } tracked_buffer.schedule_diff_update(ChangeAuthor::Agent, cx); } } @@ -1799,6 +1810,25 @@ enum ChangeAuthor { Agent, } +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +enum InferredSnapshotKind { + Created, + Edited, + Deleted, +} + +impl InferredSnapshotKind { + fn tracked_buffer_status(self) -> TrackedBufferStatus { + match self { + Self::Created => TrackedBufferStatus::Created { + existing_file_content: None, + }, + Self::Edited => TrackedBufferStatus::Modified, + Self::Deleted => TrackedBufferStatus::Deleted, + } + } +} + #[derive(Copy, Clone, Debug, PartialEq, Eq)] enum TrackedBufferMode { Normal, @@ -1811,6 +1841,7 @@ struct ExpectedExternalEdit { record_file_read_time_source_count: usize, initial_exists_on_disk: bool, observed_external_file_change: bool, + armed_explicit_reload: bool, has_attributed_change: bool, pending_delete: bool, is_disqualified: bool, @@ -4283,6 +4314,117 @@ mod tests { ); } + #[gpui::test] + async fn test_expected_external_edit_explicit_reload_arm_attributes_forced_reload( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/dir"), json!({"file": "one\ntwo\n"})) + .await; + let project = Project::test(fs.clone(), [path!("/dir").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("dir/file", cx)) + .unwrap(); + let buffer = project + .update(cx, |project, cx| project.open_buffer(file_path, cx)) + .await + .unwrap(); + + cx.update(|cx| { + action_log.update(cx, |log, cx| { + log.begin_expected_external_edit(buffer.clone(), cx); + }); + }); + + fs.save( + path!("/dir/file").as_ref(), + &"one\ntwo\nthree\n".into(), + Default::default(), + ) + .await + .unwrap(); + + cx.update(|cx| { + action_log.update(cx, |log, cx| { + log.arm_expected_external_reload(buffer.clone(), cx); + }); + }); + + let reload = project.update(cx, |project, cx| { + let mut buffers = collections::HashSet::default(); + buffers.insert(buffer.clone()); + project.reload_buffers(buffers, false, cx) + }); + reload.await.unwrap(); + cx.run_until_parked(); + + assert_eq!( + action_log.read_with(cx, |log, cx| log.changed_buffers(cx).len()), + 1, + "arming an expected reload should attribute an explicit reload before file-handle updates arrive" + ); + } + + #[gpui::test] + async fn test_expected_external_edit_does_not_attribute_dirty_non_delete_external_changes( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let fs = FakeFs::new(cx.executor()); + fs.insert_tree(path!("/dir"), json!({"file": "one\ntwo\n"})) + .await; + let project = Project::test(fs.clone(), [path!("/dir").as_ref()], cx).await; + let action_log = cx.new(|_| ActionLog::new(project.clone())); + let abs_path = PathBuf::from(path!("/dir/file")); + + let file_path = project + .read_with(cx, |project, cx| project.find_project_path("dir/file", cx)) + .unwrap(); + let buffer = project + .update(cx, |project, cx| project.open_buffer(file_path, cx)) + .await + .unwrap(); + + cx.update(|cx| { + action_log.update(cx, |log, cx| { + log.begin_expected_external_edit(buffer.clone(), cx); + }); + }); + + buffer.update(cx, |buffer, cx| { + buffer.edit([(0..0, "zero\n")], None, cx).unwrap(); + }); + cx.run_until_parked(); + + fs.save( + path!("/dir/file").as_ref(), + &"one\ntwo\nthree\n".into(), + Default::default(), + ) + .await + .unwrap(); + cx.run_until_parked(); + + assert!( + action_log.read_with(cx, |log, cx| log.changed_buffers(cx).is_empty()), + "dirty non-delete external changes should stay out of review until the behavior is explicitly supported" + ); + assert!( + action_log.read_with(cx, |log, _| log.file_read_time(&abs_path).is_none()), + "unsupported dirty external changes should not record file_read_time" + ); + assert_eq!( + buffer.read_with(cx, |buffer, _| buffer.text()), + "zero\none\ntwo\n", + "unsupported dirty external changes should preserve local buffer contents" + ); + } + #[gpui::test] async fn test_linked_expected_external_edit_tracks_review_without_parent_file_read_time( cx: &mut TestAppContext,