test: strip inherited npm_config_user_agent from bunEnv - #31928
Conversation
An outer bun run (e.g. launching the suite via bun bd test) injects npm_config_user_agent into the test runner's environment. Spawned buns keep a pre-existing value (put_default in run_command.rs), so the bunx user agent test compared the outer bun's user agent against the inner bun's Bun.version and failed whenever the two versions differ.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-harness-only env strip following the existing pattern.
Extended reasoning...
Overview
This PR adds a single line (plus a comment) to test/harness.ts: npm_config_user_agent: undefined in the bunEnv object. The existing post-spread cleanup loop already deletes undefined keys, so the net effect is that spawned test subprocesses no longer inherit npm_config_user_agent from the outer bun run / bun bd invocation. This fixes a local-only test failure in bunx.test.ts where the inherited outer-bun user agent didn't match the debug Bun.version.
Security risks
None. This is test-harness configuration only — no runtime/production code is touched, and the change only removes an env var from spawned test processes.
Level of scrutiny
Low. It's a one-line addition that follows the exact same idiom already used in this object for JSC_useJIT and FORCE_COLOR. The PR description gives a clear root-cause analysis and verification. A grep confirms no other test relies on npm_config_user_agent being present in bunEnv; the only consumer is the bunx.test.ts assertion this fixes.
Other factors
No outstanding reviewer comments, no prior reviews, and no bugs surfaced by the bug-hunt. The change is mechanical and self-contained.
|
Worried about impact? Review this PR in Change Stack to explore blast radius before you approve or request changes. 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 (1)
WalkthroughThe test harness environment initialization is updated to clear the ChangesTest Harness Environment Setup
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
CI analysis for build 60999 (final): 14 test lanes failed (Linux glibc/musl x64/aarch64, Windows x64/aarch64), and every one of them fails only on The test this PR fixes, |
|
Re-checked against current main (f426a8e) while going through open test-infra PRs, and this one is still needed.
The branch merges cleanly. Leaving it open. |
|
Hit the same failure on current main (bdb7382) with One data point, in case it is worth widening the strip while this is open:
Two ways to cover it here, if wanted: add |
### Problem
- `RunCommand::configure_env_for_run` (and its `_without_linker` twin)
take two positional bools, `log_errors` and `store_root_fd`, and three
of the four callers pass bare literals for both. At
`configure_env_for_run(ctx, &mut slot, None, true, true)` in
`bunx_command.rs` nothing says which flag is which, and swapping them
compiles.
- This is the one `bare_bool_args` finding mordant has baselined for
`src/runtime/cli/run_command.rs`.
### Fix
- Replace the two bools with a `ConfigureEnvOptions { log_errors,
store_root_fd }` struct (`run_command.rs`, next to `ExecCfg`, which
already gives `exec` its flags this way). Every caller now spells out
both fields: `filter_run.rs`, `multi_run.rs`, `bunx_command.rs`,
`pack_command.rs`, and the `exec` path inside `run_command.rs`.
- An options struct rather than two enums because two of the callers
compute `log_errors` (from `ExecCfg` and from the pack log level) and a
struct field takes the value as is.
- The private `configure_env_for_run_impl` keeps its single
`with_linker: bool`; the lint only considers functions with two or more
bool parameters, and its only callers are the two wrappers directly
above it.
- The values each caller passes are unchanged, so this is a pure rename
of the arguments; no behavior change.
- The `"bare_bool_args:src/runtime/cli/run_command.rs"` line is removed
from `mordant-baseline.toml`.
- Verified: `cargo fmt --all --check`; `bun bd test` on
`test/cli/run/run_command.test.ts`, `if-present.test.ts` (the
`log_errors: false` path), `multi-run.test.ts`,
`filter-workspace.test.ts`, `test/cli/install/bun-run.test.ts`,
`bunx.test.ts` (the `store_root_fd: true` caller), `bun-pack.test.ts`.
The only failure was `bunx.test.ts` "should set npm_config_user_agent to
bun", which fails on main too when run through `bun bd` (the outer `bun
run` exports its own `npm_config_user_agent` into the test env; #31928
addresses that) and passes with that variable unset.
### Background
- `configure_env_for_run` builds the process-lifetime `Transpiler` that
`bun run`, `bunx`, `bun run --filter`/`--parallel` and `bun pm pack` use
to resolve the working directory's `package.json` and seed the `npm_*`
environment variables for the script they are about to spawn.
- `log_errors`: when the working directory cannot be read, print the
resolver log and an error line; with it off the failure is only returned
(used by `bun <script> --if-present` and by `bun pm pack --silent`).
- `store_root_fd`: keep the working directory's fd open on the `DirInfo`
the function returns. Only `bunx` wants this, because it later reads the
resolved package's `bin` through that fd; the resolver's `store_fd` is
switched back off right after the root directory is read, so no other
directory keeps an fd.
- mordant is the dylint pack run by `bun run rust:mordant` (pinned in
`Cargo.toml`); `mordant-baseline.toml` lists pre-existing findings per
lint and file, so fixing one means deleting its line.
1 participant
Fixes #31925
Repro
On a clean tree:
Cause
The issue's diagnosis (user agent built from an unsuffixed version) is not what happens: the user agent and
Bun.versionare built from the sameGlobal::package_json_versionconstant, suffix included. The real mechanism is environment inheritance:bun bd test ...runs thebdscript under the system bun, whose run_command setsnpm_config_user_agent=bun/1.4.0 npm/? node/v24.3.0 linux x64into the script environment.bunEnvintest/harness.tsspreadsprocess.envwithout stripping it.bun x print-pmsees a pre-existingnpm_config_user_agentand keeps it (put_defaultinsrc/runtime/cli/run_command.rsandsrc/install/lib.rs, intentional inherit-if-set behavior dating back to the Zig implementation).So the test compares the outer bun's user agent against the inner bun's
Bun.versionand fails deterministically whenever the two versions differ, i.e. for anyone running the suite throughbun bd test/bun run. In CI the runner is launched by node, the variable is absent, and the test passes, so this never showed up there.Fix
Strip
npm_config_user_agentinbunEnv(test/harness.ts), next to the existingJSC_useJIT/FORCE_COLORstrips. Spawned buns then set their own user agent. No runtime change: the inherit-if-set behavior is deliberate and stays as is.Verification
test/cli/run/env.test.ts,test/cli/run/run-process-env.test.ts, and the rest ofbunx.test.tsbehave the same before and after (remaining local failures are pre-existing on clean main: the node-24@angular/clitest, tracked by Upgrade reported Node.js version to 26.3.0 #31818 / test: pin @angular/cli version in bunx node-version test #31820, and container TLS noise on the github tarball tests).Note: the CI-lane failures on
bunx.test.tsmentioned in the issue are theshould handle package that requires node 24test, not this assertion; those are owned by #31818 / #31820.