diff --git a/crates/assistant_tools/src/edit_files_tool.rs b/crates/assistant_tools/src/edit_files_tool.rs index cc22009588..4f36df2901 100644 --- a/crates/assistant_tools/src/edit_files_tool.rs +++ b/crates/assistant_tools/src/edit_files_tool.rs @@ -148,22 +148,36 @@ 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 DiffResult { + Diff(language::Diff), + SearchError(SearchError), +} + +#[derive(Debug)] +enum SearchError { NoMatch { file_path: String, search: String, @@ -242,9 +256,7 @@ impl EditToolRequest { let mut request = Self { parser: EditActionParser::new(), - 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, @@ -263,6 +275,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) @@ -313,18 +331,45 @@ 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(()) + } + + 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); + } + } + } + + 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); + } + } } async fn replace_diff( @@ -338,152 +383,166 @@ impl EditToolRequest { .file() .map_or(false, |file| file.disk_state().exists()); - return Ok(DiffResult::BadSearch(BadSearch::EmptyBuffer { + let error = SearchError::EmptyBuffer { file_path: file_path.display().to_string(), exists, search: old, - })); + }; + + return Ok(DiffResult::SearchError(error)); } - let result = + 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) = result else { - return anyhow::Ok(DiffResult::BadSearch(BadSearch::NoMatch { + let Some(diff) = replace_result else { + let error = SearchError::NoMatch { search: old, file_path: file_path.display().to_string(), - })); + }; + + return Ok(DiffResult::SearchError(error)); }; - anyhow::Ok(DiffResult::Diff(diff)) + 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(); + 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); - // 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?; - } + let parse_errors = self.parser.errors(); + let has_errors = !search_errors.is_empty() || !parse_errors.is_empty(); - self.action_log - .update(cx, |log, cx| log.buffer_edited(self.changed_buffers, cx)) - .log_err(); + if has_errors { + let error_count = search_errors.len() + parse_errors.len(); - let errors = self.parser.errors(); + 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(), + )?; - 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." - )); - } + 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:" + )?; + } - Ok(self.output) - } else { - let mut output = self.output; + let mut changed_buffers = HashSet::default(); - 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 action in applied { + changed_buffers.insert(action.buffer); + write!(&mut output, "\n\n{}", action.source)?; + } - if !self.bad_searches.is_empty() { - writeln!( - &mut output, - "\n\n# {} SEARCH/REPLACE block(s) failed to match:\n", - self.bad_searches.len() - )?; + for buffer in &changed_buffers { + self.project + .update(cx, |project, cx| project.save_buffer(buffer.clone(), cx))? + .await?; + } - 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." + )?; + } + + if !parse_errors.is_empty() { + writeln!( + &mut output, + "\n\n## {} SEARCH/REPLACE blocks failed to parse:", + parse_errors.len() + )?; + + for error in parse_errors { + writeln!(&mut output, "- {}", error)?; + } } - write!(&mut output, - "The SEARCH section must exactly match an existing block of lines including all white \ - space, comments, indentation, docstrings, etc." - )?; - } + 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.", + )?; - if !errors.is_empty() { - writeln!( - &mut output, - "\n\n# {} SEARCH/REPLACE blocks failed to parse:", - errors.len() - )?; - - for error in 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!", - )?; - } - - 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)) + } } } }