From 70c973f6c389ebc3a055a8f073d541d9679d65b4 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 13 Mar 2025 10:41:27 +0100 Subject: [PATCH] Fix issues in `EditFilesTool`, `ListDirectoryTool` and `BashTool` (#26647) Release Notes: - N/A --- assets/prompts/assistant_system_prompt.hbs | 6 +-- crates/assistant2/src/thread.rs | 12 ++++-- crates/assistant_tools/src/bash_tool.rs | 38 +++++++++++++------ .../src/bash_tool/description.md | 6 ++- crates/assistant_tools/src/edit_files_tool.rs | 37 ++++++++++++++---- .../src/edit_files_tool/description.md | 2 +- .../src/edit_files_tool/edit_prompt.md | 2 +- .../src/list_directory_tool.rs | 21 ++++++++-- .../assistant_tools/src/path_search_tool.rs | 2 +- crates/assistant_tools/src/read_file_tool.rs | 4 +- crates/prompt_store/src/prompts.rs | 21 +++++++--- 11 files changed, 110 insertions(+), 41 deletions(-) diff --git a/assets/prompts/assistant_system_prompt.hbs b/assets/prompts/assistant_system_prompt.hbs index 6ac1b785c1..12453f21b0 100644 --- a/assets/prompts/assistant_system_prompt.hbs +++ b/assets/prompts/assistant_system_prompt.hbs @@ -11,8 +11,8 @@ You should only perform actions that modify the user’s system if explicitly re Be concise and direct in your responses. -The user has opened a project that contains the following top-level directories/files: +The user has opened a project that contains the following root directories/files: -{{#each worktree_root_names}} -- {{this}} +{{#each worktrees}} +- {{root_name}} (absolute path: {{abs_path}}) {{/each}} diff --git a/crates/assistant2/src/thread.rs b/crates/assistant2/src/thread.rs index acca0b65e7..397dff50e1 100644 --- a/crates/assistant2/src/thread.rs +++ b/crates/assistant2/src/thread.rs @@ -13,7 +13,7 @@ use language_model::{ Role, StopReason, }; use project::Project; -use prompt_store::PromptBuilder; +use prompt_store::{AssistantSystemPromptWorktree, PromptBuilder}; use scripting_tool::{ScriptingSession, ScriptingTool}; use serde::{Deserialize, Serialize}; use util::{post_inc, ResultExt, TryFutureExt as _}; @@ -384,8 +384,14 @@ impl Thread { let worktree_root_names = self .project .read(cx) - .worktree_root_names(cx) - .map(ToString::to_string) + .visible_worktrees(cx) + .map(|worktree| { + let worktree = worktree.read(cx); + AssistantSystemPromptWorktree { + root_name: worktree.root_name().into(), + abs_path: worktree.abs_path(), + } + }) .collect::>(); let system_prompt = self .prompt_builder diff --git a/crates/assistant_tools/src/bash_tool.rs b/crates/assistant_tools/src/bash_tool.rs index bac4b49c86..fecec02f78 100644 --- a/crates/assistant_tools/src/bash_tool.rs +++ b/crates/assistant_tools/src/bash_tool.rs @@ -1,4 +1,4 @@ -use anyhow::{anyhow, Result}; +use anyhow::{anyhow, Context as _, Result}; use assistant_tool::Tool; use gpui::{App, Entity, Task}; use language_model::LanguageModelRequestMessage; @@ -6,11 +6,14 @@ use project::Project; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::sync::Arc; +use util::command::new_smol_command; #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct BashToolInput { /// The bash command to execute as a one-liner. command: String, + /// Working directory for the command. This must be one of the root directories of the project. + working_directory: String, } pub struct BashTool; @@ -33,7 +36,7 @@ impl Tool for BashTool { self: Arc, input: serde_json::Value, _messages: &[LanguageModelRequestMessage], - _project: Entity, + project: Entity, cx: &mut App, ) -> Task> { let input: BashToolInput = match serde_json::from_value(input) { @@ -41,23 +44,34 @@ impl Tool for BashTool { Err(err) => return Task::ready(Err(anyhow!(err))), }; + let Some(worktree) = project + .read(cx) + .worktree_for_root_name(&input.working_directory, cx) + else { + return Task::ready(Err(anyhow!("Working directory not found in the project"))); + }; + let working_directory = worktree.read(cx).abs_path(); + cx.spawn(|_| async move { - // Add 2>&1 to merge stderr into stdout for proper interleaving + // Add 2>&1 to merge stderr into stdout for proper interleaving. let command = format!("{} 2>&1", input.command); - // Spawn a blocking task to execute the command - let output = futures::executor::block_on(async { - std::process::Command::new("bash") - .arg("-c") - .arg(&command) - .output() - .map_err(|err| anyhow!("Failed to execute bash command: {}", err)) - })?; + let output = new_smol_command("bash") + .arg("-c") + .arg(&command) + .current_dir(working_directory) + .output() + .await + .context("Failed to execute bash command")?; let output_string = String::from_utf8_lossy(&output.stdout).to_string(); if output.status.success() { - Ok(output_string) + if output_string.is_empty() { + Ok("Command executed successfully.".to_string()) + } else { + Ok(output_string) + } } else { Ok(format!( "Command failed with exit code {}\n{}", diff --git a/crates/assistant_tools/src/bash_tool/description.md b/crates/assistant_tools/src/bash_tool/description.md index 596bbceae0..666a7c28c0 100644 --- a/crates/assistant_tools/src/bash_tool/description.md +++ b/crates/assistant_tools/src/bash_tool/description.md @@ -1 +1,5 @@ -Executes a bash one-liner and returns the combined output. This tool spawns a bash process, combines stdout and stderr into one interleaved stream as they are produced (preserving the order of writes), and captures that stream into a string which is returned. Use this tool when you need to run shell commands to get information about the system or process files. +Executes a bash one-liner and returns the combined output. + +This tool spawns a bash process, combines stdout and stderr into one interleaved stream as they are produced (preserving the order of writes), and captures that stream into a string which is returned. + +Remember that each invocation of this tool will spawn a new bash process, so you can't rely on any state from previous invocations. diff --git a/crates/assistant_tools/src/edit_files_tool.rs b/crates/assistant_tools/src/edit_files_tool.rs index 5862c99e1f..34c664588d 100644 --- a/crates/assistant_tools/src/edit_files_tool.rs +++ b/crates/assistant_tools/src/edit_files_tool.rs @@ -20,17 +20,40 @@ use util::ResultExt; #[derive(Debug, Serialize, Deserialize, JsonSchema)] pub struct EditFilesToolInput { - /// High-level edit instructions. These will be interpreted by a smaller model, - /// so explain the edits you want that model to make and to which files need changing. - /// The description should be concise and clear. We will show this description to the user - /// as well. + /// High-level edit instructions. These will be interpreted by a smaller + /// model, so explain the changes you want that model to make and which + /// file paths need changing. + /// + /// The description should be concise and clear. We will show this + /// description to the user as well. + /// + /// WARNING: When specifying which file paths need changing, you MUST + /// start each path with one of the project's root directories. + /// + /// WARNING: NEVER include code blocks or snippets in edit instructions. + /// Only provide natural language descriptions of the changes needed! The tool will + /// reject any instructions that contain code blocks or snippets. + /// + /// The following examples assume we have two root directories in the project: + /// - root-1 + /// - root-2 /// /// - /// If you want to rename a function you can say "Rename the function 'foo' to 'bar'". + /// If you want to introduce a new quit function to kill the process, your + /// instructions should be: "Add a new `quit` function to + /// `root-1/src/main.rs` to kill the process". + /// + /// Notice how the file path starts with root-1. Without that, the path + /// would be ambiguous and the call would fail! /// /// /// - /// If you want to add a new function you can say "Add a new method to the `User` struct that prints the age". + /// If you want to change documentation to always start with a capital + /// letter, your instructions should be: "In `root-2/db.js`, + /// `root-2/inMemory.js` and `root-2/sql.js`, change all the documentation + /// to start with a capital letter". + /// + /// Notice how we never specify code snippets in the instructions! /// pub edit_instructions: String, } @@ -212,7 +235,7 @@ impl EditFilesTool { project .update(&mut cx, |project, cx| { if let Some(file) = buffer.read(&cx).file() { - let _ = writeln!(&mut answer, "{}", &file.path().display()); + let _ = writeln!(&mut answer, "{}", &file.full_path(cx).display()); } project.save_buffer(buffer, cx) diff --git a/crates/assistant_tools/src/edit_files_tool/description.md b/crates/assistant_tools/src/edit_files_tool/description.md index a049076bd1..2f6c3ebe19 100644 --- a/crates/assistant_tools/src/edit_files_tool/description.md +++ b/crates/assistant_tools/src/edit_files_tool/description.md @@ -1,4 +1,4 @@ -Edit files in the current project. +Edit files in the current project by specifying instructions in natural language. When using this tool, you should suggest one coherent edit that can be made to the codebase. diff --git a/crates/assistant_tools/src/edit_files_tool/edit_prompt.md b/crates/assistant_tools/src/edit_files_tool/edit_prompt.md index 834b811981..fe5a4ca647 100644 --- a/crates/assistant_tools/src/edit_files_tool/edit_prompt.md +++ b/crates/assistant_tools/src/edit_files_tool/edit_prompt.md @@ -106,7 +106,7 @@ Every *SEARCH/REPLACE block* must use this format: 7. The end of the replace block: >>>>>>> REPLACE 8. The closing fence: ``` -Use the *FULL* file path, as shown to you by the user. +Use the *FULL* file path, as shown to you by the user. Make sure to include the project's root directory name at the start of the path. *NEVER* specify the absolute path of the file! Every *SEARCH* section must *EXACTLY MATCH* the existing file content, character for character, including all comments, docstrings, etc. If the file contains code or other data wrapped/escaped in json/xml/quotes or other containers, you need to propose edits to the literal contents of the file, including the container markup. diff --git a/crates/assistant_tools/src/list_directory_tool.rs b/crates/assistant_tools/src/list_directory_tool.rs index d572ecb60a..989ecdf993 100644 --- a/crates/assistant_tools/src/list_directory_tool.rs +++ b/crates/assistant_tools/src/list_directory_tool.rs @@ -12,10 +12,10 @@ pub struct ListDirectoryToolInput { /// The relative path of the directory to list. /// /// This path should never be absolute, and the first component - /// of the path should always be a top-level directory in a project. + /// of the path should always be a root directory in a project. /// /// - /// If the project has the following top-level directories: + /// If the project has the following root directories: /// /// - directory1 /// - directory2 @@ -24,7 +24,7 @@ pub struct ListDirectoryToolInput { /// /// /// - /// If the project has the following top-level directories: + /// If the project has the following root directories: /// /// - foo /// - bar @@ -72,8 +72,18 @@ impl Tool for ListDirectoryTool { return Task::ready(Err(anyhow!("Directory not found in the project"))); }; let path = input.path.strip_prefix(worktree_root_name).unwrap(); + let worktree = worktree.read(cx); + + let Some(entry) = worktree.entry_for_path(path) else { + return Task::ready(Err(anyhow!("Path not found: {}", input.path.display()))); + }; + + if !entry.is_dir() { + return Task::ready(Err(anyhow!("{} is a file.", input.path.display()))); + } + let mut output = String::new(); - for entry in worktree.read(cx).child_entries(path) { + for entry in worktree.child_entries(path) { writeln!( output, "{}", @@ -83,6 +93,9 @@ impl Tool for ListDirectoryTool { ) .unwrap(); } + if output.is_empty() { + return Task::ready(Ok(format!("{} is empty.", input.path.display()))); + } Task::ready(Ok(output)) } } diff --git a/crates/assistant_tools/src/path_search_tool.rs b/crates/assistant_tools/src/path_search_tool.rs index 42ca8fa12c..00ac703216 100644 --- a/crates/assistant_tools/src/path_search_tool.rs +++ b/crates/assistant_tools/src/path_search_tool.rs @@ -13,7 +13,7 @@ pub struct PathSearchToolInput { /// The glob to search all project paths for. /// /// - /// If the project has the following top-level directories: + /// If the project has the following root directories: /// /// - directory1/a/something.txt /// - directory2/a/things.txt diff --git a/crates/assistant_tools/src/read_file_tool.rs b/crates/assistant_tools/src/read_file_tool.rs index c23f52b423..53d73f2064 100644 --- a/crates/assistant_tools/src/read_file_tool.rs +++ b/crates/assistant_tools/src/read_file_tool.rs @@ -14,10 +14,10 @@ pub struct ReadFileToolInput { /// The relative path of the file to read. /// /// This path should never be absolute, and the first component - /// of the path should always be a top-level directory in a project. + /// of the path should always be a root directory in a project. /// /// - /// If the project has the following top-level directories: + /// If the project has the following root directories: /// /// - directory1 /// - directory2 diff --git a/crates/prompt_store/src/prompts.rs b/crates/prompt_store/src/prompts.rs index deb9465563..4c7630b6a1 100644 --- a/crates/prompt_store/src/prompts.rs +++ b/crates/prompt_store/src/prompts.rs @@ -7,13 +7,24 @@ use handlebars::{Handlebars, RenderError}; use language::{BufferSnapshot, LanguageName, Point}; use parking_lot::Mutex; use serde::Serialize; -use std::{ops::Range, path::PathBuf, sync::Arc, time::Duration}; +use std::{ + ops::Range, + path::{Path, PathBuf}, + sync::Arc, + time::Duration, +}; use text::LineEnding; use util::ResultExt; #[derive(Serialize)] pub struct AssistantSystemPromptContext { - pub worktree_root_names: Vec, + pub worktrees: Vec, +} + +#[derive(Serialize)] +pub struct AssistantSystemPromptWorktree { + pub root_name: String, + pub abs_path: Arc, } #[derive(Serialize)] @@ -223,11 +234,9 @@ impl PromptBuilder { pub fn generate_assistant_system_prompt( &self, - worktree_root_names: Vec, + worktrees: Vec, ) -> Result { - let prompt = AssistantSystemPromptContext { - worktree_root_names, - }; + let prompt = AssistantSystemPromptContext { worktrees }; self.handlebars .lock() .render("assistant_system_prompt", &prompt)