From c12e6376b847432fa3452f4de833d81ab502d4a0 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Mon, 5 May 2025 15:57:03 -0400 Subject: [PATCH] Terminal tool improvements (#29924) WIP - On macOS/Linux, run the command in bash instead of the user's shell - Try to prevent the agent from running commands that expect interaction Release Notes: - Agent Beta: Switched to using `bash` (if available) instead of the user's shell when calling the terminal tool. - Agent Beta: Prevented the agent from hanging when trying to run interactive commands. --------- Co-authored-by: WeetHet --- Cargo.lock | 2 + crates/assistant_tools/Cargo.toml | 2 + crates/assistant_tools/src/assistant_tools.rs | 2 +- crates/assistant_tools/src/terminal_tool.rs | 150 +++++++++++++++--- crates/eval/src/examples/planets.rs | 2 +- crates/project/src/project.rs | 10 ++ 6 files changed, 143 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 13c4fc06e1..dbfd36f6da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -735,6 +735,7 @@ dependencies = [ "language_model", "language_models", "linkme", + "log", "open", "paths", "portable-pty", @@ -761,6 +762,7 @@ dependencies = [ "unindent", "util", "web_search", + "which 6.0.3", "workspace", "workspace-hack", "zed_llm_client", diff --git a/crates/assistant_tools/Cargo.toml b/crates/assistant_tools/Cargo.toml index f6739e52f7..12f0b2685a 100644 --- a/crates/assistant_tools/Cargo.toml +++ b/crates/assistant_tools/Cargo.toml @@ -35,6 +35,7 @@ indoc.workspace = true itertools.workspace = true language.workspace = true language_model.workspace = true +log.workspace = true linkme.workspace = true open.workspace = true paths.workspace = true @@ -56,6 +57,7 @@ theme.workspace = true ui.workspace = true util.workspace = true web_search.workspace = true +which.workspace = true workspace-hack.workspace = true workspace.workspace = true zed_llm_client.workspace = true diff --git a/crates/assistant_tools/src/assistant_tools.rs b/crates/assistant_tools/src/assistant_tools.rs index 4b95b807d9..bd6b5528a3 100644 --- a/crates/assistant_tools/src/assistant_tools.rs +++ b/crates/assistant_tools/src/assistant_tools.rs @@ -61,7 +61,7 @@ pub fn init(http_client: Arc, cx: &mut App) { assistant_tool::init(cx); let registry = ToolRegistry::global(cx); - registry.register_tool(TerminalTool); + registry.register_tool(TerminalTool::new(cx)); registry.register_tool(CreateDirectoryTool); registry.register_tool(CopyPathTool); registry.register_tool(DeletePathTool); diff --git a/crates/assistant_tools/src/terminal_tool.rs b/crates/assistant_tools/src/terminal_tool.rs index f2f43ac5b9..a82b80298b 100644 --- a/crates/assistant_tools/src/terminal_tool.rs +++ b/crates/assistant_tools/src/terminal_tool.rs @@ -1,6 +1,7 @@ use crate::schema::json_schema_for; use anyhow::{Context as _, Result, anyhow, bail}; use assistant_tool::{ActionLog, Tool, ToolCard, ToolResult, ToolUseStatus}; +use futures::{FutureExt as _, future::Shared}; use gpui::{AnyWindowHandle, App, AppContext, Empty, Entity, EntityId, Task, WeakEntity, Window}; use language::LineEnding; use language_model::{LanguageModelRequestMessage, LanguageModelToolSchemaFormat}; @@ -33,11 +34,37 @@ pub struct TerminalToolInput { cd: String, } -pub struct TerminalTool; +pub struct TerminalTool { + determine_shell: Shared>, +} + +impl TerminalTool { + pub const NAME: &str = "terminal"; + + pub(crate) fn new(cx: &mut App) -> Self { + let determine_shell = cx.background_spawn(async move { + if cfg!(windows) { + return get_system_shell(); + } + + if which::which("bash").is_ok() { + log::info!("agent selected bash for terminal tool"); + "bash".into() + } else { + let shell = get_system_shell(); + log::info!("agent selected {shell} for terminal tool"); + shell + } + }); + Self { + determine_shell: determine_shell.shared(), + } + } +} impl Tool for TerminalTool { fn name(&self) -> String { - "terminal".to_string() + Self::NAME.to_string() } fn needs_confirmation(&self, _: &serde_json::Value, _: &App) -> bool { @@ -96,17 +123,37 @@ impl Tool for TerminalTool { Ok(dir) => dir, Err(err) => return Task::ready(Err(err)).into(), }; - let command = get_system_shell(); - let args = vec!["-c".into(), input.command.clone()]; + let program = self.determine_shell.clone(); + let command = format!("({}) project.update(cx, |project, cx| { + project.directory_environment(dir.as_path().into(), cx) + }), + None => Task::ready(None).shared(), + }; + + let env = cx.spawn(async move |_| { + let mut env = env.await.unwrap_or_default(); + if cfg!(unix) { + env.insert("PAGER".into(), "cat".into()); + } + env + }); let Some(window) = window else { // Headless setup, a test or eval. Our terminal subsystem requires a workspace, // so bypass it and provide a convincing imitation using a pty. let task = cx.background_spawn(async move { + let env = env.await; let pty_system = native_pty_system(); - let mut cmd = CommandBuilder::new(command); + let program = program.await; + let mut cmd = CommandBuilder::new(program); cmd.args(args); + for (k, v) in env { + cmd.env(k, v); + } if let Some(cwd) = cwd { cmd.cwd(cwd); } @@ -139,17 +186,28 @@ impl Tool for TerminalTool { }; }; - let terminal = project.update(cx, |project, cx| { - project.create_terminal( - TerminalKind::Task(task::SpawnInTerminal { - command, - args, - cwd, - ..Default::default() - }), - window, - cx, - ) + let terminal = cx.spawn({ + let project = project.downgrade(); + async move |cx| { + let program = program.await; + let env = env.await; + let terminal = project + .update(cx, |project, cx| { + project.create_terminal( + TerminalKind::Task(task::SpawnInTerminal { + command: program, + args, + cwd, + env, + ..Default::default() + }), + window, + cx, + ) + })? + .await; + terminal + } }); let card = cx.new(|cx| { @@ -549,14 +607,10 @@ mod tests { use super::*; - #[gpui::test] - async fn test_working_directory(executor: BackgroundExecutor, cx: &mut TestAppContext) { - if cfg!(windows) { - return; - } - + fn init_test(executor: &BackgroundExecutor, cx: &mut TestAppContext) { zlog::init(); zlog::init_output_stdout(); + executor.allow_parking(); cx.update(|cx| { let settings_store = SettingsStore::test(cx); @@ -568,6 +622,56 @@ mod tests { TerminalSettings::register(cx); EditorSettings::register(cx); }); + } + + #[gpui::test] + async fn test_interactive_command(executor: BackgroundExecutor, cx: &mut TestAppContext) { + if cfg!(windows) { + return; + } + + init_test(&executor, cx); + + let fs = Arc::new(RealFs::new(None, executor)); + let tree = TempTree::new(json!({ + "project": {}, + })); + let project: Entity = + Project::test(fs, [tree.path().join("project").as_path()], cx).await; + let action_log = cx.update(|cx| cx.new(|_| ActionLog::new(project.clone()))); + + let input = TerminalToolInput { + command: "cat".to_owned(), + cd: tree + .path() + .join("project") + .as_path() + .to_string_lossy() + .to_string(), + }; + let result = cx.update(|cx| { + TerminalTool::run( + Arc::new(TerminalTool::new(cx)), + serde_json::to_value(input).unwrap(), + &[], + project.clone(), + action_log.clone(), + None, + cx, + ) + }); + + let output = result.output.await.log_err(); + assert_eq!(output, Some("Command executed successfully.".into())); + } + + #[gpui::test] + async fn test_working_directory(executor: BackgroundExecutor, cx: &mut TestAppContext) { + if cfg!(windows) { + return; + } + + init_test(&executor, cx); let fs = Arc::new(RealFs::new(None, executor)); let tree = TempTree::new(json!({ @@ -580,7 +684,7 @@ mod tests { let check = |input, expected, cx: &mut App| { let headless_result = TerminalTool::run( - Arc::new(TerminalTool), + Arc::new(TerminalTool::new(cx)), serde_json::to_value(input).unwrap(), &[], project.clone(), diff --git a/crates/eval/src/examples/planets.rs b/crates/eval/src/examples/planets.rs index 9ccd077f94..f1b361652a 100644 --- a/crates/eval/src/examples/planets.rs +++ b/crates/eval/src/examples/planets.rs @@ -38,7 +38,7 @@ impl Example for Planets { for tool_use in response.tool_uses() { if tool_use.name == OpenTool.name() { open_tool_uses += 1; - } else if tool_use.name == TerminalTool.name() { + } else if tool_use.name == TerminalTool::NAME { terminal_tool_uses += 1; } } diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index d5ba6614a0..ba388186a4 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -1656,6 +1656,16 @@ impl Project { }) } + pub fn directory_environment( + &self, + abs_path: Arc, + cx: &mut App, + ) -> Shared>>> { + self.environment.update(cx, |environment, cx| { + environment.get_directory_environment(abs_path, cx) + }) + } + pub fn shell_environment_errors<'a>( &'a self, cx: &'a App,