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.
This commit is contained in:
Felipe Coury
2026-05-25 16:53:40 -03:00
committed by GitHub
Unverified
parent 14d80e55cd
commit 599416d733
4 changed files with 311 additions and 5 deletions
+1
View File
@@ -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();
+30 -4
View File
@@ -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<CrosstermBackend<Stdout>>;
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<InitializedTerminal> {
!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<R, F, Fut>(&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}");
}
+5 -1
View File
@@ -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(())
}
+275
View File
@@ -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<StderrState> = Mutex::new(StderrState {
owner_active: false,
saved_stderr: None,
});
#[cfg(target_os = "macos")]
struct StderrState {
owner_active: bool,
saved_stderr: Option<OwnedFd>,
}
/// 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<Self> {
#[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<Self> {
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<MutexGuard<'static, StderrState>> {
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::<libc::stat>::uninit();
let mut stderr_stat = MaybeUninit::<libc::stat>::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<Self> {
// 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(())
}
}