From 377708befadeb3b3b88fbf50622ce59e73c97f10 Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Mon, 1 Jun 2026 17:29:15 -0400 Subject: [PATCH] agent: Enable terminal output controls for everyone (#58271) Removes the feature flag around terminal output selection so the terminal tool always exposes head_lines and tail_lines. This keeps the simpler behavior from the landed implementation while avoiding a separate rollout path for the tool schema. Release Notes: - Improved agent terminal output controls by making them available without a feature flag --- crates/agent/src/tests/mod.rs | 89 --------------- crates/agent/src/thread.rs | 18 +-- crates/agent/src/tools/terminal_tool.rs | 144 ------------------------ crates/feature_flags/src/flags.rs | 12 -- 4 files changed, 5 insertions(+), 258 deletions(-) diff --git a/crates/agent/src/tests/mod.rs b/crates/agent/src/tests/mod.rs index dbe62e4cf2..47c4ee61f7 100644 --- a/crates/agent/src/tests/mod.rs +++ b/crates/agent/src/tests/mod.rs @@ -5863,95 +5863,6 @@ async fn test_max_subagent_depth_prevents_tool_registration(cx: &mut TestAppCont }); } -#[gpui::test] -async fn test_terminal_tail_tool_schema_gated_by_feature_flag(cx: &mut TestAppContext) { - init_test(cx); - - let fs = FakeFs::new(cx.executor()); - fs.insert_tree(path!("/test"), json!({})).await; - let project = Project::test(fs, [path!("/test").as_ref()], cx).await; - let project_context = cx.new(|_cx| ProjectContext::default()); - let context_server_store = project.read_with(cx, |project, _| project.context_server_store()); - let context_server_registry = cx.new(|cx| ContextServerRegistry::new(context_server_store, cx)); - let model = Arc::new(FakeLanguageModel::default()); - - let thread_without_flag = cx.new(|cx| { - let environment = Rc::new( - FakeThreadEnvironment::default().with_terminal(FakeTerminalHandle::new_never_exits(cx)), - ); - let mut thread = Thread::new( - project.clone(), - project_context.clone(), - context_server_registry.clone(), - Templates::new(), - Some(model.clone() as Arc), - cx, - ); - thread.add_default_tools(environment, cx); - thread - }); - - thread_without_flag - .update(cx, |thread, cx| { - thread.send(UserMessageId::new(), ["hello"], cx) - }) - .unwrap(); - cx.run_until_parked(); - - let completion = model.pending_completions().pop().unwrap(); - let terminal_tool = completion - .tools - .iter() - .find(|tool| tool.name == TerminalTool::NAME) - .expect("terminal tool should be present"); - let schema_text = terminal_tool.input_schema.to_string(); - assert!(!schema_text.contains("head_lines")); - assert!(!schema_text.contains("tail_lines")); - assert!(!terminal_tool.description.contains("head_lines")); - assert!(!terminal_tool.description.contains("tail_lines")); - model.end_last_completion_stream(); - cx.run_until_parked(); - - cx.update(|cx| { - cx.update_flags(false, vec!["terminal-tail".to_string()]); - }); - - let thread_with_flag = cx.new(|cx| { - let environment = Rc::new( - FakeThreadEnvironment::default().with_terminal(FakeTerminalHandle::new_never_exits(cx)), - ); - let mut thread = Thread::new( - project, - project_context, - context_server_registry, - Templates::new(), - Some(model.clone() as Arc), - cx, - ); - thread.add_default_tools(environment, cx); - thread - }); - - thread_with_flag - .update(cx, |thread, cx| { - thread.send(UserMessageId::new(), ["hello again"], cx) - }) - .unwrap(); - cx.run_until_parked(); - - let completion = model.pending_completions().pop().unwrap(); - let terminal_tool = completion - .tools - .iter() - .find(|tool| tool.name == TerminalTool::NAME) - .expect("terminal tool should be present"); - let schema_text = terminal_tool.input_schema.to_string(); - assert!(schema_text.contains("head_lines")); - assert!(schema_text.contains("tail_lines")); - assert!(terminal_tool.description.contains("head_lines")); - assert!(terminal_tool.description.contains("tail_lines")); -} - #[gpui::test] async fn test_lsp_tools_gated_by_feature_flag(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index a31e2ed70e..96aeca7ff2 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -3,16 +3,15 @@ use crate::{ DbLanguageModel, DbThread, DeletePathTool, DiagnosticsTool, EditFileTool, FetchTool, FindPathTool, FindReferencesTool, GetCodeActionsTool, GoToDefinitionTool, GrepTool, ListDirectoryTool, MovePathTool, ProjectSnapshot, ReadFileTool, RenameTool, SpawnAgentTool, - SystemPromptTemplate, Template, Templates, TerminalTool, TerminalToolWithoutTail, - ToolPermissionDecision, UpdatePlanTool, UpdateTitleTool, WebSearchTool, WriteFileTool, - decide_permission_from_settings, + SystemPromptTemplate, Template, Templates, TerminalTool, ToolPermissionDecision, + UpdatePlanTool, UpdateTitleTool, WebSearchTool, WriteFileTool, decide_permission_from_settings, }; use acp_thread::{MentionUri, UserMessageId}; use action_log::ActionLog; use agent_settings::UserAgentsMd; use feature_flags::{ - FeatureFlagAppExt as _, LspToolFeatureFlag, RenameToolFeatureFlag, TerminalTailFeatureFlag, - UpdatePlanToolFeatureFlag, UpdateTitleToolFeatureFlag, + FeatureFlagAppExt as _, LspToolFeatureFlag, RenameToolFeatureFlag, UpdatePlanToolFeatureFlag, + UpdateTitleToolFeatureFlag, }; use agent_client_protocol::schema as acp; @@ -1752,14 +1751,7 @@ impl Thread { self.action_log.clone(), update_agent_location, )); - if cx.has_flag::() { - self.add_tool(TerminalTool::new(self.project.clone(), environment.clone())); - } else { - self.add_tool(TerminalToolWithoutTail::new( - self.project.clone(), - environment.clone(), - )); - } + self.add_tool(TerminalTool::new(self.project.clone(), environment.clone())); self.add_tool(WebSearchTool); self.add_tool(DiagnosticsTool::new(self.project.clone())); diff --git a/crates/agent/src/tools/terminal_tool.rs b/crates/agent/src/tools/terminal_tool.rs index ca25834e4f..3b591d4449 100644 --- a/crates/agent/src/tools/terminal_tool.rs +++ b/crates/agent/src/tools/terminal_tool.rs @@ -89,83 +89,6 @@ pub struct TerminalToolInput { pub unsandboxed: Option, } -/// Executes a shell one-liner and returns the combined output. -/// -/// This tool spawns a process using the user's shell, reads from stdout and stderr (preserving the order of writes), and returns a string with the combined output result. -/// -/// The output results will be shown to the user already, only list it again if necessary, avoid being redundant. -/// -/// Make sure you use the `cd` parameter to navigate to one of the root directories of the project. NEVER do it as part of the `command` itself, otherwise it will error. -/// -/// Do not generate terminal commands that use shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`. Resolve those values yourself before calling this tool, or ask the user for the literal value to use. -/// -/// Do not use this tool for commands that run indefinitely, such as servers (like `npm run start`, `npm run dev`, `python -m http.server`, etc) or file watchers that don't terminate on their own. -/// -/// For potentially long-running commands, prefer specifying `timeout_ms` to bound runtime and prevent indefinite hangs. -/// -/// Remember that each invocation of this tool will spawn a new shell process, so you can't rely on any state from previous invocations. -/// -/// The terminal is an interactive pty, so any command that blocks waiting for input will hang the tool until it times out. To avoid this: -/// -/// - Always insert `--no-pager` immediately after `git` for any read-only git command, including `git log`, `git diff`, `git show`, `git blame`, and `git stash show`. Example: `git --no-pager log -n 5` (NOT `git log -n 5`). -/// - Always prepend `GIT_EDITOR=true ` to any git command that may invoke an editor, including `git rebase`, `git commit`, `git merge`, and `git tag`. Example: `GIT_EDITOR=true git rebase origin/main` (NOT `git rebase origin/main`). -/// - For other commands that may open a pager or editor, set `PAGER=cat` and/or `EDITOR=true` similarly. -#[derive(Clone, Debug, Default, Serialize, Deserialize, JsonSchema)] -pub struct TerminalToolInputWithoutTail { - /// The one-liner command to execute. Do not include shell substitutions or interpolations such as `$VAR`, `${VAR}`, `$(...)`, backticks, `$((...))`, `<(...)`, or `>(...)`; resolve those values first or ask the user. - /// - /// REMINDER: read-only git commands (`git log`, `git diff`, `git show`, `git blame`) MUST include `--no-pager` (e.g. `git --no-pager log`). Git commands that may open an editor (`git rebase`, `git commit`, `git merge`, `git tag`) MUST be prefixed with `GIT_EDITOR=true ` (e.g. `GIT_EDITOR=true git rebase origin/main`). Otherwise the terminal will hang. - pub command: String, - /// Working directory for the command. This must be one of the root directories of the project. - pub cd: String, - /// Optional maximum runtime (in milliseconds). If exceeded, the running terminal task is killed. - pub timeout_ms: Option, - /// Request network access for this command. - /// - /// Only meaningful when the system prompt's "Terminal sandbox" section - /// is present — ignored otherwise. By default sandboxed commands - /// cannot make outbound network connections; set this to `true` only - /// when the command needs network access. The user will be prompted - /// to approve before the command runs. - #[serde(default)] - pub allow_network: Option, - /// Request unrestricted filesystem-write access for this command. - /// - /// Only meaningful when the system prompt's "Terminal sandbox" section - /// is present — ignored otherwise. By default sandboxed commands can - /// only write to the project worktree directories and a per-command - /// temporary directory; set this to `true` only when the command - /// needs to write elsewhere. The user will be prompted to approve - /// before the command runs. - #[serde(default)] - pub allow_fs_write: Option, - /// Request to run this command outside the sandbox entirely. - /// - /// Only meaningful when the system prompt's "Terminal sandbox" section - /// is present — ignored otherwise. Prefer `allow_network: true` or - /// `allow_fs_write: true` when one of those is enough. Set this to - /// `true` ONLY when the command needs behavior that the sandbox can't - /// grant on a per-permission basis. The user will be prompted to - /// approve before the command runs without sandbox restrictions. - #[serde(default)] - pub unsandboxed: Option, -} - -impl From for TerminalToolInput { - fn from(input: TerminalToolInputWithoutTail) -> Self { - Self { - command: input.command, - cd: input.cd, - timeout_ms: input.timeout_ms, - head_lines: None, - tail_lines: None, - allow_network: input.allow_network, - allow_fs_write: input.allow_fs_write, - unsandboxed: input.unsandboxed, - } - } -} - pub struct TerminalTool { project: Entity, environment: Rc, @@ -180,20 +103,6 @@ impl TerminalTool { } } -pub struct TerminalToolWithoutTail { - project: Entity, - environment: Rc, -} - -impl TerminalToolWithoutTail { - pub fn new(project: Entity, environment: Rc) -> Self { - Self { - project, - environment, - } - } -} - impl AgentTool for TerminalTool { type Input = TerminalToolInput; type Output = String; @@ -232,44 +141,6 @@ impl AgentTool for TerminalTool { } } -impl AgentTool for TerminalToolWithoutTail { - type Input = TerminalToolInputWithoutTail; - type Output = String; - - const NAME: &'static str = "terminal"; - - fn kind() -> acp::ToolKind { - acp::ToolKind::Execute - } - - fn initial_title( - &self, - input: Result, - _cx: &mut App, - ) -> SharedString { - terminal_initial_title(input.map(|input| input.command)) - } - - fn run( - self: Arc, - input: ToolInput, - event_stream: ToolCallEventStream, - cx: &mut App, - ) -> Task> { - cx.spawn(async move |cx| { - let input = input.recv().await.map_err(|e| e.to_string())?; - run_terminal_tool( - self.project.clone(), - self.environment.clone(), - input.into(), - event_stream, - cx, - ) - .await - }) - } -} - fn terminal_initial_title(input: Result) -> SharedString { if let Ok(command) = input { command.into() @@ -1710,21 +1581,6 @@ mod tests { assert!(schema_text.contains("waste tokens or exceed the context window")); } - #[test] - fn test_terminal_tool_without_tail_schema_omits_head_and_tail_parameters() { - let description = ::description().to_string(); - let schema = ::input_schema( - language_model::LanguageModelToolSchemaFormat::JsonSchema, - ); - let schema_json = serde_json::to_value(schema).expect("schema should serialize"); - let schema_text = schema_json.to_string(); - - assert!(!description.contains("head_lines")); - assert!(!description.contains("tail_lines")); - assert!(!schema_text.contains("head_lines")); - assert!(!schema_text.contains("tail_lines")); - } - async fn assert_rejected_before_terminal_creation( command: &str, cx: &mut gpui::TestAppContext, diff --git a/crates/feature_flags/src/flags.rs b/crates/feature_flags/src/flags.rs index 4936c66ac1..f6febdc5c4 100644 --- a/crates/feature_flags/src/flags.rs +++ b/crates/feature_flags/src/flags.rs @@ -162,15 +162,3 @@ impl FeatureFlag for SandboxingFeatureFlag { } } register_feature_flag!(SandboxingFeatureFlag); - -pub struct TerminalTailFeatureFlag; - -impl FeatureFlag for TerminalTailFeatureFlag { - const NAME: &'static str = "terminal-tail"; - type Value = PresenceFlag; - - fn enabled_for_staff() -> bool { - false - } -} -register_feature_flag!(TerminalTailFeatureFlag);