Conversation
…ecycle scripts The install-tier configure_env_for_run shim stopped seeding npm_config_local_prefix when PackageManager stopped going through the runtime RunCommand.configure_env_for_run, and nothing in the install path exported the package.json config object any more. Seed the prefix from top_level_dir in the shim and export each package's own config strings as npm_package_config_<key> when its scripts are spawned.
|
Warning Review limit reached
Next review available in: 6 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 (3)
Comment |
|
Updated 7:30 AM PT - Aug 13th, 2026
❌ @robobun, your commit d299d23 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38110That installs a local version of the PR into your bun-38110 --bun |
|
Status: reproduced and fixed; self-review in progress, then waiting on CI. Reproduced with the released 1.4.0 binary and a debug build of main using the repro in the description ( Related: #36690 adds |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not duplicates, for anyone triaging: #36690 exports a different set of variables ( |
There was a problem hiding this comment.
LGTM — focused regression fix restoring npm_config_local_prefix and npm_package_config_* for install lifecycle scripts.
What was reviewed:
put_npm_package_config_envfollows the existingScripts::fill_from_package_jsonparse pattern (read_from→initialize_store()→parse_package_json);ScriptsListfields are owned (Box<[u8]>/ZBox) so the store reset can't invalidate them.npm_config_local_prefixuses the sametop_level_dirvalue the runtime'sconfigure_env_for_runalready exports;put(notput_default) matches npm's overwrite semantics and the test's stale-value check.- Ruled out: trailing separator on
top_level_dir()(test asserts exact equality withpackageDir); inheritednpm_package_config_*leaking into dependency scripts (per-package clone +putgives each package its own values, matching npm and improving on 1.3.14's root-only export).
Extended reasoning...
Overview
This PR restores two npm environment variables that regressed when the package manager stopped calling the full runtime RunCommand.configureEnvForRun: npm_config_local_prefix (project root) and npm_package_config_<key> (the package's own package.json config entries). The fix touches two files in src/install/: the install-tier configure_env_for_run shim in lib.rs seeds the process-wide prefix, and spawn_package_lifecycle_scripts in PackageManagerLifecycle.rs reads each package's own manifest to seed the per-package config vars into that package's cloned env map. A ~90-line test exercises root + trusted dependency, overwrite of a stale inherited prefix, and filtering of non-string/nested/empty config values.
Security risks
None. Lifecycle scripts already execute arbitrary code with the install's environment; adding two more npm-compatible variables (project root path and the package's own declared config strings) does not expand attack surface. No parsing of untrusted input reaches a new sink — parse_package_json is the same parser already invoked for these manifests elsewhere in the install path, and read/parse failures degrade to leaving the vars unset rather than propagating.
Level of scrutiny
Low-to-medium. This is a targeted regression fix restoring pre-existing (1.3.14) behavior with a narrower, more npm-correct semantics (each package sees its own config, not the root's). The Rust changes follow established in-crate patterns exactly: the JSON-parse sequence mirrors Scripts::fill_from_package_json in the same crate, and the env-var seeding mirrors the runtime's run_command.rs. The initialize_store() call is safe here because everything held across it (ScriptsList.items, cwd, the cloned script_env) is owned, and this pattern is used in ~20 other places in the crate.
Other factors
The test is thorough: it asserts exact env-var sets via toEqual (not toContain), covers both the default and waiter-thread variants via the enclosing for loop, verifies overwrite of an inherited stale prefix, verifies a dependency sees its own config keys and not the root's (including a shared key with different values), and verifies the non-string/nested/empty filtering matches bun run. Two candidate issues were raised and refuted: a trailing separator on top_level_dir() (the test's exact-equality check on localPrefix: packageDir would catch it, and the runtime uses the same unstripped value), and inherited npm_package_config_* leaking (the per-package env is cloned fresh from the base map and the test filters these from the install's own env). No prior human review comments to address.
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
#39445) Stacked on #36463 (the base branch is that PR's branch, so the diff here is only the additions). Merging this into #36463 adds the behavior changes listed below; #36463 itself now covers the #38333 install batch, the optional-peer correction, and the TOML / `bun init` fixes, so this PR no longer touches those. ### Problem - These 1.3 to 1.4 behavior changes are not in the guide at `701b3e2a0`: - MySQL: the first `caching_sha2_password` connection over plain TCP is refused unless `allowPublicKeyRetrieval: true` (#31129; 1.3.14 requested the key automatically, `MySQLConnection.zig` in the 1.3.14 tag). SQL `tls` / `ssl` options now require TLS instead of falling back to plaintext, and `?ssl=` / `?ssl-mode=` are read (`shared.ts` 1.3.14 only read `?sslmode=`; #37669). - Install: `~/.npmrc` fallback when `XDG_CONFIG_HOME` is set (#36289), credentials in `--registry` / env / bunfig object URLs are sent and outrank same-host `.npmrc` tokens (#38796, #38824), `bun outdated` exits 1 on fetch failures (#38809), new `dedupe` / `up` commands shadow scripts of those names and `bun feedback` is removed (#38333, #38444), `workspace:` ranges inside registry packages (#37669), isolated store entry names (#39014). - Runtime: `module.enableCompileCache()` / `NODE_COMPILE_CACHE` implemented (#34660), `require()` / `import` not-found messages (#34660), `AbortError` message without the period (#39277; 1.3.14's `BunCommonStrings.h` has the period), GCM IV length (#34092), `mkdtemp("")` (#34908), vm options (#38381), `server.reload` (#38697), ICU 75/73 to 78 (#38013), Compression stream chunking (#38695), `Bun.SQL` sqlite bindings (#35950). - Bundler: `splitting` with `cjs` / `iife` is an error (#32685), block-scoped `enum` lowers to `let` (#34249), exports emitted ascending instead of descending (#35957; `doStep5.zig` in 1.3.14 used `sortDesc`), minified `$` (#35668). - The TOML integer bullet did not say what the limit or the fix is. ### Fix - Adds a MySQL public key section (plus a summary table row), a TLS note under the `PGSSLMODE` section, an `.npmrc` / credentials addendum to the `bunfig.toml` section, a `module.enableCompileCache()` section, and the rest as bullets in the existing lists. - `docs/pm/overrides.mdx`: one-line change adding a pointer to this guide in the existing `lockfileVersion` 3 limitation. (The base branch briefly had a duplicate "Nested overrides" section; it removed that itself in `8257d01acb`, and this PR was rebased over it.) - Verification: each runtime claim was run against `1.4.0-canary.1+8326d1bd3` (22 commits behind main; contains every change referenced), and each install or bundler claim was checked against the source on main, with the 1.3 side taken from the `bun-v1.3.14` tag where the PR body did not state it. The `/runtime/sql#mysql` and `/upgrade-to-1.4` links resolve. `prettier --check` passes. ### Not included on purpose - Lifecycle scripts no longer receiving `npm_package_name` / `npm_package_version` / `npm_package_json` / `npm_config_local_prefix` during `bun install`, and transitive `"*"` ranges no longer deduplicating onto the root's version: regressions with open fixes (#36690, #38110, #38770). They need either the fixes or a guide line before release. - Postgres `sslmode=prefer` / `allow` (including `PGSSLMODE=prefer`, which 1.4 newly reads) hangs until the connection timeout against a server without SSL because nothing sends the startup message after the `N` reply. Same code in 1.3.14; filed as a bug instead of documented. <details> <summary>Commands used to verify the runtime claims</summary> ``` timers/promises setTimeout with an aborted signal # "The operation was aborted" bun req.cjs # Cannot find module ... Require stack: bun b.mjs (import() of a missing package / relative file) # Cannot find package 'x' imported from /path, ERR_MODULE_NOT_FOUND bun a_static.mjs (unhandled static import) # printed line still: Cannot find package 'x' from '/path' process.versions.icu # 78.3 createCipheriv("aes-128-gcm", key, Buffer.alloc(129)) # ERR_CRYPTO_INVALID_IV DecompressionStream of a 1 MiB gzip member # 16 chunks of 65536 bytes new SQL("sqlite://:memory:") with ${[1,2]} / ${new Date()} # Binding expected ... fs.mkdtempSync("") # EINVAL vm.runInThisContext("1", []) # ERR_INVALID_ARG_TYPE NODE_COMPILE_CACHE=/tmp/cc bun cc.cjs # creates /tmp/cc/v1.4.0-x86_64-<sha>-<uid> NODE_DISABLE_COMPILE_CACHE=1 + enableCompileCache() # status 3 (DISABLED) bun dedupe / bun up with package.json scripts of those names # built-in command runs bun feedback # Script not found "feedback" Bun.build({ splitting: true, format: "cjs" }) # Code splitting is currently only supported ... bun build of a function-scoped enum and import * as ns # let Color; exports a, m, z new SQL({ url: "postgres://...", tls: true }) on a non-TLS server # ERR_POSTGRES_TLS_NOT_AVAILABLE Bun.TOML.parse("a = 9007199254740993") # Integer cannot be losslessly represented ... ``` </details> <details> <summary>Previous revision</summary> The first revision of this PR (`3c5611454a`) also rewrote the package manager section for #38333 / #38853 (nested overrides and `lockfileVersion: 3`, the optional-peer correction, `bun update`, `bunfig.toml` over `.npmrc`, `--filter`) and fixed the TOML date and `bun init` lines. #36463 picked those up in its own commits the same day, so this PR was rebased onto its new head and reduced to the items above. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · docs-only change; test-proof not applicable <!-- robobun:evidence:end -->
Problem
bun install(andbun pm trust) no longer seenpm_config_local_prefixornpm_package_config_<key>. Bun 1.3.14 and npm set both; 1.4.0 printsprefix=[] cfg=[]for the repro below.npm_config_local_prefix, and packages read their ownpackage.jsonconfigthroughnpm_package_config_*(for examplesharp's install check readsnpm_package_config_libvips).PackageManager::configure_env_for_scripts_run(src/install/PackageManager.rs:1136) calls the install-tier shimRunCommand::configure_env_for_runinsrc/install/lib.rs. The shim only seedsnpm_config_user_agent,npm_execpathandBUN_FEATURE_FLAG_NO_ORPHANS; its closing comment deferred the rest to the runtime implementation, which the install path never calls. In 1.3.14 the package manager called the fullRunCommand.configureEnvForRun, which also exported the prefix and the rootpackage.json'sconfig.npm_package_name/npm_package_version/npm_package_jsonwent missing in the same way; install: set npm_package_name/version/json for dependency lifecycle scripts #36690 (open) adds those, so this PR leaves them alone.Fix
src/install/lib.rs: the shim now setsnpm_config_local_prefixtotop_level_dir.PackageManager::inithas already moved that to the project root (the workspace root when installing from inside a workspace member), which is the directory npm exports, and it is the same valueINIT_CWDis derived from a few lines later.put, notput_default, because npm overwrites this variable on every run and the only way a value is already present is an outerbun run/npm run, possibly from a different project; inheriting it would send scripts to the wrong root. run: set npm_package_name/version/json and npm_config_local_prefix per run #38071 makes the same change forbun run.src/install/PackageManager/PackageManagerLifecycle.rs:spawn_package_lifecycle_scriptsreads thepackage.jsonat the script's cwd and exports itsconfigentries asnpm_package_config_<key>into that package's script env. Reading the manifest of the package being run (rather than exporting the root'sconfiginto the shared base env as 1.3.14 did) is what npm does: a dependency's install script sees the dependency's ownconfig, and the root's keys do not leak into it. The root's scripts get the root'sconfigexactly as before.bun runalready uses (bun_resolver::PackageJSON::config, also what 1.3.14 exported): top-level entries whose key and string value are non-empty. Nested objects and non-string values are skipped here as they are forbun run; changing that would be a separate change to both paths.Scripts::fill_from_package_jsonin the same crate (initialize_store()+ParsedJson::parse_package_json); a missing or unparsable manifest just leaves the variables unset. This runs once per package that actually has scripts to run, afterPATHis built, so every caller (install_with_managerroot scripts,PackageInstaller/runTasksdependency and workspace scripts,bun pm trust) gets it.test/cli/install/bun-install-lifecycle-scripts.test.ts, "npm_config_local_prefix and npm_package_config_* are set for root and dependency scripts". One install with a rootconfig(string, number, nested object, empty string) and a trustedfile:dependency with its ownconfigsharing one key with the root; each postinstall dumpsnpm_config_local_prefixand everynpm_package_config_*variable, and the test compares the exact sets. The install is spawned with a stalenpm_config_local_prefixin its environment to cover the overwrite. Before the fix it fails withconfig: {}and the stale prefix; with the fix it passes in both the default and waiter-thread variants.bun-install-lifecycle-scripts.test.ts(the threePATH=""tests fail identically without this change in this container and pass in CI on main), the$npm_package_config_*tests inbun-run.test.tsandbun-workspaces.test.ts,cargo fmt,cargo clippy -p bun_install.Background
npm_config_local_prefixis npm's name for the project root an install operates on; npm exports it to every lifecycle script, including dependencies' scripts, andnpm_package_config_<key>is how npm exposes apackage.jsonconfigobject to that package's own scripts.configure_env_for_scripts_runbuilds one base env map once per process (PATH additions,INIT_CWD, node-gyp setup, the shim above);spawn_package_lifecycle_scriptsclones it per package with scripts, adds that package'sPATH, and hands the result to the subprocess. Process-wide values belong in the shim, per-package values in the spawn function, which is why the two variables land in different files.configure_env_for_run(the runtime crate depends on the install crate), hence the shim.initialize_store()creates or resets the thread-local AST store that parsed JSON values are materialized into; every transientpackage.jsonparse during an install calls it first, and the long-lived manifests (WorkspacePackageJSONCache) are deep-cloned into their own arenas so these resets cannot invalidate them.Repro
bun 1.3.14 and npm:
prefix=[/tmp/inst] cfg=[8080]bun 1.4.0:
prefix=[] cfg=[]this branch:
prefix=[/tmp/inst] cfg=[8080]; a trusted dependency with its ownconfigsees its own keys and the project root as the prefix.