bunfig: keep CLI flags ahead of bunfig.toml when it is loaded after argv (bun run) - #38599
bunfig: keep CLI flags ahead of bunfig.toml when it is loaded after argv (bun run)#38599robobun wants to merge 1 commit into
Conversation
`bun <file>` and `bun test` parse bunfig.toml inside Arguments::parse, before argv is applied, so flags overwrite the config. `bun run <file>`, the node shim, `bun repl` and standalone executables load bunfig.toml later, and Bunfig::parse assigned every key unconditionally, so with a bunfig.toml present these flags lost to the config: --define, --loader, --console-depth, --jsx-runtime/--jsx-factory/--jsx-fragment/ --jsx-import-source, --no-install/-i/--install and --no-macros. A `preload` key replaced the --preload list outright. Record the flags argv supplied in ContextData::cli_overrides and have Bunfig::parse still validate, but not apply, the matching keys. Config preloads are now prepended to whatever argv already added, which is the order the early load produces.
|
Status
|
|
Warning Review limit reached
Next review available in: 9 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 (5)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes CLI-vs-bunfig precedence across eight user-facing settings and reworks how preloads are merged, a human look would still be worthwhile.
What was reviewed:
- Each
CliOverridesbool traced from whereArguments::parsesets it to whereBunfig::parseguards the assignment; validation still runs when the flag is set. load_preloadprepend semantics: confirmedread_global_config()is false for Run/Auto/Test so only one bunfig is parsed per invocation and no double-prepend occurs.- The reshaped
--no-install/-i/--installchain — accepted values and error path unchanged;RunAsNodeCommandguard on-ipreserved. - New test file:
tempDir,bunEnv,port: 0registry stub,test.concurrent, andfakeNodeRunusage all match harness conventions.
Extended reasoning...
Overview
The PR fixes a precedence inversion where bun run <target> (and the node shim, bun repl, standalone executables) load bunfig.toml after argv is applied, so bunfig overwrites CLI flags for --define, --loader, --console-depth, --jsx-*, --install/-i/--no-install, --no-macros, and --preload. It adds CliOverrides (nine bools) to ContextData, sets each bool in Arguments::parse where the flag is consumed, and guards the corresponding assignment in Bunfig::parse while still parsing/validating the key. load_preload now prepends config preloads to any existing ctx.preloads instead of replacing them. A new 28-case test file plus four un-FIXME'd cases in preload.test.ts cover the matrix.
Security risks
None. This is CLI/config precedence plumbing; no auth, crypto, network parsing, or untrusted-input handling is touched. The registry stub in the install test is a local Bun.serve({ port: 0 }) that only records paths.
Level of scrutiny
Medium-high. The mechanism is straightforward (a set of bool guards following the existing path_ignore_patterns_from_cli pattern), but it changes user-visible precedence for eight settings on every bun run invocation and alters preload-merge semantics. I checked that read_global_config() excludes RunCommand/AutoCommand/TestCommand, so load_preload is called at most once per process and the prepend cannot double up global+local bunfig preloads. I also confirmed the early-load order is unchanged (ctx.preloads is empty when bunfig loads first, so prepend == replace there).
Other factors
The PR description is unusually thorough — it names the mechanism, enumerates what is and isn't covered (with cross-references to #33198 and #36644), and reports USE_SYSTEM_BUN=1 failure counts. Test coverage is comprehensive: three invocations per setting (early-load with flag, late-load with flag, late-load without flag), plus install-auto in both directions and the node-shim path. The one intentional design choice a maintainer might weigh in on is that jsx.development still follows bunfig even when --jsx-runtime is given, since no flag sets it — the PR calls this out and it matches #36209's early-order behavior, but it's a judgment call.
|
Not a duplicate of any of the four, but they are related:
|
|
The |
Problem
bunfig.tomlin the cwd,bun run --define X=4 file.mjsruns with bunfig's[define]value;bun --define X=3 file.mjsandbun test --define ...use the flag. Reproduces on 1.4.0 and main.[loader]vs--loader,[console] depthvs--console-depth(the docs promise the flag wins),jsx/jsxFactory/jsxFragment/jsxImportSourcevs--jsx-*(any bunfig.toml at all, even one without jsx keys, undid--jsx-runtime classic),[install] autovs--no-install/-i/--install(bun run --no-installstill asked the registry), and a[macros]table vs--no-macros(macros ran anyway). Apreloadkey dropped the--preload/--require/--importlist entirely;test/config/bunfig/preload.test.tshad these cases commented out as a FIXME.bun <file>andbun testload bunfig.toml insideArguments::parse(src/runtime/cli/Arguments.rs,load_config_with_cmd_args) before the flags are applied, so flags overwrite it.bun run <target>is not inALWAYS_LOADS_CONFIG, so it, thenodeshim (both boot throughRunCommand::boot,src/runtime/cli/run_command.rs),RunCommand::exec_with_cfg,boot_standaloneandbun replload bunfig.toml after argv, andBunfig::parse(src/bunfig/bunfig.rs) assigned each key unconditionally.Fix
ContextDatagetscli_overrides: CliOverrides(src/options_types/context.rs), one bool per setting;Arguments::parsesets the bool where it applies the flag.Bunfig::parsestill parses and validates each of these keys (so diagnostics do not depend on which flags were passed) but skips the assignment when the flag was given.load_preloadprepends the config's preloads to whatever is already inctx.preloadsinstead of replacing it. In the early order that list is empty, so nothing changes; in the late order it yields config preloads followed by argv preloads, which is whatArguments::parsebuilds in the early order (and what the existingcli-merge.tsfixture asserts).--no-install/-i/--installchain inArguments::parseis reshaped to compute oneOption<GlobalCache>so the override is recorded in one place; the accepted values and the error are unchanged.nodeshim,bun repland standalone executables without touching each late-load site, and it only changes behavior when a flag and its bunfig key are both present (the buggy combination). It is the patterntest.pathIgnorePatternsalready uses (path_ignore_patterns_from_cli). Moving thebun runload earlier instead would change error handling for malformed configs, start applying bunfig to--filter/--parallelruns, and still leave the node shim on the late path.jsx, onlyruntimeis guarded by--jsx-runtime;developmentkeeps following the bunfigjsxkey as it does today, since no flag sets it (this is also the behavior cli: stop --jsx-* flags from switching the automatic JSX runtime to production #36209 gives the early order).[run]keys (Make CLI flags override their [run] counterparts in bunfig.toml #33198 adds the same guards for those),origin/serve.port(nothing at runtime reads them;vm.originis never set from options), andsmol/install.prefer, whichArguments::parseoverwrites unconditionally even in the early order, so they need a different fix (reported separately).test/config/bunfig/cli-flags-override-bunfig.test.ts(new: the precedence matrix spans eight settings and two load orders, which no existing file covers): for each key,bun <flag> file,bun run <flag> fileandbun run file;[install] autouses a local registry stub and checks in both directions. With the released binary exactly the tenbun run <flag>cases fail; all 28 pass with this change.test/config/bunfig/preload.test.ts: the threerunforms from the FIXME list are enabled, plusnode -r ./preload3.tsthroughfakeNodeRun; these four fail on the released binary and pass here. The remaining commented form (bun --preload x run file) is a different bug, the subcommand detection problem cli: classify the subcommand past --cwd/--env-file values #36644 is about, and the comment now says so.test/cli/console-depth.test.ts,test/config/bunfig/bunfig-errors.test.ts,test/cli/install/bun-run-bunfig.test.ts,test/cli/run/as-node.test.ts,test/cli/run/run-autoinstall.test.ts,test/bundler/transpiler/macro-test.test.ts,test/bundler/transpiler/jsx-tsconfig-react-jsx.test.ts,test/js/bun/resolve/resolve-error.test.ts;cargo clippyon the three crates is clean.Background
ContextData(ctx) is the process-wide bag of parsed CLI state:ctx.args(TransformOptions: define, loaders, jsx, ...),ctx.debug(auto-install mode, macros),ctx.runtime_options(console depth),ctx.preloads. BothArguments::parseandBunfig::parsewrite into it; whichever runs second wins a field.ALWAYS_LOADS_CONFIG(src/options_types/command_tag.rs) lists the subcommands whose bunfig.toml is loaded duringArguments::parse.bun runwas deliberately left out and loads it later inRunCommand(load localbunfig.tomlforbun runearlier (forrun.bunoption) #16664), which is why the two orders exist.GlobalCache):--no-installdisables resolving missing packages from the registry;fallback(-i) fetches only packages that are not found locally. A manifest request to the configured registry is therefore the observable difference the install test uses.Probe matrix on the released 1.4.0 binary