From bf8dfae583b69293680cab19cdec88d10c2db3b4 Mon Sep 17 00:00:00 2001 From: MartinYe1234 <52641447+MartinYe1234@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:44:42 -0700 Subject: [PATCH] agent: Fix terminal_init commands race condition with PTY startup (#59613) Summary: - Add a shell-level startup handshake owned by the terminal. - Send a split startup marker through the shell and run the configured init command after the marker is observed, preventing local echo from satisfying the handshake. - Keep display-only/restored terminal behavior unchanged while preserving startup output when shells fail or users type first. Tests: - `cargo test -p terminal test_init_command_startup_marker -- --nocapture` - `cargo test -p terminal write_init_command_after_startup -- --nocapture` - `cargo test -p agent_ui init_command -- --nocapture` - `./script/clippy -p terminal -p agent_ui` Closes AI-426 Release Notes: - Fixed terminal init commands sometimes appearing or running incorrectly when using custom shells --------- Co-authored-by: Anant Goel --- crates/agent_ui/src/agent_panel.rs | 86 ++++++-- crates/terminal/src/terminal.rs | 311 ++++++++++++++++++++++++++++- 2 files changed, 379 insertions(+), 18 deletions(-) diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index 6920e9b75a..ffdcfc890b 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -75,6 +75,7 @@ use feature_flags::{ }; use fs::Fs; +use futures::FutureExt as _; use gpui::{ Action, Anchor, Animation, AnimationExt, AnyElement, App, AsyncWindowContext, ClipboardItem, Entity, EventEmitter, ExternalPaths, FocusHandle, Focusable, KeyContext, Pixels, @@ -109,6 +110,7 @@ const MIN_PANEL_WIDTH: Pixels = px(300.); const LAST_USED_AGENT_KEY: &str = "agent_panel__last_used_external_agent"; const LAST_CREATED_ENTRY_KIND_KEY: &str = "agent_panel__last_created_entry_kind"; const TERMINAL_AGENT_TELEMETRY_ID: &str = "terminal"; +const TERMINAL_INIT_COMMAND_STARTUP_TIMEOUT: Duration = Duration::from_secs(5); const KNOWN_TERMINAL_AGENT_COMMANDS: &[&str] = &[ "agent", // Unfortunately, both Cursor cli + grok "agy", @@ -2058,25 +2060,63 @@ impl AgentPanel { run_init_command .then(|| AgentSettings::get_global(cx).terminal_init_command.clone()) .flatten() + .filter(|command| !command.trim().is_empty()) } fn write_terminal_init_command( terminal: &Entity, init_command: Option, - cx: &mut App, + cx: &mut Context, ) { let Some(command) = init_command else { return; }; + + if !terminal.read(cx).is_pty() { + terminal.update(cx, |terminal, _| { + terminal.write_init_command(Self::terminal_init_command_input(command)) + }); + return; + } + + let startup = terminal.update(cx, |terminal, _| { + terminal.start_init_command_startup_handshake() + }); + + let terminal = terminal.downgrade(); + cx.spawn(async move |_this, cx| { + // Fall back to the timeout so the init command is still delivered if + // the shell never echoes the marker. + let timeout = cx + .background_executor() + .timer(TERMINAL_INIT_COMMAND_STARTUP_TIMEOUT); + futures::select_biased! { + _ = startup.fuse() => {} + _ = timeout.fuse() => {} + } + + let input = Self::terminal_init_command_input(command); + if let Err(error) = terminal.update(cx, move |terminal, cx| { + if !terminal.write_init_command_after_startup(input, cx) { + log::debug!( + "skipping terminal init command because the terminal is no longer eligible" + ); + } + }) { + log::debug!("skipping terminal init command because the terminal closed: {error}"); + } + anyhow::Ok(()) + }) + .detach_and_log_err(cx); + } + + fn terminal_init_command_input(command: String) -> Vec { let mut input = command.into_bytes(); // CR, not "\r\n": "\r\n" puts PowerShell into continuation // mode (same convention as the activation-script writes in // `TerminalBuilder::new`). input.push(b'\x0d'); - // `write_init_command`, not `input`: the latter sets `keyboard_input_sent`, - // which would auto-close the terminal (hiding the error) if the shell - // fails to spawn after we've written the command. - terminal.update(cx, |terminal, _| terminal.write_init_command(input)); + input } fn insert_terminal( @@ -7498,15 +7538,12 @@ mod tests { cx.executor().allow_parking(); cx.update(|_, cx| { let mut settings = AgentSettings::get_global(cx).clone(); - // The output (`init_ran_42`) is distinct from the command text - // (`init_ran_$((6*7))`), which the PTY also echoes back. Finding the - // output therefore proves the shell actually executed the command - // rather than merely echoing the keystrokes. - settings.terminal_init_command = Some("echo init_ran_$((6*7))".to_string()); + // `init_ran_42` is the command's output, not its echoed text, so finding + // it proves the shell executed the command rather than just echoing it. + settings.terminal_init_command = Some("printf 'init_ran_%s\\n' 42".to_string()); AgentSettings::override_global(settings, cx); - // Force a POSIX shell rather than relying on the developer's login - // shell, which may not support `$((...))` arithmetic (e.g. fish). + // Force a known POSIX shell so the test doesn't depend on the developer's login shell. let mut terminal_settings = TerminalSettings::get_global(cx).clone(); terminal_settings.shell = task::Shell::Program("/bin/sh".to_string()); TerminalSettings::override_global(terminal_settings, cx); @@ -7536,6 +7573,7 @@ mod tests { // sleep, matching the real-PTY test in `acp_thread`. let deadline = Instant::now() + Duration::from_secs(10); let terminal = loop { + cx.run_until_parked(); let terminal = panel.read_with(&cx, |panel, cx| { panel .terminals @@ -7549,13 +7587,29 @@ mod tests { { break terminal.clone(); } - assert!( - Instant::now() < deadline, - "init command output never appeared in the terminal" - ); + if Instant::now() >= deadline { + let terminal_created = terminal.is_some(); + let (content, input_log) = if let Some(terminal) = terminal { + let content = terminal.read_with(&cx, |terminal, _| terminal.get_content()); + let input_log = + terminal.update(&mut cx, |terminal, _| terminal.take_input_log()); + (content, input_log) + } else { + (String::new(), Vec::new()) + }; + panic!( + "init command output never appeared in the terminal; terminal_created={terminal_created}, content={content:?}, input_log={input_log:?}" + ); + } cx.executor().timer(Duration::from_millis(50)).await; }; + let input_log = terminal.update(&mut cx, |terminal, _| terminal.take_input_log()); + assert_eq!( + input_log, + vec![b"printf 'init_ran_%s\\n' 42\r".to_vec()], + "init command should be written only after terminal startup has settled" + ); assert!( !terminal.read_with(&cx, |terminal, _| terminal.keyboard_input_sent()), "writing the init command must not mark the terminal as having received \ diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index 60c9925550..b74c79d413 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -27,7 +27,7 @@ use futures::StreamExt; use pty_info::{ProcessIdGetter, PtyProcessInfo}; use serde::{Deserialize, Serialize}; use settings::Settings; -use task::{HideStrategy, Shell, SpawnInTerminal}; +use task::{HideStrategy, Shell, ShellKind, SpawnInTerminal}; use terminal_settings::{AlternateScroll, CursorShape as SettingsCursorShape, TerminalSettings}; use theme::{ActiveTheme, Theme}; use urlencoding; @@ -42,7 +42,10 @@ use std::{ ops::{BitOr, BitOrAssign, Deref, Range as StdRange}, path::{Path, PathBuf}, process::ExitStatus, - sync::Arc, + sync::{ + Arc, + atomic::{AtomicU64, Ordering}, + }, time::{Duration, Instant}, }; use thiserror::Error; @@ -850,6 +853,44 @@ impl Display for TerminalError { // https://github.com/alacritty/alacritty/blob/cb3a79dbf6472740daca8440d5166c1d4af5029e/extra/man/alacritty.5.scd?plain=1#L207-L213 const DEFAULT_SCROLL_HISTORY_LINES: usize = 10_000; pub const MAX_SCROLL_HISTORY_LINES: usize = 100_000; +static NEXT_INIT_COMMAND_STARTUP_MARKER_ID: AtomicU64 = AtomicU64::new(1); + +const INIT_COMMAND_STARTUP_MARKER_PREFIX: &str = "__zed_init_command_ready_"; +const INIT_COMMAND_STARTUP_MARKER_SUFFIX: &str = "__"; +const INIT_COMMAND_STARTUP_MARKER_SEARCH_LINES: usize = 64; + +fn init_command_startup_marker(marker_id: u64) -> String { + format!("{INIT_COMMAND_STARTUP_MARKER_PREFIX}{marker_id}{INIT_COMMAND_STARTUP_MARKER_SUFFIX}") +} + +fn init_command_startup_marker_command(shell_kind: ShellKind, marker_id: u64) -> String { + // Split the marker across the command so its echo can't satisfy the + // handshake; only the command's output contains the contiguous marker. + match shell_kind { + ShellKind::PowerShell | ShellKind::Pwsh => format!( + "Write-Output ('{INIT_COMMAND_STARTUP_MARKER_PREFIX}' + '{marker_id}' + '{INIT_COMMAND_STARTUP_MARKER_SUFFIX}')" + ), + ShellKind::Cmd => { + format!( + " { + format!( + "print $\"{INIT_COMMAND_STARTUP_MARKER_PREFIX}({marker_id}){INIT_COMMAND_STARTUP_MARKER_SUFFIX}\"" + ) + } + ShellKind::Posix + | ShellKind::Csh + | ShellKind::Tcsh + | ShellKind::Rc + | ShellKind::Fish + | ShellKind::Xonsh + | ShellKind::Elvish => format!( + "printf '%s%s%s\\n' {INIT_COMMAND_STARTUP_MARKER_PREFIX} {marker_id} {INIT_COMMAND_STARTUP_MARKER_SUFFIX}" + ), + } +} pub struct TerminalBuilder { terminal: Terminal, @@ -937,6 +978,8 @@ impl TerminalBuilder { }, child_exited: None, keyboard_input_sent: false, + init_command_startup_marker: None, + init_command_startup_tx: None, event_loop_task: Task::ready(Ok(())), background_executor: background_executor.clone(), path_style, @@ -1160,6 +1203,8 @@ impl TerminalBuilder { }, child_exited: None, keyboard_input_sent: false, + init_command_startup_marker: None, + init_command_startup_tx: None, event_loop_task: Task::ready(Ok(())), background_executor, path_style, @@ -1319,6 +1364,8 @@ pub struct Terminal { activation_script: Vec, child_exited: Option, keyboard_input_sent: bool, + init_command_startup_marker: Option, + init_command_startup_tx: Option>, event_loop_task: Task>, background_executor: BackgroundExecutor, path_style: PathStyle, @@ -1431,6 +1478,7 @@ impl Terminal { //NOOP, Handled in render } TerminalBackendEvent::Wakeup => { + self.detect_init_command_startup_marker(); cx.emit(Event::Wakeup); if let TerminalType::Pty { info, .. } = &self.terminal_type { @@ -1716,6 +1764,8 @@ impl Terminal { let mut term = self.term.lock(); self.output_processor.advance(&mut *term, &converted); + drop(term); + self.detect_init_command_startup_marker(); cx.emit(Event::Wakeup); } @@ -1863,9 +1913,72 @@ impl Terminal { pub fn input(&mut self, input: impl Into>) { self.keyboard_input_sent = true; + self.complete_init_command_startup_handshake(); self.write_input(input); } + /// Sends a shell-level marker command and returns a task that completes when + /// the marker appears in terminal output. Already complete for non-PTY + /// terminals or those whose child has exited. + /// + /// Call at most once per terminal: a second handshake drops the previous + /// `Sender`, which would write the init command twice. + pub fn start_init_command_startup_handshake(&mut self) -> Task<()> { + if !self.is_pty() || self.child_exited.is_some() { + return Task::ready(()); + } + + debug_assert!( + self.init_command_startup_tx.is_none(), + "start_init_command_startup_handshake called while a handshake is already in flight" + ); + + let (startup_tx, startup_rx) = async_channel::bounded(1); + let startup_task = self.background_executor.spawn(async move { + match startup_rx.recv().await { + Ok(()) | Err(_) => {} + } + }); + + let marker_id = NEXT_INIT_COMMAND_STARTUP_MARKER_ID.fetch_add(1, Ordering::Relaxed); + self.init_command_startup_marker = Some(init_command_startup_marker(marker_id)); + self.init_command_startup_tx = Some(startup_tx); + + let shell_kind = self.template.shell.shell_kind(self.path_style.is_windows()); + let mut input = init_command_startup_marker_command(shell_kind, marker_id).into_bytes(); + input.push(b'\x0d'); + self.write_to_pty(input); + + startup_task + } + + fn detect_init_command_startup_marker(&mut self) { + let Some(marker) = self.init_command_startup_marker.as_deref() else { + return; + }; + + let has_marker = { + let term = self.term.lock_unfair(); + last_non_empty_lines(&term, INIT_COMMAND_STARTUP_MARKER_SEARCH_LINES) + .iter() + .any(|line| line.contains(marker)) + }; + + if has_marker { + self.complete_init_command_startup_handshake(); + } + } + + fn complete_init_command_startup_handshake(&mut self) { + self.init_command_startup_marker = None; + if let Some(startup_tx) = self.init_command_startup_tx.take() { + match startup_tx.try_send(()) { + Ok(()) | Err(async_channel::TrySendError::Full(())) => {} + Err(async_channel::TrySendError::Closed(())) => {} + } + } + } + /// Write a programmatically-generated command to the PTY as if it had been /// typed, without marking the terminal as having received user keyboard /// input. @@ -1873,6 +1986,35 @@ impl Terminal { self.write_input(input); } + pub fn is_pty(&self) -> bool { + matches!(self.terminal_type, TerminalType::Pty { .. }) + } + + pub fn write_init_command_after_startup( + &mut self, + input: impl Into>, + cx: &mut Context, + ) -> bool { + // Ends the handshake even if the marker was never seen (timeout + // fallback), so detection stops scanning on every wakeup. + self.complete_init_command_startup_handshake(); + + if self.keyboard_input_sent || self.child_exited.is_some() { + return false; + } + + self.clear_for_init_command(cx); + self.write_init_command(input); + true + } + + fn clear_for_init_command(&mut self, cx: &mut Context) { + let mut term = self.term.lock_unfair(); + clear_saved_screen(&mut term); + self.last_content = make_content(&term, &self.last_content); + cx.emit(Event::Wakeup); + } + fn write_input(&mut self, input: impl Into>) { self.events.push_back(InternalEvent::Scroll(Scroll::Bottom)); self.events.push_back(InternalEvent::SetSelection(None)); @@ -2581,6 +2723,7 @@ impl Terminal { if let Some(e) = exit_status { self.child_exited = Some(e); } + self.complete_init_command_startup_handshake(); let task = match &mut self.task { Some(task) => task, None => { @@ -2919,6 +3062,66 @@ mod tests { use rand::{Rng, distr, rngs::StdRng}; use task::{Shell, ShellBuilder}; + #[test] + fn test_init_command_startup_marker_commands_do_not_contain_marker() { + let marker_id = 42; + let marker = init_command_startup_marker(marker_id); + + for shell_kind in [ + ShellKind::Posix, + ShellKind::Csh, + ShellKind::Tcsh, + ShellKind::Rc, + ShellKind::Fish, + ShellKind::PowerShell, + ShellKind::Pwsh, + ShellKind::Nushell, + ShellKind::Cmd, + ShellKind::Xonsh, + ShellKind::Elvish, + ] { + let command = init_command_startup_marker_command(shell_kind, marker_id); + assert!( + !command.contains(&marker), + "startup marker command for {shell_kind:?} should not contain the full marker, got {command:?}" + ); + } + } + + #[gpui::test] + async fn test_init_command_startup_marker_ignores_echoed_command(cx: &mut TestAppContext) { + let terminal = cx.new(|cx| { + TerminalBuilder::new_display_only( + SettingsCursorShape::default(), + AlternateScroll::On, + None, + 0, + cx.background_executor(), + PathStyle::local(), + ) + .subscribe(cx) + }); + let marker_id = 4242; + let marker = init_command_startup_marker(marker_id); + let command = init_command_startup_marker_command(ShellKind::Posix, marker_id); + let (startup_tx, startup_rx) = async_channel::bounded(1); + + terminal.update(cx, |terminal, cx| { + terminal.init_command_startup_marker = Some(marker.clone()); + terminal.init_command_startup_tx = Some(startup_tx); + terminal.write_output(command.as_bytes(), cx); + }); + assert!(matches!( + startup_rx.try_recv(), + Err(async_channel::TryRecvError::Empty) + )); + + terminal.update(cx, |terminal, cx| { + terminal.write_output(marker.as_bytes(), cx); + }); + assert!(startup_rx.try_recv().is_ok()); + } + #[test] fn test_normalize_path_command_name() { assert_eq!(normalize_path_command_name("claude"), Some("claude".into())); @@ -3524,6 +3727,110 @@ mod tests { } } + #[gpui::test] + async fn test_write_init_command_after_startup_clears_without_shell_command( + cx: &mut TestAppContext, + ) { + let terminal = cx.new(|cx| { + TerminalBuilder::new_display_only( + SettingsCursorShape::default(), + AlternateScroll::On, + None, + 0, + cx.background_executor(), + PathStyle::local(), + ) + .subscribe(cx) + }); + + terminal.update(cx, |terminal, cx| { + terminal.write_output(b"startup output\nprompt", cx); + }); + + let wrote = terminal.update(cx, |terminal, cx| { + terminal.write_init_command_after_startup(b"agent\r".to_vec(), cx) + }); + assert!(wrote); + let content = terminal.update(cx, |terminal, _| terminal.get_content()); + assert!( + !content.contains("startup output"), + "startup output should be cleared internally before writing the init command" + ); + let input_log = terminal.update(cx, |terminal, _| terminal.take_input_log()); + assert_eq!(input_log, vec![b"agent\r".to_vec()]); + } + + #[gpui::test] + async fn test_write_init_command_after_startup_skips_after_keyboard_input( + cx: &mut TestAppContext, + ) { + let terminal = cx.new(|cx| { + TerminalBuilder::new_display_only( + SettingsCursorShape::default(), + AlternateScroll::On, + None, + 0, + cx.background_executor(), + PathStyle::local(), + ) + .subscribe(cx) + }); + + let wrote = terminal.update(cx, |terminal, cx| { + terminal.write_output(b"startup output\nprompt", cx); + terminal.input(b"user input".to_vec()); + terminal.write_init_command_after_startup(b"agent\r".to_vec(), cx) + }); + assert!(!wrote); + let content = terminal.update(cx, |terminal, _| terminal.get_content()); + assert!( + content.contains("startup output"), + "startup output should be left alone when the init command is skipped" + ); + let input_log = terminal.update(cx, |terminal, _| terminal.take_input_log()); + assert_eq!(input_log, vec![b"user input".to_vec()]); + } + + #[gpui::test] + async fn test_write_init_command_after_startup_skips_after_child_exit(cx: &mut TestAppContext) { + let terminal = cx.new(|cx| { + TerminalBuilder::new_display_only( + SettingsCursorShape::default(), + AlternateScroll::On, + None, + 0, + cx.background_executor(), + PathStyle::local(), + ) + .subscribe(cx) + }); + + terminal.update(cx, |terminal, cx| { + terminal.write_output(b"shell failed to start\nprompt", cx); + #[cfg(unix)] + let exit_status = + ::from_raw(1 << 8); + #[cfg(windows)] + let exit_status = ::from_raw(1); + terminal.register_task_finished(Some(exit_status), cx); + }); + + let wrote = terminal.update(cx, |terminal, cx| { + terminal.write_init_command_after_startup(b"agent\r".to_vec(), cx) + }); + assert!(!wrote); + let content = terminal.update(cx, |terminal, _| terminal.get_content()); + assert!( + content.contains("shell failed to start"), + "startup failure output should be preserved when the init command is skipped" + ); + let input_log = terminal.update(cx, |terminal, _| terminal.take_input_log()); + assert!( + input_log.is_empty(), + "init command should not be written after the child has exited, got {input_log:?}" + ); + } + #[gpui::test] async fn test_write_output_converts_lf_to_crlf(cx: &mut TestAppContext) { let terminal = cx.new(|cx| {