From 6408ae81d1d9f0d5a495c5edadb3d1fc6650e6f9 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Thu, 20 Mar 2025 14:29:34 -0300 Subject: [PATCH] assistant2: Return no-edits response to architect model (#27200) Sometimes the editor model returns no search/replace blocks. This usually happens when the architect model calls the edit tool before reading any files. When this happens, we'll now return the raw response from the editor model to the architect model so it can recover accordingly. Release Notes: - N/A --- crates/assistant_tools/src/edit_files_tool.rs | 362 ++++++++++-------- 1 file changed, 206 insertions(+), 156 deletions(-) diff --git a/crates/assistant_tools/src/edit_files_tool.rs b/crates/assistant_tools/src/edit_files_tool.rs index f75add9ebca20cf6a9a9e0790b172e0bccb52ff0..4aff41f289106ae633f86d472e4354111640dfdf 100644 --- a/crates/assistant_tools/src/edit_files_tool.rs +++ b/crates/assistant_tools/src/edit_files_tool.rs @@ -145,22 +145,30 @@ impl Tool for EditFilesTool { struct EditToolRequest { parser: EditActionParser, - output: String, - changed_buffers: HashSet>, - bad_searches: Vec, + editor_response: EditorResponse, project: Entity, action_log: Entity, tool_log: Option<(Entity, EditToolRequestId)>, } -#[derive(Debug)] -enum DiffResult { - BadSearch(BadSearch), - Diff(language::Diff), +enum EditorResponse { + /// The editor model hasn't produced any actions yet. + /// If we don't have any by the end, we'll return its message to the architect model. + Message(String), + /// The editor model produced at least one action. + Actions { + applied: Vec, + search_errors: Vec, + }, +} + +struct AppliedAction { + source: String, + buffer: Entity, } #[derive(Debug)] -enum BadSearch { +enum SearchError { NoMatch { file_path: String, search: String, @@ -226,10 +234,7 @@ impl EditToolRequest { let mut request = Self { parser: EditActionParser::new(), - // we start with the success header so we don't need to shift the output in the common case - output: Self::SUCCESS_OUTPUT_HEADER.to_string(), - changed_buffers: HashSet::default(), - bad_searches: Vec::new(), + editor_response: EditorResponse::Message(String::with_capacity(256)), action_log, project, tool_log, @@ -246,6 +251,12 @@ impl EditToolRequest { async fn process_response_chunk(&mut self, chunk: &str, cx: &mut AsyncApp) -> Result<()> { let new_actions = self.parser.parse_chunk(chunk); + if let EditorResponse::Message(ref mut message) = self.editor_response { + if new_actions.is_empty() { + message.push_str(chunk); + } + } + if let Some((ref log, req_id)) = self.tool_log { log.update(cx, |log, cx| { log.push_editor_response_chunk(req_id, chunk, &new_actions, cx) @@ -276,6 +287,11 @@ impl EditToolRequest { .update(cx, |project, cx| project.open_buffer(project_path, cx))? .await?; + enum DiffResult { + Diff(language::Diff), + SearchError(SearchError), + } + let result = match action { EditAction::Replace { old, @@ -285,7 +301,39 @@ impl EditToolRequest { let snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot())?; cx.background_executor() - .spawn(Self::replace_diff(old, new, file_path, snapshot)) + .spawn(async move { + if snapshot.is_empty() { + let exists = snapshot + .file() + .map_or(false, |file| file.disk_state().exists()); + + let error = SearchError::EmptyBuffer { + file_path: file_path.display().to_string(), + exists, + search: old, + }; + + return anyhow::Ok(DiffResult::SearchError(error)); + } + + let replace_result = + // Try to match exactly + replace_exact(&old, &new, &snapshot) + .await + // If that fails, try being flexible about indentation + .or_else(|| replace_with_flexible_indent(&old, &new, &snapshot)); + + let Some(diff) = replace_result else { + let error = SearchError::NoMatch { + search: old, + file_path: file_path.display().to_string(), + }; + + return Ok(DiffResult::SearchError(error)); + }; + + Ok(DiffResult::Diff(diff)) + }) .await } EditAction::Write { content, .. } => Ok(DiffResult::Diff( @@ -296,177 +344,179 @@ impl EditToolRequest { }?; match result { - DiffResult::BadSearch(invalid_replace) => { - self.bad_searches.push(invalid_replace); + DiffResult::SearchError(error) => { + self.push_search_error(error); } DiffResult::Diff(diff) => { let _clock = buffer.update(cx, |buffer, cx| buffer.apply_diff(diff, cx))?; - write!(&mut self.output, "\n\n{}", source)?; - self.changed_buffers.insert(buffer); + self.push_applied_action(AppliedAction { source, buffer }); } } - Ok(()) + anyhow::Ok(()) } - async fn replace_diff( - old: String, - new: String, - file_path: std::path::PathBuf, - snapshot: language::BufferSnapshot, - ) -> Result { - if snapshot.is_empty() { - let exists = snapshot - .file() - .map_or(false, |file| file.disk_state().exists()); - - return Ok(DiffResult::BadSearch(BadSearch::EmptyBuffer { - file_path: file_path.display().to_string(), - exists, - search: old, - })); + fn push_search_error(&mut self, error: SearchError) { + match &mut self.editor_response { + EditorResponse::Message(_) => { + self.editor_response = EditorResponse::Actions { + applied: Vec::new(), + search_errors: vec![error], + }; + } + EditorResponse::Actions { search_errors, .. } => { + search_errors.push(error); + } } - - let result = - // Try to match exactly - replace_exact(&old, &new, &snapshot) - .await - // If that fails, try being flexible about indentation - .or_else(|| replace_with_flexible_indent(&old, &new, &snapshot)); - - let Some(diff) = result else { - return anyhow::Ok(DiffResult::BadSearch(BadSearch::NoMatch { - search: old, - file_path: file_path.display().to_string(), - })); - }; - - anyhow::Ok(DiffResult::Diff(diff)) } - const SUCCESS_OUTPUT_HEADER: &str = "Successfully applied. Here's a list of changes:"; - const ERROR_OUTPUT_HEADER_NO_EDITS: &str = "I couldn't apply any edits!"; - const ERROR_OUTPUT_HEADER_WITH_EDITS: &str = - "Errors occurred. First, here's a list of the edits we managed to apply:"; - - async fn finalize(self, cx: &mut AsyncApp) -> Result { - let changed_buffer_count = self.changed_buffers.len(); - - // Save each buffer once at the end - for buffer in &self.changed_buffers { - self.project - .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx))? - .await?; + fn push_applied_action(&mut self, action: AppliedAction) { + match &mut self.editor_response { + EditorResponse::Message(_) => { + self.editor_response = EditorResponse::Actions { + applied: vec![action], + search_errors: Vec::new(), + }; + } + EditorResponse::Actions { applied, .. } => { + applied.push(action); + } } + } - self.action_log - .update(cx, |log, cx| log.buffer_edited(self.changed_buffers, cx)) - .log_err(); - - let errors = self.parser.errors(); + async fn finalize(self, cx: &mut AsyncApp) -> Result { + match self.editor_response { + EditorResponse::Message(message) => Err(anyhow!( + "No edits were applied! You might need to provide more context.\n\n{}", + message + )), + EditorResponse::Actions { + applied, + search_errors, + } => { + let mut output = String::with_capacity(1024); + + let parse_errors = self.parser.errors(); + let has_errors = !search_errors.is_empty() || !parse_errors.is_empty(); + + if has_errors { + let error_count = search_errors.len() + parse_errors.len(); + + if applied.is_empty() { + writeln!( + &mut output, + "{} errors occurred! No edits were applied.", + error_count, + )?; + } else { + writeln!( + &mut output, + "{} errors occurred, but {} edits were correctly applied.", + error_count, + applied.len(), + )?; + + writeln!( + &mut output, + "# {} SEARCH/REPLACE block(s) applied:\n\nDo not re-send these since they are already applied!\n", + applied.len() + )?; + } + } else { + write!( + &mut output, + "Successfully applied! Here's a list of applied edits:" + )?; + } - if errors.is_empty() && self.bad_searches.is_empty() { - if changed_buffer_count == 0 { - return Err(anyhow!( - "The instructions didn't lead to any changes. You might need to consult the file contents first." - )); - } + let mut changed_buffers = HashSet::default(); - Ok(self.output) - } else { - let mut output = self.output; + for action in applied { + changed_buffers.insert(action.buffer); + write!(&mut output, "\n\n{}", action.source)?; + } - if output.is_empty() { - output.replace_range( - 0..Self::SUCCESS_OUTPUT_HEADER.len(), - Self::ERROR_OUTPUT_HEADER_NO_EDITS, - ); - } else { - output.replace_range( - 0..Self::SUCCESS_OUTPUT_HEADER.len(), - Self::ERROR_OUTPUT_HEADER_WITH_EDITS, - ); - } + for buffer in &changed_buffers { + self.project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx))? + .await?; + } - if !self.bad_searches.is_empty() { - writeln!( - &mut output, - "\n\n# {} SEARCH/REPLACE block(s) failed to match:\n", - self.bad_searches.len() - )?; - - for bad_search in self.bad_searches { - match bad_search { - BadSearch::NoMatch { file_path, search } => { - writeln!( - &mut output, - "## No exact match in: `{}`\n```\n{}\n```\n", - file_path, search, - )?; - } - BadSearch::EmptyBuffer { - file_path, - exists: true, - search, - } => { - writeln!( - &mut output, - "## No match because `{}` is empty:\n```\n{}\n```\n", - file_path, search, - )?; - } - BadSearch::EmptyBuffer { - file_path, - exists: false, - search, - } => { - writeln!( - &mut output, - "## No match because `{}` does not exist:\n```\n{}\n```\n", - file_path, search, - )?; + self.action_log + .update(cx, |log, cx| log.buffer_edited(changed_buffers.clone(), cx)) + .log_err(); + + if !search_errors.is_empty() { + writeln!( + &mut output, + "\n\n## {} SEARCH/REPLACE block(s) failed to match:\n", + search_errors.len() + )?; + + for error in search_errors { + match error { + SearchError::NoMatch { file_path, search } => { + writeln!( + &mut output, + "### No exact match in: `{}`\n```\n{}\n```\n", + file_path, search, + )?; + } + SearchError::EmptyBuffer { + file_path, + exists: true, + search, + } => { + writeln!( + &mut output, + "### No match because `{}` is empty:\n```\n{}\n```\n", + file_path, search, + )?; + } + SearchError::EmptyBuffer { + file_path, + exists: false, + search, + } => { + writeln!( + &mut output, + "### No match because `{}` does not exist:\n```\n{}\n```\n", + file_path, search, + )?; + } } } - } - write!(&mut output, - "The SEARCH section must exactly match an existing block of lines including all white \ - space, comments, indentation, docstrings, etc." - )?; - } + write!(&mut output, + "The SEARCH section must exactly match an existing block of lines including all white \ + space, comments, indentation, docstrings, etc." + )?; + } - if !errors.is_empty() { - writeln!( - &mut output, - "\n\n# {} SEARCH/REPLACE blocks failed to parse:", - errors.len() - )?; + if !parse_errors.is_empty() { + writeln!( + &mut output, + "\n\n## {} SEARCH/REPLACE blocks failed to parse:", + parse_errors.len() + )?; - for error in errors { - writeln!(&mut output, "- {}", error)?; + for error in parse_errors { + writeln!(&mut output, "- {}", error)?; + } } - } - if changed_buffer_count > 0 { - writeln!( - &mut output, - "\n\nThe other SEARCH/REPLACE blocks were applied successfully. Do not re-send them!", - )?; - } + if has_errors { + writeln!(&mut output, + "\n\nYou can fix errors by running the tool again. You can include instructions, \ + but errors are part of the conversation so you don't need to repeat them.", + )?; - writeln!( - &mut output, - "{}You can fix errors by running the tool again. You can include instructions, \ - but errors are part of the conversation so you don't need to repeat them.", - if changed_buffer_count == 0 { - "\n\n" + Err(anyhow!(output)) } else { - "" + Ok(output) } - )?; - - Err(anyhow!(output)) + } } } }