install: bump default lockfileVersion to 2, gate stricter parse checks behind it - #31539
Merged
Claude / Claude Code Review
completed
May 28, 2026 in 24m 50s
Code review found 2 important issues
Found 3 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 1 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/install/lockfile/bun.lock.rs:2613-2623 |
Security justification for skipping is_safe_resolved_tag does not cover the github branch |
| 🔴 Important | src/install/lockfile/bun.lock.rs:122 |
v1→v2 auto-upgrade produces a v2 lockfile that fails on next install |
| 🟡 Nit | test/cli/install/lockfile-version-2.test.ts:70-71 |
Misleading comment: offRegistry IS contacted in the v1 sub-case |
Annotations
Check failure on line 2623 in src/install/lockfile/bun.lock.rs
claude / Claude Code Review
Security justification for skipping is_safe_resolved_tag does not cover the github branch
This match arm covers **both** `ResolutionTag::Git | ResolutionTag::Github` (line 2598), but the comment's justification — "`Repository::checkout` re-validates with the same guard before building any cache path" — only holds for the **git** branch. GitHub dependencies go through the tarball-download path (never `Repository::checkout`) and feed the lockfile's `resolved` directly into `cached_github_folder_name_print` → `@GH@{resolved}…` with no `is_safe_resolved_tag` re-validation; `@G@{resolved}
Check failure on line 122 in src/install/lockfile/bun.lock.rs
claude / Claude Code Review
v1→v2 auto-upgrade produces a v2 lockfile that fails on next install
The v1 backward-compat only survives until the lockfile is re-saved: the writer always emits `Version::CURRENT` (now V2) at bun.lock.rs:263 and writes `pkg_meta.integrity` / the git `.bun-tag` verbatim with no backfill, so a v1 lockfile carrying an off-registry tarball with `""` integrity (or an unsafe `.bun-tag`) gets rewritten as `"lockfileVersion": 2` with the same empty integrity, and the *next* `bun install` rejects it with the very error this gate is meant to suppress. Any save trigger — `
Check warning on line 71 in test/cli/install/lockfile-version-2.test.ts
claude / Claude Code Review
Misleading comment: offRegistry IS contacted in the v1 sub-case
This comment is only accurate for the v2 sub-case below — in the v1 sub-case (lines 111–125) parsing now succeeds, so `bun install --frozen-lockfile` *does* attempt the tarball download from `http://127.0.0.1:${offRegistry.port}/…` and hits this server's 404 handler. The test still passes (loopback-only, and the v1 assertion only checks stderr lacks "Missing integrity hash"), but consider rewording to e.g. "For v2 parsing fails before any fetch; for v1 parsing succeeds and the download hits this
Loading