fix(lockfile): keep non-derivable tarball URLs when lockfileIncludeTarballUrl is false by zkochan · Pull Request #11509 · pnpm/pnpm

zkochan · 2026-05-07T00:24:37Z

Summary

lockfile-include-tarball-url defaults to false, so for the vast majority of users the early return added by #10621 silently dropped tarball URLs that cannot be derived from name+version+registry. This breaks pnpm install --frozen-lockfile from an empty store on registries that serve tarballs from a non-standard path:

GitHub Packages: https://npm.pkg.github.com/download/<scope>/<name>/<version>/<hash> (ERR_PNPM_FETCH_404 on fetches from the github registry #11276, #11407 follow-up)
JSR: https://npm.jsr.io/~/<n>/<scope>/<name> (pnpm update in pnpm@10.31 breaks JSR packages #10915)

After this PR, lockfileIncludeTarballUrl: false matches the historical (v10) heuristic — tarball URLs are written when they cannot be reconstructed, otherwise omitted. lockfileIncludeTarballUrl: true continues to force every tarball URL into the lockfile.

This aligns with the stated rule from @zkochan on #11407:

Tarballs should be included in the lockfile if their location cannot be automatically calculated. lockfileIncludeTarballUrl is only about adding even those tarballs to the lockfile that can be automatically calculated.

The fix removes the lockfileIncludeTarballUrl === false early return in toLockfileResolution so the URL-comparison branch always runs, which is what the v10 implementation did.

Test plan

Inverted the unit test that locked in the broken behavior; added a new test asserting GitHub Packages /download/ URLs are kept.
Inverted the integration test in installing/deps-installer/test/lockfile.ts that asserted non-standard tarball URLs were dropped.
pnpm --filter @pnpm/lockfile.utils test — 11 passed
pnpm --filter @pnpm/installing.deps-installer test -- test/lockfile.ts -t "tarball URL" — 5 passed

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

Bug Fixes
- Preserve non-derivable tarball URLs in the lockfile (e.g., registries with non-standard tarball paths), so installs with a frozen lockfile work from an empty store even when tarball URL inclusion is disabled.
Tests
- Added/updated tests to verify tarball URLs are retained for non-reconstructible cases (including GitHub Packages style URLs).

…ludeTarballUrl is false `lockfile-include-tarball-url` defaults to `false`, so for the vast majority of users the early return added by #10621 silently dropped tarball URLs that cannot be reconstructed from registry+name+version — breaking `pnpm install --frozen-lockfile` from an empty store on GitHub Packages (`https://npm.pkg.github.com/download/<scope>/<name>/<version>/<hash>`), JSR, and similar registries. `false` now matches the historical (v10) heuristic: tarball URLs are written when they are non-reconstructable, otherwise omitted. `true` continues to force every tarball URL into the lockfile. Refs #11276, #11407.

Replace "reconstructable" with "derivable" and avoid the cspell-flagged "mypkg" placeholder in the new test fixture.

coderabbitai · 2026-05-07T00:24:51Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5d2feeff-4237-485b-acf9-415cbf5ea05d

📥 Commits

Reviewing files that changed from the base of the PR and between 197d8cc and 3e5abd1.

📒 Files selected for processing (1)

lockfile/utils/src/toLockfileResolution.ts

📝 Walkthrough

Walkthrough

This PR restores a heuristic that preserves tarball URLs in pnpm-lock.yaml when those URLs cannot be derived from package name + version + registry, even if lockfileIncludeTarballUrl is false. It updates core logic and tests and documents the behavior in a changeset.

Changes

Tarball URL Preservation Heuristic

