cli: declare Node's value-taking flags so their value is not parsed as the entrypoint - #34100
cli: declare Node's value-taking flags so their value is not parsed as the entrypoint#34100robobun wants to merge 3 commits into
Conversation
…s the entrypoint `bun --experimental-loader ./hooks.mjs app.mjs` silently ran hooks.mjs as the program and never executed app.mjs: the unknown-flag skip in the arg parser moved past --experimental-loader but treated its value as the first positional. The comment above the trace-event declarations already documents this hazard. Declare --experimental-loader and the rest of Node's value-taking CLI options (with no help text, so they stay hidden from --help) in RUNTIME_PARAMS_ so the value is consumed. This covers `bun`, `bun run`, and bun-as-node.
|
Warning Review limit reached
Next review available in: 8 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 |
|
Updated 8:06 PM PT - Jul 13th, 2026
❌ @robobun, your commit 3ddbe60 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34100That installs a local version of the PR into your bun-34100 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Neither suggested issue is addressed here (verified against this branch's build):
This PR only declares Node's value-taking flags so their value is consumed instead of becoming the entrypoint. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical extension of the existing "declare Node value-taking flags" list, following the exact pattern documented at lines 305-310, with a full test matrix. The one inline nit (--test-name-pattern omitted) is an incomplete fix, not a regression, and can be a follow-up.
Extended reasoning...
Overview
Adds ~55 parse_param! declarations to RUNTIME_PARAMS_ in src/runtime/cli/Arguments.rs for Node.js value-taking CLI options that Bun does not implement. Each entry has no help text so it stays hidden from --help. This is the same pattern already used for --trace-event-categories, --trace-event-file-pattern, and --stack-trace-limit immediately above the new block, and the existing comment at lines 305-310 explicitly documents the rationale. Also adds ~120 lines of tests in test/cli/run/as-node.test.ts covering bun, bun run, and the --help hiding.
Security risks
None introduced. The flags are consumed and ignored — no new code paths read their values. A few flags are security-relevant in Node (--allow-fs-read, --disable-proto), but Bun already silently accepts the --flag=value form of these; this PR only makes the space-separated form consistent. That is the pre-existing design, not a new decision.
Level of scrutiny
Low-to-medium. The change is purely additive to a static const table and follows an established, commented pattern. I verified:
- No collisions with existing declarations: the new
--test-*flags (--test-timeout,--test-reporter,--test-shard, etc.) are distinct fromTEST_ONLY_PARAMSentries (--timeout,--reporter,--shard), soTEST_PARAMS = TEST_ONLY_PARAMS + RUNTIME_PARAMS_has no duplicates. --inspect-port/--debug-portdon't collide with Bun's existing--inspect <STR>?;--watch-path/--watch-kill-signaldon't collide with--watch;--env-file-if-existsis distinct from--env-file;--experimental-loaderis distinct from-l/--loader.RUNTIME_PARAMS_feedsAUTO_PARAMS,RUN_PARAMS,TEST_PARAMS, andBASE_RUNTIME_TRANSPILER_PARAMS, so all relevant subcommands are covered.
Other factors
- Test coverage is thorough: 55 flags × 2 (
bunandbun run) = 110 concurrent spawns asserting the correct entrypoint runs, correctprocess.argv, and correctprocess.execArgv; plus a dedicated--experimental-loadercase and a--helpvisibility check. - The one inline finding (
--test-name-patternmissing) is a nit: that flag was omitted because it already exists inTEST_ONLY_PARAMS, but that table doesn't feedAUTO_PARAMS/RUN_PARAMS. Adding it naively would create a duplicate inTEST_PARAMS, so it needs a slightly different fix — reasonable as a follow-up. It's not a regression; behavior for that one flag is unchanged from before this PR. - The
stderr: ""exact-match assertion in the primary--experimental-loadertest was flagged by finders and ruled out by verifiers; the parameterized suite uses.not.toContain(...)instead, which is the load-bearing coverage.
…nsumed --test-name-pattern lives in TEST_ONLY_PARAMS (for `bun test`), which does not feed AUTO_PARAMS or RUN_PARAMS, so `bun --test-name-pattern foo app.mjs` still parsed foo as the entrypoint. Declare it in AUTO_OR_RUN_PARAMS (hidden from --help) rather than RUNTIME_PARAMS_ to avoid a duplicate in TEST_PARAMS. Also drop the exact stderr match in the primary test in favor of the same .not.toContain check the parameterized suite uses.
There was a problem hiding this comment.
LGTM — the --test-name-pattern gap from the last pass is fixed correctly (placed in AUTO_OR_RUN_PARAMS to avoid the TEST_PARAMS collision). The remaining --security-revert note below is a minor completeness gap, not a regression, and can be a follow-up.
Extended reasoning...
Overview
Adds ~55 Node.js value-taking CLI flags as hidden parse_param! entries in RUNTIME_PARAMS_ (plus --test-name-pattern in AUTO_OR_RUN_PARAMS) so the streaming arg parser consumes their value instead of treating it as the entrypoint. Purely additive table entries following the existing documented pattern above --trace-events-enabled. 112 new parameterized tests cover every flag under both bun and bun run, plus a --help visibility check.
Security risks
None. The flags are no-ops in Bun — they're only declared so the parser knows to swallow the following token. No new codepaths execute based on the flag values.
Level of scrutiny
CLI arg parsing is load-bearing, but this change is mechanical: const-table additions with no logic changes. I verified there are no long-name collisions with existing Bun flags in the tables these feed — TEST_ONLY_PARAMS uses --timeout/--reporter/--shard/--coverage-* (not the --test--prefixed Node forms), and existing --inspect*/--watch/--env-file don't overlap the new --inspect-port/--watch-path/--env-file-if-exists. The --test-name-pattern follow-up is correctly placed in AUTO_OR_RUN_PARAMS, which feeds AUTO_PARAMS/RUN_PARAMS but not TEST_PARAMS, so it doesn't duplicate the -t/--grep entry.
Other factors
The prior review round's finding was addressed and verified. The one remaining nit (--security-revert + a couple of newer Node-main aliases) is an incomplete-coverage note, not a correctness issue — those flags behave exactly as they did before this PR, and their typical values don't resolve to files so the failure mode is loud. Test coverage is thorough and the evidence block shows fails-on-main / passes-on-PR for both debug+ASAN and release.
… is consumed Node's --security-revert (and the --security-reverts alias) binds to PerProcessOptions::security_reverts (a string vector) and requires a value. Verified against Node v26.3.0. --experimental-default-config-file is boolean there and --experimental-package-map does not exist yet, so neither is added.
There was a problem hiding this comment.
LGTM — both prior review comments (--test-name-pattern and --security-revert/--security-reverts) are addressed, and no new issues found.
Checked that none of the ~58 new entries collide with existing Bun flags in any table they flow into (AUTO_PARAMS, RUN_PARAMS, TEST_PARAMS); --test-name-pattern in AUTO_OR_RUN_PARAMS correctly stays out of TEST_PARAMS. Verified the empty-description-hides-from-help convention is the documented pattern at line 305. The --experimental-default-config-file / --experimental-package-map omissions are justified against Node v26.3.0.
Extended reasoning...
Overview
Adds ~58 hidden parse_param! declarations to RUNTIME_PARAMS_ (plus one in AUTO_OR_RUN_PARAMS) in src/runtime/cli/Arguments.rs for Node.js CLI options that take a value, so the streaming arg parser consumes the value token instead of treating it as the entrypoint. Adds a parameterized test matrix in test/cli/run/as-node.test.ts covering every declared flag under both bun and bun run, plus a --help visibility check.
Security risks
None. These flags are declared but not acted upon — the only behavior change is that their value argument is consumed rather than becoming the first positional. No new code paths process the flag values; --allow-fs-read/--allow-fs-write/--openssl-config/--tls-* etc. remain no-ops in Bun. Consuming a previously-misparsed argument cannot expand attack surface.
Level of scrutiny
Low-medium. This is a mechanical extension of an already-documented pattern (the existing comment at Arguments.rs:305-310 for --trace-event-* describes exactly this hazard and remedy). The change is additive to a static declaration table with no logic. I verified no collisions: grepped every new flag name against the file — --inspect-port/--inspect-publish-uid are distinct from Bun's --inspect/--inspect-wait/--inspect-brk; --watch-path/--watch-kill-signal distinct from --watch; --env-file-if-exists distinct from --env-file; the new --test-* entries (--test-timeout, --test-reporter, --test-shard, etc.) are distinct from TEST_ONLY_PARAMS' --timeout/--reporter/--shard. The one genuine collision (--test-name-pattern) was correctly routed to AUTO_OR_RUN_PARAMS, which feeds AUTO_PARAMS/RUN_PARAMS but not TEST_PARAMS.
Other factors
Two prior review rounds from me were addressed in follow-up commits (130f04c, 3ddbe60). The PR description's evidence block shows the test file failing 111/113 cases on main and passing on the branch under both ASAN-debug and release. The author verified against Node v26.3.0 that the two flags I flagged as version-dependent (--experimental-default-config-file, --experimental-package-map) are correctly excluded. Tests use the harness conventions (tempDir, bunEnv, bunExe, concurrent pipe drain, exit-code asserted last) and test.concurrent.each for the 58×2 matrix.
|
CI status for 3ddbe60 (build #72611): the diff's test file (
The Ready for review/merge. |
1 participant
Problem
bun --experimental-loader ./hooks.mjs app.mjssilently runshooks.mjsas the program and never executesapp.mjs. The command exits 0, so CI wrappers and tooling that still emit the legacy loader flag appear to succeed while running the wrong file.Node runs
app.mjshere (and registers the loader).Cause
The streaming arg parser in
src/clap/streaming.rsskips unrecognized long flags and resumes at the next argument. When the unrecognized flag takes a value (--experimental-loader ./hooks.mjs), the value is the next argument and gets parsed as the first positional, i.e. the entrypoint. The comment above the existing--trace-event-*declarations inArguments.rsalready describes this exact hazard: value-taking Node flags must be declared so the parser consumes the value.--experimental-loaderwas missing from that set, along with ~50 other value-taking options from Node'ssrc/node_options.cc.Fix
Declare the missing value-taking Node.js options in
RUNTIME_PARAMS_with no help text (so they remain hidden from--help). BecauseRUNTIME_PARAMS_feedsAUTO_PARAMS,RUN_PARAMS, and the run-as-node table, this coversbun,bun run, and bun-as-node. The flags are otherwise no-ops in Bun; the only behavior change is that their value argument is consumed instead of becoming the entrypoint, and they show up inprocess.execArgv(matching Node).The
--flag=valueform already worked because an unrecognized--flag=valueis skipped as a single token; only the space-separated form was affected.Verification
New parameterized tests in
test/cli/run/as-node.test.tsspawnbun <flag> ./value.mjs app.mjsfor each declared flag (and again withbun run) and assertapp.mjsruns withprocess.argv.slice(2) == ['scriptarg']andprocess.execArgv == [flag, './value.mjs']. Before the fix 111/112 of these cases ranvalue.mjsas the program instead.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file