cli: evaluate bun -p test / bun --print test / bun -pe help instead of dispatching the code as a subcommand - #38578
cli: evaluate bun -p test / bun --print test / bun -pe help instead of dispatching the code as a subcommand#38578robobun wants to merge 3 commits into
bun -p test / bun --print test / bun -pe help instead of dispatching the code as a subcommand#38578Conversation
Command::which() steps over the flags in front of the subcommand name. It only stopped at tokens starting with -e, so for -p, -pe, --print and --eval the next argv token was matched against the subcommand names: `bun -p test` ran the test runner, `bun -pe help` printed the help, and with an attached value (`bun -p=1 test`, `bun --print=1 test`) the script's first argument was dispatched the same way. Any eval flag spelling now classifies the invocation as AutoCommand, where arguments::parse already handles these flags; the -e entry in the keyword matcher was only reachable through the old carve-out.
|
Warning Review limit reached
Next review available in: 1 minute 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)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe CLI now recognizes eval and print flag variants during command detection. New tests verify that these flags take precedence over subcommand-like code and arguments. ChangesEval flag precedence
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: fix pushed, waiting on CI. Reproduced on bun 1.4.0-canary (b7a0431) and a debug build of main (eabb96d) in an empty directory: Test: test/cli/run/run-eval.test.ts, Scope: this fixes the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The is_eval_flag predicate and the loop restructuring look correct, and the test matrix is thorough. Because this changes argv→subcommand classification in Command::which() (every invocation goes through it), intentionally flips the meaning of bun -p <subcommand>, and overlaps with two other open PRs touching the same loop (#38568, #36644), a human sign-off on the behavior change and merge ordering would still be worthwhile.
What was reviewed:
is_eval_flagpattern coverage vs.AUTO_PARAMS/NODE_SHORT_ALIASES:[b'-', b'e' | b'p', ..]matches-e/-p/attached-value/-peand nothing else (second byte excludes--, lone-); long forms are exact-match plus=-prefix only.- Confirmed the removed
RootCommandMatcher::case(b"-e")arm is unreachable now that the loop returns before the matcher for any-token that is an eval flag. - Checked the empty-eval and
looks_like_run_entrypointfast paths instart()— both run beforewhich()and are unaffected. - Tests: 12 concurrent spawns in an empty
tempDir, pipes drained viaPromise.all, assertions on exact stderr/stdout/exitCode.
Extended reasoning...
Overview
The PR fixes Command::which() in src/runtime/cli/mod.rs so that every spelling of the eval/print flags (-e, -p, -pe, --eval, --print, and their attached-value forms) stops the leading-flag skip loop and returns Tag::AutoCommand, instead of only the -e prefix doing so. Previously -p, --print and --eval were stepped over as if they were boolean flags, so the code (or the first script argument) was matched against the subcommand table — bun -p test ran the test runner, bun -p a foo ran bun add foo. A new is_eval_flag helper encapsulates the predicate, the now-dead RootCommandMatcher::case(b"-e") arm is removed, and 12 new test cases in test/cli/run/run-eval.test.ts cover both the "code is a subcommand name" and "script argument is a subcommand name" shapes.
Security risks
None introduced. If anything this closes a footgun: bun -p a foo previously reached bun add foo and wrote to package.json / hit the registry, which is the more dangerous direction. The new behavior only ever routes to AutoCommand, which is what arguments::parse was already prepared to handle for these tokens.
Level of scrutiny
High. which() is called on every bun invocation and its output selects the entire subcommand path. The change is small and mechanically sound (I traced the matches! slice pattern against the actual AUTO_PARAMS definitions of -e/--eval and -p/--print and the NODE_SHORT_ALIASES entry for -pe), but it deliberately changes user-visible behavior: bun -p install now evaluates install instead of running bun install --production. The PR description argues — convincingly — that the old behavior was an accident of the skip loop and that bun install -p is unaffected, but a maintainer should confirm that trade-off is acceptable.
Other factors
- The PR notes two other open PRs (#38568 for lone
-, #36644 for--cwd/--env-filevalue stepping) that edit the same loop; merge order matters and a human should coordinate. - I verified the removed
case(b"-e")branch is truly dead: any token reaching the matcher no longer starts with-(the loop either returns or advances past every-token), so a-eliteral can never reachRootCommandMatcher::match. - The new tests follow harness conventions (
tempDir,await using,Promise.allon stdout/stderr/exited,test.concurrent, exact-value assertions) and each child runs in an empty cwd so a misdispatchedinstall/addwould have nothing to act on. - No prior review comments to address; only a CodeRabbit rate-limit notice.
|
On the two points the review asks a maintainer to confirm:
|
The doc comment claimed every spelling clap accepts is recognized, but short clusters such as `-bp <code>` are not. Link the issue from the dispatch check and the tests, and use its literal `bun --print x` as one of the cases.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// `-e` and `-p` take a value, so every `-e…` / `-p…` token (node's `-pe` | ||
| /// included) is an eval flag; clusters such as `-bp` are not recognized. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The rest of argv is the script and its arguments: `bun -p test` evaluates | ||
| // `test` (https://github.com/oven-sh/bun/issues/23631). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it deliberately changes what bun -p <subcommand> does (previously bun -p install reached the install command with --production; now it print-evals install), and the author has already flagged that for maintainer confirmation, a human sign-off on the behaviour change is worth having.
What was reviewed:
is_eval_flagprefix match[b'-', b'e' | b'p', ..]— correct because-e/-ptake a value inAUTO_PARAMS, so any-e…/-p…token is that flag with an attached value.- Removed
RootCommandMatcher::case(b"-e")— confirmed unreachable now the loop returns before the matcher for any--prefixed token. - Test cases cover detached, attached,
=-attached,-pe, and flag-before-eval spellings; the empty-eval fast path instart()is untouched.
Extended reasoning...
Overview
The PR fixes issue #23631: bun -p test, bun --print test, bun --eval test, bun -pe help and the attached-value forms (bun -p=1 test, bun -p1 test) were being dispatched as subcommands instead of evaluated. The fix is a new is_eval_flag() predicate in src/runtime/cli/mod.rs that stops Command::which()'s leading-flag skip loop and returns Tag::AutoCommand when any eval-flag spelling is seen. A now-unreachable -e case in the keyword matcher is removed. Twelve test cases are added to test/cli/run/run-eval.test.ts.
Security risks
None. This is argv classification only; no untrusted-input parsing, no filesystem/network side effects introduced.
Level of scrutiny
Medium. The code change is small and the logic is sound (the prefix match on -e/-p is exactly how arguments::parse reads these tokens, so there is no ambiguity). However, it is a user-visible CLI behaviour change: bun -p install, bun -p i, bun -p outdated and similar previously reached the package-manager commands (with -p read as --production) and now print-eval the identifier instead. The author argues convincingly that this was accidental and undocumented, and it matches Node's behaviour, but the repo's review guidance asks for maintainer agreement on default-behaviour changes.
Other factors
- The author explicitly flagged the behaviour change for maintainer confirmation in a follow-up comment.
- Two other open PRs (#38568, #36644) touch the same loop; the author has described the trivial merge resolution but a human may want to pick a landing order.
- The comment-cop bot left two inline notes on the new comments; both are two-line comments (one a doc comment stating the cluster limitation, one an issue link), which look like false positives to me, but the author may want to address or dismiss them.
- Tests follow the harness conventions (
tempDir,test.concurrent, drain pipes concurrently, assert stderr/stdout before exit code) and 9 of 12 fail on the unfixed binary per the author's verification.
|
Heads up: #38568 changes the same while first_arg_name.len() > 1 && first_arg_name[0] == b'-' {
if is_eval_flag(first_arg_name) {
return Tag::AutoCommand;
}
...
}The test blocks are in different parts of |
1 participant
Addresses the
bun --print xhalf of #23631. Its other half (bun run --eval/bun run --printprint thebun runusage) is a different code path and is not changed here.Problem
bun -p test,bun --print testandbun --eval teststart the test runner (bun test v1.4.0 ... No tests found!) instead of evaluatingtest;bun --print xprints the bunx usage (bun --print may interpret what follows as a sub-command w/ no way to escape &&bun run --evalis broken #23631);bun -pe helpprints bun's help.bun -e testis the only spelling that evaluates (ReferenceError, as node does for all of them).bun -p=1 test,bun -p1 test,bun --print=1 testandbun --eval=1 testall run the test runner. Any subcommand name in that position is dispatched:bun -p a fooisbun add foo(it writes package.json and hits the registry),bun -p iisbun install.Command::which()(src/runtime/cli/mod.rs, thewhileloop before theRootCommandMatcherlookup) steps over every leading-token to find the subcommand name, and its only stop condition is a token starting with-e.-p,-pe,--printand--evalare stepped over like boolean flags, so the token after them (the code, or the first script argument when the code is attached) is what gets matched against the subcommand names.Fix
which()now stops at any eval flag spelling and returnsAutoCommand:-e...,-p...(covers-p <code>,-p<code>,-p=<code>and node's-pe),--eval,--eval=...,--print,--print=...(newis_eval_flag).arguments::parsereads as an eval forAutoCommand, and once one of them is present, everything after it is the script or its argv by definition, so there is no subcommand to look for.-eand-ptake a value, so a token starting with either is always that flag with an attached value (clap reads-peas-pviaNODE_SHORT_ALIASES,-p1+1as-pwith1+1); the prefix rule is exactly how the token will be parsed. Matches node, wherenode -p test/node -pe help/node --print=1 testnever do anything but evaluate.-eentry in the keyword matcher is removed: it was only reachable through the old carve-out (the loop now returns before the matcher for any flag token).bun -p installand friends used to reach the install command, which then read-pas--production. That was an accident of the skip loop;bun -p <x>is documented as print-eval andbun install -p/--productionare unchanged. Short clusters such as-bp <code>are not recognised bywhich(); they were not before either (bun -bp testerrors in the test runner today) and node rejects them outright.eval flags take precedence over subcommand names, including the issue's literalbun --print x): 9 of the 12 new cases fail on the unfixed binary (the-e,-e=and-p <non-keyword> testcases are controls that already passed) and all pass with the fix; the rest of the file, test/cli/run/as-node.test.ts and test/cli/run/crash-report-command-char.test.ts still pass.Background
Command::which()classifies argv into a commandTagbefore any argument parsing happens. Because runtime flags may precede the subcommand (bun --bun test, andBUN_OPTIONSis spliced in right after argv0), it skips leading-tokens and matches the first remaining token against the subcommand names (test,help,x,add,i, ...). Anything that does not match isAutoCommand.AutoCommandis thebun <file>/bun -e <code>path:arguments::parseparses argv withAUTO_PARAMS(where-e/--evaland-p/--printtake a value and-peis aliased to-p), andexec_auto_or_runruns the eval when one was given. That side already handled every spelling; only the classifier in front of it did not.-when picking the subcommand (bun - add xranbun add x) #38568 stops it at a lone-(stdin), cli: classify the subcommand past --cwd/--env-file values #36644 steps over the values of--cwd/--env-file. Neither covers the eval flags, which must stop the search rather than be stepped over (bun -p 1 testhas to evaluate1withtestas a script argument).