Layer / File(s)	Summary
Data Guard `lockfile/utils/src/toLockfileResolution.ts`	Add explicit `tarball` extraction guard: if `tarball` is missing, return `{ integrity }`. Update `gitHosted` detection to not assume `tarball` is present.
Core Logic `lockfile/utils/src/toLockfileResolution.ts`	Remove early return for `lockfileIncludeTarballUrl === false`. Always reconstruct/normalize expected tarball URL and compare to actual; preserve `resolution.tarball` when URLs differ (non-derivable/weird URLs). Keep special-casing for `file:` and git-hosted tarballs.
Unit Tests `lockfile/utils/test/toLockfileResolution.ts`	Add tests asserting that when `lockfileIncludeTarballUrl` is `false`, tarball URLs from non-standard registries and GitHub Packages `/download/` endpoints are retained in the lockfile resolution output.
Integration Tests `installing/deps-installer/test/lockfile.ts`	Update integration test for `lockfileIncludeTarballUrl: false` to expect preservation of a non-standard `resolution.tarball` URL for a specific package.
Changeset Documentation `.changeset/preserve-non-derivable-tarballs.md`	Document restoration of the tarball-URL preservation heuristic and note that `lockfileIncludeTarballUrl: true` still forces all tarball URLs into the lockfile.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit found tarballs that hid from the name,
He hopped through the registries, sniffing each strange domain.
When URLs couldn't be built from version and host,
He kept them safe in the lockfile—no install would be lost.
thump-thump, nibble of joy 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main change: restoring preservation of non-derivable tarball URLs when lockfileIncludeTarballUrl is false, which is the core fix across all modified files.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/11407-2

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

This PR fixes a regression in pnpm v11 lockfile writing where lockfile-include-tarball-url=false (the default) could drop tarball URLs that cannot be reconstructed from name+version+registry, breaking pnpm install --frozen-lockfile on registries with non-standard tarball paths (e.g. GitHub Packages /download/ URLs, JSR).

Changes:

Removes the lockfileIncludeTarballUrl === false early-return so the “non-derivable tarball” heuristic always runs.
Updates/unit-adds tests to assert non-derivable tarball URLs are preserved even when lockfileIncludeTarballUrl is false.
Adds a changeset documenting the behavioral fix and affected packages.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
lockfile/utils/src/toLockfileResolution.ts	Restores heuristic behavior to preserve non-derivable tarball URLs even when `lockfileIncludeTarballUrl` is false.
lockfile/utils/test/toLockfileResolution.ts	Updates/adds unit tests to cover non-derivable URLs (including GitHub Packages `/download/`).
installing/deps-installer/test/lockfile.ts	Updates integration test to assert non-standard tarball URLs remain in the written lockfile.
.changeset/preserve-non-derivable-tarballs.md	Adds a changeset describing the regression and fix.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

coderabbitai

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

lockfile/utils/src/toLockfileResolution.ts (1)
48-55: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add null check before comparing tarball URLs at line 49.

The non-null assertion tarball! on line 49 is unsafe. A resolution can reach this code path with an undefined tarball if:

lockfileIncludeTarballUrl is falsy

The tarball is neither file: nor from a git host

When this occurs, tarball!.replaceAll(...) crashes with a TypeError. Add a guard before the URL comparison:
Suggested fix
  const expectedTarball = getNpmTarballUrl(pkg.name, pkg.version, { registry })
+ if (tarball == null) {
+   return {
+     integrity: resolution['integrity'],
+   }
+ }
  const actualTarball = tarball.replaceAll('%2f', '/')
🤖 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 `@lockfile/utils/src/toLockfileResolution.ts` around lines 48 - 55, The code
uses tarball! when computing actualTarball and can throw if tarball is
undefined; update the block in toLockfileResolution.ts to first check that
tarball is non-null/undefined before calling replaceAll and comparing to
expectedTarball (i.e., only compute actualTarball =
tarball.replaceAll('%2f','/') and call removeProtocol comparison when tarball !=
null); if tarball is undefined, skip this comparison and proceed (preserve
existing behavior for gitHosted by leaving the call to
preservingGitHosted(resolutionFields, gitHosted) only when the comparison
fails).

