Repository navigation
Conversation
… TTY bun --filter (and --parallel/--sequential) wires script stdout/stderr to pipes so it can prefix and redraw output. That hides the terminal from the script: isatty() is false, so chalk/supports-color and anything that gates on it drops to monochrome even though the parent is rendering its own frame in color. When the parent bun has ANSI colors enabled on stdout and the user has not set FORCE_COLOR or NO_COLOR themselves, inject FORCE_COLOR into the child environment at the depth the parent detected (1/2/3 for 16/256/truecolor). This is the same approach the parallel test runner already takes and what turbo/nx/lerna do for their task runners. Explicit FORCE_COLOR (including FORCE_COLOR=0) and NO_COLOR are left untouched, and nothing is injected when stdout is not a color terminal. Fixes #10346
|
Warning Review limit reached
Next review available in: 2 minutes 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 (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ABLE_COLORS - Move the injection logic to RunCommand::forward_color_to_piped_scripts so --filter and --parallel share one implementation. - Also skip injection when NODE_DISABLE_COLORS is set, so we do not override the user's Node-standard disable and trigger the "NODE_DISABLE_COLORS is ignored due to FORCE_COLOR" warning in every script. - Make the pty tests hermetic by clearing TMUX and TERM_PROGRAM. - Drain stderr and assert exitCode in the piped-stdout test. - Add coverage for NODE_DISABLE_COLORS and for the --parallel path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Address review feedback: - The injection helper now also requires Output::is_stdout_tty() so `bun --filter '*' build > out.log` (stderr still attached) does not fill the log with ANSI, and treats an empty NO_COLOR as unset per the no-color.org spec. On Windows, where color_depth() has no native arm, default the injected level to 3 (every supported Windows build has truecolor; see is_color_terminal()). - Gate pretty_output on is_stdout_tty() on POSIX too, matching the existing Windows guard. A nested `bun --filter` that inherits the injected FORCE_COLOR no longer turns on the cursor-driven redraw renderer while its own stdout is a pipe. - Run the --elide-lines tests on a pty (elision only exists in the redraw renderer), which also lets them exercise the real output on Windows instead of falling back to the line-prefixed check. - Clear NODE_DISABLE_COLORS in the hermetic env overrides, and assert exitCode in the pty helper. - Add a test that FORCE_COLOR on a piped stdout stays in the line-prefixed writer (no synchronized-update or cursor-up escapes).
The elide-lines tests were the only caller passing these; they now spawn on a pty directly.
There was a problem hiding this comment.
No issues found in this pass; all earlier feedback has been addressed in c1715c4 and 427df77. This changes user-visible CLI behavior (env injection into filtered scripts), tightens the pretty_output gate, and rewrites the --elide-lines tests onto a PTY, so it's worth a human look — particularly for the Windows path, since the tests weren't run locally.
What was reviewed:
forward_color_to_piped_scripts: TTY gate now requiresis_stdout_tty(),NO_COLOR/NODE_DISABLE_COLORSempty-string handling, WindowsColorDepth::None→3fallback.pretty_outputgate: verified the POSIX arm now matches the Windowswindows_is_terminal()check so an inheritedFORCE_COLORon a pipe stays in the line-prefixed writer.- Tests:
runOnPTYenv hermeticity (all varscompute_color_depthreads are pinned), exit-code assertions, and the elide-lines PTY rewrite; deadenv/elideCountparams removed fromrunInCwdSuccess.
Extended reasoning...
Overview
Adds a ~29-line RunCommand::forward_color_to_piped_scripts helper that injects FORCE_COLOR=1/2/3 into the DotEnv map when the parent's stdout is a color TTY and the user hasn't set FORCE_COLOR/NO_COLOR/NODE_DISABLE_COLORS. Called from both filter_run.rs and multi_run.rs immediately before MiniEventLoop::init_global. Also tightens filter_run's pretty_output gate on POSIX to require Output::is_stdout_tty() (matching the existing Windows arm), so FORCE_COLOR alone no longer enables the cursor-driven redraw renderer on a pipe. The --elide-lines tests are rewritten onto Bun.spawn({ terminal: … }) since elision only exists in the redraw path, and a new 8-test describe block covers the injection matrix via PTY.
Security risks
None. This only writes a well-known env var into the process's own DotEnv map before spawning package scripts; no untrusted input is parsed and no new syscalls or file access are introduced.
Level of scrutiny
Medium. The core helper is small and well-guarded, and all seven inline findings from earlier passes (Windows depth fallback, empty NO_COLOR, is_stdout_tty() gate on both the helper and pretty_output, test env hermeticity for NODE_DISABLE_COLORS, runOnPTY exit-code assertion, dead-param cleanup, and the intentional exclusion of the bun test --parallel sibling) are addressed and marked resolved. But this is a user-visible CLI behavior change that affects every --filter/--parallel invocation, the pretty_output gate change alters when the redraw renderer runs for existing users, and the elide-lines tests moved from spawnSync to PTY-based spawning with adjusted regex counts — enough surface that a maintainer should sign off.
Other factors
- Cross-platform: the
cfg!(windows)fallback to depth3and the interaction withwindows_is_terminal()are untested locally per the PR footer ("Platform-specific test(s) that do not run on this machine. Deferring to CI"). - The elide-lines target_pattern regexes were relaxed from
{20}to{10}/{15}occurrences; this reflects the final-frame content under the redraw renderer on a PTY rather than accumulated piped frames, which reads as intentional but is a change to an existing assertion. - The
bun test --parallelworker launcher's open-codedFORCE_COLOR=1was intentionally left out and the PR description now says so explicitly, per the earlier "fix the whole class" comment.
|
Diff is green. All review feedback is addressed and the automated review pass found nothing further. CI on build #81006: every red is either a known flake ( Ready for a maintainer look, particularly the Windows path ( |
|
Closing this PR because it has been inactive for more than 90 days. |
Fixes #10346.
What
bun --filter(andbun run --parallel/--sequential) wires script stdout/stderr to pipes so it can prefix and redraw the output. That hides the terminal from the script:isatty()is false, so chalk/supports-color and anything that gates on it drops to monochrome even though the parent is rendering its own cyan frame in color.Before, on a color terminal:
Fix
When the parent bun's own stdout is a color TTY and the user has not set
FORCE_COLOR,NO_COLOR, orNODE_DISABLE_COLORSthemselves, injectFORCE_COLORinto the script environment at the depth the parent detected (1/2/3for 16/256/truecolor;3on Windows where noTERM/COLORTERMis typically set but every supported build has truecolor). This matches what turbo/nx/lerna do for their task runners. The logic lives in a singleRunCommand::forward_color_to_piped_scriptshelper called from both the--filterand--parallelrunners.Explicit
FORCE_COLOR(includingFORCE_COLOR=0),NO_COLOR, andNODE_DISABLE_COLORSare passed through untouched, and nothing is injected when stdout is not itself a TTY, so piped/redirected output stays clean.The
bun test --parallelworker launcher also setsFORCE_COLORfor its workers but is intentionally left as-is here: it gates on stderr (where test output goes), it controls its own worker processes rather than arbitrary user scripts, and tightening it could change test-output snapshots. It can adopt the helper separately if wanted.Related: redraw renderer now requires a real TTY
The filter runner's
pretty_output(the cursor-driven redraw renderer) was previously enabled wheneverenable_ansi_colors_stdout()was true, so a nestedbun --filterthat inherited the injectedFORCE_COLORwould emit cursor-up/erase sequences into the outer runner's pipe and corrupt its tree.pretty_outputis now also gated onis_stdout_tty()on POSIX, matching the existing Windows check.FORCE_COLORforces color, not redraw.The
--elide-linestests have been moved onto a pty accordingly (elision only exists in the redraw path), which also lets them exercise the real output on Windows instead of falling back to the line-prefixed subset.Verification
New tests in
test/cli/run/filter-workspace.test.tsspawnbun --filterandbun --parallelon a pty viaBun.Terminaland assert the child seesFORCE_COLORandBun.enableANSIColors === true, plus negative cases forNO_COLOR,NODE_DISABLE_COLORS, explicitFORCE_COLOR=0, piped stdout, andFORCE_COLORon a pipe not enabling the redraw renderer.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/filter-workspace.test.ts