Conversation
…e.json scripts `bun run --filter`, `bun run --parallel` / `--sequential`, and `bun install` lifecycle scripts spawn each script as `bun exec <script>` when there is no POSIX shell to use (always on Windows). `bun exec` is a regular runtime command, so it also ran the default .env* auto-load in the script's directory on top of the envp the runner had already built. Scripts saw the package directory's .env files, --no-env-file and --env-file on the runner had no effect on them, and a script that sets its own NODE_ENV was pre-seeded with the wrong .env.<suffix> values (#9635). `sh -c` and the in-process shell used by plain `bun run <script>` only get envp. Build the argv in one shared helper, ScriptArgv, and pass --no-env-file to the hop there so envp is the script's whole environment at all three sites.
|
Warning Review limit reached
Next review available in: 15 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 |
|
Status: fix and tests pushed (68935d1), waiting on CI. Reproduced on Windows x64 with bun 1.4.0: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![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. Because it changes user-visible env-loading behavior across three spawn paths (--filter, --parallel/--sequential, and install lifecycle scripts) and the fail-before is Windows-only, a human look is still worthwhile.
What was reviewed:
ScriptArgvlifetime/pointer soundness —self_exe_path()is&'static,PhantomData<&'a ZStr>ties borrowed script/shell strings, and the 5-slot array with early null for the shell form matchesspawn_process's*const *const c_charcontract.bun execarg parsing —--no-env-fileis aBASE_PARAMS_flag consumed before positionals, sopositionals[1]remains the script;exec_command.rsreadsdisable_default_env_filesand passes it torun_env_loader.- POSIX path unchanged — both runners still fail
MissingShellwhen no shell is found; theSome(shell) if cfg!(unix)guard preserves the lifecycle runner's existingcmd.exeavoidance.
Extended reasoning...
Overview
The PR fixes a Windows-specific (and shell-less POSIX) leak where bun exec, used as the shell hop for bun run --filter, bun run --parallel/--sequential, and bun install lifecycle scripts, auto-loaded the package directory's .env* files on top of the envp the runner had already built. The fix adds --no-env-file to that hop's argv and consolidates the three copy-pasted argv-building blocks into a shared ScriptArgv helper in lifecycle_script_runner.rs (the lower crate all three already depend on for replace_package_manager_run). filter_run.rs and multi_run.rs now hold Option<&'static ZStr> for the shell instead of resolving bun's own path as a fake shell up front.
Security risks
None identified. The change reduces environment surface: lifecycle scripts on Windows no longer implicitly receive workspace-member .env values they weren't getting on POSIX. No new input parsing, no path handling changes.
Level of scrutiny
Medium-high. The mechanism is a one-flag argv change plus a clean refactor, and the PR description traces the mechanism precisely (verified exec_command.rs:53-54 reads disable_default_env_files, Arguments.rs:98 defines the flag, self_exe_path() is &'static, spawn_process takes *const *const c_char). But it's a user-visible behavioral change on Windows across three subsystems, aligns those paths with the existing bun run <script> contract (which env.test.ts already pins), and the fail-before can only be observed on Windows — Linux CI passes before and after by construction. That combination warrants a human confirming the intended semantics before merge.
Other factors
- Tests cover the full matrix: both runner entry points × {default,
--no-env-file,--env-file, #9635 NODE_ENV shape}, plus a lifecycle-script case with an inherited-var positive control. Alltest.concurrent, hermetic, and usetempDir. - The refactor removes ~40 lines of duplicated argv construction and the
Option<*const c_char>layout comment (now encoded once inScriptArgv's doc). ZStr::from_slice_with_nulreplacesfrom_raw_mutin the lifecycle runner — same semantics, cleaner call.- CI (#94684) was still building at the time of this review; Windows lanes are the ones that actually exercise the fix.
This branch has not been deployed
1 participant
Problem
bun exec "<script>"hop instead ofsh -c "<script>":bun run --filter(src/runtime/cli/filter_run.rs),bun run --parallel/--sequential(src/runtime/cli/multi_run.rs), andbun installlifecycle scripts (src/install/lifecycle_script_runner.rs). The first two take the hop on every Windows spawn; lifecycle scripts take it on Windows and on POSIX systems with no shell.bun execis an ordinary runtime command:src/runtime/cli/exec_command.rs:54runs the default.env*auto-load in its cwd (the package directory) on top of the envp it was handed, and only a--no-env-fileon its own argv turns that off..env/.env.<NODE_ENV>/.env.local; forbun installthat includes every workspace member's own.envreaching itspostinstall;bun run --no-env-file --filter ...still exposes them, and--env-file Xgets the default files layered in underneath;NODE_ENV=production bun startis pre-seeded with the.env.developmentvalues picked by the runner's NODE_ENV, so the bun it starts cannot read.env.production. This is NODE_ENV=production in package.json scripts no longer reads from .env.production #9635, which alternate approach to env fix #9689 fixed for the script runner; the hop reintroduces it.bun run <script>on Windows is not affected: it runs the script in-process with the runner's env map, which deliberately skips the default files (run_command.rs:674).test/cli/run/env.test.ts("does not pass variables from .env files into scripts") pins that for both shells on every platform; the three hop sites were the only paths that did not follow it.bun 1.4.0 on Windows x64 (fixtures from the tests; Linux prints an empty value in every case)
Fix
ScriptArgvinlifecycle_script_runner.rs(next toreplace_package_manager_run, the helper these same three callers already share;bun_installis the lower crate) builds the argv:<shell> -c <script>when the caller found a POSIX shell, otherwisebun exec --no-env-file <script>. All three spawn sites use it, so the rule lives in one place.--no-env-fileis right: at every site the envp passed to the spawn already is the environment the runner or installer decided on (process env plus any--env-filefiles forbun run; the installer's script env for lifecycle scripts). The hop exists only to be the shell, the roleshplays on POSIX, so it must add nothing;--no-env-fileis the existing flag that makesbun execuse envp alone.--env-filevalues still arrive because they are in envp; a nestedNODE_ENV=production bunreads its own files because nothing was pre-seeded.bun execalready accepts the flag (BASE_PARAMS_,src/runtime/cli/Arguments.rs:98) and the script stayspositionals[1].filter_run.rs/multi_run.rsnow holdOption<&ZStr>for the shell (Noneon Windows) instead of resolving bun's own path as a fake shell up front; the helper resolves it when building argv, as the lifecycle runner always did. POSIX behavior is unchanged: both still fail withMissingShellwhen no shell is found.--no-env-filefor the lifecycle hop; whichever lands second has a small conflict in that one hunk.test/cli/run/filter-workspace.test.ts: newdescribe.eachover--filterandrun --parallel, four cases each: defaults not loaded,--no-env-file,--env-file(requested file and inherited variables arrive, defaults do not), and the NODE_ENV=production in package.json scripts no longer reads from .env.production #9635 shape (script setsNODE_ENV=production, must see.env.production). Windows x64: 8 fail with bun 1.4.0 (ENV_FILE_NAME=.env.developmentin every case), 8 pass with this branch.test/cli/install/bun-install-lifecycle-scripts.test.ts: a workspace member'spostinstallmust not see the member's.envwhile an inherited variable still arrives. Windows x64: fails with bun 1.4.0 (PKG_DOTENV=leaked), passes with this branch.filter-workspace.test.ts,multi-run.test.ts, andbun-install-lifecycle-scripts.test.tson Linux and Windows x64;run-quote.test.tsandno-orphans.test.tson Windows x64.Background
.env*loading:dot_env::Loader::load(src/dotenv/env_loader.rs:649) reads.env,.env.<suffix>(suffix from NODE_ENV) and.env.localfrom a directory unless asked to skip the defaults;--no-env-fileasks for that skip; files named with--env-fileare loaded either way. Values from these files never override variables already present in the process environment.RunCommand::configure_env_for_runloads the process env and the--env-filefiles and skips the defaults, so a.env*value reaches a script only through a bun the script itself starts, which reads the files for the script's own NODE_ENV. Because process env wins over files, anything pre-seeded into the script's environment is something that nested bun can no longer correct; that is what NODE_ENV=production in package.json scripts no longer reads from .env.production #9635 was.shon Windows, so these runners use bun itself as the shell by spawningbun exec <script>, which interprets the script with Bun's shell. Unlikesh,bun execruns the runtime's env setup in its cwd before interpreting anything.find_shellreturnscmd.exeon Windows, which is why the helper keys the shell form oncfg!(unix)as the lifecycle runner did before.filter_run/multi_runrebuild it per script from the runner's env map with PATH adjusted per package; the lifecycle runner builds it once per script chain from the installer's env.