Conversation
RunCommand::exec_as_if_node treated every positional as a file name, so `node -` (argv0 = node, or the node shim bun installs) tried to load a file literally named "-" and failed with "Module not found". Route the bare "-" positional through exec_stdin, the same path `bun run -` uses, so the script is read from stdin and process.argv[1] is "-" like node.
|
Warning Review limit reached
Next review available in: 34 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 (2)
Comment |
|
Status: fix pushed, waiting on CI. Automated review so far has no findings. Reproduced on bun 1.4.0-canary.1+b7a043103 and a debug build of main (eabb96d), linux-x64: With this branch the first command prints |
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix that routes node - through the existing exec_stdin path.
What was reviewed:
- Placement of the new branch in
exec_as_if_node: after the-e/-pand empty-positionals checks, before the filename join — so-estill wins andpositionals[0]is safe to index. Ok(false)→Global::exit(1)matches how thebun run -caller handles the same return (mod.rs:1474-1475).- Only bare
-matches (./-still falls through to the filename path); tests plant a literal-file in cwd to prove the old behavior is gone. - Test coverage: argv shape across 4 variants, exit-code propagation, and
--requireordering; follows existing harness conventions in the same file.
Extended reasoning...
Overview
Two-file change: a 10-line addition in src/runtime/cli/run_command.rs inserting a positionals[0] == b"-" check into exec_as_if_node that dispatches to the pre-existing exec_stdin helper (already used by bun run -), plus a doc-comment update on exec_stdin. Tests add a describe block with 6 cases to test/cli/run/as-node.test.ts.
Security risks
None. This is CLI argument dispatch — no untrusted parsing, no memory management, no unsafe blocks. The only new comparison is a byte-slice equality against a literal.
Level of scrutiny
Low-medium. The change is a targeted Node.js CLI compat fix that reuses existing, already-tested machinery (exec_stdin) rather than introducing new logic. The Global::exit(1) on Ok(false) mirrors the existing bun run - caller in mod.rs. The branch is placed correctly relative to the -e/-p and empty-positionals guards, and only an exact - triggers it (so node ./- still runs a file). No CODEOWNERS on this path.
Other factors
Test quality is good: uses test.each for the argv matrix, asserts stdout before exitCode per repo convention, plants a decoy file named - in cwd so the old "run the file" behavior would loudly fail, and covers --require interaction and exit-code propagation. The PR description explicitly verified all 6 tests fail on the released binary and pass with the change, and that the surrounding as-node.test.ts and run-eval.test.ts still pass. The known process.execArgv divergence is called out as pre-existing and out of scope.
Integrate the independently authored and reviewed 0aba87964e2f CLI fix into the custom fork. Preserve the existing eval-argv owner and bare input-type stdin routing, including explicit-dash argv markers. Version-query upstream publication is separate; bare and empty-source followup will credit the existing oven-sh#38566 explicit-dash work.
Integrate the independently authored and reviewed 0aba87964e2f CLI fix into the custom fork. Preserve the existing eval-argv owner and bare input-type stdin routing, including explicit-dash argv markers. Version-query upstream publication is separate; bare and empty-source followup will credit the existing oven-sh#38566 explicit-dash work. (cherry picked from commit 2e3ce72)
Integrate the independently authored and reviewed 0aba87964e2f CLI fix into the custom fork. Preserve the existing eval-argv owner and bare input-type stdin routing, including explicit-dash argv markers. Version-query upstream publication is separate; bare and empty-source followup will credit the existing oven-sh#38566 explicit-dash work. (cherry picked from commit 2e3ce72)
Problem
node(argv0 isnode: the shimbun --bunputs on PATH, or anodesymlink to bun),node -does not read the script from stdin. It fails witherror: Module not found '<cwd>/-'and exit code 1; if a file named-happens to exist in cwd, it runs that file instead.some-tool | node -is the standard node way to run a generated script, and it works in plain bun (bun run -).RunCommand::exec_as_if_node(src/runtime/cli/run_command.rs) goes from the-e/-pbranch straight to "the positional is a file name" and joins it onto cwd. It never checks for the-marker. Thebun run -path (exec_with_cfg->exec_stdin) does. Not a regression: the ZigexecAsIfNodehad the same shape.Fix
exec_as_if_nodenow routes a bare-positional to the existingexec_stdin, the same functionbun run -uses. A failed stdin read exits 1, which is also whatbun run -does.echo 'console.log(process.argv.slice(1))' | node - a bprints["-","a","b"](node v26.3.0). Bun's argument parser stops at the first positional for the node command, soa bare already inctx.passthrough, andexec_stdinprepends the-itself, so the argv comes out the same.-e/-pare checked earlier in the function and still win over a-positional, as in node, andnode ./-still runs a file of that name because only the bare-is the marker.process.execArgvfor a stdin script still contains"-". That is computed elsewhere (create_exec_argvin node_process.rs) and is the same forbun run -today; it is a separate fix.node - runs the script from stdinblock (argv for-,- a b,- --flag -x,-- - a; exit code propagation;--requirepreload before the stdin script). All 6 fail on the released binary and pass with this change. The rest of as-node.test.ts and test/cli/run/run-eval.test.ts (thebun run -coverage) still pass with the debug build.Background
node,Command::which(src/runtime/cli/mod.rs) dispatches toRunAsNodeCommand, whose body isexec_as_if_node. It imitates the node CLI rather thanbun run: no package.json script lookup, nonode_modules/.binlookup, the positional is always a file.exec_stdin: reads all of stdin intoctx.runtime_options.eval.scriptand boots the VM with the synthetic entry point<cwd>/[stdin]; the module loader serves that path from the in-memory script (the same mechanism[eval]uses). It also pushes"-"to the front ofctx.passthrough, which becomesprocess.argvafter the executable.Arguments::parseusesstop_after_positional_at = 1for the node command, soctx.positionalsholds only the script argument and every later argument lands inctx.passthroughunparsed. That is why the new branch only has to look atpositionals[0].