From 599416d733378dff0557abb2c642ffb6af0670fd Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Mon, 25 May 2026 16:53:40 -0300 Subject: [PATCH] fix(tui): prevent macos stderr from corrupting composer (#24459) ## Why Fixes #17139. On macOS, runtime diagnostics such as `MallocStackLogging` messages can be written directly to process stderr while the inline TUI owns the terminal. Those bytes paint into the same viewport as the composer without passing through the renderer or composer state, making diagnostic output appear to leak into the input area. ## What Changed - Add a macOS terminal stderr guard while the inline TUI owns the viewport. - Restore stderr when Codex returns terminal ownership for external interactive programs, suspend/resume, panic handling, and normal shutdown. - Add an fd-level regression test that verifies output is suppressed only while terminal ownership is held and restored at each handoff boundary. ## How to Test 1. On macOS, launch the interactive TUI and leave the composer visible. 2. Exercise the workflow that triggers an allocator/runtime stderr diagnostic during an active session, as reported in #17139. 3. Confirm the diagnostic no longer overwrites the active composer region. 4. Suspend or exit the TUI and confirm subsequent terminal stderr output remains visible. The platform diagnostic is environment-dependent, so the deterministic regression check is the new fd-lifecycle test in `tui::terminal_stderr::tests::suppresses_stderr_only_while_terminal_is_owned`. Targeted validation: - `just argument-comment-lint-from-source -p codex-tui` passed. - `just test -p codex-tui` exercised and passed the new stderr-guard regression test. The full invocation currently fails in two unrelated guardian-policy tests, `update_feature_flags_disabling_guardian_clears_review_policy_and_restores_default` and `update_feature_flags_disabling_guardian_clears_manual_review_policy_without_history`, which reproduce when rerun in isolation. --- codex-rs/tui/src/lib.rs | 1 + codex-rs/tui/src/tui.rs | 34 ++- codex-rs/tui/src/tui/job_control.rs | 6 +- codex-rs/tui/src/tui/terminal_stderr.rs | 275 ++++++++++++++++++++++++ 4 files changed, 311 insertions(+), 5 deletions(-) create mode 100644 codex-rs/tui/src/tui/terminal_stderr.rs diff --git a/codex-rs/tui/src/lib.rs b/codex-rs/tui/src/lib.rs index d7a605c73..11be48b2c 100644 --- a/codex-rs/tui/src/lib.rs +++ b/codex-rs/tui/src/lib.rs @@ -1312,6 +1312,7 @@ async fn run_ratatui_app( let mut tui = Tui::new( initialized_terminal.terminal, initialized_terminal.enhanced_keys_supported, + initialized_terminal.stderr_guard, ); let mut terminal_restore_guard = TerminalRestoreGuard::new(); diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index 47a6aeb6e..49d838e86 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -56,6 +56,7 @@ mod frame_requester; #[cfg(unix)] mod job_control; mod keyboard_modes; +mod terminal_stderr; /// Target frame interval for UI redraw scheduling. pub(crate) const TARGET_FRAME_INTERVAL: Duration = frame_rate_limiter::MIN_FRAME_INTERVAL; @@ -66,6 +67,7 @@ pub type Terminal = CustomTerminal>; pub(crate) struct InitializedTerminal { pub(crate) terminal: Terminal, pub(crate) enhanced_keys_supported: bool, + pub(crate) stderr_guard: terminal_stderr::TerminalStderrGuard, } pub(crate) fn running_in_vscode_terminal() -> bool { @@ -281,7 +283,16 @@ pub fn restore() -> 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_common(RawModeRestore::Disable, KeyboardRestore::ResetAfterExit) + let mut first_error = + restore_common(RawModeRestore::Disable, KeyboardRestore::ResetAfterExit).err(); + if let Err(err) = terminal_stderr::finish() { + first_error.get_or_insert(err); + } + + match first_error { + Some(err) => Err(err), + None => Ok(()), + } } /// Restore the terminal to its original state, but keep raw mode enabled. @@ -425,9 +436,11 @@ pub(crate) fn init() -> Result { !keyboard_modes::keyboard_enhancement_disabled() && detect_keyboard_enhancement_supported(); let tui = CustomTerminal::with_options_and_cursor_position(backend, cursor_pos)?; + let stderr_guard = terminal_stderr::TerminalStderrGuard::install()?; Ok(InitializedTerminal { terminal: tui, enhanced_keys_supported, + stderr_guard, }) } @@ -489,6 +502,8 @@ pub struct Tui { notification_condition: NotificationCondition, // When false, enter_alt_screen() becomes a no-op. alt_screen_enabled: bool, + // Keeps unmanaged process stderr writes out of the inline viewport. + _stderr_guard: terminal_stderr::TerminalStderrGuard, } struct PendingHistoryLines { @@ -509,7 +524,11 @@ where } impl Tui { - pub fn new(terminal: Terminal, enhanced_keys_supported: bool) -> Self { + pub(crate) fn new( + terminal: Terminal, + enhanced_keys_supported: bool, + stderr_guard: terminal_stderr::TerminalStderrGuard, + ) -> Self { let (draw_tx, _) = broadcast::channel(1); let frame_requester = FrameRequester::new(draw_tx.clone()); @@ -534,6 +553,7 @@ impl Tui { notification_backend: Some(detect_backend(NotificationMethod::default())), notification_condition: NotificationCondition::default(), alt_screen_enabled: true, + _stderr_guard: stderr_guard, } } @@ -577,8 +597,8 @@ impl Tui { /// Temporarily restore terminal state to run an external interactive program `f`. /// /// This pauses crossterm's stdin polling by dropping the underlying event stream, restores - /// terminal modes (optionally keeping raw mode enabled), then re-applies Codex TUI modes and - /// flushes pending stdin input before resuming events. + /// terminal modes and stderr (optionally keeping raw mode enabled), then re-applies Codex TUI + /// modes and stderr suppression before resuming events. pub async fn with_restored(&mut self, mode: RestoreMode, f: F) -> R where F: FnOnce() -> Fut, @@ -596,9 +616,15 @@ impl Tui { if let Err(err) = mode.restore() { tracing::warn!("failed to restore terminal modes before external program: {err}"); } + if let Err(err) = terminal_stderr::pause() { + tracing::warn!("failed to restore terminal stderr before external program: {err}"); + } let output = f().await; + if let Err(err) = terminal_stderr::resume() { + tracing::warn!("failed to suppress terminal stderr after external program: {err}"); + } if let Err(err) = set_modes() { tracing::warn!("failed to re-enable terminal modes after external program: {err}"); } diff --git a/codex-rs/tui/src/tui/job_control.rs b/codex-rs/tui/src/tui/job_control.rs index 368041948..a07d42840 100644 --- a/codex-rs/tui/src/tui/job_control.rs +++ b/codex-rs/tui/src/tui/job_control.rs @@ -175,8 +175,12 @@ impl PreparedResumeAction { /// Deliver SIGTSTP after restoring terminal state, then re-applies terminal modes once resumed. fn suspend_process() -> Result<()> { super::restore()?; - unsafe { libc::kill(0, libc::SIGTSTP) }; + super::terminal_stderr::pause()?; + unsafe { + libc::kill(/*pid*/ 0, libc::SIGTSTP) + }; // After the process resumes, reapply terminal modes so drawing can continue. + super::terminal_stderr::resume()?; super::set_modes()?; Ok(()) } diff --git a/codex-rs/tui/src/tui/terminal_stderr.rs b/codex-rs/tui/src/tui/terminal_stderr.rs new file mode 100644 index 000000000..e1e0c0cdb --- /dev/null +++ b/codex-rs/tui/src/tui/terminal_stderr.rs @@ -0,0 +1,275 @@ +//! Protect the inline viewport from unmanaged macOS writes to stderr. +//! +//! Some macOS frameworks and runtime diagnostics write directly to file +//! descriptor 2. While the inline TUI is active, those writes paint into the +//! same terminal region as the composer without going through the renderer. +//! Keep them off the terminal until the TUI releases terminal ownership. + +use std::io; + +#[cfg(target_os = "macos")] +use std::fs::OpenOptions; +#[cfg(target_os = "macos")] +use std::io::IsTerminal; +#[cfg(target_os = "macos")] +use std::mem::MaybeUninit; +#[cfg(target_os = "macos")] +use std::os::fd::AsRawFd; +#[cfg(target_os = "macos")] +use std::os::fd::FromRawFd; +#[cfg(target_os = "macos")] +use std::os::fd::OwnedFd; +#[cfg(target_os = "macos")] +use std::sync::Mutex; +#[cfg(target_os = "macos")] +use std::sync::MutexGuard; + +#[cfg(target_os = "macos")] +static STDERR_STATE: Mutex = Mutex::new(StderrState { + owner_active: false, + saved_stderr: None, +}); + +#[cfg(target_os = "macos")] +struct StderrState { + owner_active: bool, + saved_stderr: Option, +} + +/// Keeps unmanaged stderr output away from the terminal while the TUI owns it. +pub(crate) struct TerminalStderrGuard { + active: bool, +} + +impl TerminalStderrGuard { + pub(super) fn install() -> io::Result { + #[cfg(target_os = "macos")] + { + if stderr_targets_stdout_terminal() { + return Self::install_suppression(); + } + } + + Ok(Self { active: false }) + } + + #[cfg(target_os = "macos")] + fn install_suppression() -> io::Result { + let mut state = lock_state()?; + if state.owner_active { + return Err(io::Error::new( + io::ErrorKind::AlreadyExists, + "terminal stderr suppression is already active", + )); + } + suppress_locked(&mut state)?; + state.owner_active = true; + Ok(Self { active: true }) + } +} + +impl Drop for TerminalStderrGuard { + fn drop(&mut self) { + if self.active { + let _ = finish(); + self.active = false; + } + } +} + +/// Restores stderr while terminal ownership is temporarily released. +pub(super) fn pause() -> io::Result<()> { + #[cfg(target_os = "macos")] + { + let mut state = lock_state()?; + if state.owner_active { + restore_locked(&mut state)?; + } + } + + Ok(()) +} + +/// Suppresses stderr again when terminal ownership returns to the TUI. +pub(super) fn resume() -> io::Result<()> { + #[cfg(target_os = "macos")] + { + let mut state = lock_state()?; + if state.owner_active { + suppress_locked(&mut state)?; + } + } + + Ok(()) +} + +/// Restores stderr permanently when the TUI session ends. +pub(super) fn finish() -> io::Result<()> { + #[cfg(target_os = "macos")] + { + let mut state = lock_state()?; + if state.owner_active { + restore_locked(&mut state)?; + state.owner_active = false; + } + } + + Ok(()) +} + +#[cfg(target_os = "macos")] +fn lock_state() -> io::Result> { + STDERR_STATE + .lock() + .map_err(|_| io::Error::other("terminal stderr suppression lock poisoned")) +} + +#[cfg(target_os = "macos")] +fn stderr_targets_stdout_terminal() -> bool { + if !io::stdout().is_terminal() || !io::stderr().is_terminal() { + return false; + } + + let mut stdout_stat = MaybeUninit::::uninit(); + let mut stderr_stat = MaybeUninit::::uninit(); + // SAFETY: both output pointers reference valid storage for libc to initialize. + if unsafe { + libc::fstat(libc::STDOUT_FILENO, stdout_stat.as_mut_ptr()) != 0 + || libc::fstat(libc::STDERR_FILENO, stderr_stat.as_mut_ptr()) != 0 + } { + return false; + } + // SAFETY: both fstat calls above returned successfully. + let (stdout_stat, stderr_stat) = + unsafe { (stdout_stat.assume_init(), stderr_stat.assume_init()) }; + stdout_stat.st_dev == stderr_stat.st_dev && stdout_stat.st_ino == stderr_stat.st_ino +} + +#[cfg(target_os = "macos")] +fn suppress_locked(state: &mut StderrState) -> io::Result<()> { + if state.saved_stderr.is_some() { + return Ok(()); + } + + // SAFETY: dup returns a newly owned file descriptor on success. + let saved_stderr = unsafe { libc::dup(libc::STDERR_FILENO) }; + if saved_stderr == -1 { + return Err(io::Error::last_os_error()); + } + // SAFETY: saved_stderr is a fresh descriptor returned by dup above. + let saved_stderr = unsafe { OwnedFd::from_raw_fd(saved_stderr) }; + let devnull = OpenOptions::new().write(true).open("/dev/null")?; + // SAFETY: both descriptors are valid for the duration of this call. + if unsafe { libc::dup2(devnull.as_raw_fd(), libc::STDERR_FILENO) } == -1 { + return Err(io::Error::last_os_error()); + } + state.saved_stderr = Some(saved_stderr); + Ok(()) +} + +#[cfg(target_os = "macos")] +fn restore_locked(state: &mut StderrState) -> io::Result<()> { + let Some(saved_stderr) = state.saved_stderr.as_ref() else { + return Ok(()); + }; + + // SAFETY: saved_stderr was duplicated from stderr and remains owned here. + if unsafe { libc::dup2(saved_stderr.as_raw_fd(), libc::STDERR_FILENO) } == -1 { + return Err(io::Error::last_os_error()); + } + state.saved_stderr = None; + Ok(()) +} + +#[cfg(all(test, target_os = "macos"))] +mod tests { + use std::fs::File; + use std::io::Read; + use std::io::Seek; + use std::io::Write; + use std::os::fd::AsRawFd; + use std::os::fd::FromRawFd; + use std::os::fd::OwnedFd; + + use pretty_assertions::assert_eq; + use serial_test::serial; + + use super::TerminalStderrGuard; + use super::finish; + use super::pause; + use super::resume; + + struct CapturedStderr { + saved_stderr: OwnedFd, + } + + impl CapturedStderr { + fn start(file: &File) -> std::io::Result { + // SAFETY: dup returns a newly owned file descriptor on success. + let saved_stderr = unsafe { libc::dup(libc::STDERR_FILENO) }; + if saved_stderr == -1 { + return Err(std::io::Error::last_os_error()); + } + // SAFETY: saved_stderr is a fresh descriptor returned by dup above. + let saved_stderr = unsafe { OwnedFd::from_raw_fd(saved_stderr) }; + // SAFETY: both descriptors are valid for the duration of this call. + if unsafe { libc::dup2(file.as_raw_fd(), libc::STDERR_FILENO) } == -1 { + return Err(std::io::Error::last_os_error()); + } + Ok(Self { saved_stderr }) + } + } + + impl Drop for CapturedStderr { + fn drop(&mut self) { + // SAFETY: saved_stderr remains owned for the duration of this call. + let _ = unsafe { libc::dup2(self.saved_stderr.as_raw_fd(), libc::STDERR_FILENO) }; + } + } + + fn write_stderr(message: &str) -> std::io::Result<()> { + let mut stderr = std::io::stderr().lock(); + stderr.write_all(message.as_bytes())?; + stderr.flush() + } + + #[test] + #[serial] + fn suppresses_stderr_only_while_terminal_is_owned() -> std::io::Result<()> { + let mut output = tempfile::tempfile()?; + let capture = CapturedStderr::start(&output)?; + + let _guard = TerminalStderrGuard::install_suppression()?; + write_stderr("hidden while active\n")?; + pause()?; + write_stderr("visible while paused\n")?; + resume()?; + write_stderr("hidden after resume\n")?; + finish()?; + write_stderr("visible after finish\n")?; + + drop(capture); + output.rewind()?; + let mut captured = String::new(); + output.read_to_string(&mut captured)?; + assert_eq!(captured, "visible while paused\nvisible after finish\n"); + Ok(()) + } + + #[test] + #[serial] + fn preserves_stderr_when_already_redirected() -> std::io::Result<()> { + let mut output = tempfile::tempfile()?; + let capture = CapturedStderr::start(&output)?; + + let _guard = TerminalStderrGuard::install()?; + write_stderr("visible while redirected\n")?; + + drop(capture); + output.rewind()?; + let mut captured = String::new(); + output.read_to_string(&mut captured)?; + assert_eq!(captured, "visible while redirected\n"); + Ok(()) + } +}