cli: do not skip a lone - when picking the subcommand (bun - add x ran bun add x) - #38568
cli: do not skip a lone - when picking the subcommand (bun - add x ran bun add x)#38568robobun wants to merge 4 commits into
- when picking the subcommand (bun - add x ran bun add x)#38568Conversation
… stdin Command::which() steps over leading flags to find the subcommand name and treated "-" as one of them, so the argument after it was dispatched as the command: `bun - help` printed help, `bun - add x` ran `bun add x`, and `bun - run x` dropped "run" from process.argv. A lone "-" is the stdin entry point, so the search ends there and the invocation is AutoCommand, as it already is for `bun run - <args>`.
|
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)
Walkthrough
ChangesStdin CLI execution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. The diff is done; CI is red only on tests this change does not touch. Reproduced on bun 1.4.0-canary.1 and a debug build of main (eabb96d), linux-x64: The new cases in CI: the final build for a7f3b42 passed 169/179 jobs (all Linux and macOS lanes); the 7 Windows jobs were canceled because the Windows agents failed to provision, and the same code (build 96193, only the comment wording differed) passed every Windows lane. The 3 failed jobs are memory-leak tests timing out on the debian 13 x64-asan lane ( Related, independent PRs touching the same loop: #38578 (eval flags; conflicts on the same |
There was a problem hiding this comment.
LGTM — a one-condition fix that stops the flag-skip loop at a lone -, with focused tests.
What was reviewed
- Enumerated the loop condition against all input shapes (empty,
-,-e…,-x/--x, non-flag): only lone-changes classification, everything else is byte-for-byte equivalent to before. - Confirmed the
start()fast path (looks_like_run_entrypoint) already returns false for-, sowhich()is reached and falls through toAutoCommand→exec_stdin. - Tests: 5 concurrent cases in an empty tempDir asserting exact
process.argv, covering keyword-after-dash, therunargv-drop case, and-reached after a real flag; pipes drained concurrently,usingcleanup.
Extended reasoning...
Overview
One-line change to the leading-flag skip loop in Command::which() (src/runtime/cli/mod.rs): the condition now requires len() > 1 instead of !is_empty(), so a lone - no longer qualifies as a flag to step over. A 4-line comment explains why. The test file adds a 5-case describe block spawning bun [flags] - <subcommand-name> and asserting the stdin script sees the full argv.
Correctness of the condition rewrite
I walked every input class through both the old and new predicate:
- empty: both stop (old via
!is_empty(), new vialen() > 1) →AutoCommand. - lone
-: old stepped over (the bug); new stops →RootCommandMatchermatches nothing →AutoCommand. This is the sole behavior change. -e/-e…: both stop (old via the negated conjunct, new via[1] != b'e'being false).- any other
-x/--…: both step over. - non-
-token: both stop.
The rewrite is a strict simplification of the boolean plus the one intended narrowing. The reserved_command::exec loop in the same file already uses this exact len() > 1 && [0] == '-' shape, so the idiom is established. Once which() returns AutoCommand, the existing stop_after_positional_at = 1 argv parse and exec_stdin path handle passthrough, which the tests verify end-to-end via process.argv.
Security risks
None. This is argv classification for CLI dispatch; no untrusted-input parsing, no filesystem/network side effects introduced. The fix actually removes an unintended side-effecting path (bun - a b writing node_modules).
Level of scrutiny
Low-medium. The change is a single boolean condition on the CLI cold path, mechanically verifiable by case enumeration. It sits in production dispatch code but is not memory-safety-sensitive, not a hot path, and has no cross-platform branching.
Other factors
- Tests follow the harness conventions cleanly:
tempDir+using,bunEnv/bunExe(), concurrent pipe drain viaPromise.all,test.concurrent, exact-value assertions on stdout/stderr before exitCode. The empty cwd ensures a mis-dispatched subcommand can't accidentally succeed. - Author reports all 5 new tests fail under
USE_SYSTEM_BUN=1and pass underbun bd test, plus the rest ofrun-eval.test.tsandtest/cli/bun.test.tsstill pass. - No prior human or bot review comments to address; no CODEOWNERS on these paths.
There was a problem hiding this comment.
LGTM — the loop-condition rewrite only reclassifies a lone -, and the new tests pin the argv passthrough.
What was reviewed:
- Truth-tabled the old vs new
whilecondition: identical for empty,-e*,-x,--*; only-(len==1) changes from skipped to stop, then falls through the keyword table toAutoCommand. - Checked that stopping at
-cannot match anyRootCommandMatcher::case, and thatreserved_command::execalready uses the samelen() > 1 && [0] == '-'shape for "is a flag". - Tests: hermetic (empty
tempDir), drain both pipes concurrently, assert exactprocess.argv, and cover the flag-before--case; verified they'd fail on the old dispatch (help/add/test/run each mis-route).
Extended reasoning...
Overview
Two files: a one-line condition change in the leading-flag skip loop of Command::which() (src/runtime/cli/mod.rs) plus a one-line comment, and a new 5-case describe block in test/cli/run/run-eval.test.ts. The old condition !is_empty() && [0]=='-' && !(len>1 && [1]=='e') skipped a lone -; the new len()>1 && [0]=='-' && [1]!='e' stops on it. For every token of length ≥ 2 the two conditions are algebraically identical, and both stop on the empty token, so the only reclassified input is exactly -. After the loop stops, - matches no keyword and which() returns AutoCommand, which already routes a - positional to exec_stdin — so the downstream path is unchanged, only the dispatch is fixed.
Security risks
None. This is argv classification for the CLI dispatcher; no untrusted-data parsing, no auth/crypto, no filesystem writes. If anything the fix removes a mild footgun (bun - a b previously ran bun add and mutated the cwd's package.json).
Level of scrutiny
Low-to-moderate. It's a tight, mechanical predicate change on a hot startup path, but the effect surface is narrow and the truth table is exhaustively checkable by hand. The PR description enumerates every input class and I verified them independently. The comment-cop bot feedback (paragraph-long comment) was addressed in a7f3b42 and the inline threads are resolved.
Other factors
Tests follow REVIEW.md conventions: tempDir + using, test.concurrent for independent subprocesses, Promise.all on stdout/stderr/exited, exact-string assertions on JSON.stringify(process.argv.slice(1)), stderr asserted empty, exit code last. The five cases cover distinct pre-fix failure modes (help printed, add invoked, test runner, argv drop via run, and - reached after a real flag). Author reports all 5 fail with USE_SYSTEM_BUN=1 and pass with the debug build; the rest of the file and test/cli/bun.test.ts still pass. The PR also documents the textual conflict with #38578 and its resolution, and correctly scopes out -- and process.execArgv as separate concerns.
|
Updated 6:47 PM PT - Aug 14th, 2026
❌ @robobun, your commit a7f3b42 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 38568That installs a local version of the PR into your bun-38568 --bun |
Problem
bun - <args>is the short form ofbun run -(run the script piped on stdin), but when an argument after the-spells a subcommand, that subcommand runs instead and stdin is never read:bun - helpprints bun's help (exit 0),bun - teststarts the test runner,bun - upgraderuns the upgrader.bun - a bandbun --bun - irunbun add: they fetch packages from the registry and writepackage.json,bun.lockandnode_modulesinto the cwd.bun - run xdoes read stdin, butprocess.argvcomes out as["-", "x"]instead of["-", "run", "x"].bun run - helpandnode - helpboth run the stdin script with argv["-", "help"]; only thebun -spelling misbehaves, and only when the next word happens to be a keyword, sobun - fooworks and hides it.Command::which()(src/runtime/cli/mod.rs:940) steps over every token that starts with-before matching the subcommand name. A lone-passes that test, so the token after it is the one matched.Fix
-x,--xy). A lone-stops it, matches no keyword, andwhich()returnsAutoCommand, the same way it already stops at-e.reserved_command::execin the same file already uses thislen() > 1 && [0] == '-'test for "is a flag".-is an operand of the run path, not a flag.AutoCommandparses it as the first positional (stop_after_positional_at = 1),RunCommand::exec_with_cfgsends a-target toexec_stdin, andexec_stdinalready prepends-to the passthrough args to mirrornode -. So oncewhich()stops at the-, every following argument reaches the script unchanged, which is exactly whatbun run - <args>andnode - <args>do today.-e..., and every other-...token take the same branch as before, andbun -alone orbun - <non-keyword>already ended up asAutoCommand.--before a keyword (bun -- test,bun -- a b) still selects the keyword. It is the same loop, but--is not the stdin marker and changing it changes whatbun -- <subcommand>means (npm also resolvesnpm -- helpto the command), so that is a separate decision. Happy to add it here if wanted.process.execArgvof a stdin script is["-"](node:[]), which also breaksfork()from such a script. That is pre-existing forbun run -too, lives insrc/runtime/node/node_process.rs, and is fixed by process.execArgv: stop at the "-" stdin marker and "--" like a script name #38577.bun -p test/bun --print test/bun -pe helpinstead of dispatching the code as a subcommand #38578 stops the loop at the other eval flags (bun -p testcurrently starts the test runner) and rewrites the samewhileheader; it conflicts textually with this PR. Whichever lands second keeps this PR'slen() > 1guard, i.e.while first_arg_name.len() > 1 && first_arg_name[0] == b'-' { if is_eval_flag(..) { return Tag::AutoCommand; } .. }.--cwd/--env-fileinside the loop body; it does not touch the-case and composes with this change (bun --cwd dir - helpneeds both).test/cli/run/run-eval.test.ts, newbun - <args>block:help,add --help,test,run x, and--silent - upgrade --help(the-reached after a real flag). Each runs in an empty temp dir and asserts the stdin script printed["-", ...args]. All 5 fail on bun 1.4.0-canary.1 (USE_SYSTEM_BUN=1: help text / add usage / test runner output /["-","x"]) and pass withbun bd test.run-eval.test.ts(42 tests) andtest/cli/bun.test.tspass with the debug build.cargo fmt --all -- --checkis clean.Background
Command::which()chooses the subcommand (Tag) straight from argv before any option parsing: it skips leading global flags such as--bunor--silent, then compares the next token against the keyword table (add/a,install/i,x,test,help,run, ...). Any other token meansAutoCommand, the path that runs a file, a package.json script, or stdin, and that path parses argv with the real CLI parser afterwards.-eis the existing precedent for a token that ends the search: its value is code, so the loop stops on it and the invocation becomesAutoCommandregardless of what the code looks like.process.execArgvis rebuilt separately from argv bynode_process.rs, which is why the dispatch fix here and the execArgv fix in process.execArgv: stop at the "-" stdin marker and "--" like a script name #38577 are different files.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/run-eval.test.ts