install: skip yarn.lock entries the migration cannot build a resolution for - #39001
install: skip yarn.lock entries the migration cannot build a resolution for#39001robobun wants to merge 1 commit into
Conversation
…on for migrate_yarn_lockfile appended a package with an Uninitialized resolution for entries whose version is not semver, that have no version line, or that have no resolved line (npm: aliases, URL specs). Debug builds then panicked in Resolution::eql while cleaning the lockfile, and release builds wrote a bun.lock that listed the dependency without a package, so the next install rejected it. Such entries are now skipped with a warning naming the entry and the field at fault. Package ids are assigned as packages are appended, and the entry-to-package map is rewritten through them, so a skipped entry leaves its dependents with INVALID_PACKAGE_ID edges, the same as a spec with no yarn.lock entry. Also drops the unused package_dependents pass, which would have indexed by the invalid id.
|
Warning Review limit reached
Next review available in: 27 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 (2)
Comment |
|
Status: fix and tests pushed, self-review in progress. Reproduced on an unfixed debug build with a one-entry yarn.lock whose Tests: |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #38985; the two are complementary and both PR descriptions point at each other.
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it introduces a two-pass package-id remapping in the yarn.lock migrator and deletes ~80 lines of the package_dependents pass, a human look would still be worthwhile.
What was reviewed:
- Traced every downstream reader of
yarn_entry_to_package_idafter the remap — each already guards onINVALID_PACKAGE_IDor writes it intoresolutionsas an unresolved edge; the one that didn't (package_names) gets a guard here. - Confirmed
package_versionsandpackage_id_to_yarn_idxare not read after the append loop, so they need no remapping;appended_package_ids[0](root) is never indexed. - Verified
package_dependentshad no readers anywhere insrc/— the pass was dead, and would have panicked on a remappedINVALID_PACKAGE_IDindex if kept. - New tests are hermetic (local
Bun.serveregistry, per-test cache dir), drain pipes concurrently, and cover root/transitive deps, all five uninitialized-resolution shapes, multi-spec entries, and thescoped_packagesretain path.
Extended reasoning...
Overview
The PR changes migrate_yarn_lockfile in src/install/yarn.rs so that a yarn.lock entry whose resolution block returns ResolutionTag::Uninitialized is skipped (with a warning) instead of being appended as a package with an invalid resolution. Because package ids must equal their index in this.packages, it introduces an appended_package_ids vector to remap the first-pass ids to the actually-appended positions, then rewrites yarn_entry_to_package_id and prunes scoped_packages through it. It also removes an ~80-line package_dependents computation that was never read, resizes package_names to this.packages.len(), and adds an INVALID_PACKAGE_ID guard to the package_names loop. A new describe.concurrent block in yarn-lock-migration.test.ts covers the change.
Security risks
None. This is lockfile-format migration logic operating on local file input; no auth, crypto, network trust, or path handling changes. The removed code was pure in-memory bookkeeping.
Level of scrutiny
Medium-high. The migrator writes bun.lock, and a wrong package-id after remapping would silently corrupt migrated lockfiles for users whose yarn.lock has both a skippable entry and other packages after it. The id-remapping is the load-bearing part and is subtle enough (two id spaces, one used only as a dedup key inside the append loop, one used everywhere after) that a maintainer familiar with the install lockfile invariants should confirm the shape.
Other factors
- I traced every consumer of
yarn_entry_to_package_idafter the remap:process_deps(writes the id intores_buf, INVALID is the intended unresolved-edge value), the root-deps first pass (same), the per-entry deps loop,package_names,usage_count/root_packages/scoped_names/spec_to_package_id/final deps loops — all either already checkedINVALID_PACKAGE_IDor now do.package_versionsandpackage_id_to_yarn_idxare only read inside their own building loops (pre-remap) so need no update; index 0 ofappended_package_idsis never accessed because first-pass ids start at 1. - The deleted
package_dependentsblock has no remaining references insrc/; it built a Vec<Vec> and never read it. Keeping it would have required its own INVALID guard (it indexed byyarn_entry_to_package_id[idx]), so removal is both cleanup and correctness. - Test coverage is thorough: both
bun install(exercises the crash path) andbun pm migrate, root vs transitive dependents, five distinct input shapes plus a multi-spec header, and a same-name/other-version case for thescoped_packagesretain. Tests use a localport: 0manifest-only registry and a per-test cache dir, so they're hermetic. The PR states the existingyarn-cli-reposnapshot is byte-identical, which is the main guard against regressing the no-skip path. - Not approving because this is not a simple/mechanical change: the remapping scheme and the dead-code deletion are the kind of thing an install-code owner should sanity-check, even though I found nothing wrong.
|
For whoever picks this up, the id remapping reduces to two things to check:
|
|
Updated 10:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 83d7dfe has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39001That installs a local version of the PR into your bun-39001 --bun |
Problem
Uninitialized. Inputs that do this: aversionline that is not semver (with or withoutresolved), noversionline, and noresolvedline on annpm:alias or URL-spec entry (install: migrate yarn.lock registry entries that have no resolved field #38985 handles plain registry entries withoutresolved; the inputs above still fall through with it).bun installon such a project, debug build:panic: internal error: entered unreachable codeatResolution::eql(src/install/resolution.rs:523, no arm forUninitialized), reached from thedebug_assert!inLockfile::append_package_with_id(src/install/lockfile.rs:2287) whileLockfile::clean_with_loggerclones the package. Exit 134.bun install(release) andbun pm migrate(any build): the bun.lock writer skipsUninitializedpackages (src/install/lockfile/bun.lock.rs:707) but the edges pointing at them are still written, so the file lists the dependency without a package for it and the nextbun installrejects it (Failed to resolve root prod dependency 'foo',InvalidLockfile, "Ignoring lockfile"). Nothing is reported at migration time;bun pm migrateexits 0.migrate_yarn_lockfile(src/install/yarn.rs) assigns package ids in a first pass over the entries and computes resolutions in a second pass, and the second pass appended the package even when the resolution block fell through toResolution::default().Fix
Uninitialized, the entry is not appended. A warning names the entry and the field at fault, in the shape the package-lock.json and pnpm-lock.yaml migrators use for entries they skip:skipped "foo@^1.0.0" from yarn.lock: invalid version "not-a-version"(ormissing "version" field/missing "resolved" field). Suppressed under--silentlike the other migrators' warnings.appended_package_idsafterwards (both the per-entry map and the multi-versionscoped_packageslists), so the remaining packages still haveid == indexafter a skip. A skipped entry maps toINVALID_PACKAGE_ID, so the later passes, which already check for it, give its dependents the same unresolved edge a spec with no yarn.lock entry gets today; the writer andbun installalready handle that edge (bun installreportsfoo@^1.0.0 failed to resolve, exit 1, instead of crashing or writing a lockfile it will reject).Resolution::eql'sunreachable!()is left alone: it is the assertion that caught this, and no valid lockfile has anUninitializedpackage.package_dependentspass is removed: it indexed by package id without the invalid-id check and was never read.test/cli/install/migration/yarn-lock-migration.test.ts, new describe "yarn.lock entries the migration cannot build a resolution for":bun installwith a root dependency and with a transitive dependency on a skipped entry (warning,failed to resolve, exit 1),bun pm migrateover the five input shapes above plus a multi-spec entry (warning text, exit 0, the other entry still written), and a package whose other version is skipped (exercises thescoped_packagesremap). On the unfixed debug build the install cases exit 134 with the panic above and none of the cases print the warning.yarn-cli-reposnapshot byte for byte (it needs a timeout above the default under the debug build with or without this change, test(install): fix yarn-lock-migration yarn-cli-repo case under debug+ASAN #35377); a real-world 7900-line yarn.lock skips nothing.migrate.test.ts,lockfile-only.test.tsandnested-overrides.test.tspass;cargo fmt --checkand the source lints are clean.Background
name@rangespecs, thenversion, usuallyresolved(the tarball URL), usuallyintegrity, and the entry's dependency ranges. The migrator works out a package's origin from these fields.Resolution(src/install/resolution.rs): the tagged value a lockfile package carries saying where it comes from (Npm,RemoteTarball,Git,Workspace, ...).Resolution::default()is theUninitializedtag; it is what the migrator's resolution block returns when no arm matched, and no consumer expects to see it on a package.Lockfileare indices intolockfile.packages; a dependency edge is aPackageIDinbuffers.resolutions, andINVALID_PACKAGE_IDthere means the edge is unresolved. The bun.lock writer skips such edges, andbun installreports non-optional ones after resolving.Lockfile::clean_with_loggerrebuilds the lockfile from the packages reachable from the root after every install, re-appending each one throughappend_package_with_id, whose debug assertion looks the package up again by name and resolution; that lookup is whereeqlran on theUninitializedresolution.Before / after on the repro from the report
package.json
{"dependencies":{"foo":"^1.0.0"}}, yarn.lock with onefoo@^1.0.0entry whose version line is"not-a-version", registry pointed at a closed port.Before (debug build):
After:
bun pm migratestill writes the dependency without a package, as it does today for a spec with no entry at all; the warning is what changes there.