Skip to content

install: migrate pnpm importer rows in the shape package.json parsing produces - #38791

Open
robobun wants to merge 1 commit into
mainfrom
farm/29621a98/pnpm-migration-importer-deps
Open

install: migrate pnpm importer rows in the shape package.json parsing produces#38791
robobun wants to merge 1 commit into
mainfrom
farm/29621a98/pnpm-migration-importer-deps

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • bun install on a pnpm workspace migrates pnpm-lock.yaml and then moves direct dependencies pnpm had locked to an older version onto the newest version already in the lockfile. vuejs/core: packages/compiler-sfc declares postcss: ^8.5.19, pnpm-lock.yaml has it at 8.5.23 (vite brings 8.5.25); after bun install, compiler-sfc gets 8.5.25 and 8.5.23 is gone. bun pm migrate by itself keeps 8.5.23, so the conversion is right and the install step after it moves things. Repros on b7a0431 too, so it predates install: pnpm parity — dedupe, prune, pm licenses, audit fix, add --filter/--catalog, nested overrides, transitive update, and workspace fixes #38333.
  • bun pm migrate followed by bun install --frozen-lockfile fails with lockfile had changes, but lockfile is frozen on any project whose package.json uses catalog:.
  • Cause: the migration (src/install/pnpm.rs) builds root and workspace rows in a different shape than parsing the same package.json produces, so the differ (Diff::generate_inner, src/install/lockfile/Package.rs) counts them as updated and re-resolves the whole importer; a re-resolved range takes the highest in-lock version. Three shapes differ:
    • append_importer_dependency replaced a catalog: row with the catalog entry's range (bun.lock got "@babel/parser": "^7.29.8" where package.json says catalog:); package.json parsing and the bun.lock reader keep the catalog: reference.
    • a row pnpm resolved to link: (a workspace:* row, or a plain range under link-workspace-packages) kept its parsed literal value (* or an npm range), while Package::parse_dependency stores the member's path under the workspace tag, which is what Dependency::eql compares.
    • member versions were read from the lockfile importer entry, which pnpm never writes, so workspace_versions stayed empty and migrated bun.lock files had no member "version" rows (the spurious re-resolve was what added them back).

Fix

  • append_importer_dependency only checks that the catalog entry exists (same error as before) and appends the row from its catalog: specifier like any other row; the importer's version: field binds it as before.
  • Root and workspace edges are bound through bind_importer_dependency!, which sets the resolution and, when the target is a workspace package, gives the edge the workspace shape. That rewrite is the block the bun.lock reader already had in map_dep_to_pkg, split out as bun_lock::adopt_workspace_resolution and shared; map_dep_to_pkg is unchanged in behavior.
  • Member versions come from the member's package.json under the workspaces array parser's rule (parses, no wildcard); the old branch would have failed the migration on an unparsable version.
  • Correct because every migrated importer row now holds exactly what a fresh parse of the same package.json holds, so the differ keeps the migrated resolutions. The text round trip was already fine, which is why bun pm migrate output looked right while the in-process install did not.
  • Test: test/cli/install/migration/pnpm-lock-v9.test.ts, new importer rows match a package.json parse block. pm migrate + --frozen-lockfile on a catalog project; bun install straight from pnpm-lock.yaml writing the same bun.lock as pm migrate and installing the locked 1.0.0 while 1.0.1 is also in the lock, for a catalog: user, a workspace:* user and a linked-range user; a member with an unparsable version gets no version row. All four fail on the unfixed build (the direct-install diff shows no-deps@1.0.0 replaced by 1.0.1).
  • pnpm-migration-complete.test.ts asserted the old "react": "18.2.0" row; updated to catalog: with its snapshot, and two workspace snapshots gain "version" rows.
  • vuejs/core checkout: bun pm migrate and bun install --lockfile-only now write byte-identical bun.lock files (postcss 8.5.23 at the root, vite/postcss 8.5.25), --frozen-lockfile after pm migrate passes, and the only differences from the previous pm migrate output are the catalog literals and 16 version rows.
  • Other pnpm migration suites plus bun-lock, catalogs, bun-workspaces and migrate pass locally. The yarn and package-lock.json migrations build neither catalog: nor linked rows, so they are not affected.

