agent: Detect overlapping edits in StreamingEditFileTool (#50181)
Release Notes: - N/A
This commit is contained in:
Bennet Bo Fenner authored and GitHub committed
1 parent
83398d2863
commit
3372e25a2c
1 file changed
+195
-85
@@ -21,7 +21,7 @@ use serde::{Deserialize, Serialize};
|
||||
use std::ops::Range;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use text::BufferSnapshot;
|
||||
use text::{BufferSnapshot, ToOffset as _};
|
||||
use ui::SharedString;
|
||||
use util::rel_path::RelPath;
|
||||
use util::{Deferred, ResultExt, debug_panic};
|
||||
@@ -146,9 +146,15 @@ enum StreamingEditState {
|
||||
|
||||
#[derive(Default)]
|
||||
struct IncrementalEditState {
|
||||
applied_count: usize,
|
||||
in_progress_matcher: Option<StreamingFuzzyMatcher>,
|
||||
last_old_text_len: usize,
|
||||
applied_ranges: Vec<Range<text::Anchor>>,
|
||||
}
|
||||
|
||||
impl IncrementalEditState {
|
||||
fn applied_count(&self) -> usize {
|
||||
self.applied_ranges.len()
|
||||
}
|
||||
}
|
||||
|
||||
impl StreamingEditState {
|
||||
@@ -172,7 +178,7 @@ impl StreamingEditState {
|
||||
.await?;
|
||||
0
|
||||
}
|
||||
StreamingEditState::BufferResolved { edit_state, .. } => edit_state.applied_count,
|
||||
StreamingEditState::BufferResolved { edit_state, .. } => edit_state.applied_count(),
|
||||
};
|
||||
|
||||
let StreamingEditState::BufferResolved {
|
||||
@@ -449,17 +455,15 @@ impl StreamingEditState {
|
||||
let completed_count = edits.len().saturating_sub(1);
|
||||
|
||||
// Apply newly-complete edits
|
||||
while edit_state.applied_count < completed_count {
|
||||
let edit_index = edit_state.applied_count;
|
||||
while edit_state.applied_count() < completed_count {
|
||||
let edit_index = edit_state.applied_count();
|
||||
let partial_edit = &edits[edit_index];
|
||||
|
||||
let old_text = match &partial_edit.old_text {
|
||||
Some(t) => t.clone(),
|
||||
None => {
|
||||
edit_state.applied_count += 1;
|
||||
continue;
|
||||
let old_text = partial_edit.old_text.clone().ok_or_else(|| {
|
||||
StreamingEditFileToolOutput::Error {
|
||||
error: format!("Edit at index {} is missing old_text.", edit_index),
|
||||
}
|
||||
};
|
||||
})?;
|
||||
let new_text = partial_edit.new_text.clone().unwrap_or_default();
|
||||
|
||||
edit_state.in_progress_matcher = None;
|
||||
@@ -476,7 +480,7 @@ impl StreamingEditState {
|
||||
.ok();
|
||||
|
||||
// On the first edit, mark the buffer as read
|
||||
if edit_state.applied_count == 0 {
|
||||
if edit_state.applied_count() == 0 {
|
||||
if let Some(action_log) = &action_log {
|
||||
action_log.update(cx, |log, cx| {
|
||||
log.buffer_read(buffer.clone(), cx);
|
||||
@@ -484,21 +488,78 @@ impl StreamingEditState {
|
||||
}
|
||||
}
|
||||
|
||||
resolve_reveal_and_apply_edit(
|
||||
buffer,
|
||||
diff,
|
||||
&edit_op,
|
||||
edit_index,
|
||||
abs_path,
|
||||
action_log.as_ref(),
|
||||
event_stream,
|
||||
cx,
|
||||
)
|
||||
.map_err(|e| StreamingEditFileToolOutput::Error {
|
||||
error: e.to_string(),
|
||||
})?;
|
||||
let snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot());
|
||||
|
||||
edit_state.applied_count += 1;
|
||||
let (range, new_text) =
|
||||
match resolve_and_reveal_edit(buffer, diff, &snapshot, &edit_op, cx) {
|
||||
Ok(resolved) => resolved,
|
||||
Err(EditResolveError::NotFound) => {
|
||||
return Err(StreamingEditFileToolOutput::Error {
|
||||
error: format!(
|
||||
"Could not find matching text for edit at index {}. \
|
||||
The old_text did not match any content in the file. \
|
||||
Please read the file again to get the current content.",
|
||||
edit_index
|
||||
),
|
||||
});
|
||||
}
|
||||
Err(EditResolveError::Ambiguous(ranges)) => {
|
||||
let lines = ranges
|
||||
.iter()
|
||||
.map(|r| (snapshot.offset_to_point(r.start).row + 1).to_string())
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
return Err(StreamingEditFileToolOutput::Error {
|
||||
error: format!(
|
||||
"Edit {} matched multiple locations in the file at lines: {}. \
|
||||
Please provide more context in old_text to uniquely \
|
||||
identify the location.",
|
||||
edit_index, lines
|
||||
),
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
for previous_range in &edit_state.applied_ranges {
|
||||
let previous_start = previous_range.start.to_offset(&snapshot);
|
||||
let previous_end = previous_range.end.to_offset(&snapshot);
|
||||
if range.start < previous_end && previous_start < range.end {
|
||||
let earlier_start_line = snapshot.offset_to_point(previous_start).row + 1;
|
||||
let earlier_end_line = snapshot.offset_to_point(previous_end).row + 1;
|
||||
let later_start_line = snapshot.offset_to_point(range.start).row + 1;
|
||||
let later_end_line = snapshot.offset_to_point(range.end).row + 1;
|
||||
return Err(StreamingEditFileToolOutput::Error {
|
||||
error: format!(
|
||||
"Conflicting edit ranges detected: lines {}-{} \
|
||||
conflicts with lines {}-{}. Conflicting edit \
|
||||
ranges are not allowed, as they would overwrite \
|
||||
each other.",
|
||||
earlier_start_line, earlier_end_line, later_start_line, later_end_line,
|
||||
),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
let anchor_range =
|
||||
buffer.read_with(cx, |buffer, _cx| buffer.anchor_range_between(range.clone()));
|
||||
edit_state.applied_ranges.push(anchor_range);
|
||||
|
||||
let line = snapshot.offset_to_point(range.start).row;
|
||||
event_stream.update_fields(
|
||||
ToolCallUpdateFields::new()
|
||||
.locations(vec![ToolCallLocation::new(abs_path).line(Some(line))]),
|
||||
);
|
||||
|
||||
if let Some(action_log) = action_log {
|
||||
cx.update(|cx| {
|
||||
buffer.update(cx, |buffer, cx| {
|
||||
buffer.edit([(range, new_text.as_str())], None, cx);
|
||||
});
|
||||
action_log.update(cx, |log, cx| {
|
||||
log.buffer_edited(buffer.clone(), cx);
|
||||
});
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
// Feed the in-progress last edit's old_text to the matcher for live preview
|
||||
@@ -912,65 +973,6 @@ fn apply_edits(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Resolves, reveals, and applies a single edit to the buffer. Emits
|
||||
/// a location update and reports the change to the action log.
|
||||
fn resolve_reveal_and_apply_edit(
|
||||
buffer: &Entity<Buffer>,
|
||||
diff: &Entity<Diff>,
|
||||
edit: &EditOperation,
|
||||
edit_index: usize,
|
||||
abs_path: &PathBuf,
|
||||
action_log: Option<&Entity<action_log::ActionLog>>,
|
||||
event_stream: &ToolCallEventStream,
|
||||
cx: &mut AsyncApp,
|
||||
) -> Result<()> {
|
||||
let snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot());
|
||||
|
||||
match resolve_and_reveal_edit(buffer, diff, &snapshot, edit, cx) {
|
||||
Ok((range, new_text)) => {
|
||||
let line = snapshot.offset_to_point(range.start).row;
|
||||
event_stream.update_fields(
|
||||
ToolCallUpdateFields::new()
|
||||
.locations(vec![ToolCallLocation::new(abs_path).line(Some(line))]),
|
||||
);
|
||||
|
||||
if let Some(action_log) = action_log {
|
||||
cx.update(|cx| {
|
||||
buffer.update(cx, |buffer, cx| {
|
||||
buffer.edit([(range, new_text.as_str())], None, cx);
|
||||
});
|
||||
action_log.update(cx, |log, cx| {
|
||||
log.buffer_edited(buffer.clone(), cx);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
Err(EditResolveError::NotFound) => {
|
||||
anyhow::bail!(
|
||||
"Could not find matching text for edit at index {}. \
|
||||
The old_text did not match any content in the file. \
|
||||
Please read the file again to get the current content.",
|
||||
edit_index
|
||||
);
|
||||
}
|
||||
Err(EditResolveError::Ambiguous(ranges)) => {
|
||||
let lines = ranges
|
||||
.iter()
|
||||
.map(|r| (snapshot.offset_to_point(r.start).row + 1).to_string())
|
||||
.collect::<Vec<_>>()
|
||||
.join(", ");
|
||||
anyhow::bail!(
|
||||
"Edit {} matched multiple locations in the file at lines: {}. \
|
||||
Please provide more context in old_text to uniquely identify the location.",
|
||||
edit_index,
|
||||
lines
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
enum EditResolveError {
|
||||
NotFound,
|
||||
Ambiguous(Vec<Range<usize>>),
|
||||
@@ -4385,6 +4387,114 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_streaming_overlapping_edits_detected_early(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
let fs = project::FakeFs::new(cx.executor());
|
||||
// The file content is crafted so that edit 1's replacement still
|
||||
// contains the old_text of edit 2 as a contiguous substring.
|
||||
// Without early overlap detection, edit 2 would silently match
|
||||
// inside the already-modified region and corrupt the file instead
|
||||
// of producing a clear "Conflicting edit ranges" error.
|
||||
fs.insert_tree(
|
||||
"/root",
|
||||
json!({
|
||||
"file.txt": "aaa\nbbb\nccc\nddd\neee\n"
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await;
|
||||
let language_registry = project.read_with(cx, |project, _cx| project.languages().clone());
|
||||
let context_server_registry =
|
||||
cx.new(|cx| ContextServerRegistry::new(project.read(cx).context_server_store(), cx));
|
||||
let model = Arc::new(FakeLanguageModel::default());
|
||||
let thread = cx.new(|cx| {
|
||||
crate::Thread::new(
|
||||
project.clone(),
|
||||
cx.new(|_cx| ProjectContext::default()),
|
||||
context_server_registry,
|
||||
Templates::new(),
|
||||
Some(model),
|
||||
cx,
|
||||
)
|
||||
});
|
||||
|
||||
let (sender, input) = ToolInput::<StreamingEditFileToolInput>::test();
|
||||
let (event_stream, _receiver) = ToolCallEventStream::test();
|
||||
|
||||
let tool = Arc::new(StreamingEditFileTool::new(
|
||||
project.clone(),
|
||||
thread.downgrade(),
|
||||
language_registry,
|
||||
));
|
||||
|
||||
let task = cx.update(|cx| tool.run(input, event_stream, cx));
|
||||
|
||||
// Setup: resolve the buffer
|
||||
sender.send_partial(json!({
|
||||
"display_description": "Overlapping edits",
|
||||
"path": "root/file.txt",
|
||||
"mode": "edit"
|
||||
}));
|
||||
cx.run_until_parked();
|
||||
|
||||
// Edit 1 targets "bbb\nccc" (lines 2-3) and replaces it with
|
||||
// text that preserves "ccc\nddd" as a contiguous substring in the
|
||||
// buffer — so edit 2's old_text will still match after edit 1 is
|
||||
// applied.
|
||||
//
|
||||
// Edit 2 targets "ccc\nddd" (lines 3-4), overlapping with edit 1 on
|
||||
// line 3 ("ccc"). After edit 1 runs, the buffer becomes:
|
||||
// "aaa\nXXX\nccc\nddd\nddd\neee\n"
|
||||
// and "ccc\nddd" is still present, so edit 2 would silently
|
||||
// succeed without early overlap detection.
|
||||
//
|
||||
// Edit 3 exists only to mark edit 2 as "complete" during streaming.
|
||||
sender.send_partial(json!({
|
||||
"display_description": "Overlapping edits",
|
||||
"path": "root/file.txt",
|
||||
"mode": "edit",
|
||||
"edits": [
|
||||
{"old_text": "bbb\nccc", "new_text": "XXX\nccc\nddd"},
|
||||
{"old_text": "ccc\nddd", "new_text": "ZZZ"},
|
||||
{"old_text": "eee", "new_text": "DUMMY"}
|
||||
]
|
||||
}));
|
||||
cx.run_until_parked();
|
||||
|
||||
// Send the final input with all three edits.
|
||||
sender.send_final(json!({
|
||||
"display_description": "Overlapping edits",
|
||||
"path": "root/file.txt",
|
||||
"mode": "edit",
|
||||
"edits": [
|
||||
{"old_text": "bbb\nccc", "new_text": "XXX\nccc\nddd"},
|
||||
{"old_text": "ccc\nddd", "new_text": "ZZZ"},
|
||||
{"old_text": "eee", "new_text": "DUMMY"}
|
||||
]
|
||||
}));
|
||||
|
||||
let result = task.await;
|
||||
// We expect a "Conflicting edit ranges" error. Currently the overlap
|
||||
// goes undetected during streaming and the file gets silently
|
||||
// corrupted, so this assertion will fail until we add early overlap
|
||||
// detection.
|
||||
match result {
|
||||
Err(StreamingEditFileToolOutput::Error { error })
|
||||
if error.contains("Conflicting edit ranges") => {}
|
||||
Err(StreamingEditFileToolOutput::Error { error }) => {
|
||||
panic!("Expected 'Conflicting edit ranges' error, got different error: {error}");
|
||||
}
|
||||
Ok(output) => {
|
||||
panic!("Expected 'Conflicting edit ranges' error, but got success: {output}");
|
||||
}
|
||||
Err(other) => {
|
||||
panic!("Expected 'Conflicting edit ranges' error, got unexpected output: {other}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn init_test(cx: &mut TestAppContext) {
|
||||
cx.update(|cx| {
|
||||
let settings_store = SettingsStore::test(cx);
|
||||
|
||||
Reference in new issue
Block a user