cli: classify the subcommand past --cwd/--env-file values - #36644
cli: classify the subcommand past --cwd/--env-file values#36644robobun wants to merge 9 commits into
Conversation
Command::which() scans argv for the first non-dash token to pick the subcommand, but treated every dash-prefixed token as valueless. The value of --cwd / --env-file was therefore read as the subcommand name: bun --cwd ./dir test foo.test.ts -> AutoCommand, 'Script not found test' bun --env-file .env run app.ts -> AutoCommand, prints help which() now steps past the value of the required-value BASE_PARAMS_ flags and records the argv index of the keyword it lands on. Every handler that re-reads raw argv at a hard-coded offset (init, info, create, bunx via 'bun x', upgrade, pm, completions, reserved) uses that index instead, so 'bun --cwd d <cmd> ...' and 'bun <cmd> ...' share the same downstream parse. For the package-manager family, --cwd and --config were already declared in SHARED_PARAMS and are consumed correctly; --env-file is added as a hidden param so its path never leaks into cli.positionals as a spurious package name. -c/--config is intentionally not stepped over: it is declared OneOptional, so clap never consumes a separate token for it and the downstream parse would still see the path as a positional. #34983 changes that declaration.
|
Status: diff is ready. Build #87469 ran the full suite; 16 new tests in Reproduced on 1.4.0-canary: Review follow-ups addressed in 9ae4055..b12f741:
Spot-checked post-fix: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 6:43 PM PT - Aug 1st, 2026
❌ @robobun, your commit 6c3de46 has 3 failures in
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.64324d42-1b5c-475e-a280-ac95e785e2f3
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.ccd2a31a-457d-4964-8c75-2b6f539be99e
UnsupportedThe requested configuration is currently not supported. Please check the documentation for supported configurations.f701a30a-e5d0-4308-850f-8e3bbd120172🧪 To try this PR locally: bunx bun-pr 36644That installs a local version of the PR into your bun-36644 --bun |
WalkthroughChangesCLI subcommand indexing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cli/create_command.rs`:
- Around line 235-246: Update the create-command argument handling around the
positional scan to use the global parser’s detected subcommand index rather than
matching positional text; remove argv entries through that index so option
values equal to “c” or “create” are preserved correctly. Add regression coverage
for both --cwd and --env-file values using each accepted command spelling,
including the supported option spellings.
In `@src/runtime/cli/mod.rs`:
- Around line 1513-1514: Update the direct InitCommand::exec dispatch in the CLI
path to process the validated global --cwd option before invoking init,
preserving the requested working directory for commands such as bun --cwd target
init --yes. Add a regression test verifying that package.json is created under
the requested directory.
- Around line 1952-1955: Update the argument parsing around the info
subcommand’s found_package loop to consume --env-file together with its separate
value, rather than treating that value as a positional package or property path;
handle repeated --env-file options consistently. Prefer the existing parsed
positional arguments or declared option arity, and add coverage for separate and
repeated values plus invalid/adversarial cases.
In `@test/cli/bun.test.ts`:
- Around line 263-270: Strengthen the test around run(["--cwd", "sub", "add",
"--dry-run"]) to assert the expected no-package bun add usage diagnostic and
nonzero exitCode. Keep the existing resolver and “Script not found” assertions,
using the returned execution result so unrelated failures cannot satisfy the
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dc2e9ea5-a667-4fb2-b1a3-31a765d56c8e
📒 Files selected for processing (6)
src/install/PackageManager/CommandLineArguments.rssrc/runtime/cli/create_command.rssrc/runtime/cli/mod.rssrc/runtime/cli/package_manager_command.rssrc/runtime/cli/upgrade_command.rstest/cli/bun.test.ts
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…bunx/create - bun_info reads cli.positionals from the INFO_PARAMS clap parse instead of re-scanning raw argv, so value-taking flags after 'info' (--cwd, --registry, --env-file) no longer leak their value as the package name. - exec_init / exec_bunx / bun_create chdir into a leading --cwd before running, so 'bun --cwd target init' scaffolds in target/ rather than silently ignoring the flag (none of these route through a parser that honors --cwd on their own). - Tighten the 'bun --cwd sub add --dry-run' test to assert the zero-positional diagnostic; change the init test to a non-'.' --cwd and assert package.json lands in the subdirectory. - Trim doc comments flagged by comment-cop to one line each.
…_leading_cwd Changing exec_bunx's start_idx to subcommand_argv_index() stripped every leading dash token, not just --cwd/--env-file, so 'bun --bun x vite' lost --bun. Revert to passing argv[1..] and instead teach bunx's Options::parse and bun_create's remainder scan to step past the value of --cwd/--env-file themselves. apply_leading_cwd now also matches the attached --cwd=dir form.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/cli/mod.rs (1)
1571-1579: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle
--cwdfor directbunxinvocations.
bunxskipsarguments::parse, andSUBCOMMAND_ARGV_INDEXremains1.apply_leading_cwd()therefore ignoresbunx --cwd <dir> <package>andbunx --cwd=<dir> <package>. Handle these flags beforeBunxCommand::execand add direct-executable regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/mod.rs` around lines 1571 - 1579, Update the direct bunx invocation flow around apply_leading_cwd and BunxCommand::exec to detect and apply both --cwd <dir> and --cwd=<dir> before executing the package, since bunx bypasses arguments::parse. Preserve the existing argv slicing behavior for executable and non-executable paths, and add regression coverage for both flag forms when invoking the direct bunx executable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/bun.test.ts`:
- Around line 176-180: Replace the imperative loop around the
leading-global-flag cases in test/cli/bun.test.ts lines 176-180 with a
test.concurrent.each() matrix, preserving each case’s isolated temporary
directory and subprocess assertions. Also replace the --cwd loop in
test/cli/bun.test.ts lines 243-252 with a separate test.concurrent.each()
matrix, giving each input an independent test result; then run bun bd test
test/cli/bun.test.ts.
---
Outside diff comments:
In `@src/runtime/cli/mod.rs`:
- Around line 1571-1579: Update the direct bunx invocation flow around
apply_leading_cwd and BunxCommand::exec to detect and apply both --cwd <dir> and
--cwd=<dir> before executing the package, since bunx bypasses arguments::parse.
Preserve the existing argv slicing behavior for executable and non-executable
paths, and add regression coverage for both flag forms when invoking the direct
bunx executable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59dc8349-1410-425e-85dd-2e005616b0f1
📒 Files selected for processing (4)
src/runtime/cli/bunx_command.rssrc/runtime/cli/create_command.rssrc/runtime/cli/mod.rstest/cli/bun.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/cli/create_command.rs
There was a problem hiding this comment.
Commit 61a8324 addresses both of my earlier unresolved comments — exec_bunx is back to start_idx = 1 so bun --bun x <bin> keeps its leading flags (verified by the new test), bun_create re-scans &args[1..] with a --cwd/--env-file skip arm so --bun before create is preserved, and apply_leading_cwd() now handles the --cwd=dir attached form with the corrected i < end bound. Those two threads can be resolved.
Extended reasoning...
The two 🔴/🟡 inline comments I left on the previous revision (leading-flag stripping in exec_bunx/bun_create, and the --cwd=dir gap in apply_leading_cwd) are fixed by 61a8324; I re-traced bun --bun x vite, bun --cwd sub x cowsay, bun --bun create next-app, and bun --cwd=target init -y against the current diff and each now dispatches correctly with the flag applied. The new inline findings from this run cover what remains.
The skip(cmd_idx) slice is correct for TrustCommand's args[2..] positional handling, but it also stripped leading flags so 'bun --all list' / 'bun --all pm trust' lost --all. Flag probes now scan the full argv; positional slicing still starts at the subcommand keyword.
It fired before the full-argv --all scan, so 'bun --all pm trust' still errored. The '!trust_all && packages_to_trust.is_empty()' check just below covers the same case.
There was a problem hiding this comment.
All five earlier findings are addressed (61a8324 → b12f741) and this pass found nothing new. Deferring rather than approving because the change is cross-cutting: a new process-global SUBCOMMAND_ARGV_INDEX + apply_leading_cwd() threaded through ~10 subcommand handlers, re-attempting a previously-reverted approach (#34983). Each review round surfaced a fresh sibling-site regression, so a maintainer should sign off on the shape of the fix.
What was reviewed:
which()index tracking and everysubcommand_argv_index()consumer for off-by-one / stripped-flag regressionsapply_leading_cwd()for--cwd=dir, repeated--cwd, and last-wins semantics vs. clap- pm
ls/trustflag scans againstall_argsso pre-keyword--all/--trustedstill work; redundantargs.len()==2guard dropped - New tests for hermeticity (Windows shebang guard,
--dry-runon install/add)
Extended reasoning...
Overview
The PR fixes Command::which() misclassifying the value of a leading --cwd/--env-file as the subcommand keyword. It introduces a process-global SUBCOMMAND_ARGV_INDEX recorded during which(), a helper apply_leading_cwd(), and rewires every handler that previously read raw bun::argv() at a hardcoded offset (exec_init, exec_bunx, bun_create, bun_info, exec_install_completions, reserved_command, upgrade_command, package_manager_command, pm_trusted_command, CreateOptions::parse, and bunx's Options::parse). Install's SHARED_PARAMS gains a hidden --env-file entry. 8 files, ~150 lines of new tests.
Security risks
None identified. Pure argv-classification / dispatch logic; no auth, crypto, or untrusted-input parsing beyond argv itself.
Level of scrutiny
High. This is the CLI entry point — every bun <subcommand> invocation flows through which() and one of the touched handlers. The exact same approach was tried before (#34983) and reverted because handlers reading raw argv at fixed offsets broke. Over five review iterations on this PR, each fix surfaced another sibling-site regression (bunx --bun stripping, --cwd=dir attached form, first-vs-last --cwd wins, bun --all list flag stripping, pm trust len==2 guard). All were fixed, but the pattern indicates the invariants here are subtle and easy to miss.
Other factors
- All five of my prior inline findings are resolved by commits 61a8324, 506da7e, 7185a41, 4518bad, b12f741; the bug-hunting system found nothing on the current revision.
- The PR description honestly documents a remaining limitation (leading
--env-filefor the install family is accepted but not loaded), and the bunx--cwd-after-xcomment now truthfully says "not honored yet" rather than claiming it was applied. SUBCOMMAND_ARGV_INDEX+apply_leading_cwd()are new cross-cutting mechanisms; per the repo's own guidance, new cross-cutting abstractions benefit from maintainer agreement on the shape before landing.- One open CodeRabbit nit (convert two
forloops totest.concurrent.each) is stylistic and non-blocking.
There was a problem hiding this comment.
No new issues found — all five earlier findings (Windows shebang test, --cwd= attached form, last-wins --cwd, pre-keyword --all/--trusted for pm ls/trust, and the redundant args.len()==2 guard) are addressed in the latest commits. Deferring to a human for sign-off given this is a cross-cutting CLI-dispatch change that touches which() and seven downstream raw-argv consumers, was previously attempted and reverted (#34983), and carries two documented not-yet-honored gaps (--env-file for the install family; --cwd after the x keyword).
What was reviewed:
which()value-skip andSUBCOMMAND_ARGV_INDEXthreading through init/bunx/create/info/completions/upgrade/pm/reserved.apply_leading_cwdfor both--cwd vand--cwd=v, last-occurrence semantics, and thebun_createpositional scan skipping flag values.package_manager_command'sall_argsvs slicedargssplit so pre-keyword--all/--trustedstill resolve;TrustCommandfull-argv--allscan with the len==2 guard removed.- New tests: hermetic (
--dry-run, local.binstub), Windows-guarded, concurrent, and cover run/test/build/exec/init/install/add/bunx.
Extended reasoning...
Overview
This PR fixes Command::which() so that the value token of --cwd / --env-file is not misread as the subcommand keyword (e.g. bun --cwd ./dir test foo previously dispatched AutoCommand on ./dir). It introduces a process-global SUBCOMMAND_ARGV_INDEX recorded by which() and rewires every handler that previously sliced raw bun::argv() at a hard-coded offset — exec_init, exec_bunx, bun_create, bun_info, exec_install_completions, reserved_command, upgrade_command, package_manager_command, pm_trusted_command — plus adds a hidden --env-file param to install's SHARED_PARAMS and an apply_leading_cwd() helper for handlers that don't route through clap. ~150 lines of Rust across 7 files, plus ~150 lines of new tests in test/cli/bun.test.ts.
Security risks
None identified. The change is argv-classification only; no new I/O, network, or privilege surface. apply_leading_cwd() chdirs to a user-supplied path, but that's the existing --cwd semantics already applied by arguments::parse for other subcommands.
Level of scrutiny
High. which() runs on every bun invocation and every downstream handler that re-reads raw argv is a potential mis-slice. The prior attempt (#34983) was reverted for exactly that reason, and this PR's own review history surfaced five separate edge cases across five iterations (Windows shebang test, --cwd= attached form, first-vs-last --cwd, pre-keyword --all for pm, and the len==2 trust guard). All are now addressed, but the pattern — plus two documented remaining gaps (install-family --env-file not loaded; post-x --cwd silently stepped over) — makes this the kind of change a maintainer should sign off on rather than auto-approve.
Other factors
- All prior inline findings from this reviewer are resolved in commits 506da7e / 7185a41 / 4518bad / b12f741.
- The new test suite is reasonably broad (run/test/build/exec/init/install/add/bunx across
--cwd,--cwd=,--env-file, and mixed), uses--dry-run/ a local.binstub to stay hermetic, and is Windows-guarded where needed. - The
bun_inforewrite from a manual argv scan tocli.positionalsis a net simplification and correctly strips the leadinginfokeyword. CreateOptions::parsenow drains up to and including the firstc/createpositional rather than assuming index 0 — traced againstbun_create's positional collection and looks correct.
|
Heads up: #39379 now carries the reader-side part of this change, using the same |
1 participant
What
Command::which()scans argv for the first non-dash token to pick the subcommand. It treats every dash-prefixed token as valueless, so the value of a global flag is read as the subcommand name:In each case
which()lands on the flag's value (./dir,.env,sub), matches no keyword, and dispatchesAutoCommand.Why the obvious patch is wrong
Having
which()simply step past the value of--cwd/--env-file/--configwas tried in #34983 and reverted (ff4255a): once the subcommand is classified correctly, several handlers that re-read rawbun::argv()at a hard-coded offset see the wrong token. For examplebun --env-file .env installwould reachInstallCommand, whose parser does not declare--env-file, so.envandinstallbecome positionals and it behaves likebun add .env install.bun info,bun init,bun create,bun x,bun upgradeandbun pm trusthave the same shape (argv().iter().skip(2)/&argv[2..]/&args[1..]).Fix
which()now steps past the value of the required-valueBASE_PARAMS_flags (--cwd,--env-file) and records the argv index of the keyword it lands on inSUBCOMMAND_ARGV_INDEX.subcommand_argv_index()instead (exec_init,exec_bunx,bun_info,bun_create,exec_install_completions,reserved_command,upgrade_command,package_manager_command). For the defaultbun <cmd> ...shape the index is 1, so behavior is unchanged.CreateOptions::parsestrips positionals up to and including thec/createkeyword by name rather than assuming it is at[0].SHARED_PARAMSgains a hidden--env-file <STR>...entry so the path is consumed instead of leaking intocli.positionals(--cwdand--configwere already declared there).-c, --configis intentionally not stepped over: it is declaredValues::OneOptional, so the downstream clap parse never consumes a separate token for it either and the path would still surface as a positional. #34983 changes that declaration to required-value; once that lands,--configcan be added to the skip set and will behave identically to--cwd.Verification
New tests in
test/cli/bun.test.tscoverrun,test,build,exec,init,install,addwith--cwd/--env-filebefore the subcommand, plus the #34983 regression guard (bun --env-file my.env installmust not treat the path as a package name). All 14 fail withUSE_SYSTEM_BUN=1and pass with the fix.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/bun.test.ts
Known limitation
For the install family (
install/add/remove/update/pm/…), a leading--env-fileis now accepted and its path no longer leaks as a positional, but the env file is not yet loaded:PackageManager::init'senv.load(entries, &[], …)still passes an empty explicit-files list. This was already the case forbun install --env-file .envon main (where the flag was unrecognized and the path mis-routed tobun add), so no working invocation regresses. Threading the option through toenv.loadis a separate change.Fixes #10333