Repository navigation
orderfile: hand the interactive workloads their stdin under node - #33358
Conversation
The generator spawns its workloads with spawnSync(input) but stdin set to ignore. Node only delivers input when stdin is a pipe and silently drops it otherwise, so under node the pipe workload reads nothing and exits 0 without ever driving readline, and the tty one waits for a line that never arrives until WORKLOAD_TIMEOUT_MS kills the whole trace. Bun's spawnSync delivers input either way, so only CI notices: it builds with node --experimental-strip-types scripts/build.ts. Pipe stdin when there is input to deliver, which is what the rest of the repo already does (scripts/utils.mjs, scripts/build/fetch-cli.ts). run() captured nothing from its closure, so hoist it to module scope as runCommand and export it: the test drives it under node, which is the only runtime where this is observable.
|
Updated 6:14 AM PT - Jul 5th, 2026
❌ @robobun, your commit 1e39eed has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33358That installs a local version of the PR into your bun-33358 --bun |
|
Warning Review limit reached
Next review available in: 10 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 (1)
WalkthroughRefactors scripts/orderfile/generate.ts by extracting command execution into an exported ChangesOrderfile Command Execution and Interactive Stdin Testing
Related PRs: None identified. Suggested labels: Suggested reviewers: None identified. Poem 🚥 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: 1
🤖 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 `@test/js/bun/perf/linker-order.test.ts`:
- Around line 179-189: The perf test currently drains proc.stderr in Promise.all
but drops it from the assertion output, so failures from the fixture or
runCommand are hard to diagnose. Update the assertion in linker-order.test.ts to
capture stderr alongside stdout and include it in the expected object or failure
diff, using the existing proc.stdout.text(), proc.stderr.text(), and proc.exited
flow so the diagnostic stays visible when the test fails.
🪄 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: 29d1950b-c0bb-4fe4-be89-1a11f8eac525
📒 Files selected for processing (3)
scripts/orderfile/generate.tstest/js/bun/perf/linker-order.test.tstest/js/bun/perf/orderfile-workload-fixture.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted one-line stdio fix in build tooling with a well-constructed node-runtime regression test.
Extended reasoning...
Overview
This PR touches only build/test tooling: scripts/orderfile/generate.ts (the linker order-file generator), test/js/bun/perf/linker-order.test.ts, and a new test fixture. The functional change is a single conditional — stdio[0] becomes options.input === undefined ? "ignore" : "pipe" so that Node's spawnSync actually delivers input to the interactive workloads. The rest is a mechanical hoist of the inner run() helper to an exported module-level runCommand() (it captured nothing from its closure) plus 6 call-site renames, done solely so the test can import and drive it under Node.
Security risks
None. This is an internal build script that spawns cc, nm, ptyrun, and the freshly-built bun-profile with fixed argument lists. No user-controlled input, no network, no auth/crypto/permissions surface. The change only adjusts which end of a child's stdin is a pipe vs /dev/null.
Level of scrutiny
Low. Nothing here ships in the runtime binary — it affects how the release-build order file is generated on Linux CI. The fix matches the established pattern the PR cites elsewhere in scripts/, and gating on === undefined (presence, not truthiness) is the correct choice. The hoist is behavior-preserving: same body, same option handling, just moved and exported.
Other factors
The added test is well-designed for this bug class: it spawns node --experimental-strip-types on a fixture that imports the real runCommand and runs the real cli-fixture.js, so it fails under Node with the old "ignore" wiring and passes with the fix — exactly the CI-only failure mode described. It's guarded with describe.skipIf(process.platform !== "linux" || !nodeExe()), drains stdout/stderr/exited concurrently, and asserts a combined object. generate.ts uses only strippable TS (interfaces, annotations, as) and node:-prefixed imports, so it loads cleanly under Node's type-stripping. No CODEOWNERS cover these paths, no bugs were flagged by the bug-hunting system, and there are no outstanding reviewer comments.
The stdin test drained stderr and dropped it, so a fixture that threw before writing anything (a hung workload, a spawn error) failed with a bare greeted/read mismatch and no reason. node warns about the fixture's module type on every run, so stderr is never empty and cannot be asserted whole. Pull out the uncaught error line instead and assert there isn't one. That also catches generate.ts growing TypeScript node cannot strip, which breaks the real build the same way.
There was a problem hiding this comment.
LGTM — one-line stdin-wiring fix in build tooling, hoisted for testability, with a node-driven regression test that demonstrably fails without the fix.
Extended reasoning...
Overview
This PR touches three files: scripts/orderfile/generate.ts (build tooling for the linux release linker order file), test/js/bun/perf/linker-order.test.ts, and a new 18-line test fixture. The behavioral change is a single expression: stdio[0] becomes options.input === undefined ? "ignore" : "pipe" instead of unconditionally "ignore". The rest is hoisting the previously-local run() helper to module scope as an exported runCommand() (it captured nothing from its closure) so the test can drive it under Node, plus renaming the five call sites.
Security risks
None. This is a build-time script that runs spawnSync on fixed local commands (cc, nm, the built bun-profile binary) during release builds. No user-controlled input reaches the command array, and the change only affects whether a pipe is opened for stdin.
Level of scrutiny
Low. This is build tooling under scripts/, not shipped runtime code. The worst-case failure mode is that the orderfile generation misbehaves on CI, which build.ts already catches and annotates rather than failing the build. The fix follows the pattern the PR description confirms is already used at three other repo call sites (scripts/utils.mjs, scripts/build/fetch-cli.ts).
Other factors
The PR description includes empirical before/after output under both runtimes, and the new test spawns node --experimental-strip-types on the actual runCommand + cli-fixture.js to prove stdin reaches the workload — with a shown failing diff when the fix is reverted. The one CodeRabbit nit (surface stderr in the failure diff) was addressed in commit 432cb95 via the crash field, and the thread is resolved. No CODEOWNERS cover these paths, and the bug hunter found nothing.
darwin-26-aarch64 test-bun failed downloading the build artifact from the agent (120s timeout, zero tests run). Its sibling shard on the same platform and the same artifact passed, as did all 285 other jobs.
There was a problem hiding this comment.
LGTM — one-line stdin wiring fix in build tooling with a well-designed Node-driven regression test.
Extended reasoning...
Overview
This PR touches three files: scripts/orderfile/generate.ts (build tooling for the linker symbol-ordering file), test/js/bun/perf/linker-order.test.ts (adds one test case), and a new 18-line test fixture. The functional change is a single conditional: stdio: [options.input === undefined ? "ignore" : "pipe", ...], fixing Node's spawnSync silently dropping input when stdin is "ignore". The rest is a mechanical hoist of the local run() helper to an exported runCommand() (5 call sites renamed, no other logic change) so the test can drive it under Node — the only runtime where the bug is observable.
Security risks
None. This is a build-time script that runs on CI/locally, spawning known toolchain commands (cc, nm, the built bun-profile) against files it created itself. No user input, no network, no auth, no crypto.
Level of scrutiny
Low. This is not shipped runtime code — it generates a linker input file during release builds. The worst-case failure mode is that orderfile generation continues to behave as it did before (interactive workloads still miss their ~2k tty/readline symbols), or the new test is flaky/skipped. Neither affects the correctness of the shipped binary. The fix follows the pattern already used elsewhere in the repo (scripts/utils.mjs per the PR description), and the condition correctly gates on presence (=== undefined) rather than truthiness.
Other factors
The PR description includes empirical before/after evidence under both runtimes and shows the new test fails with the fix reverted. The one CodeRabbit nit (surface stderr in the failure diff) was addressed in 432cb95 with a better solution than suggested — a falsifiable crash regex extraction rather than expect.any(String) — and the thread is resolved. The test correctly drains pipes concurrently, uses a combined-object assertion, and skips on non-Linux / when Node is unavailable. No outstanding review comments.
|
CI is red on darwin only, and it is not this diff. I have already spent my one retrigger, so this needs a maintainer. Two builds of identical code (the second is
Everything else is green in both, 285 and 284 jobs: every linux lane, every windows lane. Nothing outside darwin has failed once. The hang is Nothing here can reach darwin in the first place: Where the change is actually exercised it runs and passes. From the The 1 skip is Ready to merge from my side, with a retry on the darwin jobs. |
2 participants
Follow-up to #33345, fixing the
run()stdin wiring flagged in this review.The bug
run()asks forinputand for stdin to be"ignore"at the same time:Node only delivers
inputwhen stdin is a pipe. With"ignore"it drops it on the floor, no error. Bun'sspawnSyncdelivers it either way, which is whybun run orderfileworks locally and nobody noticed: CI builds withnode --experimental-strip-types scripts/build.ts(.buildkite/ci.mjs:595), so on CI the two interactive workloads are the only ones that are typed anything and they get nothing.Running the real
cli-fixture.jsthrough the realrun()options, once under each runtime:So on CI today the pipe workload reads
0lines, exits0, and never drives readline. The tty one is worse:ptyrungets/dev/nullfor stdin, types its^Dbefore bun has even started, readline then waits for a line that never arrives, and the workload sits there untilWORKLOAD_TIMEOUT_MS(120s) kills it.run()turns that into a throw,build.tscatches it and annotates, and the build ships unordered. Either way the ~2k tty and readline functions those two workloads exist to capture are missing, which is the whole reason they were added.The fix
Pipe stdin when there is input to deliver. That is what the rest of the repo already does (
scripts/utils.mjs:246,scripts/utils.mjs:358,scripts/build/fetch-cli.ts:250);generate.tswas the only site that got it wrong, and I grepped the others to be sure.run()captured nothing from its closure, so it is hoisted to module scope asrunCommandand exported. That is what lets the test drive it under node, which is the only runtime where any of this is observable.Verification
test/js/bun/perf/linker-order.test.tsgains a case that spawnsnode --experimental-strip-typeson a fixture importingrunCommand, runscli-fixture.jsthrough it, and checks the fixture was actually typed its input. An in-process test would be useless here: it would run under bun and pass either way.With stdin reverted to
"ignore":and with the fix,
17 pass, 1 skip, 0 failfor the file. Agents install node 26.3.0 (scripts/bootstrap.sh), so the case runs rather than skips.