Conversation
…istry An explicit registry in bunfig.toml or .npmrc now wins over the npm-compat NPM_CONFIG_REGISTRY / npm_config_registry env vars, which become a fallback for when no registry is configured. BUN_CONFIG_REGISTRY and --registry keep overriding the config files. Fixes #34168
|
Updated 12:06 PM PT - Jul 14th, 2026
❌ @robobun, your commit 3d91bfe has 3 failures in
🧪 To try this PR locally: bunx bun-pr 34170That installs a local version of the PR into your bun-34170 --bun |
WalkthroughRegistry loading now preserves explicit ChangesRegistry precedence
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
docs/pm/cli/install.mdx (1)
344-356: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBlanket statement now contradicts the new registry fallback rule.
Line 344 states "Environment variables take priority over
bunfig.toml," but the newly added carve-out saysNPM_CONFIG_REGISTRY/npm_config_registryapply only when no registry is configured — i.e., these specific env vars do not take priority overbunfig.toml. Consider qualifying the blanket statement (e.g., "...exceptNPM_CONFIG_REGISTRY/npm_config_registry, see below") to avoid misleading readers.📝 Suggested wording tweak
-Environment variables take priority over `bunfig.toml`. +Environment variables take priority over `bunfig.toml`, with the exception of `NPM_CONFIG_REGISTRY`/`npm_config_registry` (see below).🤖 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 `@docs/pm/cli/install.mdx` around lines 344 - 356, Qualify the introductory “Environment variables take priority over bunfig.toml” statement to exclude NPM_CONFIG_REGISTRY and npm_config_registry, which only act as fallbacks when no registry is configured. Keep the existing registry precedence explanation below unchanged.
🤖 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 `@docs/pm/cli/install.mdx`:
- Around line 344-356: Qualify the introductory “Environment variables take
priority over bunfig.toml” statement to exclude NPM_CONFIG_REGISTRY and
npm_config_registry, which only act as fallbacks when no registry is configured.
Keep the existing registry precedence explanation below unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f3ff2fe-0782-492c-9125-808a7558337a
📒 Files selected for processing (3)
docs/pm/cli/install.mdxsrc/install/PackageManager/PackageManagerOptions.rstest/cli/install/bun-install-registry.test.ts
|
Good catch on the docs contradiction: the blanket "Environment variables take priority over bunfig.toml" line predated the fallback carve-out. Reworded it in 05d5924 to "These environment variables take priority over bunfig.toml", scoping it to the BUN_CONFIG_* table above the npm-compat fallback note. |
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)
docs/pm/cli/install.mdx (1)
355-356: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify that an empty registry is treated as unset.
Options::load()only considers a registry configured when its URL is non-empty, soregistry = ""still allowsNPM_CONFIG_REGISTRY/npm_config_registryfallback. Change “no registry is explicitly configured” to “no non-empty registry is configured” to match the implementation.🤖 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 `@docs/pm/cli/install.mdx` around lines 355 - 356, Update the registry fallback documentation in the npm compatibility section to say it applies only when no non-empty registry is configured in bunfig.toml or .npmrc. Preserve the stated override precedence for BUN_CONFIG_REGISTRY and --registry.
🤖 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 `@docs/pm/cli/install.mdx`:
- Around line 355-356: Update the registry fallback documentation in the npm
compatibility section to say it applies only when no non-empty registry is
configured in bunfig.toml or .npmrc. Preserve the stated override precedence for
BUN_CONFIG_REGISTRY and --registry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74d1a1b7-f307-4a22-983c-3bb548efba9d
📒 Files selected for processing (1)
docs/pm/cli/install.mdx
|
Leaving the docs wording as is: "explicitly configured" describes intended usage, and an empty registry = "" in bunfig.toml is a degenerate value rather than a configuration someone writes on purpose. Treating it as unset (so the env fallback applies) is the sensible behavior, and qualifying the user docs with "non-empty" would add noise for an edge case nobody hits. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
An .npmrc containing only an auth line for the default registry host (what npm login writes) backfilled default_registry.url with the default URL, which made the new explicit-registry check treat it as a configured registry and drop the npm_config_registry fallback. Leave the url empty on that path, matching how bunfig represents auth-only registry config, and let Options::load fill in the default.
There was a problem hiding this comment.
The fix in 3d91bfe addresses my earlier concern — the .npmrc auth-only synthesis now leaves url empty, the host-comparison in load_npmrc falls back to DEFAULT_URL when empty, and I confirmed Options::load and the ini_jsc test API are the only other consumers of default_registry.url (both handle empty). Deferring to a maintainer to sign off on the precedence model itself, since registry selection controls where packages are fetched from and where credentials are sent.
Extended reasoning...
Overview
Changes registry-selection precedence in bun install so an explicit registry in bunfig.toml/.npmrc wins over NPM_CONFIG_REGISTRY/npm_config_registry, while BUN_CONFIG_REGISTRY and --registry still override config files. Touches PackageManagerOptions.rs (env-key selection), src/ini/lib.rs (auth-only .npmrc now leaves url empty instead of backfilling DEFAULT_URL), docs, and adds three end-to-end tests plus updates four loadNpmrc unit assertions.
Follow-up on prior review
My earlier finding (auth-only ~/.npmrc from npm login would defeat the !url.is_empty() guard) was fixed by changing the .npmrc parser to match bunfig's empty-url representation for auth-only config. I re-audited consumers of default_registry.url: Options::load already fills empty with DEFAULT_URL, load_npmrc's host/pathname derivation now guards on is_empty() and falls back to DEFAULT_URL, and ini_jsc.rs is test-only. The multi-.npmrc token-merge path is preserved because the second load_npmrc call sees url.is_empty() and uses the default host for matching. The token-carry logic in the env-var override path (prev_url.host == new_url.host) correctly drops the npmjs.org token when redirecting to a different mirror host, which the new test asserts.
Security risks
Registry selection is security-adjacent: it determines which server receives package requests and which credentials are attached. The change narrows when an env var can silently redirect traffic (good), and the token-host check already prevents cross-host token leakage. No new credential-forwarding path is introduced.
Level of scrutiny
This is a deliberate behavioral/design change to config precedence rather than a mechanical fix. The ordering (--registry > BUN_CONFIG_REGISTRY > config files > npm_config_* > default) is reasonable and documented, but it's the kind of user-facing contract a maintainer should confirm — particularly the asymmetry between BUN_CONFIG_REGISTRY (still overrides) and npm_config_registry (now fallback), and the .npmrc representation change.
Other factors
Three new e2e tests cover both directions (bunfig wins; env var still applies with no config; auth-only .npmrc doesn't count as configured). CI build #72974 was still running at review time.
…35327) ### What does this PR do? The registry/token env-var scan loops in `Options::load` (`src/install/PackageManager/PackageManagerOptions.rs`) carried a `did_set` flag with an `if !did_set` guard in place of `break`. This was a workaround for a Zig stage1 compiler bug where `break` inside `inline for` was broken, ported verbatim into the Rust rewrite along with its explanatory comment: ```rust self.scope.token = registry_.into(); did_set = true; // stage1 bug: break inside inline is broken // break :load_registry; ``` The Zig sources were removed in #32621 and the stage1 compiler no longer exists. In Rust this is just a plain `for` loop, so use `break` directly and drop: - the `did_set` flag and `if !did_set` wrapper (both loops) - the dead `// break :load_registry;` Zig-syntax comment - the `// load_registry:` label comment - the two `// was \`inline for\`; homogeneous elements -> plain for.` porting notes ### Behavior **No observable change.** The `if !did_set` guard already ensured the first matching env var wins; this PR just expresses that with `break`. Verified empirically against the released binary: ``` BUN_CONFIG_TOKEN=a NPM_CONFIG_TOKEN=b npm_config_token=c bun install -> Authorization: Bearer a ``` ### Tests Added to `test/cli/install/bun-install-registry.test.ts` to lock in the priority order (previously untested): - `BUN_CONFIG_TOKEN` wins over `NPM_CONFIG_TOKEN` and `npm_config_token` - empty `BUN_CONFIG_TOKEN` falls through to `NPM_CONFIG_TOKEN` - `BUN_CONFIG_REGISTRY` wins over `NPM_CONFIG_REGISTRY` / `npm_config_registry` These pass on the released binary as well; they exist to pin the refactor and guard against future changes to the key ordering. Related: #34170 touches the same block for a different concern (bunfig vs. env-var precedence); whichever lands second will need a small rebase. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-14, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#34168) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What does this PR do?
Fixes #34168
When
npm_config_registry(orNPM_CONFIG_REGISTRY) is set in the environment, an explicitregistryinbunfig.tomlor.npmrcwas silently ignored and the install failed with a confusingNo version matching ... founderror. The lowercase variant is injected into lifecycle-script environments by npm and pnpm and is commonly exported in shell profiles for mirrors, so it often leaks intobun installwithout the user asking for it.Registry precedence is now:
--registryBUN_CONFIG_REGISTRYbunfig.toml/.npmrcNPM_CONFIG_REGISTRY/npm_config_registryhttps://registry.npmjs.org/)The npm-compat env vars become a fallback that only applies when no registry is explicitly configured. Bun's own
BUN_CONFIG_REGISTRYkeeps overriding the config files, so CI setups that force a mirror device-wide still have an env-var escape hatch (as does--registry).The token env vars (
BUN_CONFIG_TOKEN,NPM_CONFIG_TOKEN,npm_config_token) are intentionally unchanged: a token overlaid on the configured registry is the common auth-injection pattern and does not redirect traffic.Reproduction
Implementation
Options::loadinsrc/install/PackageManager/PackageManagerOptions.rsreadBUN_CONFIG_REGISTRY,NPM_CONFIG_REGISTRY, andnpm_config_registryafter applying the bunfig/.npmrc config and unconditionally overwrote the default scope. Now the two npm-compat keys are skipped when the config set a non-empty registry URL.An auth-only
.npmrc(whatnpm loginwrites://registry.npmjs.org/:_authToken=...with noregistry=line) used to synthesize adefault_registrywith the default URL backfilled, which would have counted as an explicitly configured registry and dropped the env var fallback. The.npmrcparser now leaves the URL empty on that path, matching how bunfig represents auth-only registry config, andOptions::loadfills in the default as before.Tests
Added to
test/cli/install/bun-install-registry.test.ts:registry from bunfig.toml takes precedence over npm_config_registry env vars: two local registries; asserts the manifest request reaches the bunfig registry and nothing reaches the env-var registry, for both spellings. Fails on bun 1.4.0, passes with this change.npm_config_registry env var is used when bunfig.toml has no registry: pins the fallback behavior.npm_config_registry env var is used when .npmrc only has auth for the default registry: thenpm loginsetup; also asserts the saved token is not sent to the env var registry.The existing
registry override from a project .env ...test continues to coverBUN_CONFIG_REGISTRYoverridingbunfig.tomland still passes.Also documents the fallback in
docs/pm/cli/install.mdx.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-install-registry.test.ts