tty(windows): make non-stdin ReadStream#setRawMode use VT raw mode - #34788
tty(windows): make non-stdin ReadStream#setRawMode use VT raw mode#34788robobun wants to merge 6 commits into
Conversation
On Windows, process.stdin.setRawMode already requests UV_TTY_MODE_RAW_VT
(ENABLE_VIRTUAL_TERMINAL_INPUT) so the terminal supplies VT input such as
bracketed paste. A tty.ReadStream on any other console fd, e.g.
new tty.ReadStream(fs.openSync("CONIN$", "r")), took the $bunNativePtr
branch, which fs.ReadStream never populates, so it always emitted
"setRawMode failed because it was called on something that is not a TTY"
and never reached Source::set_raw_mode (which itself would have used
UV_TTY_MODE_RAW, not RAW_VT).
Unify on the VT raw mode:
* Source::set_raw_mode now requests TtyMode::Vt.
* Source__setRawModeStdin is generalised to Source__setRawModeTty(fd).
fd 0 keeps the stdin uv_tty_t singleton so the mode change is
coordinated with any in-flight libuv console read. For other console
fds the caller's fd is checked with GetConsoleMode and the same
RAW_VT / NORMAL console-mode masks libuv uses are written on a fresh
CONIN$ handle (SetConsoleMode needs GENERIC_READ|GENERIC_WRITE, which
an O_RDONLY handle lacks, and a transient uv_tty_t would _close() the
caller's fd on uv_close).
* jsTTYSetMode on Windows now takes (fd, flag) and tty.ts calls it for
every fd, dropping the dead $bunNativePtr branch.
Console input mode is per input buffer, so after setRawMode(true) on
either path, GetConsoleMode on any console input handle reports the same
ENABLE_WINDOW_INPUT | ENABLE_VIRTUAL_TERMINAL_INPUT mask.
|
Updated 4:02 AM PT - Jul 20th, 2026
✅ @robobun, your commit 7165d46fec0a5623146b0e3f9b0e45d20a573475 passed in 🧪 To try this PR locally: bunx bun-pr 34788That installs a local version of the PR into your bun-34788 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related to #32153 but not a duplicate. Both make
The specific goal here was unifying on the VT raw mode so bracketed paste and other VT-only input works regardless of which fd the program raw-modes, which #32153 on its own does not change (it passes mode |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughChangesWindows TTY raw-mode routing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/tty.ts`:
- Around line 59-64: Condense the comment surrounding Source__setRawModeTty to
three lines or fewer, preserving only the durable Windows VT-path and POSIX
termios distinction. Remove the fd coordination details and first-person
explanation.
In `@test/js/node/tty.test.ts`:
- Around line 194-200: Shorten the comment above the Windows raw-mode test to
three lines or fewer, retaining only the durable invariant that CONIN$ and stdin
must resolve to the same VT raw console mode. Remove historical implementation
details and references to prior failures or bun:ffi limitations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e9da91c-6087-40c0-8ad8-e9f5fc60f6df
📒 Files selected for processing (4)
src/io/source.rssrc/js/node/tty.tssrc/jsc/bindings/ProcessBindingTTYWrap.cpptest/js/node/tty.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Address claude[bot] findings on #34788: * uv_tty_set_mode short-circuits when the requested mode equals the cached tty.rd.mode.mode on the stdin singleton, so writing the console mode via a side channel for fd != 0 could leave that cache stale and make a later process.stdin.setRawMode(true) no-op. Route every console-input fd through the singleton; fall back to direct SetConsoleMode on CONIN$ only when fd 0 itself is not a console (no cache to desynchronise then). * Gate fd != 0 with GetNumberOfConsoleInputEvents so console output handles (CONOUT$, fd 1/2) are rejected with ENOTTY instead of silently raw-moding the input buffer. * jsTTYSetMode on Windows now validates fd with isNumber() + toInt32 under a throw scope; this.fd is a public user-mutable property that fs.ReadStream sets to null on close. The test is extended to cover both the cache-desync sequence and the output-handle rejection.
There was a problem hiding this comment.
The three findings from the earlier pass (libuv mode-cache desync on the CONIN$ path, .asInt32() on user-mutable this.fd, and GetConsoleMode accepting output handles) are all correctly addressed in f8ba2a8 — verified against the current diff, and the test now covers each. Also checked that Source::set_raw_mode's Raw→Vt change has no other reachable JS callers (the removed $bunNativePtr branch was the only path to it via src/js/).
Extended reasoning...
Confirming prior findings are resolved and recording one additional thing ruled out this pass; the remaining inline comment is a placement nit with no behavioral impact. Deferring rather than approving because this is moderately complex Windows-only console/libuv coordination that CI has not yet verified on-target, and there is a competing design in #32153 a maintainer should weigh.
There was a problem hiding this comment.
The three earlier findings (libuv mode-cache desync, asInt32 on user-mutable this.fd, GetConsoleMode accepting output handles) and the ENABLE_WINDOW_INPUT placement nit are all addressed — nothing new to flag on this revision. Deferring to a human because this is non-trivial Windows-native console/libuv logic whose test only runs on Windows x64 CI, and it overlaps with #32153's alternative approach.
What was reviewed:
Source__setRawModeTty: fd gating viaGetNumberOfConsoleInputEvents, stdin-singleton routing keeps libuv's cached mode coherent, CONIN$ fallback masks/close match libuv'suv_tty_set_mode.jsTTYSetMode(Windows): now mirrors the POSIX branch's throw-scope +isNumber()/toInt32shape.- Test asserts stdin↔CONIN$ parity, cache-coherence sequence, and
ReadStream(1).setRawModeerror — matches the fixes.
Extended reasoning...
Overview
Generalises Windows tty.ReadStream#setRawMode so non-stdin console fds (e.g. fs.openSync("CONIN$", "r")) enter the same UV_TTY_MODE_RAW_VT state as process.stdin. Touches src/io/source.rs (rewrites Source__setRawModeStdin → Source__setRawModeTty, ~80 new lines of Win32/libuv logic), src/jsc/bindings/ProcessBindingTTYWrap.cpp (Windows branch of jsTTYSetMode now takes (fd, flag) with proper coercion), src/js/node/tty.ts (drops the dead $bunNativePtr branch), plus a new GetNumberOfConsoleInputEvents extern and ENABLE_WINDOW_INPUT const in the Windows sys crates, and a Windows-only ConPTY test.
Security risks
None identified. No auth/crypto/permissions surface; the input is a JS fd that is validated (isNumber() + toInt32) before being handed to uv_get_osfhandle and gated by GetNumberOfConsoleInputEvents. The fresh CONIN$ handle is opened RW and closed in the same block on all paths.
Level of scrutiny
Medium-high. This is platform-specific native code with subtle correctness concerns: libuv's per-handle mode cache, per-buffer vs per-handle console-mode semantics, and interaction with an in-flight stdin reader. The prior review round surfaced three real bugs in exactly those areas; the author fixed all of them cleanly, and this run's re-review found nothing further. But the fix's design (route every console-input fd through the stdin uv_tty_t singleton, fall back to direct SetConsoleMode only when fd 0 isn't a console) is a judgement call that differs from the competing #32153 approach, and the only test coverage runs on Windows x64 CI — the author could not verify locally.
Other factors
- All four prior inline findings are resolved and the fixes are covered by new assertions in the test (cache-coherence sequence,
ReadStream(1)error). - POSIX is untouched (
source.rsiscfg(windows); the POSIXtty.tsbranch is unchanged). - The
Source::set_raw_modechange fromTtyMode::Raw→TtyMode::Vtalso affects any other Windows caller of that method; that's intentional per the PR's stated goal but worth a maintainer glance. - Given the platform scope, the design overlap with #32153, and CI-only verification, a human sign-off is appropriate rather than auto-approval.
1 participant
What
On Windows,
process.stdin.setRawMode(true)requestsUV_TTY_MODE_RAW_VT(ENABLE_VIRTUAL_TERMINAL_INPUT) so the terminal supplies VT input such as bracketed paste. Atty.ReadStreamon any other console fd took a branch that depended onthis.$bunNativePtr, whichfs.ReadStreamnever populates, so it always emittedsetRawMode failed because it was called on something that is not a TTYand never reachedSource::set_raw_mode(which itself would have requested plainUV_TTY_MODE_RAW, leaving the VT-input flag off).Fix
Source::set_raw_modenow requestsTtyMode::Vt.Source__setRawModeStdinis generalised toSource__setRawModeTty(loop, fd, raw). Console input mode is a property of the input buffer (not the handle), anduv_tty_set_modeshort-circuits when the requested mode matches the cachedtty.rd.mode.modeon the stdin singleton, so every console-input fd routes the actual mode change throughuv_tty_set_modeon that singleton. Forfd != 0the caller's fd is only used to gate that it is a console-input handle (GetNumberOfConsoleInputEvents, which rejects screen-buffer handles withENOTTY). When fd 0 itself is not a console (piped stdin + CONIN$ reopen) there is no libuv cache on the input buffer, and the sameUV_TTY_MODE_RAW_VT/UV_TTY_MODE_NORMALconsole-mode masks libuv uses are written on a fresh RW CONIN$ handle instead; a transientuv_tty_ton the caller's fd is not usable here becauseuv__tty_closecalls_close(fd), andSetConsoleModeneedsGENERIC_READ | GENERIC_WRITEwhich anO_RDONLYCONIN$ handle lacks.jsTTYSetModeon Windows now takes(fd, flag)(validated withisNumber()/toInt32under a throw scope, sincethis.fdis user-mutable and set tonullon close).src/js/node/tty.tscalls it for every fd, dropping the dead$bunNativePtrbranch.Verification
New Windows-only test in
test/js/node/tty.test.tsspawns a child underBun.Terminal(ConPTY) and asserts viaGetConsoleMode(bun:ffi) that:process.stdinand atty.ReadStreamonfs.openSync("CONIN$", "r")leave the console in the same VT raw / cooked masks,process.stdin.setRawMode(true)still takes effect after a CONIN$ stream has restored cooked mode in between (libuv's mode cache stays coherent), andnew tty.ReadStream(1).setRawMode(true)emits an error rather than touching the input buffer.Skipped on arm64 (no bun:ffi backend there). On Windows x64:
POSIX is untouched (
source.rsiscfg(windows); the POSIX branch intty.tsis unchanged).no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/tty.test.ts