🤖 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 `@lockfile/utils/src/toLockfileResolution.ts`:
- Around line 48-55: The code uses tarball! when computing actualTarball and can
throw if tarball is undefined; update the block in toLockfileResolution.ts to
first check that tarball is non-null/undefined before calling replaceAll and
comparing to expectedTarball (i.e., only compute actualTarball =
tarball.replaceAll('%2f','/') and call removeProtocol comparison when tarball !=
null); if tarball is undefined, skip this comparison and proceed (preserve
existing behavior for gitHosted by leaving the call to
preservingGitHosted(resolutionFields, gitHosted) only when the comparison
fails).

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2a2b6eb0-c00a-4caf-ac2f-60528ffe93fa

📥 Commits

Reviewing files that changed from the base of the PR and between 0378970 and 197d8cc.

📒 Files selected for processing (4)

.changeset/preserve-non-derivable-tarballs.md
installing/deps-installer/test/lockfile.ts
lockfile/utils/src/toLockfileResolution.ts
lockfile/utils/test/toLockfileResolution.ts

…ution `TarballResolution.tarball` is typed as required, but callers that deserialize resolutions from external state can violate that. Return early with just `integrity` if the tarball URL is missing instead of asserting non-null at the use site (which previously paired a `as string | undefined` cast with `tarball!.replaceAll(...)` — contradictory signals that confused both readers and review tools).

…rballUrl is false (#11509) * fix(lockfile): keep non-reconstructable tarball URLs when lockfileIncludeTarballUrl is false `lockfile-include-tarball-url` defaults to `false`, so for the vast majority of users the early return added by #10621 silently dropped tarball URLs that cannot be reconstructed from registry+name+version — breaking `pnpm install --frozen-lockfile` from an empty store on GitHub Packages (`https://npm.pkg.github.com/download/<scope>/<name>/<version>/<hash>`), JSR, and similar registries. `false` now matches the historical (v10) heuristic: tarball URLs are written when they are non-reconstructable, otherwise omitted. `true` continues to force every tarball URL into the lockfile. Refs #11276, #11407. * chore: appease cspell Replace "reconstructable" with "derivable" and avoid the cspell-flagged "mypkg" placeholder in the new test fixture. * docs(changeset): use camelCase setting name * fix(lockfile): guard against missing tarball field in toLockfileResolution `TarballResolution.tarball` is typed as required, but callers that deserialize resolutions from external state can violate that. Return early with just `integrity` if the tarball URL is missing instead of asserting non-null at the use site (which previously paired a `as string | undefined` cast with `tarball!.replaceAll(...)` — contradictory signals that confused both readers and review tools).

zkochan added 3 commits May 7, 2026 02:20

chore: appease cspell

f936ef4

Replace "reconstructable" with "derivable" and avoid the cspell-flagged "mypkg" placeholder in the new test fixture.

docs(changeset): use camelCase setting name

197d8cc

Copilot AI review requested due to automatic review settings May 7, 2026 00:24

Copilot started reviewing on behalf of zkochan May 7, 2026 00:25 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread lockfile/utils/src/toLockfileResolution.ts Outdated

coderabbitai Bot reviewed May 7, 2026

View reviewed changes

zkochan merged commit b682b6e into main May 7, 2026
13 checks passed

zkochan deleted the fix/11407-2 branch May 7, 2026 06:14

coderabbitai Bot mentioned this pull request May 26, 2026

fix: require integrity for tarball-shaped lockfile resolutions #11966

Merged

7 tasks

coderabbitai Bot mentioned this pull request Jun 12, 2026

fix: keep the path of git-hosted tarball resolutions in the lockfile #12344

Merged

TrevorBurnham mentioned this pull request Jun 19, 2026

feat(resolving): vendor get-npm-tarball-url as @pnpm/resolving.tarball-url + canonical-URL helper #12513

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(lockfile): keep non-derivable tarball URLs when lockfileIncludeTarballUrl is false#11509

fix(lockfile): keep non-derivable tarball URLs when lockfileIncludeTarballUrl is false#11509
zkochan merged 4 commits into
mainfrom
fix/11407-2

zkochan commented May 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 7, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

zkochan commented May 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zkochan commented May 7, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 7, 2026 •

edited

Loading