Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/js/bun/terminal/terminal-spawn.test.ts (1)

249-253: ⚠️ Potential issue | 🟡 Minor

Assert the PTY starts cooked before clearing ICANON/ECHO.

Without checking the pre-state, this still passes if the PTY is already raw, so the regression guard becomes vacuous. Snapshot before = terminal.localFlags, assert both bits are set, then clear them from that snapshot.

Suggested change

-    terminal.localFlags = terminal.localFlags & ~(ICANON | ECHO);
+    const before = terminal.localFlags;
+    expect(before & ICANON).not.toBe(0);
+    expect(before & ECHO).not.toBe(0);
+    terminal.localFlags = before & ~(ICANON | ECHO);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/js/bun/terminal/terminal-spawn.test.ts` around lines 249 - 253, Capture
the current flags into a snapshot (e.g., before = terminal.localFlags), assert
that both ICANON and ECHO are set on that snapshot (expect(before &
ICANON).not.toBe(0) and expect(before & ECHO).not.toBe(0)), then compute the
cleared value from that snapshot and apply/verify clearing (e.g., newFlags =
before & ~(ICANON | ECHO); expect(newFlags & ICANON).toBe(0); expect(newFlags &
ECHO).toBe(0) or assign back to terminal.localFlags as needed). This ensures the
test verifies the PTY started cooked before flipping the bits.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Overview

This PR gates bun_restore_stdio() on a new bun_stdio_modified[fd] flag so Bun only writes its startup termios snapshot back at exit (or via the SIGINT/SIGTERM onExitSignal path) for stdio fds it actually touched via Bun__ttySetMode. Touches c-bindings.cpp (~12 lines), wtf-bindings.cpp (~10 lines), and adds two PTY-driven regression tests.

Security risks

None. No auth, crypto, untrusted input, or privilege boundaries involved — this is purely termios bookkeeping at process exit.

Level of scrutiny

Medium-high. The diff is small and the reasoning is sound (#29592 is a real, well-explained bug), but bun_restore_stdio and onExitSignal run on every Bun process with a TTY, including the SIGINT/SIGTERM signal handler. Removing the unconditional restore is a behavior change with broad reach. I grepped for other termios writers: ProcessBindingTTYWrap.cpp and tty.zig both route through Bun__ttySetMode (covered); ansi_renderer.zig calls bun.tty.setMode(0, .raw) before its direct tcsetattr restore, so the flag is set there too. I didn't find a path that would regress, but a maintainer who knows the REPL/prompt/progress-bar history should sanity-check that nothing was leaning on the unconditional restore as a safety net.

Other factors

• My two prior nits (timer-based ordering, Linux-only ICANON constant) are addressed and resolved.
• CodeRabbit's last minor suggestion (precondition-assert that the PTY starts cooked in test 1) was not applied; it's defensive only since Bun.Terminal always initializes cooked.
• robobun shows build-zig failures on 5d8ce2a; this PR touches no Zig, and 84e00bc is newer, so likely unrelated — but worth confirming green CI before merge.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Overview

Adds a volatile sig_atomic_t bun_stdio_modified[3] flag in c-bindings.cpp, sets it inside Bun__ttySetMode (wtf-bindings.cpp) after a successful tcsetattr on fd 0-2, and gates the per-fd restore loop in bun_restore_stdio() on that flag. Two new Bun.Terminal-driven regression tests cover both the "never touched termios" and "called setRawMode" paths with deterministic stdin handshakes. All earlier inline nits (timer-based ordering, platform-specific ICANON value, chunk-safe READY buffer, cooked-precondition assert, mid-run raw-mode assert, sig_atomic_t type) have been folded in.

Security risks

None. No new inputs, no parsing, no privilege changes — this only narrows when an existing exit-time tcsetattr runs.

Level of scrutiny

Moderate. The diff is small and strictly more conservative (it removes an unconditional write that was clobbering shared device state), and I verified that every in-tree path that flips stdio termios funnels through Bun__ttySetMode (tty.zig, ProcessBindingTTYWrap.cpp, md/ansi_renderer.zig via bun.tty.setMode; the FileReader.zig reference is commented out). That said, bun_restore_stdio is invoked from the SIGINT/SIGTERM handler installed in bun_initialize_process and from Zig's output.zig restore path, so this changes signal-time behavior for every TTY-attached Bun process on POSIX. That's the kind of process-lifecycle change a maintainer should sign off on rather than a bot.

Other factors

The interaction the PR description relies on — that uv_tty_reset_mode's atexit hook still restores fds Bun did modify even though bun_restore_stdio now also fires for them — looks correct (both hold the same startup snapshot, and the second test asserts cooked-after-exit), but it's the one piece I'd want a human to confirm before merging.

Overview

The PR gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp) behind a new volatile sig_atomic_t bun_stdio_modified[3] flag, set only inside Bun__ttySetMode() (wtf-bindings.cpp) after a successful uv__tcsetattr. Two new Bun.Terminal-driven regression tests verify (a) a child that never touches termios leaves the shared PTY device alone at exit, and (b) a child that calls setRawMode(true) still restores cooked mode. All prior review feedback (my two nits on timing-based tests and platform-specific ICANON, plus CodeRabbit's signal-safety, chunk-buffering, precondition-assertion, and exit-code-ordering comments) has been addressed and the threads are resolved.

Security risks

None. No user-controlled input reaches the new code; the flag is a process-local boolean array indexed by hardcoded fd ∈ {0,1,2} with an explicit bounds check.

Level of scrutiny

Medium-high. The logic change is ~5 lines and strictly reduces what bun_restore_stdio does (it can only skip a restore, never add one), so it can't introduce a new clobber. But it runs from signal context (onExitSignal for SIGINT/SIGTERM) and on every TTY-attached process exit, and its correctness depends on the invariant that every code path which mutates stdio termios on Bun's behalf routes through Bun__ttySetMode. A quick grep shows the only other in-tree tcsetattr on fd 0 is src/md/ansi_renderer.zig, which calls bun.tty.setMode(0, .raw) first (setting the flag) and has its own defer restore — so it looks fine, but a maintainer familiar with the TTY/REPL/prompt paths should confirm there's no other entry point that previously relied on the unconditional restore.

Other factors

The tests are now deterministic (stdin handshake instead of setTimeout), buffer PTY output across chunks, assert the cooked-mode precondition, and verify the device is observably raw while the child is alive — so the regression coverage is solid. No CODEOWNERS match these files. CI build #47272 is referenced but I can't see its status.

Overview

Fixes #29592 by gating the exit-time tcsetattr in bun_restore_stdio() behind a new volatile sig_atomic_t bun_stdio_modified[3] flag that is only set inside Bun__ttySetMode() after a successful tcsetattr. Touches c-bindings.cpp (definition + guard), wtf-bindings.cpp (extern declaration + flag write + <signal.h> include), and adds two POSIX-only regression tests in terminal-spawn.test.ts.

Security risks

None. No user-controlled input, no auth/crypto/permissions surface. The change strictly reduces the number of tcsetattr calls Bun makes at exit; it cannot cause Bun to write termios state it wasn't already writing before.

Level of scrutiny

Medium-low. This is process exit / signal-handler adjacent, but the change is purely additive guarding: skip a write that was previously unconditional. The signal-safety concern was raised and correctly addressed with volatile sig_atomic_t. The only code path that modifies stdio termios in Bun is Bun__ttySetMode (verified via grep — the FileReader.zig tcsetattr is commented out, Terminal.zig/ansi_renderer.zig operate on non-stdio fds), so flag coverage is complete. The fd write is bounds-checked because the PTY master fd from Bun.Terminal also calls through Bun__ttySetMode.

Other factors

All prior review feedback has been addressed: my two nits (Darwin ICANON constant, deterministic stdin handshake instead of setTimeout) and CodeRabbit's five points (sig_atomic_t, chunk-safe READY buffering, cooked-mode precondition, mid-run raw-mode assertion, exit-code-last ordering) are all reflected in the final diff. No CODEOWNERS coverage for these files. The two new tests passed on every CI lane including ASAN; the three reported failures are unrelated infra/pre-existing flakes per the PR description.

Overview

Three files: c-bindings.cpp adds a volatile sig_atomic_t bun_stdio_modified[3] and gates the per-fd tcsetattr in bun_restore_stdio() on it; wtf-bindings.cpp sets the flag inside Bun__ttySetMode after a successful uv__tcsetattr (bounds-checked to fds 0–2); terminal-spawn.test.ts adds two PTY-driven regression tests. Net runtime logic change is ~5 lines; the rest is comments and tests.

Security risks

None. No untrusted input, no auth/crypto, no new syscalls — the change strictly skips an existing tcsetattr when Bun never modified termios.

Level of scrutiny

Medium. The diff is small and conservative (only narrows existing behavior), but bun_restore_stdio runs from onExitSignal (SIGINT/SIGTERM) and the atexit path on every TTY-attached Bun process, so a maintainer familiar with the exit/signal handling should confirm the gating point (Bun__ttySetMode) covers all paths that intentionally mutate stdio termios. I spot-checked other tcsetattr callers: FileReader.zig's is commented out, Terminal.zig operates on PTY master fds (not 0–2), and ansi_renderer.zig goes through bun.tty.setMode first, so the flag is set there too.

Other factors

All seven prior review threads (my two nits on test timing / Darwin ICANON, plus CodeRabbit's signal-safety, chunk-buffering, precondition, mid-run raw assertion, and exit-code-ordering comments) are addressed and resolved in the current diff. The bug-hunting pass found nothing. New tests passed on all CI lanes including ASAN per the PR description; the three CI annotations are unrelated infra/flakes.

Overview

The PR gates bun_restore_stdio()'s exit-time tcsetattr behind a new volatile sig_atomic_t bun_stdio_modified[3] flag that is only set inside Bun__ttySetMode() after a successful tcsetattr. Net effect: Bun no longer writes its startup termios snapshot back to the terminal device at exit unless it actually modified termios during the run, fixing #29592 (bun ... | less going unresponsive). Two new Bun.Terminal-driven regression tests cover both directions (untouched fd → no clobber; setRawMode(true) → still restored). I verified that every POSIX path that flips termios on fd 0–2 (process.stdin.setRawMode via ProcessBindingTTYWrap.cpp, bun.tty.setMode in ansi_renderer.zig) routes through Bun__ttySetMode, so the flag is set in all relevant cases.

Security risks

None. No untrusted input, no auth/crypto/permissions surface; it only narrows when an existing tcsetattr runs.

Level of scrutiny

Medium-high. The diff is ~20 effective C++ lines plus tests, and the logic is straightforward, but it lives in the process-exit path and the SIGINT/SIGTERM handler (onExitSignal → bun_restore_stdio) that executes on every TTY-attached Bun process. The behavior change is strictly more conservative (skip a write that used to be unconditional), so the only failure mode is "didn't restore when we should have" — covered by the second new test and the existing uv_tty_reset_mode atexit hook. Still, exit/signal-handler semantics in a runtime are the kind of thing a maintainer should eyeball.

Other factors

All seven inline review threads (my two nits on setTimeout-based ordering and the Darwin ICANON constant, plus coderabbit's chunk-buffering, signal-safety, precondition, mid-run raw-mode assertion, and exit-code-ordering comments) are addressed in df80788…a68d21a and marked resolved. No CODEOWNERS apply to the touched files. CI on the latest substantive commit passed the new tests on every lane including ASAN; the remaining annotations are unrelated infra/flake. No bugs were found by the bug-hunting system on the current revision.

Overview

Gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp:424-429) on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is only set inside Bun__ttySetMode() (wtf-bindings.cpp:169-178) after a successful uv__tcsetattr. Net effect: a Bun process that never called setRawMode no longer writes its startup termios snapshot back to the shared /dev/pts/* device at exit, fixing the bun ... | less raw-mode clobber in #29592. Two new Bun.Terminal-driven regression tests cover both directions (untouched fd left alone; modified fd still restored via the existing uv_tty_reset_mode atexit hook).

Security risks

None. No new inputs, parsing, auth, or privilege boundaries. The only state added is a process-local 3-element sig_atomic_t array written from Bun__ttySetMode and read from bun_restore_stdio/onExitSignal. volatile sig_atomic_t is the correct type for the signal-context read, and the fd is bounds-checked before indexing.

Level of scrutiny

Medium-high. The diff is small (~5 logic lines in C++) and the reasoning is sound, but bun_restore_stdio runs from both the normal exit path and the SIGINT/SIGTERM handler for every Bun process started on a TTY. The change replaces an unconditional defensive restore with a conditional one — that's a deliberate policy shift (don't touch termios we didn't modify) rather than a pure bugfix, and a maintainer familiar with the original intent of the always-restore behavior should sign off. I checked the other tcsetattr call sites (ansi_renderer.zig goes through bun.tty.setMode → Bun__ttySetMode, FileReader.zig is commented out, Terminal.zig operates on PTY master fds > 2) and none bypass the new flag.

Other factors

All seven inline review threads (my two nits on test timing/ICANON constants, plus CodeRabbit's chunk-buffering, sig_atomic_t, cooked-precondition, mid-run raw assertion, and exit-code-last suggestions) are resolved and reflected in the current diff. The two new tests are now deterministic (stdin handshake, no sleeps), platform-correct (Darwin vs Linux ICANON), and passed on every CI lane including ASAN; the lone remaining CI failure is the known worker_threads 0x10 segfault flake, unrelated to this change.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is only set inside Bun__ttySetMode() after a successful termios write. The change spans ~10 lines of logic across c-bindings.cpp and wtf-bindings.cpp, plus two new deterministic Bun.Terminal-driven regression tests.

Security risks

None. No user input parsing, no auth/crypto, no privilege boundaries. The change strictly reduces the syscalls made at process exit (skips tcsetattr on fds Bun never touched).

Level of scrutiny

This is signal-handler / process-exit code that runs on every TTY-attached Bun process, so it deserves careful review — but the change is purely subtractive in effect and cannot cause crashes or memory corruption. The worst-case failure mode is "terminal left in wrong mode after exit," which is the exact bug class this PR is fixing in the opposite direction. I verified that all in-tree paths that modify stdio termios route through Bun__ttySetMode (src/tty.zig wraps it; the FileReader.zig tcsetattr is commented out; ansi_renderer.zig uses bun.tty.setMode; Terminal.zig operates on the PTY master fd, covered by the fd < 3 bounds check). So the gating cannot skip a restore that was previously needed.

Other factors

All six prior review threads (mine and CodeRabbit's) are resolved: the tests now use a deterministic stdin handshake instead of setTimeout, buffer PTY output across chunks, use platform-correct ICANON constants, assert cooked-before-raw preconditions, use volatile sig_atomic_t, and order termios assertions before exit-code assertions. The two new tests passed on every CI lane including debian-13-x64-asan. The one remaining inline nit (flag written after tcsetattr returns, leaving a few-instruction SIGTERM window) is correctly classified as near-zero practical impact and does not block.

Overview

Three files: c-bindings.cpp adds a volatile sig_atomic_t bun_stdio_modified[3] global and a single if (!bun_stdio_modified[fd]) continue; guard in bun_restore_stdio(); wtf-bindings.cpp declares the extern, includes <signal.h>, and writes bun_stdio_modified[fd] = 1 (bounds-checked) immediately before the uv__tcsetattr call in Bun__ttySetMode; terminal-spawn.test.ts adds two PTY-driven regression tests. Net effect: Bun no longer writes its startup termios snapshot back to a TTY at exit unless it actually called setRawMode on that fd, fixing the bun ... | less raw-mode clobber in #29592.

Security risks

None. No user-controlled input reaches the new code; the flag is a process-local boolean array indexed only by the literal range 0..2, and the guard strictly reduces the set of tcsetattr calls made at exit.

Level of scrutiny

Medium-high. The logic change is ~5 lines and conceptually simple, but it lives in onExitSignal (SIGINT/SIGTERM handler) and the atexit path, which run on every process termination. The signal-safety details have been worked through across review rounds: the flag is volatile sig_atomic_t, and the write was moved before the tcsetattr syscall so a signal landing in the gap can't skip the restore. I also verified the only other in-tree termios writer on stdio (src/md/ansi_renderer.zig:2090) goes through bun.tty.setMode → Bun__ttySetMode, so it correctly flips the new flag. Terminal.zig's tcsetattr calls target PTY slave fds, not 0/1/2.

Other factors

All seven inline review threads (two from me, five from CodeRabbit) are resolved and reflected in the current diff: deterministic stdin handshake instead of setTimeout, platform-correct ICANON constant, chunk-safe READY buffering, cooked-precondition assertions, mid-run raw-mode assertion in the companion test, exit-code-last ordering, sig_atomic_t type, and the write-before-syscall reorder. The bug-hunting system found nothing on the current revision. CI is green on the new tests across lanes including debian-asan per the PR description; remaining annotations are documented unrelated infra/flake. Given the change alters behaviour that has shipped for a long time (unconditional termios restore at exit), a maintainer glance feels appropriate even though I believe the change is correct.

Overview

Adds a volatile sig_atomic_t bun_stdio_modified[3] gate in c-bindings.cpp so bun_restore_stdio() only calls tcsetattr at exit for stdio fds that Bun actually modified via Bun__ttySetMode (wtf-bindings.cpp). Two new Bun.Terminal-driven regression tests in terminal-spawn.test.ts cover both the negative (child never touched termios → device left alone) and positive (child called setRawMode → device restored) cases.

Security risks

None. No user-controlled input reaches the new code; the only new state is a 3-element process-local flag array indexed by a bounds-checked fd. No auth, crypto, or permission surfaces touched.

Level of scrutiny

This sits in the process-exit / SIGINT/SIGTERM handler path, so I gave it close attention across three review passes. The change is reductive: it skips a tcsetattr that was previously unconditional, so the worst-case regression would be a terminal left in raw mode after Ctrl-C — and the PR specifically closes that window by writing the flag before the syscall (dda545e), uses the correct volatile sig_atomic_t type for signal-context access, and bounds-checks the fd. Bun__ttySetMode is verified as the single chokepoint for stdio termios writes (ProcessBindingTTYWrap.cpp and tty.zig both route through it; the FileReader.zig reference is commented-out code).

Other factors

Every prior inline comment (mine on timing/ICANON-layout/signal-ordering, CodeRabbit's on chunk-safe READY buffering, sig_atomic_t, cooked-precondition assert, mid-run raw-mode assert, exit-code-last ordering) is addressed in the current diff and marked resolved. The tests are now handshake-driven with no wall-clock dependencies, use platform-correct ICANON values, and assert the full cooked→raw→cooked lifecycle. PR description reports the new tests green on all lanes including ASAN; remaining CI annotations are documented unrelated flakes followed by empty retrigger commits.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp) on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is set inside Bun__ttySetMode() (wtf-bindings.cpp) before the uv__tcsetattr call. Two new POSIX-only Bun.Terminal integration tests in terminal-spawn.test.ts verify (a) a child that never touches termios leaves the shared PTY's raw mode intact at exit, and (b) a child that does call setRawMode(true) still restores cooked mode via the existing uv_tty_reset_mode atexit hook.

Security risks

None. No user-controlled input reaches the new code; it's a 3-element global flag array indexed only by bounds-checked stdio fd numbers. No auth, crypto, permissions, or data-exposure surface is touched.

Level of scrutiny

Moderate — process exit / signal-handler code runs on every TTY-attached Bun process, so a mistake here could leave terminals in raw mode. However, the change is strictly narrowing: it only skips a tcsetattr that previously fired unconditionally, and only for fds Bun never modified. The signal-safety details (type volatile sig_atomic_t, write-before-syscall ordering) were raised in earlier review rounds and have all been addressed. I verified via grep that Bun__ttySetMode is the only path that mutates stdio termios on Bun's behalf (the Terminal.zig and ansi_renderer.zig callers either go through Bun__ttySetMode or operate on non-stdio PTY fds), so the flag covers all writers.

Other factors

This PR has been through four rounds of bot review (my three inline comments on test determinism, ICANON platform constants, and the signal-race ordering, plus CodeRabbit's on chunk-safe READY parsing, sig_atomic_t, cooked-precondition assertion, mid-run raw-mode assertion, and exit-code ordering). Every comment is marked resolved and the diff reflects each fix. The bug-hunting system found nothing on the current revision. The two new tests passed on all CI lanes including ASAN per the PR description, and the remaining CI annotations are documented pre-existing flakes unrelated to this change. Given the small, well-reasoned diff and the thoroughness of the iterative review, I'm comfortable approving without further human review.

Overview

Gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp) on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is set in Bun__ttySetMode() (wtf-bindings.cpp) just before applying termios changes. Adds two POSIX integration tests in terminal-spawn.test.ts. Net runtime change is ~6 lines of logic plus a flag declaration; the rest is comments and tests.

Security risks

None. No user-controlled input reaches the new code; the flag is a process-local bookkeeping bit written only from Bun__ttySetMode (the existing process.stdin.setRawMode path) and read only from the existing exit/signal handler. No auth, crypto, or external data.

Level of scrutiny

Moderate — this runs on every TTY-connected Bun process at exit and inside a signal handler, so it warranted careful review, which it got: across the timeline the test timing race, platform-specific ICANON value, chunk-split READY handshake, cooked-precondition assert, mid-run raw-mode assert, exit-code-last ordering, sig_atomic_t signal safety, and the write-before-syscall ordering were all raised and fixed. The change is strictly narrowing (skips a tcsetattr that used to fire unconditionally), so the failure mode is bounded to "termios not restored" rather than new writes, and the existing uv_tty_reset_mode atexit hook still covers the fds Bun actually touched. I confirmed all POSIX setRawMode paths in ProcessBindingTTYWrap.cpp route through Bun__ttySetMode, so the gating doesn't miss any callers.

Other factors

All inline comments are resolved. The bug-hunting pass found nothing on the current revision. The two new tests passed on every CI lane including debian-13-x64-asan; remaining build annotations are documented infra/flake retriggers (the last four commits are empty retriggers). Nothing has changed since my last comment except the fix it requested.

Overview

Adds a volatile sig_atomic_t bun_stdio_modified[3] flag (defined in c-bindings.cpp, written from Bun__ttySetMode in wtf-bindings.cpp) and gates the per-fd tcsetattr in bun_restore_stdio() on it. Net effect: at exit (and in the SIGINT/SIGTERM handler), Bun no longer writes its startup termios snapshot back to stdio fds it never modified — fixing #29592 where bun … | less clobbers the pager's raw mode. Two new Bun.Terminal-driven regression tests in terminal-spawn.test.ts cover both the don't-clobber case and the still-restores-after-setRawMode case.

Security risks

None. No auth, crypto, network, or untrusted-input surface; the only state touched is a process-local 3-element flag array and the existing termios restore loop.

Level of scrutiny

Medium-high. The diff is small (~30 logic lines) and the reasoning is sound, but it changes a code path that runs on exit/signal for every TTY-attached Bun process. The behavioral delta — Bun used to unconditionally restore termios at exit and now only does so if it was the one that changed it — is the intended fix, but it's a user-visible default change with broad reach, so a maintainer should confirm the trade-off is acceptable.

Other factors

• Every prior inline comment (mine and CodeRabbit's) is resolved in the current diff: deterministic stdin handshake instead of setTimeout, platform-correct ICANON value, chunk-safe READY buffering, volatile sig_atomic_t typing, cooked-precondition asserts, mid-run raw-mode assertion, exit-code-last ordering, and the set-flag-before-tcsetattr reorder (commit dda545e) closing the signal-race gap I flagged.
• Verified Bun__ttySetMode is the funnel for all in-tree stdio termios writes (ProcessBindingTTYWrap.cpp, tty.zig, ansi_renderer.zig all route through it), so the flag covers the relevant paths.
• No CODEOWNERS for the touched files.
• CI on the latest commit still shows failures per robobun, with four empty retrigger commits chasing infra/flake issues; the PR description's analysis of the earlier build attributes failures to known flakes/infra, but the most recent build (#47368) hasn't been re-analyzed in-thread.

Overview

This PR gates bun_restore_stdio()'s exit-time tcsetattr on a new volatile sig_atomic_t bun_stdio_modified[3] flag, set inside Bun__ttySetMode immediately before the device-modifying uv__tcsetattr call. Net effect: a Bun process that never calls setRawMode no longer writes its startup termios snapshot back to the shared /dev/pts/* device at exit, fixing #29592 where a downstream pipeline consumer's raw mode was clobbered. Two new Bun.Terminal-driven regression tests cover both directions (unmodified child leaves raw mode alone; modifying child still restores cooked mode via the existing uv_tty_reset_mode atexit hook).

Since my last inline comment, the only substantive change is dda545e, which moved the bun_stdio_modified[fd] = 1 write to before uv__tcsetattr — closing the signal-race window I flagged. All other commits since are empty CI retriggers. Every review thread (mine and CodeRabbit's: deterministic stdin handshake, platform-correct ICANON, chunk-safe READY buffering, cooked-precondition assertions, sig_atomic_t type, mid-run raw assertion, exit-code-last ordering, flag-write ordering) is resolved and reflected in the current diff.

Security risks

None. No untrusted input parsing, auth, crypto, or privilege boundaries. The change only narrows when an existing tcsetattr is issued on the process's own stdio fds.

Level of scrutiny

Moderate-to-high. The C++ change is ~20 lines, but it alters behavior on a code path that runs at exit (and from the SIGINT/SIGTERM handler) for every Bun process with a TTY on fd 0/1/2. The fix removes a previously unconditional safety net — restoring the startup termios snapshot — in favor of a gated one. The reasoning is sound, well-commented, and the signal-safety details (volatile sig_atomic_t, write-before-syscall ordering) are correct. But a regression here would surface as "terminal left in raw mode after running bun" for end users, so the design trade-off (skip restore for unmodified fds vs. e.g. compare-and-restore-if-unchanged) deserves a maintainer's eyes rather than bot-only approval.

Other factors

• The two new tests passed on every CI lane including ASAN per the PR description; remaining CI failures (serve-body-leak, bun-run-bunfig, ssg-pages-router on macOS x64) are unrelated to this change.
• No CODEOWNERS entry covers src/bun.js/bindings/{c,wtf}-bindings.cpp.
• I have not previously posted a top-level review verdict on this PR — only inline nits, all now resolved.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp) on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is set inside Bun__ttySetMode() (wtf-bindings.cpp) immediately before Bun applies a termios change. Net effect: a Bun process that never calls setRawMode no longer writes its startup termios snapshot back to the shared PTY device at exit, which was clobbering raw-mode state set by downstream pipeline consumers like less/fzf (#29592). Two new Bun.Terminal-driven integration tests cover both the regression (child that never touches termios leaves raw mode alone) and the non-regression (child that calls setRawMode(true) still restores cooked mode via the existing uv_tty_reset_mode atexit hook).

Security risks

None. No user-controlled input reaches the new code; the flag is a process-local 3-element array indexed by hardcoded fd 0/1/2 with an explicit bounds check. The change strictly removes a syscall in the common case rather than adding one.

Level of scrutiny

Moderate — this touches the SIGINT/SIGTERM handler path and process-exit cleanup, so signal-safety matters. That concern was raised and addressed across review rounds: the flag is volatile sig_atomic_t (signal-safe access), and per my last comment the write was moved before uv__tcsetattr so a signal landing mid-transition can't skip the restore. The behavioural change is strictly narrowing (skip a previously-unconditional tcsetattr), so the worst-case downside is "termios not restored" — which the second new test guards directly.

Other factors

Every inline thread on this PR (two from me, four from CodeRabbit) is resolved and reflected in the final diff: deterministic stdin-handshake instead of setTimeout, platform-correct ICANON constant, chunk-safe READY buffering, cooked-precondition assertions, mid-run raw-mode assertion in the companion test, exit-code-last assertion ordering, and the flag-before-syscall reorder. The only commits since my last review are dda545e (that reorder) plus six empty CI-retrigger commits. No CODEOWNERS cover the modified files. The C++ change is ~20 logic lines, heavily commented, and the bug-hunting system found no issues this round.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is set inside Bun__ttySetMode() before the device is flipped. Net change is ~20 lines of logic across c-bindings.cpp and wtf-bindings.cpp, plus two deterministic PTY regression tests in terminal-spawn.test.ts. The fix strictly reduces exit-time side effects: fds Bun never touched are left alone; fds it did touch keep both restoration paths (uv_tty_reset_mode atexit + bun_restore_stdio).

Security risks

None. This is termios/TTY exit-path bookkeeping — no auth, crypto, permissions, network, or untrusted-input handling.

Level of scrutiny

Moderate, because it touches a signal handler (onExitSignal) and process-exit path. That scrutiny has already been applied across three review rounds: signal-safety (volatile sig_atomic_t), write-before-syscall ordering to close the SIGTERM race, deterministic test handshakes, chunk-safe READY buffering, platform-correct ICANON constants, cooked-mode preconditions, and exit-code-last assertion ordering — all addressed and all inline threads resolved. I verified the only other stdio-fd termios mutator (md/ansi_renderer.zig) routes through Bun__ttySetMode, so the flag is set there too.

Other factors

No CODEOWNERS coverage on the touched paths. The companion test guards the obvious regression (a child that does call setRawMode still leaves the device cooked on exit). The PR description documents that the new tests passed on every lane including debian-13-x64-asan; subsequent commits are CI retriggers for unrelated infra/flake failures. The bug hunting system found nothing on this revision.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp:424-429) on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is set in Bun__ttySetMode() (wtf-bindings.cpp:167-181) just before Bun applies a termios change. Net effect: Bun processes that never call setRawMode no longer write their startup termios snapshot back to the shared /dev/pts/* device at exit, fixing #29592 where piping to less/fzf left the pager unresponsive. Two new Bun.Terminal-driven regression tests are added to terminal-spawn.test.ts.

Security risks

None. No user input is parsed, no auth/crypto/permissions are touched. The change strictly narrows an existing side effect (writes fewer tcsetattr calls at exit), and the flag is a process-local sig_atomic_t only ever set to 1.

Level of scrutiny

Medium — process-exit / signal-handler code is sensitive, but the change is tiny (one new global, one continue guard, one flag store) and conservative in direction. Signal-safety was handled correctly after review iteration: volatile sig_atomic_t for cross-signal-context access, and the flag is written before uv__tcsetattr so a SIGINT/SIGTERM landing mid-call still triggers the restore. The only Zig caller (output.zig restore()) is unaffected — it just inherits the new gating, which is the intended behavior.

Other factors

This PR has been through multiple review rounds and every comment thread is resolved: my earlier nits (timing-based test → stdin handshake, platform-correct ICANON constant, flag-write ordering vs. signal window) and CodeRabbit's (chunk-safe READY buffering, sig_atomic_t, cooked-precondition asserts, exit-code-last ordering, mid-run raw-mode verification) are all reflected in the current diff. The two new tests are deterministic (no sleeps), platform-aware, and passed on every CI lane including debian-13-x64-asan; the lone remaining CI failure is the known-unrelated node-http-backpressure-max darwin flake. No CODEOWNERS cover these paths.

Overview

Gates the exit-time tcsetattr in bun_restore_stdio() on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is set inside Bun__ttySetMode() just before the device is flipped. Net effect: a Bun process that never calls setRawMode no longer writes its startup termios snapshot back to the shared PTY device at exit, fixing #29592 (pipeline consumers like less/fzf getting knocked out of raw mode). Two new Bun.Terminal-driven regression tests cover both the never-touched and touched-then-restored paths.

Security risks

None. No user-controlled input reaches the new code; the flag is a 3-element process-local array indexed only by the bounds-checked stdio fd. No auth, crypto, network, or filesystem surface.

Level of scrutiny

Moderate — process-exit / signal-handler territory is subtle, but the change is small (~15 lines of logic), strictly narrows when restoration runs, and has been through several review rounds:

• volatile sig_atomic_t for signal-context safety (CodeRabbit, addressed in d3fd737).
• Flag set before uv__tcsetattr so a SIGINT/SIGTERM in the gap can't skip the restore (my prior note, now applied — and the spurious-set case is the harmless pre-PR behaviour).
• Tests hardened: deterministic stdin handshake instead of setTimeout, chunk-safe READY buffering, platform-correct ICANON constant, cooked-precondition assertions, mid-run raw-mode assertion, exit-code asserted last.

Other factors

The new tests pass on all lanes including debian-asan per the PR description; the remaining CI annotation (bun-run-bunfig.test.ts on macOS-14-x64) is in install/bunfig code this PR does not touch, and the recent commits are explicit CI-retry empties for known flakes. No CODEOWNERS for the touched paths. All inline threads on the PR are resolved.

Overview

Adds a volatile sig_atomic_t bun_stdio_modified[3] flag, set in Bun__ttySetMode (wtf-bindings.cpp) just before uv__tcsetattr, and checked in bun_restore_stdio (c-bindings.cpp) so the exit-time/SIGINT/SIGTERM termios restore only runs for stdio fds Bun actually touched. Two new Bun.Terminal-driven regression tests in terminal-spawn.test.ts cover both directions (child that never touches termios leaves the parent's raw mode alone; child that calls setRawMode(true) still restores cooked mode on exit).

All inline feedback from prior rounds has been applied: deterministic stdin handshake instead of setTimeout, platform-correct ICANON constant, chunk-safe READY buffering, volatile sig_atomic_t for signal-context safety, cooked-precondition assertions, raw-while-live assertion in the companion test, exit-code asserted last, and the flag write hoisted before uv__tcsetattr to close the signal-gap window. The current diff matches every suggested fix.

Security risks

None. No untrusted input handling, auth, crypto, or network surface — just gating an existing tcsetattr call on a process-local boolean. The bounds check fd >= 0 && fd < 3 prevents OOB writes from non-stdio fds (e.g. Bun.Terminal's PTY master fd).

Level of scrutiny

Moderate-to-high. The C++ change is small (~20 net lines) and well-reasoned, but it alters behavior on the exit path and inside a signal handler (onExitSignal) for every Bun process that starts with a TTY on fd 0/1/2. The trade-off — dropping the unconditional restore so downstream pipeline consumers like less/fzf aren't clobbered — is the right call (matches Node.js semantics), but it does remove a pre-existing safety-net that would have papered over termios mutations from native addons or other code paths that bypass Bun__ttySetMode. I verified the only other in-tree termios writer on fd 0 (src/md/ansi_renderer.zig) goes through bun.tty.setMode first, so the flag would be set there too; FileReader.zig's tcsetattr is commented out.

Other factors

• No CODEOWNERS entries cover the touched files.
• Tests are deterministic and pass on ASAN per the PR description; the two new tests are well-constructed after the review iterations.
• Latest CI on 8b88644 shows a 🍎 x64 build-cpp failure and a 🪟 aarch64 agent-provisioning failure; the chain of ci: retry commits suggests infra, but worth confirming the macOS build is green before merge.
• This is a deliberate behavior change in process lifecycle, not a mechanical fix, so a maintainer should confirm the design choice.

Overview

Gates bun_restore_stdio() on a new volatile sig_atomic_t bun_stdio_modified[3] flag set inside Bun__ttySetMode() before the tcsetattr call, so Bun no longer writes its startup termios snapshot back to the TTY device at exit unless it actually modified termios. Touches c-bindings.cpp (definition + guard in the restore loop), wtf-bindings.cpp (extern decl, <signal.h> include, flag write), and adds two PTY-driven regression tests in terminal-spawn.test.ts.

Security risks

None. No untrusted input, no auth/crypto/permissions surface; the change strictly reduces the set of tcsetattr calls Bun issues at exit.

Level of scrutiny

Medium-high. The C++ diff is small (~15 lines of logic) and conservative, but it lives in the SIGINT/SIGTERM handler and atexit path that executes on every TTY-connected Bun process, and it inverts the prior defensive default ("always restore" → "only restore if we touched it"). I verified that every in-tree termios modifier on fds 0–2 (repl.zig, init_command.zig, update_interactive_command.zig, ansi_renderer.zig, ProcessBindingTTYWrap.cpp) routes through Bun__ttySetMode/bun.tty.setMode, so the gating doesn't strand any known caller. The FileReader.zig reference is commented-out code; Terminal.zig operates on the PTY master fd and is correctly excluded by the fd < 3 bounds check.

Other factors

All seven prior review threads (mine and CodeRabbit's) are resolved and reflected in the current diff: deterministic stdin handshake, chunk-safe READY buffer, platform-correct ICANON, volatile sig_atomic_t, cooked-precondition asserts, mid-run raw-mode assertion, exit-code-last ordering, and the flag-before-tcsetattr reorder. The two new tests are well-constructed and would catch a regression on either side. No CODEOWNERS apply. The remaining reason I'm not auto-approving is (a) this is a process-lifecycle policy change a maintainer should sign off on, and (b) the most recent robobun CI report (#47480 on 7cd1d13) still shows build-stage failures across multiple lanes — likely infra given the string of ci: retry commits, but I can't independently confirm the new tests pass on the final revision.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp) on a new volatile sig_atomic_t bun_stdio_modified[3] flag, written from Bun__ttySetMode() (wtf-bindings.cpp) before it applies a termios change. Two new Bun.Terminal-driven regression tests in terminal-spawn.test.ts verify (a) a child that never touches termios leaves a downstream raw-mode consumer alone, and (b) a child that calls setRawMode(true) still restores cooked mode at exit.

Since my last review the author moved the bun_stdio_modified[fd] = 1 write to before uv__tcsetattr(), closing the few-instruction signal window I flagged. All earlier nits (deterministic stdin handshake, chunk-safe READY buffer, platform-specific ICANON constant, sig_atomic_t type, cooked-precondition assert, mid-run raw-mode assert, exit-code-last ordering) are reflected in the final diff and every inline thread is resolved. The bug-hunting system found nothing in this run.

Security risks

None. No untrusted input is parsed; the change only narrows when an existing tcsetattr syscall fires at process exit.

Level of scrutiny

Medium-high. The diff is small (~30 substantive lines) and the logic is sound, but bun_restore_stdio() runs from the SIGINT/SIGTERM handler and the atexit path on every Bun process with a TTY. This PR removes the unconditional safety-net restore in favor of gating on Bun__ttySetMode being the sole writer. I spot-checked other tcsetattr callers in src/: ansi_renderer.zig goes through bun.tty.setMode (→ Bun__ttySetMode), FileReader.zig's call is commented out, Terminal.zig operates on PTY master fds (>2), and parse_args.zig is unrelated — so the gate appears complete. Still, changing signal-context exit behavior for all interactive invocations is the kind of design call a maintainer should confirm.

Other factors

The two new tests are deterministic (stdin-gated handshake, no sleeps), platform-aware, and cover both the regression and the still-restores companion case. CI on the latest commit shows only unrelated infra/flake annotations per the PR description. No CODEOWNERS apply to the touched files.

Overview

This PR gates bun_restore_stdio()'s exit-time tcsetattr on a new volatile sig_atomic_t bun_stdio_modified[3] flag, set in Bun__ttySetMode before the uv__tcsetattr call. Net effect: a Bun process that never called setRawMode no longer writes its startup termios snapshot back to the shared /dev/pts/* device at exit, fixing the bun … | less clobber in #29592. Two new Bun.Terminal-driven tests cover both directions (untouched fd → left alone; setRawMode(true) → restored).

Security risks

None. No new inputs, no parsing, no privilege changes — just a guard that skips a syscall it previously made unconditionally.

Level of scrutiny

Medium-high. The diff is small (~20 effective C++ lines) and strictly narrowing, but it lives in process init/exit and the SIGINT/SIGTERM handler path (onExitSignal → bun_restore_stdio). I confirmed via grep that Bun__ttySetMode is the only active code path that mutates stdio termios (the ansi_renderer.zig probe goes through bun.tty.setMode → Bun__ttySetMode; FileReader.zig's tcsetattr is commented out; Terminal.zig operates on the PTY master fd, not 0-2), so the new gate shouldn't drop restoration for any existing caller. Still, exit-time + signal-context termios handling is exactly the kind of subtle area where a maintainer should glance at the trade-off (we now rely solely on uv_tty_reset_mode's atexit hook for the normal-exit restore, and on bun_restore_stdio only for the signal path).

Other factors

Every prior inline comment (timer-based race → stdin handshake, Linux-only ICANON constant → platform-conditional, chunk-split READY → streaming buffer, int32_t → volatile sig_atomic_t, cooked-precondition assertion, raw-while-live assertion, exit-code-last ordering, and my flag-before-tcsetattr reorder) is resolved and visible in the current diff. No CODEOWNERS apply to the touched files. The recent ci: retry commits carry no code changes.

Overview

Gates bun_restore_stdio() (called from onExitSignal, Bun__onExit, crash_handler.zig, and Global.zig exit paths) on a new volatile sig_atomic_t bun_stdio_modified[3] flag that is only set inside Bun__ttySetMode. ~20 lines of logic across c-bindings.cpp and wtf-bindings.cpp, plus two new Bun.Terminal-driven regression tests in terminal-spawn.test.ts. Fixes #29592 (Bun clobbering a downstream pager's raw mode on the shared PTY device).

Security risks

None. No auth, crypto, untrusted input, or privilege boundaries involved — purely local termios bookkeeping on stdio fds 0–2.

Level of scrutiny

Medium-high. The diff itself is small and mechanically simple, but it changes the default exit behavior: previously every TTY-attached Bun process unconditionally wrote its startup termios snapshot back at exit/SIGINT/SIGTERM/crash; now it only does so if Bun__ttySetMode was ever called for that fd. I verified the only other in-tree termios writer on stdio (src/md/ansi_renderer.zig) goes through bun.tty.setMode → Bun__ttySetMode, so it sets the flag. But this is the kind of semantic narrowing of a long-standing safety net that a maintainer should explicitly bless.

Other factors

This PR has been through ~6 rounds of review iteration; every inline thread (deterministic stdin handshake instead of setTimeout, platform-correct ICANON, chunk-safe READY buffer, cooked-mode preconditions, volatile sig_atomic_t, raw-while-live assertion, exit-code-last ordering, and the flag-write-before-tcsetattr reorder) is now resolved and reflected in the current diff. The bug-hunting pass found nothing on this revision. The two new tests are well-constructed and the PR description reports them green on all CI lanes including ASAN. I'm not approving solely because process-exit / signal-handler code is a critical path per my guidelines, not because of any outstanding concern with the implementation.

Overview

Adds a volatile sig_atomic_t bun_stdio_modified[3] flag (defined in c-bindings.cpp, written from Bun__ttySetMode in wtf-bindings.cpp) and gates the per-fd tcsetattr in bun_restore_stdio() on it. Net effect: Bun no longer writes its startup termios snapshot back to the TTY device at exit unless Bun itself changed termios via setRawMode. Two new Bun.Terminal-driven regression tests cover both directions (untouched-fd left alone; setRawMode(true) still restored on exit).

Security risks

None. No new inputs, no parsing, no auth/permissions surface. The only state touched is process-local termios on stdio fds; the change strictly removes an exit-time syscall in the common case.

Level of scrutiny

Higher than the line count suggests. bun_restore_stdio() is on the normal-exit and SIGINT/SIGTERM signal-handler path of every Bun process that has a TTY on fd 0/1/2, so a mistake here would leave users' shells in raw mode. That said, the change is conservative (narrows an existing side effect), the flag is the correct signal-safe type, and the write is now ordered before the syscall so a signal landing mid-setRawMode still triggers restoration. I verified the only in-tree termios mutator on stdio fds routes through Bun__ttySetMode (src/tty.zig → Bun__ttySetMode; the FileReader.zig callsite is commented out; Terminal.zig operates on PTY master fds excluded by the fd < 3 bounds check).

Other factors

Every prior review thread (mine and CodeRabbit's) is resolved in the current diff: deterministic stdin handshake instead of setTimeout, platform-correct ICANON constant, chunk-safe READY buffering, cooked-state precondition asserts, mid-run raw-mode assertion in the companion test, exit-code asserted last, sig_atomic_t type, and the flag-before-syscall reorder. No CODEOWNERS for the touched paths. Deferring solely because this is process-exit / signal-handler code with runtime-wide blast radius — a maintainer should give the final nod.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() so that a Bun process acting as a pipeline producer (stdout is not a TTY) no longer writes its startup termios snapshot back to the shared /dev/pts/* device unless Bun itself modified termios via Bun__ttySetMode. It touches src/bun.js/bindings/c-bindings.cpp (the gate + a new volatile sig_atomic_t bun_stdio_modified[3]), src/bun.js/bindings/wtf-bindings.cpp (marking the flag before uv__tcsetattr), and adds three integration tests in test/js/bun/terminal/terminal-spawn.test.ts covering the pipeline-producer, interactive-wrapper, and setRawMode paths.

Security risks

None identified. The change only narrows when an existing tcsetattr fires; it introduces no new inputs, no parsing, no auth/crypto/permission surface. The new global is a 3-element sig_atomic_t array written only from Bun__ttySetMode with a bounds check.

Level of scrutiny

Moderate-to-high. While the diff is small, it changes behavior in code that runs on every process exit and inside the SIGINT/SIGTERM handler, and it encodes a heuristic (bun_stdio_tty[1] == 0 ⇒ "pipeline producer, skip restore") that trades the old unconditional safety net against #29592. After my earlier 🔴 about the run_command/bunx/crash-handler call sites, the author reworked the gate to preserve the unconditional restore when stdout is a TTY and added a test pinning that behavior — a reasonable resolution, but still a judgment call that a maintainer should ratify.

Other factors

The PR went through ~8 rounds of bot review; every inline comment (timing-based test handshake, chunk-safe READY buffer, sig_atomic_t, flag-write ordering vs. signal delivery, Darwin c_lflag offset, musl dlopen sonames, static imports, comment arithmetic) is now resolved and reflected in the current diff. The bug-hunting system found nothing on this revision. The three new tests are deterministic (stdin-handshake, no sleeps), platform-aware (Darwin/musl/glibc), and cover both branches of the new gate plus the pre-existing uv_tty_reset_mode path. Given all that, this looks correct to me — I'm deferring only because the pipeline_producer line is a product-behavior decision in lifecycle code, not because of any outstanding concern.

Overview

Touches src/bun.js/bindings/c-bindings.cpp (adds volatile sig_atomic_t bun_stdio_modified[3] and gates the per-fd tcsetattr in bun_restore_stdio on pipeline_producer && !bun_stdio_modified[fd]), src/bun.js/bindings/wtf-bindings.cpp (sets the flag inside Bun__ttySetMode before uv__tcsetattr), and adds three POSIX integration tests in test/js/bun/terminal/terminal-spawn.test.ts exercising the pipeline-producer, interactive-wrapper, and setRawMode restore paths via bun:ffi + openpty.

Security risks

None identified. No untrusted input is parsed; the change only narrows when an existing tcsetattr(fd, TCSANOW, &startup_snapshot) fires. The flag is bounds-checked (fd >= 0 && fd < 3) and uses volatile sig_atomic_t for signal-context safety.

Level of scrutiny

Medium-high. bun_restore_stdio runs on every process exit (atexit/at_quick_exit), from the SIGINT/SIGTERM handler, the crash handler, --watch reload, and the run_command/bunx/lifecycle-script signal-death re-raise. The PR went through ~8 review iterations that materially changed the design (original: gate purely on bun_stdio_modified; final: gate only when stdout is not a TTY so the bun run <tui>-after-crash safety net is preserved). That heuristic is reasonable and well-commented in the code, but it is a behavioural policy decision — e.g. bun foo.js > out.txt (stdout redirected to a file in an interactive shell) now also skips the restore — and a maintainer should confirm the trade-off is the one they want.

Other factors

All prior inline threads (mine and CodeRabbit's) are resolved: sig_atomic_t, flag-write-before-tcsetattr ordering, Darwin tcflag_t offset, musl/glibc soname split, handshake-driven tests, exit-code-last assertion order, and the PR description rewrite. The bug-hunting pass on the current revision found nothing. The remaining uncertainty is CI: HEAD is four consecutive ci: retry empties and build #47539 was still in progress at last update, so green CI on the FFI/PTY tests across darwin + glibc + musl lanes hasn't been confirmed yet. No CODEOWNERS entry covers these files.

Overview

This PR gates the exit-time tcsetattr in bun_restore_stdio() so that when Bun is a pipeline producer (stdout is not a TTY) and never modified termios itself, it leaves the shared /dev/pts/* device alone instead of clobbering raw mode set by a downstream consumer like less/fzf. Runtime changes are ~40 lines across c-bindings.cpp (the gate + volatile sig_atomic_t bun_stdio_modified[3]) and wtf-bindings.cpp (setting the flag inside Bun__ttySetMode before the tcsetattr). Three new handshake-driven PTY tests in terminal-spawn.test.ts cover the pipeline-producer case, the interactive-wrapper case (stdout-is-TTY keeps the unconditional restore), and the setRawMode round-trip.

This PR went through ~10 review iterations; every issue I and CodeRabbit raised (timing-based test, ICANON platform constant, signal-safety of the flag type, flag-write ordering vs SIGTERM, the dropped bun run <tui> safety net, Darwin tcflag_t offset, musl/glibc soname differences, dynamic-import convention, comment arithmetic, PR-description drift) is now resolved in the diff and the description has been updated to match the final design. The bug-hunting system found nothing on the current revision; commits since my last comment are CI retries only.

Security risks

None. No untrusted input is parsed; the change only narrows when an already-existing tcsetattr of a startup snapshot fires. Signal-handler safety is correctly handled via volatile sig_atomic_t with the flag written before the syscall.

Level of scrutiny

Moderate-to-high. The runtime change is small and well-commented, but bun_restore_stdio() runs on every process exit (atexit, SIGINT/SIGTERM, crash handler, --watch reload, bun run/bunx/lifecycle re-raise), so a wrong heuristic here affects every user. The chosen discriminator — bun_stdio_tty[1] == 0 (stdout-is-pipe ⇒ suppress; stdout-is-TTY ⇒ keep unconditional restore) — is well-reasoned and covered by both positive and negative tests, but it is a behavioural heuristic rather than a mechanical fix, and I'd want a maintainer to confirm it's the trade-off they want (e.g. bun foo.js 2>&1 | less with stdin still on the TTY would still restore fd 0 since stdout is a pipe → pipeline_producer true → fd 0 untouched, which is correct; but edge cases like stderr-only redirection are worth a human's mental check).

Other factors

Test coverage is thorough and platform-aware (glibc/musl/Darwin sonames, Darwin 8-byte tcflag_t offset, handshake-driven, exit-code asserted last). No CODEOWNERS entry for these files. CI is currently on retry commits for known infra flakes, not test failures.

Overview

This PR changes bun_restore_stdio() in src/bun.js/bindings/c-bindings.cpp to skip the exit-time tcsetattr for stdio fds Bun never modified, but only when stdout is not a TTY (the bun foo.js | less pipeline-producer shape). It adds a volatile sig_atomic_t bun_stdio_modified[3] flag written from Bun__ttySetMode in wtf-bindings.cpp, plus three PTY/FFI-based regression tests in test/js/bun/terminal/terminal-spawn.test.ts.

Security risks

None identified. No auth/crypto/permissions surface; the change only affects whether a tcsetattr is issued at process exit.

Level of scrutiny

Medium-high. The C++ change is small but lives in code that runs on every process exit and inside the SIGINT/SIGTERM handler, and the bun_stdio_tty[1] == 0 heuristic is a design judgment that determines when Bun does vs. doesn't restore termios for the user's shell. This went through ~8 review iterations including a substantive mid-review redesign (the stdout-is-TTY carve-out was added after I flagged that the original gate silently dropped the bun run <tui> / crash-handler / --watch safety net). The tests exercise platform-specific termios struct layouts and libc sonames via FFI across glibc/musl/darwin.

Other factors

All prior inline threads (mine and CodeRabbit's) are marked resolved, the PR description has been updated to reflect the final semantics, and the bug-hunting system found nothing new this run. CI on the latest commit shows build-step failures that the commit messages identify as known infra flakes (exit 128, zig compiler panic), not test failures. Given the design trade-off embedded here and the fact that it touches every Bun process's exit path, this warrants a human maintainer's explicit sign-off rather than a bot approval.

Overview

Gates the exit-time tcsetattr in bun_restore_stdio() (c-bindings.cpp) on a new volatile sig_atomic_t bun_stdio_modified[3] flag, but only when stdout is not a TTY ("pipeline producer"). The flag is set in Bun__ttySetMode (wtf-bindings.cpp) before the tcsetattr syscall. Adds three handshake-driven PTY integration tests (~230 lines) using bun:ffi openpty/tcgetattr with platform-specific struct offsets and sonames (glibc/musl/darwin).

Security risks

None. No untrusted input, auth, or data exposure. The change is local to process-exit termios restoration.

Level of scrutiny

High. bun_restore_stdio() runs on every process exit, from atexit/at_quick_exit, the SIGINT/SIGTERM handler, the crash handler, --watch reload, and the run_command/bunx/lifecycle-script signal-death re-raise. The bun_stdio_tty[1] == 0 heuristic for "pipeline producer vs interactive wrapper" is a reasonable design call, but it is a design call — it was added in response to review feedback after the initial unconditional gating would have regressed bun run <tui> cleanup. A maintainer should confirm they're happy with that boundary (e.g. bun foo.js > file also takes the suppressed branch, which is fine but worth being conscious of).

Other factors

The PR went through ~8 review iterations (signal-safety type, flag-write ordering, Darwin tcflag_t offset, musl sonames, test handshakes, the stdout-TTY carve-out itself) and every thread is now resolved. The test suite is thorough and covers both sides of the gate plus the setRawMode restore path. Recent commits are CI-retry only (unrelated infra flakes). The C++ change itself is small and well-commented; my hesitation is purely that this is a behavioural policy change to a signal-context exit path, not a mechanical fix.

Overview

Touches src/bun.js/bindings/c-bindings.cpp (bun_restore_stdio, new bun_stdio_modified[3] global), src/bun.js/bindings/wtf-bindings.cpp (Bun__ttySetMode now marks the fd before tcsetattr), and adds three POSIX-only PTY/termios integration tests in test/js/bun/terminal/terminal-spawn.test.ts using bun:ffi openpty/tcgetattr/tcsetattr with platform-specific sonames and struct offsets.

Security risks

None. No untrusted input, no auth/crypto/permissions surface. The change is local to the process's own stdio termios handling; the new global is a per-fd volatile sig_atomic_t flag read from signal context, which is the correct type for that boundary.

Level of scrutiny

Medium-high. The C++ change is small but sits on a hot, subtle path: it runs at process exit and from the SIGINT/SIGTERM handler on every Bun invocation, and it changes when the startup termios snapshot is written back. The fix evolved through review from a blanket bun_stdio_modified[fd] gate to a pipeline_producer && !bun_stdio_modified[fd] gate specifically to preserve the bun run <tui> / crash-handler / --watch restore safety net — that carve-out is a judgment call about user-facing terminal behaviour that a maintainer should ratify. The FFI-based test is also fairly intricate (Darwin tcflag_t offset, glibc/musl/darwin soname branching) and went through several correctness iterations.

Other factors

This PR has been through ~8 rounds of bot review; every inline comment (mine and CodeRabbit's) is marked resolved, the PR description now matches the landed semantics, and the bug-hunting system found nothing on the current revision. The remaining open question is purely the design heuristic, not a code defect, so I'm deferring rather than approving.

Overview

Touches src/bun.js/bindings/c-bindings.cpp (bun_restore_stdio, runs from atexit + SIGINT/SIGTERM handler + Output.Source.Stdio.restore() via crash-handler/--watch/run_command), src/bun.js/bindings/wtf-bindings.cpp (Bun__ttySetMode), three new FFI/PTY regression tests in terminal-spawn.test.ts, and an unrelated Windows leak-bound bump in socket.test.ts.

Security risks

None. No untrusted input, no auth/crypto/permissions. Signal-safety was iterated on (volatile sig_atomic_t, flag-write-before-syscall) and looks correct.

Level of scrutiny

High. The C++ change is ~30 lines but it alters exit-time termios handling for every Bun process on POSIX, including signal-context code. The design went through several reversals during review (pure bun_stdio_modified gate → pipeline_producer && gate to preserve the bun run <tui> safety net; flag-write after → before tcsetattr). The final bun_stdio_tty[1] == 0 heuristic is reasonable and well-commented, but using stdout-tty-ness as the sole proxy for "interactive wrapper vs pipeline producer" is a judgment call a maintainer should ratify — it determines behaviour for edge shapes like bun foo.js > file, bun foo.js 2>&1 | less, etc.

Other factors

• All ~10 prior inline threads (mine and CodeRabbit's) are resolved; the current diff reflects every fix.
• The tests are thorough but lean on platform-specific FFI (dlopen sonames, c_lflag struct offsets) across glibc/musl/darwin — the kind of thing where an unexercised CI lane can still surprise.
• The socket.test.ts 3→4 bump is an unrelated chronic-flake workaround that the author explicitly flagged as droppable; a maintainer should decide whether it ships here or separately.
• The PR description has been updated to match the final design.

This is well outside "simple, mechanical, or obvious" — deferring.