diff --git a/codex-rs/tui/src/app/startup.rs b/codex-rs/tui/src/app/startup.rs index 21badd2f67..3d56ce5167 100644 --- a/codex-rs/tui/src/app/startup.rs +++ b/codex-rs/tui/src/app/startup.rs @@ -1677,6 +1677,8 @@ See the Codex keymap documentation for supported actions and examples." } } }; + // Let the pending terminal poll unwind while the app server shuts down. + tui.pause_events(); app.chat_widget.mention_usage_mut().flush().await; if let Err(err) = backends.shutdown().await { tracing::warn!(error = %err, "failed to shut down embedded app server"); diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index 14fe332a4b..aaaaa6e57a 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -2215,8 +2215,8 @@ async fn run_ratatui_app( clippy::print_stderr, reason = "TUI should no longer be displayed, so we can write to stderr." )] -fn restore() { - if let Err(err) = tui::restore_after_exit() { +fn report_restore_error(result: color_eyre::Result<()>) { + if let Err(err) = result { eprintln!( "failed to restore terminal. Run `reset` or restart your terminal to recover: {err}" ); @@ -2225,27 +2225,28 @@ fn restore() { struct TerminalRestoreGuard { active: bool, + events: Option>, } impl TerminalRestoreGuard { fn new() -> Self { - Self { active: true } + Self { + active: true, + events: None, + } } - #[cfg_attr(debug_assertions, allow(dead_code))] fn restore(&mut self) -> color_eyre::Result<()> { if self.active { - crate::tui::restore_after_exit()?; + tui::restore_after_exit_with_events(self.events.take().as_deref())?; self.active = false; } Ok(()) } fn restore_silently(&mut self) { - if self.active { - restore(); - self.active = false; - } + report_restore_error(self.restore()); + self.active = false; } } diff --git a/codex-rs/tui/src/startup_draft.rs b/codex-rs/tui/src/startup_draft.rs index e0a4d5286b..09ea01548a 100644 --- a/codex-rs/tui/src/startup_draft.rs +++ b/codex-rs/tui/src/startup_draft.rs @@ -123,7 +123,7 @@ impl StartupDraft { /// Adopt the initialized terminal and apply editing policy before the first composer paint. pub(crate) fn new( mut initialized_terminal: tui::InitializedTerminal, - terminal_restore_guard: TerminalRestoreGuard, + mut terminal_restore_guard: TerminalRestoreGuard, initial_screen: StartupDraftInitialScreen, session_action: StartupDraftSessionAction, screen: StartupScreen, @@ -135,6 +135,7 @@ impl StartupDraft { initialized_terminal.enhanced_keys_supported, initialized_terminal.stderr_guard, ); + terminal_restore_guard.events = Some(tui.event_broker.clone()); tui.terminal_app_over_ssh = initialized_terminal.terminal_app_over_ssh; tui.set_alt_screen_enabled(screen.use_alt_screen); let mut pump = StartupDraftPump::new(&tui, initial_screen, session_action); diff --git a/codex-rs/tui/src/terminal_probe.rs b/codex-rs/tui/src/terminal_probe.rs index f816935450..08118acd48 100644 --- a/codex-rs/tui/src/terminal_probe.rs +++ b/codex-rs/tui/src/terminal_probe.rs @@ -1,4 +1,4 @@ -//! Short, best-effort terminal response probes for TUI startup and resume. +//! Short, best-effort terminal response probes for TUI startup, resume, and exit. //! //! Crossterm's public helpers wait up to two seconds for terminal responses. That is too long for //! TUI startup and resume, where unsupported terminals should simply fall back to conservative @@ -48,6 +48,8 @@ mod imp { use std::io::Write; use std::os::fd::AsRawFd; use std::os::fd::FromRawFd; + use std::sync::atomic::AtomicBool; + use std::sync::atomic::Ordering; use std::time::Duration; use std::time::Instant; @@ -59,6 +61,8 @@ mod imp { const MAX_STARTUP_PROBE_BYTES: usize = 64 * 1024; const MAX_TERMINAL_CSI_BYTES: usize = 64; + static DA1_SUPPORTED: AtomicBool = AtomicBool::new(false); + /// Results from the TUI's one-shot startup terminal probe. #[derive(Debug, Clone, Copy, Eq, PartialEq)] pub(crate) struct StartupProbe { @@ -253,10 +257,7 @@ mod imp { pub(crate) fn default_colors(timeout: Duration) -> io::Result> { let mut tty = Tty::open()?; tty.write_all(b"\x1B]10;?\x1B\\\x1B]11;?\x1B\\")?; - let Some(colors) = read_until(&mut tty, timeout, parse_default_colors)? else { - return Ok(None); - }; - Ok(Some(colors)) + read_until(&mut tty, timeout, |buffer| parse_default_colors(buffer)) } /// Queries the terminal cursor position while normal input polling is paused. @@ -267,7 +268,24 @@ mod imp { pub(crate) fn cursor_position(timeout: Duration) -> io::Result> { let mut tty = Tty::open()?; tty.write_all(b"\x1B[6n")?; - read_until(&mut tty, timeout, parse_cursor_position) + read_until(&mut tty, timeout, |buffer| parse_cursor_position(buffer)) + } + + /// A DA1 reply follows reports the terminal generated before we disabled reporting. + pub(crate) fn wait_for_exit_reports() -> io::Result<()> { + if !DA1_SUPPORTED.load(Ordering::Relaxed) { + return Ok(()); + } + let mut tty = Tty::open()?; + tty.write_all(b"\x1b[c")?; + read_until(&mut tty, Duration::from_secs(/*secs*/ 2), |buffer| { + let reply = find_primary_device_attributes(buffer); + // Keep a possible split reply and discard older input so bursts cannot fill the probe. + let keep = b"\x1b[?".len() + MAX_TERMINAL_CSI_BYTES; + buffer.drain(..buffer.len().saturating_sub(keep)); + reply + })? + .ok_or_else(|| io::Error::new(io::ErrorKind::TimedOut, "no DA1 reply")) } /// Runs the optional terminal queries needed during TUI startup under one shared deadline. @@ -278,20 +296,19 @@ mod imp { timeout: Duration, keyboard_probe: StartupKeyboardEnhancementProbe, ) -> io::Result { + DA1_SUPPORTED.store(false, Ordering::Relaxed); let mut tty = Tty::open()?; let query_identity = codex_terminal_detection::terminal_info() .multiplexer .is_none() && (std::env::var_os("SSH_TTY").is_some() || std::env::var_os("SSH_CONNECTION").is_some()); - let mut query = b"\x1B[6n\x1B]10;?\x1B\\\x1B]11;?\x1B\\".to_vec(); + let mut query = Vec::new(); if keyboard_probe == StartupKeyboardEnhancementProbe::Query { query.extend_from_slice(b"\x1B[?u"); } - // DA1 must follow the keyboard query: its reply also finishes keyboard detection. - if keyboard_probe == StartupKeyboardEnhancementProbe::Query || query_identity { - query.extend_from_slice(b"\x1B[c"); - } + // DA1 finishes keyboard detection and establishes whether we can use it at exit. + query.extend_from_slice(b"\x1B[c\x1B[6n\x1B]10;?\x1B\\\x1B]11;?\x1B\\"); if query_identity { query.extend_from_slice(b"\x1B[>c"); } @@ -306,24 +323,27 @@ mod imp { &mut buffer, ); crossterm::event::buffer_input(&startup_replay_input(&buffer))?; + let saw_da1 = find_primary_device_attributes(&buffer).is_some(); + DA1_SUPPORTED.store(saw_da1, Ordering::Relaxed); result } /// Reads available terminal bytes until `parse` recognizes a probe response or time expires. /// + /// A parser can discard bytes it has ruled out to continue past the byte budget at exit. /// The accumulated buffer may include unrelated terminal input. This helper intentionally does /// not try to replay those bytes, so callers must use it only during short, exclusive probe /// windows before normal crossterm input polling begins or while that polling is paused. fn read_until( tty: &mut Tty, timeout: Duration, - mut parse: impl FnMut(&[u8]) -> Option, + mut parse: impl FnMut(&mut Vec) -> Option, ) -> io::Result> { let deadline = Instant::now() + timeout; let mut buffer = Vec::new(); loop { tty.read_available(&mut buffer, deadline, MAX_TERMINAL_PROBE_BYTES)?; - if let Some(value) = parse(&buffer) { + if let Some(value) = parse(&mut buffer) { return Ok(Some(value)); } let now = Instant::now(); diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 48b5b14fd3..019f820411 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -52,7 +52,7 @@ use crate::notifications::detect_backend; use crate::terminal_hyperlinks::HyperlinkLine; use crate::terminal_hyperlinks::plain_hyperlink_lines; use crate::tui::alternate_screen::ALTERNATE_SCREEN; -use crate::tui::event_stream::EventBroker; +pub(super) use crate::tui::event_stream::EventBroker; use crate::tui::event_stream::TuiEventStream; #[cfg(unix)] use crate::tui::job_control::SuspendContext; @@ -315,6 +315,7 @@ impl Command for DisableAlternateScroll { #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum RawModeRestore { + DrainAndDisable, Disable, Keep, } @@ -356,7 +357,14 @@ fn restore_common( first_error.get_or_insert(err); } let _ = execute!(stdout(), DisableFocusChange); - if matches!(raw_mode_restore, RawModeRestore::Disable) + if raw_mode_restore == RawModeRestore::DrainAndDisable { + #[cfg(unix)] + if let Err(err) = crate::terminal_probe::wait_for_exit_reports() { + tracing::debug!(%err, "failed to drain outstanding terminal reports"); + } + flush_terminal_input_buffer(); + } + if raw_mode_restore != RawModeRestore::Keep && let Err(err) = disable_raw_mode() { first_error.get_or_insert(err); @@ -406,8 +414,25 @@ pub(super) fn reapply_raw_mode_after_resume() -> Result<()> { /// Uses a stronger keyboard reset than `restore` so the parent shell recovers even if a /// terminal missed the stack pop that normally pairs with [`set_modes`]. pub fn restore_after_exit() -> Result<()> { + restore_after_exit_with_events(/*events*/ None) +} + +/// Discard reports and interleaved keystrokes before handing the terminal back to the shell. +pub(super) fn restore_after_exit_with_events(events: Option<&EventBroker>) -> Result<()> { + let raw_mode_restore = match events { + Some(_) if std::thread::panicking() => { + // The panic hook restored terminal modes. Flush queued reports without locking the reader. + flush_terminal_input_buffer(); + RawModeRestore::Disable + } + Some(events) => { + events.pause_events(); + RawModeRestore::DrainAndDisable + } + None => RawModeRestore::Disable, + }; let mut first_error = restore_common( - RawModeRestore::Disable, + raw_mode_restore, KeyboardRestore::ResetAfterExit, TerminalHandoff::Restore, ) @@ -667,7 +692,7 @@ pub struct Tui { pub(crate) link_hover: link_pointer::LinkHover, frame_requester: FrameRequester, draw_tx: broadcast::Sender<()>, - event_broker: Arc, + pub(super) event_broker: Arc, pub(crate) terminal_size_tags: crate::terminal_size_telemetry::LaunchTags, pub(crate) terminal: Terminal, pending_history_lines: Vec, diff --git a/codex-rs/tui/src/tui_panic_tests.rs b/codex-rs/tui/src/tui_panic_tests.rs index 95dca4af55..e36a947e30 100644 --- a/codex-rs/tui/src/tui_panic_tests.rs +++ b/codex-rs/tui/src/tui_panic_tests.rs @@ -45,6 +45,9 @@ fn caught_panic_rehides_cursor_before_repainting_and_restores_its_shape() { let mut master = unsafe { File::from_raw_fd(master) }; // SAFETY: the slave descriptor is distinct from the master and is likewise newly owned. let slave = unsafe { File::from_raw_fd(slave) }; + master + .write_all(b"\x1b[<51;2;56M\n") + .expect("queue mouse report"); let child = Command::new(std::env::current_exe().expect("current test binary")) .args([ "--exact", @@ -157,7 +160,17 @@ async fn caught_panic_pty_child() { mark(BEFORE_VISIBLE_FRAME).expect("mark before visible frame"); draw(&mut tui, "after panic", Some((4, 1))).expect("draw recovered visible cursor"); mark(AFTER_VISIBLE_FRAME).expect("mark after visible frame"); - super::restore_after_exit().expect("restore terminal after test"); + assert!(super::terminal_input_is_readable().expect("caught panic preserves pending input")); + let events = tui.event_broker.clone(); + let unwind = std::panic::catch_unwind(std::panic::AssertUnwindSafe(move || { + let _guard = crate::TerminalRestoreGuard { + active: true, + events: Some(events), + }; + panic!("unwind the terminal owner"); + })); + assert!(unwind.is_err()); + assert!(!super::terminal_input_is_readable().expect("check shell input after unwind")); } fn draw(tui: &mut super::Tui, text: &str, cursor: Option<(u16, u16)>) -> std::io::Result<()> { diff --git a/codex-rs/tui/tests/suite/focus_palette.rs b/codex-rs/tui/tests/suite/focus_palette.rs index 645f115c35..084599ce1c 100644 --- a/codex-rs/tui/tests/suite/focus_palette.rs +++ b/codex-rs/tui/tests/suite/focus_palette.rs @@ -227,8 +227,22 @@ fn default_owned_screen_entry_paints_before_sync_ends_and_exit_clears_inline_dra (1, (u16::try_from(composer_row)?, 2), false), "first owned-screen frame must contain one composer with its visible cursor" ); + let exit_start = terminal.output.len(); terminal.write_input(b"\x1b[99;5u")?; let deadline = Instant::now() + STARTUP_TIMEOUT; + while !contains_bytes(&terminal.output[exit_start..], b"\x1b[c") { + terminal.ensure_running()?; + ensure!(Instant::now() < deadline, "missing exit DA1 query"); + terminal.read_output(Duration::from_millis(/*millis*/ 50))?; + } + assert_eq!( + terminal.parser.screen().mouse_protocol_mode(), + vt100::MouseProtocolMode::None + ); + // The DA1 reply straddles the first 1 KiB of in-flight mouse reports. + let mut reports = b"\x1b[<51;23;56M".repeat(85); + reports.extend_from_slice(b"\x1b[?1;2c"); + terminal.write_input(&reports)?; loop { terminal.read_output(Duration::from_millis(/*millis*/ 50))?; if let Some(status) = terminal.child.try_wait()? { @@ -241,6 +255,10 @@ fn default_owned_screen_entry_paints_before_sync_ends_and_exit_clears_inline_dra terminal.screen_contents() ); } + terminal.write_input(b"shell\n")?; + let mut shell_input = [0; 6]; + terminal.slave.read_exact(&mut shell_input)?; + assert_eq!(&shell_input, b"shell\n"); ensure!( !terminal.parser.screen().alternate_screen(), "alternate screen was not restored" @@ -278,6 +296,7 @@ fn fullscreen_transcript_can_opt_out_to_terminal_scrollback() -> Result<()> { pub(super) struct PtyCodex { master: File, + slave: File, child: Child, parser: vt100::Parser, output: Vec, @@ -370,12 +389,13 @@ impl PtyCodex { .env("CODEX_HOME", codex_home.path()) .stdin(stdin) .stdout(stdout) - .stderr(slave) + .stderr(slave.try_clone()?) .spawn() .context("start Codex in focus-test pseudo-terminal")?; Ok(Self { master, + slave, child, parser: vt100::Parser::new( /*rows*/ 32, /*cols*/ 120, /*scrollback_len*/ 0, @@ -446,8 +466,10 @@ impl PtyCodex { self.cursor_answered = true; } - if !self.keyboard_answered && contains_bytes(&self.output, b"\x1b[?u") { - self.write_input(b"\x1b[?7u")?; + if !self.keyboard_answered && contains_bytes(&self.output, b"\x1b[c") { + if contains_bytes(&self.output, b"\x1b[?u") { + self.write_input(b"\x1b[?7u")?; + } self.write_input(b"\x1b[?1;2c")?; self.keyboard_answered = true; }