Systematically optimize agentic editing performance (#28961)

Now that we've established a proper eval in tree, this PR is reboots of
our agent loop back to a set of minimal tools and simpler prompts. We
should aim to get this branch feeling subjectively competitive with
what's on main and then merge it, and build from there.

Let's invest in our eval and use it to drive better performance of the
agent loop. How you can help: Pick an example, and then make the outcome
faster or better. It's fine to even use your own subjective judgment, as
our evaluation criteria likely need tuning as well at this point. Focus
on making the agent work better in your own subjective experience first.
Let's focus on simple/practical improvements to make this thing work
better, then determine how we can craft our judgment criteria to lock
those improvements in.

Release Notes:

- N/A

---------

Co-authored-by: Max <max@zed.dev>
Co-authored-by: Antonio <antonio@zed.dev>
Co-authored-by: Agus <agus@zed.dev>
Co-authored-by: Richard <richard@zed.dev>
Co-authored-by: Max Brunsfeld <maxbrunsfeld@gmail.com>
Co-authored-by: Antonio Scandurra <me@as-cii.com>
Co-authored-by: Michael Sloan <mgsloan@gmail.com>

crates/assistant_tools/Cargo.toml

@@ -22,6 +22,7 @@ futures.workspace = true
 gpui.workspace = true
 html_to_markdown.workspace = true
 http_client.workspace = true
+indoc.workspace = true
 itertools.workspace = true
 language.workspace = true
 language_model.workspace = true
@@ -45,5 +46,8 @@ gpui = { workspace = true, features = ["test-support"] }
 language = { workspace = true, features = ["test-support"] }
 project = { workspace = true, features = ["test-support"] }
 rand.workspace = true
+pretty_assertions.workspace = true
+settings = { workspace = true, features = ["test-support"] }
+tree-sitter-rust.workspace = true
 workspace = { workspace = true, features = ["test-support"] }
 unindent.workspace = true

crates/assistant_tools/src/assistant_tools.rs

@@ -7,8 +7,8 @@ mod create_directory_tool;
 mod create_file_tool;
 mod delete_path_tool;
 mod diagnostics_tool;
+mod edit_file_tool;
 mod fetch_tool;
-mod find_replace_file_tool;
 mod list_directory_tool;
 mod move_path_tool;
 mod now_tool;
@@ -42,8 +42,8 @@ use crate::create_directory_tool::CreateDirectoryTool;
 use crate::create_file_tool::CreateFileTool;
 use crate::delete_path_tool::DeletePathTool;
 use crate::diagnostics_tool::DiagnosticsTool;
+use crate::edit_file_tool::EditFileTool;
 use crate::fetch_tool::FetchTool;
-use crate::find_replace_file_tool::FindReplaceFileTool;
 use crate::list_directory_tool::ListDirectoryTool;
 use crate::now_tool::NowTool;
 use crate::open_tool::OpenTool;
@@ -59,28 +59,28 @@ pub fn init(http_client: Arc<HttpClientWithUrl>, cx: &mut App) {
     assistant_tool::init(cx);
 
     let registry = ToolRegistry::global(cx);
+    registry.register_tool(TerminalTool);
     registry.register_tool(BatchTool);
-    registry.register_tool(CodeActionTool);
-    registry.register_tool(CodeSymbolsTool);
-    registry.register_tool(ContentsTool);
-    registry.register_tool(CopyPathTool);
     registry.register_tool(CreateDirectoryTool);
     registry.register_tool(CreateFileTool);
+    registry.register_tool(CopyPathTool);
     registry.register_tool(DeletePathTool);
-    registry.register_tool(DiagnosticsTool);
-    registry.register_tool(FetchTool::new(http_client));
-    registry.register_tool(FindReplaceFileTool);
-    registry.register_tool(ListDirectoryTool);
+    registry.register_tool(EditFileTool);
+    registry.register_tool(SymbolInfoTool);
+    registry.register_tool(CodeActionTool);
     registry.register_tool(MovePathTool);
+    registry.register_tool(DiagnosticsTool);
+    registry.register_tool(ListDirectoryTool);
     registry.register_tool(NowTool);
     registry.register_tool(OpenTool);
+    registry.register_tool(CodeSymbolsTool);
+    registry.register_tool(ContentsTool);
     registry.register_tool(PathSearchTool);
     registry.register_tool(ReadFileTool);
     registry.register_tool(RegexSearchTool);
     registry.register_tool(RenameTool);
-    registry.register_tool(SymbolInfoTool);
-    registry.register_tool(TerminalTool);
     registry.register_tool(ThinkingTool);
+    registry.register_tool(FetchTool::new(http_client));
 
     cx.observe_flag::<feature_flags::ZedProWebSearchTool, _>({
         move |is_enabled, cx| {

crates/assistant_tools/src/code_symbols_tool.rs

@@ -147,7 +147,7 @@ impl Tool for CodeSymbolsTool {
         };
 
         cx.spawn(async move |cx| match input.path {
-            Some(path) => file_outline(project, path, action_log, regex, input.offset, cx).await,
+            Some(path) => file_outline(project, path, action_log, regex, cx).await,
             None => project_symbols(project, regex, input.offset, cx).await,
         })
         .into()
@@ -159,7 +159,6 @@ pub async fn file_outline(
     path: String,
     action_log: Entity<ActionLog>,
     regex: Option<Regex>,
-    offset: u32,
     cx: &mut AsyncApp,
 ) -> anyhow::Result<String> {
     let buffer = {
@@ -195,7 +194,8 @@ pub async fn file_outline(
             .into_iter()
             .map(|item| item.to_point(&snapshot)),
         regex,
-        offset,
+        0,
+        usize::MAX,
     )
     .await
 }
@@ -294,11 +294,10 @@ async fn project_symbols(
 async fn render_outline(
     items: impl IntoIterator<Item = OutlineItem<Point>>,
     regex: Option<Regex>,
-    offset: u32,
+    offset: usize,
+    results_per_page: usize,
 ) -> Result<String> {
-    const RESULTS_PER_PAGE_USIZE: usize = RESULTS_PER_PAGE as usize;
-
-    let mut items = items.into_iter().skip(offset as usize);
+    let mut items = items.into_iter().skip(offset);
 
     let entries = items
         .by_ref()
@@ -307,7 +306,7 @@ async fn render_outline(
                 .as_ref()
                 .is_none_or(|regex| regex.is_match(&item.text))
         })
-        .take(RESULTS_PER_PAGE_USIZE)
+        .take(results_per_page)
         .collect::<Vec<_>>();
     let has_more = items.next().is_some();
 
@@ -338,7 +337,10 @@ async fn render_outline(
     Ok(output)
 }
 
-fn render_entries(output: &mut String, items: impl IntoIterator<Item = OutlineItem<Point>>) -> u32 {
+fn render_entries(
+    output: &mut String,
+    items: impl IntoIterator<Item = OutlineItem<Point>>,
+) -> usize {
     let mut entries_rendered = 0;
 
     for item in items {

crates/assistant_tools/src/contents_tool.rs

@@ -228,7 +228,7 @@ impl Tool for ContentsTool {
                     } else {
                         // File is too big, so return its outline and a suggestion to
                         // read again with a line number range specified.
-                        let outline = file_outline(project, file_path, action_log, None, 0, cx).await?;
+                        let outline = file_outline(project, file_path, action_log, None, cx).await?;
 
                         Ok(format!("This file was too big to read all at once. Here is an outline of its symbols:\n\n{outline}\n\nUsing the line numbers in this outline, you can call this tool again while specifying the start and end fields to see the implementations of symbols in the outline."))
                     }

crates/assistant_tools/src/diagnostics_tool/description.md

@@ -15,6 +15,7 @@ To get a project-wide diagnostic summary:
 {}
 </example>
 
-IMPORTANT: When you're done making changes, you **MUST** get the **project** diagnostics (input: `{}`) at the end of your edits so you can fix any problems you might have introduced. **DO NOT** tell the user you're done before doing this!
-
-You may only attempt to fix these up to 3 times. If you have tried 3 times to fix them, and there are still problems remaining, you must not continue trying to fix them, and must instead tell the user that there are problems remaining - and ask if the user would like you to attempt to solve them further.
+<guidelines>
+- If you think you can fix a diagnostic, make 1-2 attempts and then give up.
+- Don't remove code you've generated just because you can't fix an error. The user can help you fix it.
+</guidelines>

crates/assistant_tools/src/edit_file_tool.rs

@@ -0,0 +1,183 @@
+use crate::{replace::replace_with_flexible_indent, schema::json_schema_for};
+use anyhow::{Context as _, Result, anyhow};
+use assistant_tool::{ActionLog, Tool, ToolResult};
+use gpui::{App, AppContext, AsyncApp, Entity, Task};
+use language_model::{LanguageModelRequestMessage, LanguageModelToolSchemaFormat};
+use project::Project;
+use schemars::JsonSchema;
+use serde::{Deserialize, Serialize};
+use std::{path::PathBuf, sync::Arc};
+use ui::IconName;
+
+use crate::replace::replace_exact;
+
+#[derive(Debug, Serialize, Deserialize, JsonSchema)]
+pub struct EditFileToolInput {
+    /// The full path of the file to modify in the project.
+    ///
+    /// WARNING: When specifying which file path need changing, you MUST
+    /// start each path with one of the project's root directories.
+    ///
+    /// The following examples assume we have two root directories in the project:
+    /// - backend
+    /// - frontend
+    ///
+    /// <example>
+    /// `backend/src/main.rs`
+    ///
+    /// Notice how the file path starts with root-1. Without that, the path
+    /// would be ambiguous and the call would fail!
+    /// </example>
+    ///
+    /// <example>
+    /// `frontend/db.js`
+    /// </example>
+    pub path: PathBuf,
+
+    /// A user-friendly markdown description of what's being replaced. This will be shown in the UI.
+    ///
+    /// <example>Fix API endpoint URLs</example>
+    /// <example>Update copyright year in `page_footer`</example>
+    pub display_description: String,
+
+    /// The text to replace.
+    pub old_string: String,
+
+    /// The text to replace it with.
+    pub new_string: String,
+}
+
+pub struct EditFileTool;
+
+impl Tool for EditFileTool {
+    fn name(&self) -> String {
+        "edit_file".into()
+    }
+
+    fn needs_confirmation(&self, _: &serde_json::Value, _: &App) -> bool {
+        false
+    }
+
+    fn description(&self) -> String {
+        include_str!("edit_file_tool/description.md").to_string()
+    }
+
+    fn icon(&self) -> IconName {
+        IconName::Pencil
+    }
+
+    fn input_schema(&self, format: LanguageModelToolSchemaFormat) -> Result<serde_json::Value> {
+        json_schema_for::<EditFileToolInput>(format)
+    }
+
+    fn ui_text(&self, input: &serde_json::Value) -> String {
+        match serde_json::from_value::<EditFileToolInput>(input.clone()) {
+            Ok(input) => input.display_description,
+            Err(_) => "Edit file".to_string(),
+        }
+    }
+
+    fn run(
+        self: Arc<Self>,
+        input: serde_json::Value,
+        _messages: &[LanguageModelRequestMessage],
+        project: Entity<Project>,
+        action_log: Entity<ActionLog>,
+        cx: &mut App,
+    ) -> ToolResult {
+        let input = match serde_json::from_value::<EditFileToolInput>(input) {
+            Ok(input) => input,
+            Err(err) => return Task::ready(Err(anyhow!(err))).into(),
+        };
+
+        cx.spawn(async move |cx: &mut AsyncApp| {
+            let project_path = project.read_with(cx, |project, cx| {
+                project
+                    .find_project_path(&input.path, cx)
+                    .context("Path not found in project")
+            })??;
+
+            let buffer = project
+                .update(cx, |project, cx| project.open_buffer(project_path, cx))?
+                .await?;
+
+            let snapshot = buffer.read_with(cx, |buffer, _cx| buffer.snapshot())?;
+
+            if input.old_string.is_empty() {
+                return Err(anyhow!("`old_string` cannot be empty. Use a different tool if you want to create a file."));
+            }
+
+            if input.old_string == input.new_string {
+                return Err(anyhow!("The `old_string` and `new_string` are identical, so no changes would be made."));
+            }
+
+            let result = cx
+                .background_spawn(async move {
+                    // Try to match exactly
+                    let diff = replace_exact(&input.old_string, &input.new_string, &snapshot)
+                    .await
+                    // If that fails, try being flexible about indentation
+                    .or_else(|| replace_with_flexible_indent(&input.old_string, &input.new_string, &snapshot))?;
+
+                    if diff.edits.is_empty() {
+                        return None;
+                    }
+
+                    let old_text = snapshot.text();
+
+                    Some((old_text, diff))
+                })
+                .await;
+
+            let Some((old_text, diff)) = result else {
+                let err = buffer.read_with(cx, |buffer, _cx| {
+                    let file_exists = buffer
+                        .file()
+                        .map_or(false, |file| file.disk_state().exists());
+
+                    if !file_exists {
+                        anyhow!("{} does not exist", input.path.display())
+                    } else if buffer.is_empty() {
+                        anyhow!(
+                            "{} is empty, so the provided `old_string` wasn't found.",
+                            input.path.display()
+                        )
+                    } else {
+                        anyhow!("Failed to match the provided `old_string`")
+                    }
+                })?;
+
+                return Err(err)
+            };
+
+            let snapshot = cx.update(|cx| {
+                action_log.update(cx, |log, cx| {
+                    log.buffer_read(buffer.clone(), cx)
+                });
+                let snapshot = buffer.update(cx, |buffer, cx| {
+                    buffer.finalize_last_transaction();
+                    buffer.apply_diff(diff, cx);
+                    buffer.finalize_last_transaction();
+                    buffer.snapshot()
+                });
+                action_log.update(cx, |log, cx| {
+                    log.buffer_edited(buffer.clone(), cx)
+                });
+                snapshot
+            })?;
+
+            project.update( cx, |project, cx| {
+                project.save_buffer(buffer, cx)
+            })?.await?;
+
+            let diff_str = cx.background_spawn(async move {
+                let new_text = snapshot.text();
+                language::unified_diff(&old_text, &new_text)
+            }).await;
+
+
+            Ok(format!("Edited {}:\n\n```diff\n{}\n```", input.path.display(), diff_str))
+
+        }).into()
+    }
+}

crates/assistant_tools/src/edit_file_tool/description.md

@@ -0,0 +1,45 @@
+This is a tool for editing files. For moving or renaming files, you should generally use the `terminal` tool with the 'mv' command instead. For larger edits, use the `create_file` tool to overwrite files.
+
+Before using this tool:
+
+1. Use the `read_file` tool to understand the file's contents and context
+
+2. Verify the directory path is correct (only applicable when creating new files):
+   - Use the `list_directory` tool to verify the parent directory exists and is the correct location
+
+To make a file edit, provide the following:
+1. path: The full path to the file you wish to modify in the project. This path must include the root directory in the project.
+2. old_string: The text to replace (must be unique within the file, and must match the file contents exactly, including all whitespace and indentation)
+3. new_string: The edited text, which will replace the old_string in the file.
+
+The tool will replace ONE occurrence of old_string with new_string in the specified file.
+
+CRITICAL REQUIREMENTS FOR USING THIS TOOL:
+
+1. UNIQUENESS: The old_string MUST uniquely identify the specific instance you want to change. This means:
+   - Include AT LEAST 3-5 lines of context BEFORE the change point
+   - Include AT LEAST 3-5 lines of context AFTER the change point
+   - Include all whitespace, indentation, and surrounding code exactly as it appears in the file
+
+2. SINGLE INSTANCE: This tool can only change ONE instance at a time. If you need to change multiple instances:
+   - Make separate calls to this tool for each instance
+   - Each call must uniquely identify its specific instance using extensive context
+
+3. VERIFICATION: Before using this tool:
+   - Check how many instances of the target text exist in the file
+   - If multiple instances exist, gather enough context to uniquely identify each one
+   - Plan separate tool calls for each instance
+
+WARNING: If you do not follow these requirements:
+   - The tool will fail if old_string matches multiple locations
+   - The tool will fail if old_string doesn't match exactly (including whitespace)
+   - You may change the wrong instance if you don't include enough context
+
+When making edits:
+   - Ensure the edit results in idiomatic, correct code
+   - Do not leave the code in a broken state
+   - Always use fully-qualified project paths (starting with the name of one of the project's root directories)
+
+If you want to create a new file, use the `create_file` tool instead of this tool. Don't pass an empty `old_string`.
+
+Remember: when making multiple file edits in a row to the same file, you should prefer to send all edits in a single message with multiple calls to this tool, rather than multiple messages with a single call each.

crates/assistant_tools/src/list_directory_tool.rs

@@ -12,7 +12,7 @@ use util::markdown::MarkdownString;
 
 #[derive(Debug, Serialize, Deserialize, JsonSchema)]
 pub struct ListDirectoryToolInput {
-    /// The relative path of the directory to list.
+    /// The fully-qualified path of the directory to list in the project.
     ///
     /// This path should never be absolute, and the first component
     /// of the path should always be a root directory in a project.

crates/assistant_tools/src/list_directory_tool/description.md

@@ -1 +1 @@
-Lists files and directories in a given path.
+Lists files and directories in a given path. Prefer the `regex_search` or `path_search` tools when searching the codebase.

crates/assistant_tools/src/path_search_tool.rs

@@ -6,14 +6,14 @@ use language_model::{LanguageModelRequestMessage, LanguageModelToolSchemaFormat}
 use project::Project;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use std::{path::PathBuf, sync::Arc};
+use std::{cmp, fmt::Write as _, path::PathBuf, sync::Arc};
 use ui::IconName;
 use util::paths::PathMatcher;
 use worktree::Snapshot;
 
 #[derive(Debug, Serialize, Deserialize, JsonSchema)]
 pub struct PathSearchToolInput {
-    /// The glob to search all project paths for.
+    /// The glob to match against every path in the project.
     ///
     /// <example>
     /// If the project has the following root directories:
@@ -76,66 +76,125 @@ impl Tool for PathSearchTool {
             Ok(input) => (input.offset, input.glob),
             Err(err) => return Task::ready(Err(anyhow!(err))).into(),
         };
-
-        let path_matcher = match PathMatcher::new([
-            // Sometimes models try to search for "". In this case, return all paths in the project.
-            if glob.is_empty() { "*" } else { &glob },
-        ]) {
-            Ok(matcher) => matcher,
-            Err(err) => return Task::ready(Err(anyhow!("Invalid glob: {err}"))).into(),
-        };
-        let snapshots: Vec<Snapshot> = project
-            .read(cx)
-            .worktrees(cx)
-            .map(|worktree| worktree.read(cx).snapshot())
-            .collect();
-
+        let offset = offset as usize;
+        let task = search_paths(&glob, project, cx);
         cx.background_spawn(async move {
-            let mut matches = Vec::new();
-
-            for worktree in snapshots {
-                let root_name = worktree.root_name();
-
-                // Don't consider ignored entries.
-                for entry in worktree.entries(false, 0) {
-                    if path_matcher.is_match(&entry.path) {
-                        matches.push(
-                            PathBuf::from(root_name)
-                                .join(&entry.path)
-                                .to_string_lossy()
-                                .to_string(),
-                        );
-                    }
-                }
-            }
+            let matches = task.await?;
+            let paginated_matches = &matches[cmp::min(offset, matches.len())
+                ..cmp::min(offset + RESULTS_PER_PAGE, matches.len())];
 
             if matches.is_empty() {
-                Ok(format!("No paths in the project matched the glob {glob:?}"))
+                Ok("No matches found".to_string())
             } else {
-                // Sort to group entries in the same directory together.
-                matches.sort();
-
-                let total_matches = matches.len();
-                let response = if total_matches > RESULTS_PER_PAGE + offset as usize {
-                let paginated_matches: Vec<_> = matches
-                      .into_iter()
-                      .skip(offset as usize)
-                      .take(RESULTS_PER_PAGE)
-                      .collect();
-
-                    format!(
-                        "Found {} total matches. Showing results {}-{} (provide 'offset' parameter for more results):\n\n{}",
-                        total_matches,
+                let mut message = format!("Found {} total matches.", matches.len());
+                if matches.len() > RESULTS_PER_PAGE {
+                    write!(
+                        &mut message,
+                        "\nShowing results {}-{} (provide 'offset' parameter for more results):",
                         offset + 1,
-                        offset as usize + paginated_matches.len(),
-                        paginated_matches.join("\n")
+                        offset + paginated_matches.len()
                     )
-                } else {
-                    matches.join("\n")
-                };
-
-                Ok(response)
+                    .unwrap();
+                }
+                for mat in matches.into_iter().skip(offset).take(RESULTS_PER_PAGE) {
+                    write!(&mut message, "\n{}", mat.display()).unwrap();
+                }
+                Ok(message)
             }
-        }).into()
+        })
+        .into()
+    }
+}
+
+fn search_paths(glob: &str, project: Entity<Project>, cx: &mut App) -> Task<Result<Vec<PathBuf>>> {
+    let path_matcher = match PathMatcher::new([
+        // Sometimes models try to search for "". In this case, return all paths in the project.
+        if glob.is_empty() { "*" } else { glob },
+    ]) {
+        Ok(matcher) => matcher,
+        Err(err) => return Task::ready(Err(anyhow!("Invalid glob: {err}"))),
+    };
+    let snapshots: Vec<Snapshot> = project
+        .read(cx)
+        .worktrees(cx)
+        .map(|worktree| worktree.read(cx).snapshot())
+        .collect();
+
+    cx.background_spawn(async move {
+        Ok(snapshots
+            .iter()
+            .flat_map(|snapshot| {
+                let root_name = PathBuf::from(snapshot.root_name());
+                snapshot
+                    .entries(false, 0)
+                    .map(move |entry| root_name.join(&entry.path))
+                    .filter(|path| path_matcher.is_match(&path))
+            })
+            .collect())
+    })
+}
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use gpui::TestAppContext;
+    use project::{FakeFs, Project};
+    use settings::SettingsStore;
+    use util::path;
+
+    #[gpui::test]
+    async fn test_path_search_tool(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/root",
+            serde_json::json!({
+                "apple": {
+                    "banana": {
+                        "carrot": "1",
+                    },
+                    "bandana": {
+                        "carbonara": "2",
+                    },
+                    "endive": "3"
+                }
+            }),
+        )
+        .await;
+        let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await;
+
+        let matches = cx
+            .update(|cx| search_paths("root/**/car*", project.clone(), cx))
+            .await
+            .unwrap();
+        assert_eq!(
+            matches,
+            &[
+                PathBuf::from("root/apple/banana/carrot"),
+                PathBuf::from("root/apple/bandana/carbonara")
+            ]
+        );
+
+        let matches = cx
+            .update(|cx| search_paths("**/car*", project.clone(), cx))
+            .await
+            .unwrap();
+        assert_eq!(
+            matches,
+            &[
+                PathBuf::from("root/apple/banana/carrot"),
+                PathBuf::from("root/apple/bandana/carbonara")
+            ]
+        );
+    }
+
+    fn init_test(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            let settings_store = SettingsStore::test(cx);
+            cx.set_global(settings_store);
+            language::init(cx);
+            Project::init_settings(cx);
+        });
     }
 }

crates/assistant_tools/src/path_search_tool/description.md

@@ -1,3 +1,7 @@
-Returns paths in the project which match the given glob.
+Fast file pattern matching tool that works with any codebase size
 
-Results are paginated with 50 matches per page. Use the optional 'offset' parameter to request subsequent pages.
+- Supports glob patterns like "**/*.js" or "src/**/*.ts"
+- Returns matching file paths sorted alphabetically
+- Prefer the `regex_search` tool to this tool when searching for symbols unless you have specific information about paths.
+- Use this tool when you need to find files by name patterns
+- Results are paginated with 50 matches per page. Use the optional 'offset' parameter to request subsequent pages.

crates/assistant_tools/src/read_file_tool.rs

@@ -1,14 +1,14 @@
-use std::sync::Arc;
-
 use crate::{code_symbols_tool::file_outline, schema::json_schema_for};
 use anyhow::{Result, anyhow};
 use assistant_tool::{ActionLog, Tool, ToolResult};
 use gpui::{App, Entity, Task};
+use indoc::formatdoc;
 use itertools::Itertools;
 use language_model::{LanguageModelRequestMessage, LanguageModelToolSchemaFormat};
 use project::Project;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
+use std::sync::Arc;
 use ui::IconName;
 use util::markdown::MarkdownString;
 
@@ -95,11 +95,24 @@ impl Tool for ReadFileTool {
         };
 
         let Some(project_path) = project.read(cx).find_project_path(&input.path, cx) else {
-            return Task::ready(Err(anyhow!("Path {} not found in project", &input.path,))).into();
+            return Task::ready(Err(anyhow!("Path {} not found in project", &input.path))).into();
         };
+        let Some(worktree) = project
+            .read(cx)
+            .worktree_for_id(project_path.worktree_id, cx)
+        else {
+            return Task::ready(Err(anyhow!("Worktree not found for project path"))).into();
+        };
+        let exists = worktree.update(cx, |worktree, cx| {
+            worktree.file_exists(&project_path.path, cx)
+        });
 
         let file_path = input.path.clone();
         cx.spawn(async move |cx| {
+            if !exists.await? {
+                return Err(anyhow!("{} not found", file_path))
+            }
+
             let buffer = cx
                 .update(|cx| {
                     project.update(cx, |project, cx| project.open_buffer(project_path, cx))
@@ -141,11 +154,231 @@ impl Tool for ReadFileTool {
                 } else {
                     // File is too big, so return an error with the outline
                     // and a suggestion to read again with line numbers.
-                    let outline = file_outline(project, file_path, action_log, None, 0, cx).await?;
+                    let outline = file_outline(project, file_path, action_log, None, cx).await?;
+                    Ok(formatdoc! {"
+                        This file was too big to read all at once. Here is an outline of its symbols:
 
-                    Ok(format!("This file was too big to read all at once. Here is an outline of its symbols:\n\n{outline}\n\nUsing the line numbers in this outline, you can call this tool again while specifying the start_line and end_line fields to see the implementations of symbols in the outline."))
+                        {outline}
+
+                        Using the line numbers in this outline, you can call this tool again while specifying
+                        the start_line and end_line fields to see the implementations of symbols in the outline."
+                    })
                 }
             }
         }).into()
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use gpui::{AppContext, TestAppContext};
+    use language::{Language, LanguageConfig, LanguageMatcher};
+    use project::{FakeFs, Project};
+    use serde_json::json;
+    use settings::SettingsStore;
+    use util::path;
+
+    #[gpui::test]
+    async fn test_read_nonexistent_file(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree("/root", json!({})).await;
+        let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await;
+        let action_log = cx.new(|_| ActionLog::new(project.clone()));
+        let result = cx
+            .update(|cx| {
+                let input = json!({
+                    "path": "root/nonexistent_file.txt"
+                });
+                Arc::new(ReadFileTool)
+                    .run(input, &[], project.clone(), action_log, cx)
+                    .output
+            })
+            .await;
+        assert_eq!(
+            result.unwrap_err().to_string(),
+            "root/nonexistent_file.txt not found"
+        );
+    }
+
+    #[gpui::test]
+    async fn test_read_small_file(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/root",
+            json!({
+                "small_file.txt": "This is a small file content"
+            }),
+        )
+        .await;
+        let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await;
+        let action_log = cx.new(|_| ActionLog::new(project.clone()));
+        let result = cx
+            .update(|cx| {
+                let input = json!({
+                    "path": "root/small_file.txt"
+                });
+                Arc::new(ReadFileTool)
+                    .run(input, &[], project.clone(), action_log, cx)
+                    .output
+            })
+            .await;
+        assert_eq!(result.unwrap(), "This is a small file content");
+    }
+
+    #[gpui::test]
+    async fn test_read_large_file(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/root",
+            json!({
+                "large_file.rs": (0..1000).map(|i| format!("struct Test{} {{\n    a: u32,\n    b: usize,\n}}", i)).collect::<Vec<_>>().join("\n")
+            }),
+        )
+        .await;
+        let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await;
+        let language_registry = project.read_with(cx, |project, _| project.languages().clone());
+        language_registry.add(Arc::new(rust_lang()));
+        let action_log = cx.new(|_| ActionLog::new(project.clone()));
+
+        let result = cx
+            .update(|cx| {
+                let input = json!({
+                    "path": "root/large_file.rs"
+                });
+                Arc::new(ReadFileTool)
+                    .run(input, &[], project.clone(), action_log.clone(), cx)
+                    .output
+            })
+            .await;
+        let content = result.unwrap();
+        assert_eq!(
+            content.lines().skip(2).take(6).collect::<Vec<_>>(),
+            vec![
+                "struct Test0 [L1-4]",
+                " a [L2]",
+                " b [L3]",
+                "struct Test1 [L5-8]",
+                " a [L6]",
+                " b [L7]",
+            ]
+        );
+
+        let result = cx
+            .update(|cx| {
+                let input = json!({
+                    "path": "root/large_file.rs",
+                    "offset": 1
+                });
+                Arc::new(ReadFileTool)
+                    .run(input, &[], project.clone(), action_log, cx)
+                    .output
+            })
+            .await;
+        let content = result.unwrap();
+        let expected_content = (0..1000)
+            .flat_map(|i| {
+                vec![
+                    format!("struct Test{} [L{}-{}]", i, i * 4 + 1, i * 4 + 4),
+                    format!(" a [L{}]", i * 4 + 2),
+                    format!(" b [L{}]", i * 4 + 3),
+                ]
+            })
+            .collect::<Vec<_>>();
+        pretty_assertions::assert_eq!(
+            content
+                .lines()
+                .skip(2)
+                .take(expected_content.len())
+                .collect::<Vec<_>>(),
+            expected_content
+        );
+    }
+
+    #[gpui::test]
+    async fn test_read_file_with_line_range(cx: &mut TestAppContext) {
+        init_test(cx);
+
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree(
+            "/root",
+            json!({
+                "multiline.txt": "Line 1\nLine 2\nLine 3\nLine 4\nLine 5"
+            }),
+        )
+        .await;
+        let project = Project::test(fs.clone(), [path!("/root").as_ref()], cx).await;
+        let action_log = cx.new(|_| ActionLog::new(project.clone()));
+        let result = cx
+            .update(|cx| {
+                let input = json!({
+                    "path": "root/multiline.txt",
+                    "start_line": 2,
+                    "end_line": 4
+                });
+                Arc::new(ReadFileTool)
+                    .run(input, &[], project.clone(), action_log, cx)
+                    .output
+            })
+            .await;
+        assert_eq!(result.unwrap(), "Line 2\nLine 3");
+    }
+
+    fn init_test(cx: &mut TestAppContext) {
+        cx.update(|cx| {
+            let settings_store = SettingsStore::test(cx);
+            cx.set_global(settings_store);
+            language::init(cx);
+            Project::init_settings(cx);
+        });
+    }
+
+    fn rust_lang() -> Language {
+        Language::new(
+            LanguageConfig {
+                name: "Rust".into(),
+                matcher: LanguageMatcher {
+                    path_suffixes: vec!["rs".to_string()],
+                    ..Default::default()
+                },
+                ..Default::default()
+            },
+            Some(tree_sitter_rust::LANGUAGE.into()),
+        )
+        .with_outline_query(
+            r#"
+            (line_comment) @annotation
+
+            (struct_item
+                "struct" @context
+                name: (_) @name) @item
+            (enum_item
+                "enum" @context
+                name: (_) @name) @item
+            (enum_variant
+                name: (_) @name) @item
+            (field_declaration
+                name: (_) @name) @item
+            (impl_item
+                "impl" @context
+                trait: (_)? @name
+                "for"? @context
+                type: (_) @name
+                body: (_ "{" (_)* "}")) @item
+            (function_item
+                "fn" @context
+                name: (_) @name) @item
+            (mod_item
+                "mod" @context
+                name: (_) @name) @item
+            "#,
+        )
+        .unwrap()
+    }
+}

crates/assistant_tools/src/read_file_tool/description.md

@@ -1,6 +1,3 @@
 Reads the content of the given file in the project.
 
-If the file is too big to read all at once, and neither a start line
-nor an end line was specified, then this returns an outline of the
-file's symbols (with line numbers) instead of the file's contents,
-so that it can be called again with line ranges.
+- Never attempt to read a path that hasn't been previously mentioned.

crates/assistant_tools/src/regex_search_tool/description.md

@@ -1,7 +1,6 @@
 Searches the entire project for the given regular expression.
 
-Returns a list of paths that matched the query. For each path, it returns some excerpts of the matched text.
-
-Results are paginated with 20 matches per page. Use the optional 'offset' parameter to request subsequent pages.
-
-This tool is not aware of semantics and does not use any information from language servers, so it should only be used when no available semantic tool (e.g. one that uses language servers) could fit a particular use case instead.
+- Prefer this tool when searching for files containing symbols in the project.
+- Supports full regex syntax (eg. "log.*Error", "function\\s+\\w+", etc.)
+- Use this tool when you need to find files containing specific patterns
+- Results are paginated with 20 matches per page. Use the optional 'offset' parameter to request subsequent pages.