Background

  • pnpm-lock.yaml has an importers section, one entry per workspace package plus . for the root, listing each declared dependency's specifier (what package.json says) and version (what it resolved to; link:../x for a sibling). The migration turns each importer into a lockfile package whose rows come from those specifiers.
  • A catalog: specifier names a range kept in the workspace catalog (pnpm-workspace.yaml, moved into package.json workspaces.catalog by the migration). bun keeps such rows as references and looks the range up when resolving, so bun.lock prints catalog: and a catalog edit is detected as a change.
  • The differ: each install parses package.json into a scratch lockfile and compares its rows against the loaded (here, just migrated) lockfile's rows. Equal rows keep their resolution; a workspace with any unequal row is re-resolved in full, and resolution prefers versions already in the lockfile, highest first, which is why the drift always lands on the other in-lock version.
  • workspace_versions is the lockfile's member name to version map; the bun.lock writer prints it as each member's "version" row.

… produces

The pnpm-lock.yaml migration stored a root or workspace row differently
from what parsing the same package.json yields: a `catalog:` row became
the catalog's range, a row pnpm linked to a workspace member kept the
parsed literal instead of the member's path, and workspace versions were
read from the importer entry, which never carries one. The differ on the
following install therefore saw every such importer as changed and
re-resolved it, moving any range with a second in-lock version (vuejs/core:
compiler-sfc's postcss ^8.5.19 went from the locked 8.5.23 to 8.5.25), and
`bun pm migrate` followed by `bun install --frozen-lockfile` failed on any
project using catalogs.

Catalog rows now stay `catalog:` references (the entry's existence is still
checked), importer edges bound to a member adopt the workspace shape the
bun.lock reader already uses, and member versions come from the member's
package.json under the same rule as the workspaces array parser.
@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b54a1fc8-7dc2-45c5-9853-c1d1c6c8202f

📥 Commits

Reviewing files that changed from the base of the PR and between 2c2ef7c and d049ee9.

⛔ Files ignored due to path filters (2)
  • test/cli/install/migration/__snapshots__/pnpm-lock-migration.test.ts.snap is excluded by !**/*.snap
  • test/cli/install/migration/__snapshots__/pnpm-migration-complete.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (5)
  • docs/pm/cli/install.mdx
  • src/install/lockfile/bun.lock.rs
  • src/install/pnpm.rs
  • test/cli/install/migration/pnpm-lock-v9.test.ts
  • test/cli/install/migration/pnpm-migration-complete.test.ts

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

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: ready for review and merge. Build 97041 finished with 177 of 179 jobs green; the two remaining jobs (:darwin: 14 aarch64 - test-bun, both shards) never started: they expired unstarted three times because that agent tier has a multi-hour backlog across all open builds, so the build is marked failed without any test having failed. The handful of tests listed as failing on other lanes passed on retry or when re-run alone and are unrelated to this change (shell, terminal, sourcemaps, a verdaccio crash in bun-lockb.test.ts). Nothing in this diff is platform-specific; pnpm-lock-v9.test.ts passed on every lane that ran. I am not retriggering CI because a new build would queue into the same backlog; re-run or retry the two jobs once that tier drains.

Reproduced on a vuejs/core checkout (bun install --lockfile-only moves compiler-sfc's postcss from the locked 8.5.23 to 8.5.25 and drops 8.5.23; bun pm migrate then bun install --frozen-lockfile exits 1) and reduced to the fixtures in test/cli/install/migration/pnpm-lock-v9.test.ts (importer rows match a package.json parse), which fail on the unfixed build the same way: the direct-install bun.lock replaces no-deps@1.0.0 with 1.0.1.

With this branch both flows on vuejs/core write byte-identical bun.lock files and the frozen install passes.

Scope note: the macro is applied to the seven bindings in the root and workspace importer loops; the three plain assignments left in the loop over registry packages are not importer rows, and the differ never compares them.

One neighbouring gap is left as is: a catalog entry that no importer uses is in pnpm-workspace.yaml but not in pnpm-lock.yaml's catalogs: section, so the migrated bun.lock lacks it and the first plain bun install afterwards rewrites the lockfile once to add it (--frozen-lockfile already tolerates it with a note). That is independent of the importer rows fixed here and lives in the code #38754 is changing, so it is tracked separately.

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

I reviewed this PR and didn't find any bugs. Because it changes how migrated importer rows are shaped (catalog literals kept, workspace edges rewritten, member versions sourced from package.json) in src/install/, a human look would still be worthwhile.

What was reviewed:

  • adopt_workspace_resolution extraction is byte-for-byte with the old map_dep_to_pkg block; bind_importer_dependency! is applied to all 8 importer-loop bindings and correctly skips the transitive-package loop at workspace_pkgs_end...
  • CatalogMap::find already handles the ""/"default" equivalence the removed normalization did, so the missing-entry check is unchanged.
  • Workspace version rule (parsed.valid && wildcard == None) matches the workspaces array parser at Package.rs:2865; the old branch read value.get("version") from the pnpm importer entry, which pnpm never writes.
  • Snapshot updates checked against their fixtures — every new "version" row corresponds to a workspace package.json that declares one.
Extended reasoning...

Overview

This PR fixes pnpm lockfile migration so that migrated importer rows are shaped identically to what a fresh package.json parse produces, preventing the post-migration differ from re-resolving importers and moving locked versions. Three shapes are corrected: catalog: rows keep their literal instead of the resolved range; edges bound to workspace members adopt the workspace tag carrying the member path (via a shared adopt_workspace_resolution extracted from bun.lock.rs's map_dep_to_pkg); and workspace_versions is now populated from the member's own package.json instead of a field pnpm never writes. Files touched: src/install/pnpm.rs (main fix), src/install/lockfile/bun.lock.rs (helper extraction), four test/snapshot files, and one docs bullet.

Security risks

None identified. The change is confined to how in-memory lockfile rows are shaped during pnpm→bun migration. No new untrusted-input parsing paths, no allocation-size arithmetic, no filesystem-write surface change. The catalog check still fails migration on a missing entry.

Level of scrutiny

Moderate-to-high. src/install/ is production-critical: an incorrectly shaped row could cause frozen-lockfile false positives/negatives or silently move locked versions across a whole ecosystem of pnpm workspaces migrating to Bun. The change also alters user-visible bun.lock output (catalog rows now print catalog: instead of the resolved range; workspace members gain "version" rows). While the change is well-reasoned and well-tested, it is a behavioral change to a critical subsystem rather than a mechanical fix, so it should get a human sign-off.

Other factors

  • The PR description is unusually thorough and the code matches it exactly. I traced each of the three shape mismatches to the corresponding fix and confirmed the reference implementations (Package.rs version-parsing rule, CatalogMap::find default equivalence, map_dep_to_pkg workspace-adoption block) agree with the new code.
  • The remaining direct resolutions[dep_id] = pkg_id assignments (lines ~1589/1612/1632) are in the transitive-package loop, not importer rows, so leaving them un-adopted is correct — the differ only compares importer rows.
  • Test coverage is strong: four new tests cover catalog:, workspace:*, link-workspace-packages ranges, and an unparsable member version, each asserting both the migrated bun.lock shape and the actually-installed version (1.0.0 stays 1.0.0 while 1.0.1 is also in the lock). All snapshot updates are consistent with fixtures having the corresponding "version" field.
  • No prior reviews on this PR; only a CodeRabbit rate-limit notice.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 9:05 PM PT - Aug 14th, 2026

@robobun, your commit d049ee9 has some failures in Build #97041 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38791

That installs a local version of the PR into your bun-38791 executable, so you can run:

bun-38791 --bun

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants