Skip to content

install: support omit-lockfile-registry-resolved in .npmrc - #37147

Open
willjcim wants to merge 4 commits into
oven-sh:mainfrom
willjcim:main
Open

willjcim wants to merge 4 commits into
oven-sh:mainfrom
willjcim:main

Conversation

@willjcim

@willjcim willjcim commented Aug 7, 2026

Copy link
Copy Markdown

Implements omit-lockfile-registry-resolved (#15448).

Problem

bun.lock records an absolute tarball URL for every package not served by the default registry:

"lodash": ["lodash@4.17.21", "https://npm.example.com/lodash/-/lodash-4.17.21.tgz", {}, "sha512-..."],

Since the lockfile is committed, this pins the whole dependency graph to whichever registry host the last person to update it happened to be using. Anyone who can't reach that host — CI, a deploy environment, a teammate off the VPN — fails on every tarball:

error: GET https://npm.example.com/lodash/-/lodash-4.17.21.tgz - 401

The absolute URL also overrides the reader's own registry configuration, so it can't be worked around downstream.

npm has omit-lockfile-registry-resolved for exactly this. This adds the same option to bun, read from .npmrc.

Cause

The round trip in src/install/lockfile/bun.lock.rs is asymmetric. The parser resolves "" against the configured registry for the package's scope:

if registry_str.is_empty() {
    let registry_url = mgr.scope_for_package_name(name_str).url.href();
    let url = ExtractTarball::build_url(registry_url, ...);

but the writer only collapses URLs under the hardcoded default registry back to "":

if url_is_under_registry(url_slice, Npm::Registry::DEFAULT_URL.as_bytes()) {
    b"" as &[u8]
} else {
    url_slice
}

So the reader already understands a registry-agnostic lockfile; only the writer can't produce one.

Change

With omit-lockfile-registry-resolved=true, the writer also collapses a URL served by the configured registry for that package's scope. "" is bun's existing representation for "resolve from the configured registry", so no new lockfile semantics are introduced — the output is the same shape an install against the default registry already produces.

registry=https://npm.example.com/
omit-lockfile-registry-resolved=true
"lodash": ["lodash@4.17.21", "", {}, "sha512-..."],

Plumbing follows the existing save-exact/save_text_lockfile path: BunInstall field → .npmrc parse in src/ini/lib.rs → PackageManager::Options → the writer. Opt-in, so default behavior is unchanged.

Packages without a supported integrity hash keep their absolute URL

This is the one case where omitting the URL would be a real regression rather than a portability win. If a package has no integrity hash and its URL is dropped, a reader without this registry configured resolves it from the default registry and installs whatever is published there — silently. With an integrity hash, the same situation is a hash mismatch and a hard failure.

That's the dependency-confusion hazard the existing version_to_write comment already worries about for private scopes, so those packages are left alone. It also means version_to_write's v1 stamping for them is unaffected, and no change was needed there.

Verification

Three tests in test/cli/install/bun-lock.test.ts, using the existing VerdaccioRegistry (a non-default registry, so it exercises the real path):

  • default behavior still writes the configured registry's tarball URL
  • with the option, the entry is "", the lockfile contains no reference to the registry URL, integrity is still present, and --frozen-lockfile reinstall reproduces the lockfile byte-for-byte
  • bun install <pkg> — which rewrites every entry — does not reintroduce URLs

willjcim and others added 2 commits August 7, 2026 12:00
`bun.lock` records an absolute tarball URL for every package not served by
the default registry. Installs routed through a mirror or proxy therefore
pin the whole dependency graph to whichever host the last person to touch
the lockfile used, which other environments may not be able to reach.

npm solves this with `omit-lockfile-registry-resolved`; implement the same
option. When enabled, a URL served by the configured registry for that
package's scope serializes as `""`, exactly as default-registry entries
already do. The parser resolves `""` against the reader's own configured
registry, so the round trip is symmetric and the lockfile is portable.

Packages without a supported integrity hash keep their absolute URL.
Dropping the URL there would let a reader that lacks this registry config
silently resolve a private package from the default registry rather than
fail on a hash mismatch. That also leaves `version_to_write`'s v1 stamping
for those packages unchanged.

Closes oven-sh#15448
install: support omit-lockfile-registry-resolved in .npmrc

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Adds the omit-lockfile-registry-resolved install option. Configuration flows into package-manager options, and lockfile serialization omits configured registry URLs when supported integrity hashes exist. Tests cover default, frozen, scoped-package, fallback, and dependency-addition flows.

Changes

Configured registry URL omission

Layer / File(s) Summary
Configuration and option wiring
src/options_types/schema.rs, src/ini/lib.rs, src/install/PackageManager/PackageManagerOptions.rs, docs/pm/npmrc.mdx
Defines omit_lockfile_registry_resolved, loads it from .npmrc, defaults it to false, and documents its behavior.
Lockfile registry URL serialization
src/install/lockfile/bun.lock.rs
Omits configured registry URLs when enabled and supported integrity data exists.
Registry serialization validation
test/cli/install/bun-lock.test.ts
Tests default URL output, configured URL omission, scoped packages, preserved integrity data, frozen installs, dependency additions, and unsupported-integrity fallback.

Suggested reviewers: robobun

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely identifies support for the .npmrc option, which is the pull request's primary change.
Description check ✅ Passed The description explains the problem, implementation, behavior, safety rationale, and verification coverage in sufficient detail.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@docs/pm/npmrc.mdx`:
- Around line 133-135: Update the documentation sentence describing packages
without a supported integrity hash to avoid guaranteeing failure for readers
lacking registry configuration. Explain that retaining the absolute URL forces
resolution against the recorded registry and prevents silent fallback to another
registry.

In `@test/cli/install/bun-lock.test.ts`:
- Around line 1034-1088: Expand the install-lockfile coverage around the
omit-lockfile-registry-resolved tests to include a scoped package resolved
through scope_for_package_name(), asserting its tarball URL is omitted while
supported sha512 integrity remains. Add an unsupported-integrity fixture and
assert that its absolute registry URL is retained, covering the fallback branch
without changing the existing unscoped cases.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: fbeb9c13-d4db-463c-9f61-0bf5ceaa0a63

📥 Commits

Reviewing files that changed from the base of the PR and between 45ee955 and 98ce41f.

📒 Files selected for processing (6)
  • docs/pm/npmrc.mdx
  • src/ini/lib.rs
  • src/install/PackageManager/PackageManagerOptions.rs
  • src/install/lockfile/bun.lock.rs
  • src/options_types/schema.rs
  • test/cli/install/bun-lock.test.ts

Comment thread docs/pm/npmrc.mdx Resolved
Comment thread test/cli/install/bun-lock.test.ts Resolved
Keeping the absolute URL pins resolution to the registry that served the
package; it does not guarantee that a reader without that registry
configured fails. The registry may well be reachable anonymously. Describe
what the behavior actually is: no silent switch to a different registry.
…registry-resolved

The existing tests only used an unscoped package with sha512 integrity, so
neither the scope lookup nor the integrity guard was exercised.

Scoped case installs `@types/no-deps` behind a `@types:registry=` entry, so
`scope_for_package_name()` resolves through the scope map instead of falling
back to the default scope, and asserts the URL is still omitted with
integrity intact.

Unsupported-integrity case serves a manifest with neither `integrity` nor
`shasum` from a local mock registry and asserts the absolute URL is
retained. Verdaccio always computes a shasum, so that branch is not
reachable through the shared fixture registry.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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.

Inline comments:
In `@test/cli/install/bun-lock.test.ts`:
- Around line 1159-1166: Update the subprocess handling around spawn so
proc.stdout is consumed concurrently with proc.stderr and proc.exited in the
same Promise.all call. Capture the stdout result and preserve the test’s
assertions for the combined subprocess output and ordered stage outputs.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8db966ea-d18c-4509-a0f9-783ac0eae572

📥 Commits

Reviewing files that changed from the base of the PR and between 98ce41f and fd2e11b.

📒 Files selected for processing (2)
  • docs/pm/npmrc.mdx
  • test/cli/install/bun-lock.test.ts

Comment on lines +1159 to +1166
await using proc = spawn({
cmd: [bunExe(), "install"],
cwd: String(dir),
env: { ...env, BUN_INSTALL_CACHE_DIR: join(String(dir), ".cache") },
stdout: "pipe",
stderr: "pipe",
});
const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Drain the piped stdout stream.

Line 1166 waits for stderr and process exit but does not consume proc.stdout. A verbose install can fill the stdout pipe and block before proc.exited resolves. Read stdout in the same Promise.all() call.

As per coding guidelines, “Subprocess tests must drain stdout, stderr, and process exit concurrently and assert the combined result and ordered stage outputs.”

🤖 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 `@test/cli/install/bun-lock.test.ts` around lines 1159 - 1166, Update the
subprocess handling around spawn so proc.stdout is consumed concurrently with
proc.stderr and proc.exited in the same Promise.all call. Capture the stdout
result and preserve the test’s assertions for the combined subprocess output and
ordered stage outputs.

Source: Coding guidelines

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant