cli: make -c/--config require a path; stop treating the config path as a package to install - #34983
cli: make -c/--config require a path; stop treating the config path as a package to install#34983robobun wants to merge 2 commits into
Conversation
WalkthroughThe change separates valueless and one-optional-value short-flag parsing, makes ChangesConfig flag parsing
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:46 AM PT - Aug 16th, 2026
✅ @robobun, your commit 551d4dd3c38b9d15d99fbca739cfba8096934eff passed in 🧪 To try this PR locally: bunx bun-pr 34983That installs a local version of the PR into your bun-34983 --bun |
|
Status: rebased onto current main (squashed; the only conflict was install's param list being split into Reproduced on 1.4.0 before rebasing: of
|
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related: #31077 fixes the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/config/bunfig/config-flag.test.ts`:
- Around line 32-36: Update the install regression test around the temp
project’s cfg.toml and dependency resolution to use a local HTTP registry/server
bound to port 0, retrieving its assigned port dynamically. Ensure both the
configured-registry path and any fallback/default-registry path resolve against
local deterministic handlers, so the test never contacts the public npm registry
and does not use hard-coded ports.
🪄 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: 7bc7a841-89c7-4a3d-b0ba-4befcd0f9e84
📒 Files selected for processing (5)
src/clap/streaming.rssrc/install/PackageManager/CommandLineArguments.rssrc/runtime/cli/Arguments.rstest/cli/install/bun-run.test.tstest/config/bunfig/config-flag.test.ts
There was a problem hiding this comment.
Beyond the two inline nits, the parser fix and tests look correct — but this changes user-facing CLI surface (bare -c/--config now hard-errors instead of forcing a bunfig.toml load), so a maintainer should sign off on the contract change.
Also checked: -c, --config is the only in-tree <...>? param (no other OneOptional flags affected by the chainging reflow), and no existing tests pass bare -c/--config to Bun. Traced load_config in src/bunfig/arguments.rs: bare -c previously stored Some(b"") and hit the user_config_path_.is_some() branch to force-load bunfig.toml for commands not in ALWAYS_LOADS_CONFIG (e.g. RunCommand) — that path is now unreachable.
Extended reasoning...
Overview
The PR touches five files: a control-flow fix in src/clap/streaming.rs::chainging so Values::OneOptional short flags honor =value and attached-value forms; two one-character param-spec changes (<PATH>? → <PATH>) in src/runtime/cli/Arguments.rs and src/install/PackageManager/CommandLineArguments.rs; a small cleanup in test/cli/install/bun-run.test.ts (unconditional bunfig.toml write now that -c=path is honored, dropping a stale TODO); and a new hermetic integration test test/config/bunfig/config-flag.test.ts plus a short_one_optional unit test in the clap crate.
Security risks
The bug being fixed has a mild supply-chain angle — bun install --config cfg.toml currently routes cfg.toml to bun add against the public registry with the intended private-registry config never loaded. The fix closes that. The change itself introduces no new attack surface: it tightens argv parsing and adds no new I/O, no crypto, no auth paths.
Level of scrutiny
This is a user-facing CLI contract change, not a mechanical bugfix. Two behaviors change:
- Bare
-c/--configmoves from accepted-with-default to a hard "requires a value" error. I tracedargs.option(b"--config")→load_configinsrc/bunfig/arguments.rs: with the oldOneOptionalspec, a bare flag storedSome(b""), which trippeduser_config_path_.is_some()at line 171 and force-loadedbunfig.tomleven for commands outsideALWAYS_LOADS_CONFIG(notablyRunCommand). That capability is now removed. The PR description addresses install ("already a no-op") but not the runtime side. bun run -c script.jspreviously (via the very bug being fixed) leftscript.jsas the positional; now it is consumed as the config path. This aligns with #6300's expectation but is still an observable change.
Per the repo's own guidance, changing user-facing API surface warrants maintainer review. There is also an open overlapping PR (#31077) to reconcile.
Other factors
- The
chaingingfix is minimal and clearly correct against the surroundingValues::Onehandling; the newshort_one_optionalunit test covers-c,-c=v,-cv, chained-ac*forms, and the space-form-stays-positional case. - I verified via grep that
<...>?appears in no other.rsparam spec —--configwas the only in-treeOneOptionalflag, so the required-value change has no collateral effect on other flags. - Grepped the test tree: no existing test passes bare
-cor--configto Bun, so no suite breakage from the new error. - The integration test is hermetic (per-case
Bun.serve({port:0})404 registry, both auto-loaded and explicit configs pointing at it with distinct prefixes), drains stdout/stderr/exited concurrently, and asserts the strongest invariant (registry hit path, not just exit code). CodeRabbit's earlier hermeticity concern was addressed in fb728a2. - The two inline findings are style-level nits (missing
describe.concurrent, 4-line header comment) and would not by themselves block.
There was a problem hiding this comment.
Both earlier nits are addressed (describe.concurrent on the install block, header comment trimmed to invariant + issue URLs). No correctness issues found. Deferring only because making bare -c/--config a hard error is a user-facing CLI behavior change — worth a maintainer confirming that's the intended shape vs. the narrower #31077 approach that keeps OneOptional and only fixes the parser.
What was reviewed:
chaingingchange:OneOptionalnow falls through to the=/attached-value path and only yieldsNonewhen nothing follows; verified against the newshort_one_optionalunit test cases.- Confirmed
-c, --configwas the only in-treeOneOptionalparam with a short flag, so the parser change has no other consumers today. - Checked internal
b"-c"/c"-c"argv sites (filter_run, multi_run, run_command, ChangedFilesFilter) — all aresh -c/git -c, unaffected. - Install test hermeticity: per-test
Bun.serve({port:0})with/default/vs/fromcfg/prefixes andcache = false, so a regression stays local.
Extended reasoning...
Overview
Five files: a small logic change in src/clap/streaming.rs (chainging) so OneOptional short flags honor =/attached values instead of unconditionally returning None; two one-character param-table edits removing the ? from -c, --config <PATH>? in the runtime and install CLI tables; a new hermetic test file covering all five spellings for both bun install and bun <script>; and a small cleanup in bun-run.test.ts to always write bunfig.toml now that -c=path is actually honored.
Security risks
None introduced by the diff. The PR closes a mild footgun where bun install --config cfg.toml would silently bun add cfg.toml against the public registry with the intended config never loaded — the direction of the change is strictly safer.
Level of scrutiny
Moderate. The parser edit is small and mechanically verifiable against the new unit test, and the param-table edits are trivial. What raises the bar is that this is a user-facing CLI contract change: bare -c / --config now errors with "requires a value but none was supplied" where it previously defaulted (no-op for install; auto-load bunfig.toml for runtime via the user_config_path_.is_some() branch in load_config). PR #31077 takes the narrower route of fixing only the -c=path parser bug while keeping the flag optional-value. A maintainer should pick which shape lands.
Other factors
All prior feedback is addressed and threads resolved: CodeRabbit's hermeticity concern (fb728a2), and my two nits on describe.concurrent and the header comment (e7d673d). I grep-verified the PR's claim that -c, --config is the only in-tree <...>? param with a short name — the remaining OneOptional params (--inspect*, --metafile*, --sourcemap, --bail, --changed, --parallel) are long-only and go through normal(), not chainging(). The internal -c argv occurrences in filter_run.rs, multi_run.rs, run_command.rs, and ChangedFilesFilter.rs are shell/git -c and unaffected. Tests are hermetic, concurrent, and assert the strongest invariant (registry path prefix hit list, not just exit code).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/config/bunfig/config-flag.test.ts (1)
20-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCover bare
-cand runtime missing-value errors.The added missing-value test covers only
bun install --config. Addbun install -c,bun --config, andbun -ccases so both aliases and both parser entry points retain the required-value contract.As per coding guidelines, tests must cover the complete variant matrix, including sibling entry points and flag states, and every behavioral change needs an automated regression test in the same change.
🤖 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 `@test/config/bunfig/config-flag.test.ts` around lines 20 - 27, Add missing-value regression cases for bare -c and --config across both parser entry points: bun install -c, bun install --config, bun -c, and bun --config. Extend the existing spellings/missing-value test coverage in config-flag.test.ts and assert each invocation produces the required-value error.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.
Outside diff comments:
In `@test/config/bunfig/config-flag.test.ts`:
- Around line 20-27: Add missing-value regression cases for bare -c and --config
across both parser entry points: bun install -c, bun install --config, bun -c,
and bun --config. Extend the existing spellings/missing-value test coverage in
config-flag.test.ts and assert each invocation produces the required-value
error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 281f0f80-38fd-4abc-b66b-01cd512fc96d
📒 Files selected for processing (1)
test/config/bunfig/config-flag.test.ts
There was a problem hiding this comment.
No issues found — the parser fix and tests look correct, and all prior review nits are addressed. Deferring to a human on the one design decision here: making -c, --config require a value turns bare -c from a silent no-op into a hard error, and #31077 takes the narrower parser-only approach. A maintainer should pick which to land.
What was reviewed:
chainging()restructure:OneOptionalnow falls through to the=/attached-value path, only returningNonewhen nothing follows;Values::Nonebehavior and state transitions unchanged.- Confirmed no other in-tree
OneOptionalshort flags and no existing tests pass bare-c/--configto bun. bun-run.test.tstweak is correct —-c=pathnow actually binds, sobunfig.tomlmust exist in thewithLogLevel=falsebranch too.- New tests are hermetic (per-case
Bun.serve({port:0}),tempDir,cache=false), concurrent, and the bare-flag matrix now renders distinct titles.
Extended reasoning...
Overview
Five files: a ~10-line restructure in src/clap/streaming.rs::chainging() so Values::OneOptional short flags honor =/attached values instead of unconditionally returning value: None; two one-character param-table edits (<PATH>? → <PATH>) in the runtime and install CLI tables; a small adaptation in test/cli/install/bun-run.test.ts (always create bunfig.toml now that -c=path actually binds, and drop the now-answered TODO); and a new 91-line integration test file covering all five spellings across bun install and bun <script> plus the bare-flag error across both param tables. A new short_one_optional cargo unit test locks the parser semantics.
Security risks
None introduced. The change removes a security-adjacent footgun: bun install --config cfg.toml previously routed cfg.toml to bun add, GET-ing the public npm registry for a package named after the config file while ignoring the intended private-registry config.
Level of scrutiny
Medium. The Rust parser change is small and mechanically verifiable against the new unit test (walked -c, -c=v, -cv, -ac, -ac=v, -acv, -c p through the new control flow — all match). I grepped for other OneOptional users and for existing bare--c/--config invocations in test/ and docs/; none rely on the removed optional-value behavior, and run_command.rs/install already auto-load bunfig.toml unconditionally, so bare -c was a no-op. However, changing a documented flag's arity is a user-facing CLI surface change (REVIEW.md → "API design"), and #31077 is an open alternative that fixes only the parser without the arity change — that's a maintainer call.
Other factors
All four prior review threads (CodeRabbit hermeticity; my .concurrent, header-comment, bare-flag-matrix, and %p-title nits) are addressed at HEAD 64128ea. The bug hunter found nothing this run. The tests follow harness conventions (tempDir, bunEnv, port: 0, concurrent describe blocks, drain-then-assert-exit-code). The one thing I can't sign off on is whether Bun wants bun -c to error going forward vs. keeping it a silent alias for auto-load — that's product surface, not correctness.
64128ea to
6b2ef3b
Compare
e70fd1f to
ff4255a
Compare
There was a problem hiding this comment.
The descope in ff4255a addresses my earlier note — the commit message, title, and description now match what ships, and I traced both linked-issue repros (bun -c bunfig.toml ., bun -c=cfg.toml index.ts) through the unchanged which() → AutoCommand path and they now work with the required-value param tables. Deferring only because the remaining change is a user-facing CLI tightening (bare -c/--config now errors instead of silently no-op'ing) and a shared bun_clap parser edit — worth a maintainer glance.
What was reviewed:
chainging()restructure:OneOptionalnow falls through to the=v/attached-value path and only yieldsNonewhen nothing follows; walked all six unit-test inputs plus the-c ppositional case.- Confirmed
-c, --configis the only in-tree shortOneOptionalparam, so the parser change has no other live consumers. bun-run.test.tscleanup: writing an emptybunfig.tomlunconditionally is required now that-c=pathactually opens the file; the removed TODO is answered by this fix.- Checked
load_config_with_cmd_args→ bare-cwas previously a true no-op (args.optionreturnedNone→ auto-load), so the new error isn't dropping a real use case.
Extended reasoning...
Overview
After the ff4255a revert the PR is down to: an 8-line StreamingClap::chainging fix (split Values::None from Values::OneOptional so short optional-value flags honour =v/attached values) with an 84-line unit test; two one-character param-table edits (<PATH>? → <PATH> in the runtime and install --config declarations); a 3-line bun-run.test.ts cleanup; and a new 91-line integration test file covering all five spellings for bun install and bun <script>, plus the four bare-flag error cases. src/runtime/cli/mod.rs is no longer touched.
Follow-up on my previous comment
My 05:09 🔴 asked for either restoring the classifier fix or descoping honestly. The author chose descope: force-pushed an honest commit message on the revert, retitled the PR, and rewrote the description to explicitly note which() is untouched and why. I re-verified the two Fixes # claims still hold without the classifier change: both repros land in Tag::AutoCommand (config path is not a subcommand keyword), whose arguments::parse now consumes the value via the required-<PATH> declaration, leaving the intended entry point as the first positional. So keeping both Fixes # links is correct.
Security risks
None identified. The change removes a case where a config-file path could be sent to the registry as a package name (bun install --config cfg.toml → bun add cfg.toml), which was the mild info-leak/supply-chain concern the PR body describes. The new integration test uses a local port-0 404 server so nothing reaches the public network even on regression.
Level of scrutiny
Medium. The streaming.rs change is in a shared parser used by every CLI entry point, but it's a strict correctness fix for a path (OneOptional short flags) with exactly one in-tree consumer, and the new unit test pins the full -c/-c=v/-cv/-ac/-ac=v/-acv/-c p matrix. The param-table edits are a user-facing behaviour change — bare -c/--config now hard-errors — which is why I'm deferring rather than approving: it's small and I believe correct (I checked load_config_with_cmd_args and bare -c was already equivalent to omitting the flag), but tightening CLI validation is an API-surface call a maintainer should sign off on.
Other factors
The PR went through significant churn (classifier added in 6b2ef3b, reverted in ff4255a after two 🔴 findings). The final diff is small and self-contained; test coverage is thorough (unit + integration, both entry points, all spellings, error paths). The bun-run.test.ts edit is a necessary consequence of the fix — previously -c=path was silently dropped so the file didn't need to exist in the !withLogLevel branch.
…opped -c, --config was declared as an optional-value flag in both the runtime and install param tables, and StreamingClap::chainging returned value: None for every OneOptional short flag. Only --config=path worked: -c=path and -cpath dropped the value, and --config path / -c path left the path as a positional, which bun install routed to bun add. Declare -c, --config as a required-value flag in both tables so the space-separated forms bind the path, and let OneOptional short flags take the same =value / attached-value path as One, returning None only when nothing follows.
Adds the `bun run <flag> app.ts` and `bun <flag> run app.ts` argv shapes from #31077 to the config-flag matrix, on top of the `bun <flag> app.ts` shape that was already covered. Co-authored-by: jinhyuk9714 <jinhyuk9714@gmail.com>
ff4255a to
551d4dd
Compare
There was a problem hiding this comment.
I re-reviewed after the rebase and found no further issues — all earlier threads are addressed, mod.rs is untouched, and the description now matches what ships. Deferring only because making bare -c/--config an error (and having -c <token> consume the next token as the path) is a user-facing CLI change a maintainer should sign off on.
What was reviewed:
chainging():OneOptionalnow falls through to the=/attached-value path; only returnsNonewhen nothing follows. Verified the state-transition hoist above it is unchanged and no other in-tree shortOneOptionalflag exists (only a clap unit test atsrc/clap/lib.rs:803).- Both param-table edits are one-character (
?dropped); the bare-flag error is covered for both aliases across both tables. bun-run.test.ts: now always writesbunfig.tomlbecause-c=<path>actually binds after the fix — previously the value was dropped and the test relied on cwd auto-load.
Extended reasoning...
Overview
Five files: a targeted fix in StreamingClap::chainging so short OneOptional flags honour an attached/= value, two one-character param-table edits (<PATH>? → <PATH>) in the runtime and install CLIs, a new integration test file covering all five spellings across bun install and bun [run], and a small adjustment to an existing bun-run.test.ts case that previously depended on the bug being fixed. The earlier Command::which() classifier change (which I flagged as introducing new failure modes for --env-file/info/init/create) was reverted and is no longer in the diff; the description and Fixes # list were updated to match.
Security risks
None introduced. If anything this closes a mild footgun where bun install --config cfg.toml would silently bun add cfg.toml against the public registry with the intended (private-registry) config never loaded. The new tests are hermetic — install cases spin up a local Bun.serve({port:0}) 404 registry with distinct /default/ vs /fromcfg/ prefixes, so a parsing regression is caught locally rather than reaching npmjs.org.
Level of scrutiny
Medium. The parser change is small and unit-tested (short_one_optional covers -c, -c=v, -cv, -ac, -ac=v, -acv, and the space-form-stays-positional case), and I confirmed -c, --config was the only in-tree short OneOptional flag, so the chainging restructure has no other live consumers to regress. What keeps this from an auto-approve is the API-surface decision: bare -c/--config now errors instead of defaulting, and bun -c script.ts now consumes script.ts as the config path where it was previously a positional. That is the intended fix, and the prior behaviour was largely redundant with auto-load, but it is a user-visible break that falls under the repo's API-design review lane.
Other factors
Every inline thread from prior runs is resolved. The branch has been rebased/squashed to two commits on current main; the cumulative diff matches what I last examined after the which() revert. The bug-hunting pass found nothing this run. The one remaining known gap — bun --config cfg.toml <subcommand> classification — is explicitly called out in the description as tracked separately, which is the right scoping given the argv-offset consumers (info, init, create) it would have to coordinate with.
Problem
--config=pathloads the named file (reproduced on 1.4.0):-c=pathand-cpathsilently drop the path and load the defaultbunfig.tomlinstead.--config pathand-c pathleavepathas a positional:bun -c bunfig.toml .fails withCannot run "bunfig.toml"(File not found when runningbun --config bunfig.toml#6300), andbun install --config cfg.tomlbecomesbun add cfg.toml, a registry lookup for a package named after the config file while the config itself (registry, scopes, tokens) is never loaded.StreamingClap::chainging(src/clap/streaming.rs:245) groupsValues::OneOptionalwithValues::Noneand returnsvalue: Nonewithout looking at the rest of the token, so-c=path/-cpathlose their value.-c, --configis declared<PATH>?(optional value) in src/runtime/cli/Arguments.rs:103 and src/install/PackageManager/CommandLineArguments.rs:54. An optional-value flag never consumes the next argv token, so the space forms cannot bind.Fix
-c, --configtakes a required<PATH>in both param tables.--config=p,--config p,-c=p,-c pand-cpall bind the path; a bare-c/--confignow errors withrequires a value but none was supplied.chaingingonly short-circuits forValues::None;OneOptionalgoes through the same=value/ attached-value handling asOneand yieldsNoneonly when the token ends at the flag. The otherOneOptionalflags in tree (--inspect,--bail,--sourcemap,--catalog, ...) are long-only and never reachchainging, so after the table change this path has no in-tree user; the long-flag path is untouched.-chad no meaning beyond the default that applies without it (forinstallit was already a no-op, since install always auto-loadsbunfig.toml). The help text (--config=<val>), the docs (--configtyped as string) and the shell completions (-c|--configcompletes a.tomlfile; the zsh entry's example is-c bunfig.toml) all already describe a required path; the parser was the odd one out.bun --config cfg.toml run app.ts(space form before a subcommand keyword) is still not routed torun, becauseCommand::which()picks the keyword before any flag value is parsed. It fails as before; cli: classify the subcommand past --cwd/--env-file values #36644 handles that classifier separately and is waiting on this change to add--configto its skip set.test/config/bunfig/config-flag.test.ts: the five spellings forbun install(against a local port 0 registry, asserting which config's registry is hit and that nothing is routed tobun add), forbun <flag> app.ts,bun run <flag> app.tsandbun <flag> run app.ts, plus the bare-flag error for both tables. 18 of 22 cases fail on 1.4.0 (the four--config=pathcases already passed), 22 pass with this change.bun run rust:miri -p bun_clap: newshort_one_optionalunit test covers-c,-c=v,-cv,-ac,-ac=v,-acvand-c p.test/cli/install/bun-run.test.tsnow writesbunfig.tomlunconditionally: its-c=<path>used to be silently ignored and now opens the file. Passes along with the other tests that pass-c=/--config=(bun-run-bunfig, bun-workspaces, bun-install-registry, preload, bun.test.ts).-c=pathcase inchainging; itsrun-keyword argv shapes are carried over into the test matrix here.Background
bun_clap(src/clap) is Bun's own argv parser. Each flag declarestakes_valueasNone(boolean flag),One(required value:--x=v,--x v,-x=v,-xv,-x v),OneOptional(value only when attached to the same token) orMany. The?suffix in aparam!spec selectsOneOptional.chaingingis the short-flag path: it walks a token such as-acvone character at a time so several boolean flags can share a token, and the first value-taking flag claims the rest of the token.Command::which()(src/runtime/cli/mod.rs) decides the subcommand before any parsing by skipping dash-prefixed tokens and matching the first other token against the keyword list. That is why a flag value given as a separate token beforerun/installis misread; it is independent of how the flag is declared.Fixes #6300
Fixes #21431
Earlier iterations
An earlier revision also taught
which()to step past--config/--cwd/--env-filevalues. It was reverted within this PR: once the keyword is classified correctly, handlers that re-read raw argv at a fixed offset (info,init,create) and install's param table (no--env-file) misbehave, e.g.bun --env-file .env installbecamebun add install. That work now lives in #36644.The branch was squashed and rebased after main split install's
SHARED_PARAMSintoSHARED_HEAD_PARAMS/PRODUCTION_PARAMS/SHARED_TAIL_PARAMS; the one-line declaration change moved accordingly.