Conversation
… / -p exec_as_if_node built the [eval] entry and booted directly, but Arguments::parse stops at the first positional for RunAsNodeCommand, so `node -e code a b` arrived as positionals=["a"], passthrough=["b"] and boot() only hands passthrough to process.argv. Route the branch through exec_eval, which already folds positionals into passthrough for `bun -e`.
|
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 (2)
Comment |
|
Status: ready for review. The diff is green; CI is blocked only on macOS agent capacity.
|
There was a problem hiding this comment.
LGTM — deduplicates the node-emulation -e/-p path onto exec_eval, which fixes the dropped first positional in process.argv.
What was reviewed:
- Confirmed the deleted block in
exec_as_if_nodeis byte-identical to the tail ofexec_eval(run_command.rs:2870-2880); the only delta is the positionals→passthrough merge, which is the fix. - Checked that
ctx.args.disable_default_env_files = true(set earlier inexec_as_if_node) still flows through sinceexec_evalreceives the samectx. - Test matrix covers
-e/-p/-pe, single/multi/flag-like args, and the empty case (guards against regressing the no-positional path).
Extended reasoning...
Overview
This PR fixes a Node.js-compat bug where bun running as node (via argv0 emulation) drops the first positional argument from process.argv when using -e / -p / -pe. The fix replaces an 11-line duplicated block in RunCommand::exec_as_if_node with a call to the existing RunCommand::exec_eval, which already handles the positionals→passthrough merge correctly for the plain bun -e path. A 6-case test.each is added to test/cli/run/as-node.test.ts.
Security risks
None. This is CLI argument routing for how positional args after -e <code> land in process.argv. No untrusted input parsing, no file/network I/O, no auth or crypto surface.
Level of scrutiny
Low-to-medium. The change is a net -8 lines in one function, replacing a copy-pasted block with a call to the shared helper it was copied from. I verified the deleted block is byte-for-byte the tail of exec_eval (lines 2870-2880), so the only behavioral delta is the prepended merge of ctx.positionals into ctx.passthrough — exactly the fix. The node-specific state set earlier in exec_as_if_node (disable_default_env_files, the --interactive REPL routing) happens on ctx before the call and is preserved. This also satisfies the repo's "one implementation" rule — the previous state had two byte-identical eval-entry-path builders.
Other factors
The PR description documents USE_SYSTEM_BUN=1 failure (5 of 6 new cases fail on the released bun) and passing on the debug build, plus regression runs of run-eval.test.ts, the REPL tests, and env argv0 tests. The test uses test.each per harness convention, exercises the empty-positionals branch (guarding the if !ctx.positionals.is_empty() skip in exec_eval), and includes a flag-like arg case to confirm passthrough doesn't re-parse. No prior reviewer comments to address; timeline only has a CodeRabbit rate-limit notice.
Problem
node(thenodesymlink bun installs,bun --bun node ..., or anything that spawns bun withargv0 = node),-e/-p/-pedrop the first argument after the code fromprocess.argv:node -e code a) it is lost entirely andprocess.argv.slice(1)is[].Arguments::parseparsesRunAsNodeCommandwithstop_after_positional_at: 1(src/runtime/cli/Arguments.rs:786, same asAutoCommand), sonode -e code a barrives asctx.positionals = ["a"],ctx.passthrough = ["b"]. The-ebranch ofRunCommand::exec_as_if_node(src/runtime/cli/run_command.rs:2903) built the syntheticcwd/[eval]entry and calledboot()directly, andboot()only handsctx.passthroughtovm.argv(run_command.rs:966).positionals[0]was never copied over.bun -edoes not have the bug becauseCommand::exec_auto_or_runroutes it throughRunCommand::exec_eval(run_command.rs:2859), which prependspositionalsontopassthroughbefore booting. Thebun -eform of this bug was bun --eval ignores first command line arg #12209 (closed); theexec_as_if_nodebranch was a copy ofexec_evalminus that step (the pre-port ZigexecAsIfNodehad the same shape, so this is not a regression).Fix
exec_as_if_node's-e/-pbranch now callsSelf::exec_eval(ctx)instead of re-implementing it; the fixing line is thereturn Self::exec_eval(ctx);hunk, the rest of the diff is the deleted copy.-e/-pnever take a script positional, so under node everything after the code isprocess.argv(output above);exec_evalis the one place that already encodes that for thebun -epath, and the two commands split argv identically (stop_after_positional_at: 1), so the node path needs the identical merge. The[eval]entry path andboot()call are unchanged,exec_evalbuilds the same ones.node -e codeis unaffected:exec_evalskips the merge whenpositionalsis empty.node --interactive -e codeis unaffected: it is routed toexec_node_replbefore this branch, as before.test/cli/run/as-node.test.tsgains atest.eachcovering-ewith one, two, and flag-like arguments,-ewith none,-p, and-pe. With the released bun (USE_SYSTEM_BUN=1) the five cases with arguments fail (each receives the list minus its first entry); with this build all 17 tests in the file pass.test/cli/run/run-eval.test.ts(37 pass, covers the sharedexec_evalpath forbun -e), thebun-as-nodetests intest/js/bun/repl/repl.test.ts, and theargv0=nodetests intest/cli/run/env.test.ts.Background
Command::which(src/runtime/cli/mod.rs) dispatches toRunAsNodeCommandwhenargv[0]isnode;RunCommand::exec_as_if_nodeis that command's body. It is howbun --bunmakes child processes that execnoderun under bun.positionalsvspassthrough: the CLI parser collects non-flag arguments intoctx.positionalsuntil it has seenstop_after_positional_atof them; everything after that point, flags included, is left unparsed inctx.passthrough. For the run-like commands the limit is 1 so that flags after the script name belong to the script.boot()installsctx.passthroughasvm.argv, which becomesprocess.argvafter the executable path.[eval]:-ecode has no file, so bun boots the VM with a synthetic<cwd>/[eval]entry path, and the module loader servesctx.runtime_options.eval.scriptfor that path.Related, not changed here
Other node-CLI shapes in the same
exec_as_if_node/ argv area, each already owned by its own open issue or PR, so this PR stays one hunk:node -reading the script from stdin: cli: read the script from stdin fornode -in node emulation #38566. Barenodewith piped stdin, and--portbeing remapped to--printin node mode, are also filed separately.--among the user arguments being stripped fromprocess.argv(bun file.js -- xandnode -e code a -- xboth lose the--, node keeps it): Bun strips '--' from process.argv #13984 / cli: preserve a bare--in process.argv when running a file #36684.process.execArgvnot stopping at-/--:process.execArgvincludes user options with the same names as bun's options #25387 / process.execArgv: stop at "-", "--", and the script after --inspect or --config #38577.node -v/--versionin the shim: cli: handle -v/--version in the node argv0 shim #36128.--checkand the wider node v26 CLI compatibility work: Node v26 CLI compatibility: make node:cli tests pass (+9 upstream tests) #32622.