install: reuse lockfile-resolved git packages for branch and bare refs when re-resolving

[robobun]

Symptom

Whenever bun install re-resolves (most commonly: any dependency edit in a workspace member, which re-parses the member and re-enqueues its whole dependency list), every git or github dependency whose committish is a branch, a tag, or absent (git+url#main, github:owner/repo, ...) is fetched from the remote again, despite a warm cache and an unchanged lockfile pin. The extract then appends a transient duplicate package that the lockfile clean prunes, so bun.lock stays correct and the cost is the redundant network round trip on every such install. Sha-pinned refs have the same re-resolution bug with a different cause, fixed separately in #37142; this PR covers the refs #37142 explicitly leaves out.

With BUN_DEBUG_PackageManager=1 the second install shows the git dependency going back to the network:

[packagemanager] enqueueDependency(3, git, branch-dep, git+http://127.0.0.1:4873/branch-dep.git#main) = http://127.0.0.1:4873/branch-dep.git

Cause

bun.lock writes the resolved commit in the committish position of the resolution string: a dependency on git+url#main is stored as branch-dep@git+url#<sha>. Reloading the lockfile parses that sha back as the committish, so the original ref ("main", "v1", or empty) is lost. On re-resolution, the git/github arms of enqueue_dependency_with_main_and_success_fn call lockfile.get_package_id(...), whose Repository::eql compares the fresh dependency's committish against the loaded one: "main" vs <sha> never matches, the lookup misses, and a clone/fetch (git) or tarball download (github) task is created.

Root-level dependencies are protected by Diff::generate's mapping (unchanged deps keep their resolution slot), but a changed workspace member's dependency list is re-parsed with invalid slots, so every one of its git deps takes this path on every install that dirties the lockfile.

Fix

When the exact-committish lookup misses, reuse the package an identical dependency (same name hash, same version literal) is already bound to in the loaded lockfile, after checking the bound package's resolution is the same kind of repository with the same repo/owner. The dependency literal round-trips through bun.lock unchanged, so "an identical literal previously resolved to this package" is the lossless record of the prior resolution, for branch, tag, bare, and sha refs alike. Cases where the literal is the wrong authority are already handled upstream of this lookup: a changed override or catalog invalidates the old bindings before re-enqueueing, and the repo/owner check rejects a binding whose effective resolution was redirected.

The reuse is skipped for bun update targets (same update-target test as Diff::generate): an update must keep re-resolving the ref against the remote. That keeps #36689 (bun update re-resolves git deps) working unchanged; all five of its tests pass with both changes applied together.

Three adjustments that review surfaced:

• The overrides/catalogs re-resolution loops in install_with_manager.rs cleared and re-enqueued affected dependencies one at a time, so with several dependencies sharing a name the first re-resolution could have rebound to a sibling's not-yet-cleared binding from the old override (a committish-only override change, git+url#v1 to #v2, would have been silently ignored). Both loops now invalidate every affected slot before enqueueing any of them, which is what makes the literal the safe authority here. Covered by tests for both the override and the catalog variant.
• With a cold cache under the isolated linker, the store entry for a git dependency waits on the locked commit's checkout id, but the resolve-phase re-enqueue after a clone derived the checkout from find_commit on the dependency's committish, which follows the ref. With a branch head that moved since lock, the reuse left the dependency correctly bound to the locked package while the locked checkout never ran: bun install hung forever (without the reuse, the same flow instead silently floated the pin to the new head, and a clean-lockfile cold-cache install already hung before this PR). The re-enqueue now checks out the bound package's resolved commit, the same way the clone-failure drain and the hoisted installer already key it. Covered by a cold-cache leg in the reuse test: moved branch head, wiped cache and node_modules, isolated linker; asserts the install terminates, keeps the locked sha, and installs the locked content.
• scp-form repos (git@host:path) are intentionally excluded from the reuse: the lockfile writer serializes them with an ssh:// prefix the dependency parse lacks, and every git task id downstream keys on the exact repo bytes, so binding across the two spellings would strand the isolated store's checkout waiter (it waits under the package-repo id while the re-enqueue keys on the dependency-repo id). scp dependencies keep the pre-existing fetch path, which is redundant but correct. Aligning the two spellings at parse time is the real fix and is its own pre-existing issue (the same id mismatch can already strand a lockfile-bound scp dependency on a cold cache under the isolated linker, with no reuse involved).

The reuse test also pins the must-not-reuse boundary: changing a dependency's ref literal consults the remote again.

Beyond the redundant traffic, the re-fetch becomes a correctness problem the moment the cached repo's refs actually refresh, which is exactly what #36689 fixes (git fetch on the bare cache currently updates no refs, so today's re-fetch accidentally returns the stale pin). Verified with #36689's refspec fix applied and this fix absent: a plain bun install after an unrelated member edit silently floats #main from the locked sha to the remote's new HEAD, rewriting the lockfile pin. With both fixes applied the pin stays put on bun install and still moves on bun update.

Verification

Three new tests in test/cli/install/bun-lock.test.ts, all against local servers (bare git repos over dumb HTTP; fake codeload tarball server for github:), no network:

• re-resolving reuses branch and bare ref git dependencies from the lockfile instead of re-fetching: a workspace member depends on git+url (bare), git+url#main (branch), and github:owner/repo#main; install, dirty the member with an unrelated dep, install again, assert the request counters did not move and the locked shas are unchanged. On unfixed bun the second install makes 4 extra git requests and re-downloads the github tarball.
• `bun update` still re-resolves a branch ref git dependency against the remote: pins the update-target gate; passes before and after this change.
• a changed git override re-resolves every dependent instead of reusing the old pin: two workspace members share an overridden dependency, the override moves from #v1 to #v2 of one repo; asserts the lockfile and installed content move. Passes on released bun, fails with the reuse alone (the interleaved invalidation), passes with the two-pass invalidation.

Ran locally with the debug build:

• test/cli/install/bun-lock.test.ts: 19 pass
• test/cli/install/isolated-install.test.ts: 62 pass
• test/cli/install/bun-workspaces.test.ts: 63 pass
• test/cli/install/bun-update.test.ts: 6 pass, catalogs.test.ts: 18 pass, overrides.test.ts: 7 pass, bun-add.test.ts: 54 pass
• test/cli/install/bun-install.test.ts -t git: 9 pass, 12 failures from blocked external hosts in the sandbox (bitbucket.org/gitlab.com), identical on released bun

Complementary to #37142, which fixes the empty-package-name lookup miss for these dependency types: that change makes sha-pinned refs match the loaded committish, this one recovers everything the committish comparison cannot express. Neither subsumes the other and they merge cleanly in either order.

---

no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/install/bun-lock.test.ts

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Git and GitHub dependency resolution now reuses matching locked packages. bun update and changed overrides invalidate reuse. Catalog and override dependencies use separate invalidation passes. Integration tests verify network requests, lockfile commits, and installed contents.

Changes

Git dependency reuse

Layer / File(s)	Summary
Match and reuse locked packages src/install/PackageManager/PackageManagerEnqueue.rs	The resolver matches locked Git and GitHub packages by dependency and repository identity. It normalizes SCP-style paths. Matching packages bypass remote resolution. Git checkouts use the locked commit when available. bun update targets still resolve remotely.
Invalidate overrides before re-enqueue src/install/PackageManager/install_with_manager.rs	Override and catalog resolutions are cleared in separate passes before affected dependencies are re-enqueued.
Validate reuse and update behavior test/cli/install/bun-lock.test.ts	Integration tests cover Git, bare-ref, and GitHub reuse, update refetching, changed overrides and catalogs, request counts, lockfile commits, installed contents, isolated fixtures, tarball creation, and retry handling.

Possibly related issues

• install: Git dependencies from one repo: silent skips installing from a lockfile on cold cache (exit 0), plus resolve failures for duplicated direct+transitive dependencies #35420 — The changes address Git dependency lockfile reuse and repository path normalization.

Possibly related PRs

• oven-sh/bun#36360 — Both changes affect dependency re-resolution during bun update.
• oven-sh/bun#36688 — Both changes bypass locked-package reuse for bun update targets.
• oven-sh/bun#37142 — Both changes extend Git and GitHub lockfile reuse in the same resolver paths.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly summarizes the main change: reusing lockfile-resolved Git packages during re-resolution.
Description check	✅ Passed	The description explains the symptom, cause, fix, scope, and verification results, although it uses headings different from the template.

---

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

[coderabbitai]

Actionable comments posted: 4

🤖 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 1197-1213: Extract the duplicated git fixture setup into a
module-level makeGitFixture(packageDir) helper that creates the shared gitEnv,
git() function, and gitconfig initialization, returning the environment, helper,
and readiness promise. Replace the inline setup in both tests with this helper
and await its returned ready value before using git.
- Around line 1136-1145: Update the second install retry logic to snapshot
gitRequests and githubDownloads before invoking install, restore those snapshots
when retrying after a SIGKILL, and only retry when the failure identifies the
spawned git child rather than any stderr containing “signal 9”. Leave the
counter handling in the helper at the later retry block unchanged because its
assertion is non-strict.
- Around line 1266-1272: Strengthen the test around the install/update flow by
advancing the bare repository’s main branch to a new commit after the initial
install and before run(["update", "git-dep"]). Capture the new commit SHA, then
assert that bun.lock records this SHA after the update, while retaining the
request-count assertion as needed.
- Around line 1069-1075: Update the git helper and both tests in
bun-lock.test.ts to check Bun.which("git") before running, skipping each test
with a clear reason when unavailable. Pass the resolved git executable path to
spawn instead of hardcoding "git", while preserving the existing command
arguments and assertions.

🪄 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

Run ID: e3d26c02-633f-4e93-aedd-5e626068f96d

📥 Commits

Reviewing files that changed from the base of the PR and between 45ee955 and 01f22da.

📒 Files selected for processing (2)

• src/install/PackageManager/PackageManagerEnqueue.rs
• test/cli/install/bun-lock.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/install/PackageManager/PackageManagerEnqueue.rs:2019-2022 — The locked.repo.eql(repo.repo, ...) guard never matches for SCP-form SSH git specifiers (e.g. git+git@host:org/repo.git#main): the lockfile writer prepends ssh:// when serializing an SCP-like repo (repository.rs:1097), but neither parse_append_git on reload nor the fresh dependency parse strips it, so the loaded resolution's repo is ssh://git@host:... while the fresh dependency's is git@host:.... Not a regression — those deps just fall through to the pre-PR fetch path — but the reuse silently doesn't cover the private-git-server SSH variant; consider stripping a leading ssh:// from locked.repo before comparing (or normalizing both sides through the same path the writer uses), or note the exclusion.

  Extended reasoning...

  What the guard misses

  find_locked_git_package accepts a candidate only when the loaded resolution's repo field is byte-equal to the freshly-parsed dependency's repo field (line 2020). For an SCP-like git specifier — the standard SSH clone URL for a private git server, e.g. git+git@my-server.com:org/repo.git#main — those two strings are never equal after a lockfile round-trip, so the reuse path returns None and the dependency goes back to the network exactly as it did before this PR.

  Step-by-step trace

  1. Fresh dependency parse (dependency.rs, Tag::Git arm): strips only the git+ prefix, so repo.repo = "git@my-server.com:org/repo.git" and owner is empty. is_scp_like_path("git@my-server.com:org/repo.git") is true (an @ appears, the first : is not followed by //).
  2. Lockfile write (bun.lock.rs → Repository::fmt("git+", buf) → repository.rs:1084-1100): owner is empty and is_scp_like_path(repo) is true, so the formatter writes ssh:// before the repo, producing the resolution string git+ssh://git@my-server.com:org/repo.git#<sha>. The existing snapshot at test/cli/install/__snapshots__/bun-install.test.ts.snap shows exactly this shape for an SCP git dep.
  3. Lockfile reload (Resolution git arm → Repository::parse_append_git, repository.rs:425-441): strips only git+, so the loaded resolution's repo = "ssh://git@my-server.com:org/repo.git".
  4. Guard (PackageManagerEnqueue.rs:2020): locked.repo.eql(repo.repo, buf, buf) compares "ssh://git@my-server.com:org/repo.git" against "git@my-server.com:org/repo.git" byte-for-byte → false → continue. The loop exhausts and returns None.

  The literal check (line 2003-2006), name_hash, and version.tag all match — dependency literals round-trip through bun.lock unchanged, exactly as the PR description says. The repo-string mismatch introduced by the writer's normalization is the only thing rejecting the reuse.

  Why nothing else catches it

  is_scp_like_path is one-directional: the writer applies it to add ssh://, but neither parse_append_git nor the git-tag dependency parser reverses it, and is_scp_like_path("ssh://git@…") itself returns false (the first : is followed by //), so the two sides can never converge. Note that SCP URLs to github.com/gitlab.com are classified Tag::Github via hosted-git detection and take a different arm; this specifically affects non-hosted private git servers over SSH — still a real and common variant.

  Impact

  No correctness regression: the guard failing means find_locked_git_package returns None and control falls through to the exact network-fetch path that existed before this PR. SSH SCP-form git deps in an edited workspace member keep re-cloning on every install, i.e. the symptom this PR fixes for http(s) and github: specifiers is left in place for git@host:path specifiers, with no test covering that variant and no mention of the exclusion. Per REVIEW.md's "Cover the variant matrix, not just the repro" and "If a site is intentionally excluded, say so in the PR", it's worth either closing the gap or noting it.

  Fix direction

  Normalize before comparing — e.g. strip a leading ssh:// from locked.repo's slice when the fresh repo.repo lacks it (or vice-versa), or run both sides through the same normalization the lockfile writer applies (prepend ssh:// to the fresh repo when is_scp_like_path is true) so the byte comparison sees equal inputs.

• 🔴 src/install/PackageManager/PackageManagerEnqueue.rs:1997-2024 — The comment "a changed override/catalog also invalidates the old binding before re-enqueueing, so a stale binding is never reachable here" is not true: the overrides_changed / catalogs_changed loops in install_with_manager.rs:485-528 clear-then-enqueue per dependency (interleaved), not clear-all-first. When ≥2 dependencies share a name and an override/catalog changes from git+url#v1 to git+url#v2 (same URL, different ref), the first re-enqueued dep finds its not-yet-cleared sibling — same raw dependency.version.literal, same repo/owner — and both slots get rebound to the stale v1 package, silently ignoring the override change. Fix: skip the reuse when this.summary.overrides_changed && all_name_hashes.contains(dependency.name_hash) (and analogously for catalogs_changed + Tag::Catalog), or make the two invalidation loops two-pass (clear all affected slots, then enqueue).

  Extended reasoning...

  What the bug is

  find_locked_git_package walks every (dependency, resolution) pair in the lockfile and reuses the first bound package whose raw dependency has the same name_hash, same version.tag, and same version.literal, and whose resolved package has the same repo/owner. It never compares committish. The doc comment justifies this by claiming that the override/catalog invalidation path clears stale bindings before re-enqueueing, so a stale binding "is never reachable here." That claim does not match the code: the invalidation loops in install_with_manager.rs are interleaved (clear slot i, immediately enqueue i, then move on to i+1), not two-pass. When more than one dependency shares a name — the same transitive dep from multiple parents, or the same dep in multiple workspace members — the first one's enqueue runs while the later ones' resolution slots still point at the old package.

  Code path

  At install_with_manager.rs:341-350 the new overrides / catalogs maps are copied into manager.lockfile before the loops, so inside enqueue_dependency_with_main_and_success_fn the effective version local (lines 657-754) is already the new override target (e.g. git+url#v2). The match version.tag at line 757 routes to the Git arm at line 1175, and get_package_id at line 1180 compares the new committish "v2" against the stored resolved sha for v1 — miss. Control falls through to find_locked_git_package at line 1189, which is passed the outer function's raw dependency parameter (line 628), not the post-override version local.

  Inside find_locked_git_package, the comparison at lines 2001-2006 checks other.version.tag == dependency.version.tag and other.version.literal.eql(dependency.version.literal, ...). Both sides are the raw pre-override values — e.g. both Npm / "^1.0.0", or both Catalog / "catalog:" — so any two same-named deps with the same package.json spelling match regardless of what override/catalog they now resolve to. The guard at line 2020 checks only locked.repo and locked.owner against the effective repo; when only the committish changed (#v1 → #v2, same URL), the guard passes.

  Why the invalidation doesn't protect this

  install_with_manager.rs:485-503 (overrides) and 506-527 (catalogs):

  for dependency_i in 0..dependencies_len {
      let dependency = manager.lockfile.buffers.dependencies[dependency_i].clone();
      if all_name_hashes.contains(&dependency.name_hash) {
          manager.lockfile.buffers.resolutions[dependency_i] = invalid_package_id;
          enqueue_dependency_with_main(manager, dependency_i as u32, &dependency, ...);
      }
  }

  When dep i is enqueued, dep j > i's resolution slot still holds the old package id. find_locked_git_package iterates all (dependency, resolution) pairs, finds j, and returns its stale package id via success_fn (which is assign_resolution, writing buffers.resolutions[i] = stale_pkg). The loop then advances to j, clears it, enqueues it — and find_locked_git_package now finds i (just bound to the stale package) and reuses it too. Both slots end up back at the v1 package.

  Step-by-step proof

  1. Root package.json has overrides: { "foo": "git+https://example/foo.git#v1" }. Two workspace members (or two transitive parents) each declare "foo": "^1.0.0".
  2. First bun install: both foo deps resolve to package P1 (sha-for-v1). buffers.resolutions = [.., P1, .., P1, ..].
  3. User edits root override to "git+https://example/foo.git#v2" (same URL, different ref). Runs bun install.
  4. Diff::generate sets summary.overrides_changed = true; all_name_hashes = [hash("foo")]. lf.overrides is updated to the new map (#v2).
  5. Loop iteration for dep A (first foo): resolutions[A] = invalid; enqueue. Effective version = git+url#v2. get_package_id misses ("v2" ≠ stored sha). find_locked_git_package scans, finds dep B (second foo): resolutions[B] == P1 (not yet cleared), raw tag/literal both Npm/"^1.0.0" → match, P1.repo == url → match. Returns P1. success_fn writes resolutions[A] = P1.
  6. Loop iteration for dep B: resolutions[B] = invalid; enqueue. find_locked_git_package finds dep A at P1. Returns P1. resolutions[B] = P1.
  7. Both slots stay at P1; the override change to #v2 is silently ignored and bun.lock keeps the v1 sha.

  Before this PR the git arm had no reuse path — after get_package_id missed it fell through to a fresh clone/fetch task that would resolve #v2. So this is a correctness regression introduced by this PR, not a pre-existing issue.

  Impact

  Silent: no error, no warning. The user changes an override or catalog from one git ref to another on the same repo, runs bun install, and the lockfile stays pinned to the old commit. This is exactly the class of change overrides/catalogs exist for (pinning a fork to a specific fix commit, then bumping it), and the ≥2-same-named-deps precondition is common (any transitive dep pulled in by more than one parent, or any dep declared in more than one workspace member).

  Fix

  Two straightforward options:

  • In find_locked_git_package, bail early with None when this.summary.overrides_changed and the dep's name_hash is in the overrides-changed set (and analogously when this.summary.catalogs_changed and the raw dependency.version.tag == Catalog). This targets exactly the case where the raw literal is not authoritative.
  • Or restructure the two loops in install_with_manager.rs to be two-pass: first clear all affected resolutions[i] to invalid_package_id, then enqueue all of them. That makes the doc comment true.

  Either way, please add a test covering the ≥2-same-named-deps + committish-only override change case, since neither of the new tests exercises the overrides_changed / catalogs_changed re-enqueue path.

[robobun]

Review findings addressed in e91879a:

• Interleaved override/catalog invalidation: confirmed the regression exactly as described (two members sharing an overridden name, override moved #v1 to #v2 of one repo: the reuse rebound both to the v1 package). Both loops in install_with_manager.rs now clear every affected slot before enqueueing any of them, and a new test covers the two-members + committish-only override change; it fails with the reuse alone and passes with the two-pass invalidation (and on released bun).
• scp-form repos: the repo comparison now accepts the ssh:// prefix the lockfile writer adds for scp-like paths, mirroring the writer's is_scp_like_path branch. No automated test (the variant needs a real ssh transport); noted in the PR body.
• The git fixture dedup was already applied in d2dc988 (makeGitFixture).

Separately, this run also surfaced why the ASAN lane flaked earlier: a git child killed between clone and checkout leaves the per-commit cache folder half-created, and the next install trusts it and silently resolves an empty package. That is a pre-existing bug in the checkout cache layer, reported separately; the tests here now retry from a clean cache when the kill happens.

[robobun]

CI status: the last two runs failed in build-cpp on infra, not on this diff: the agents could not download prebuilts from github.com releases ("Failed to download after 5 attempts: fetch failed" for bun-webkit-linux-amd64-lto in build 90145, bun-webkit-freebsd-amd64 in build 90146, mimalloc likewise). One retrigger is already spent on it. The test lanes that did run are green apart from failures tracked separately (an ASAN worker_threads stress crash and a filesystem_router segfault on aarch64, both unrelated to install). Happy to re-run once the download outage clears.

[robobun]

^{Updated 11:56 AM PT - Aug 7th, 2026}

✅ @robobun, your commit 2fb4cbf9763d55bf2f56b25c22abdfd14fc8e4e0 passed in Build #90193! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37143

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

bun-37143 --bun

[claude]

No new issues found; the earlier findings (interleaved override/catalog invalidation, the ssh:// prefix mismatch for scp-like repos, and the duplicated git fixture) are all addressed. Deferring to a human because this changes lockfile→resolution binding semantics for git/github deps in the enqueue path, plus the ordering of the override/catalog re-resolve loops — a wrong reuse installs the wrong commit silently, and the safety argument leans on several cross-file invariants.

Reviewed: the new find_locked_git_package scan (literal + repo/owner match, update-target skip, scp ssh:// normalization) and both call sites; the two-pass invalidate-then-enqueue reshape in install_with_manager.rs; and the three new tests against local dumb-HTTP git + fake codeload servers. Also re-checked the override committish-only-change path against the two-pass fix — the third test pins it.

Extended reasoning...

Overview

Adds find_locked_git_package in PackageManagerEnqueue.rs and calls it from the Tag::Git and Tag::Github arms of enqueue_dependency_with_main_and_success_fn after get_package_id misses: when an identical dependency literal (same name hash, tag, version literal) is already bound in the loaded lockfile to a package whose resolution is the same repo/owner and resolution tag, that binding is reused instead of scheduling a clone/fetch. Skips update targets. In install_with_manager.rs the override and catalog re-resolution loops are reshaped to two passes (clear every affected resolution slot, then enqueue), so the reuse scan cannot see a sibling's not-yet-cleared binding from the old override. Three new tests in bun-lock.test.ts cover branch/bare/github reuse, the bun update bypass, and the changed-git-override-with-two-dependents case.

Security risks

Lockfile resolution is supply-chain-sensitive: a false reuse could pin a dependency to a commit the user did not intend. The guard set — exact literal match, resolution-tag match, owner match, repo match modulo the writer-added ssh:// prefix, plus upstream two-pass invalidation of changed overrides/catalogs — is designed to make reuse strictly a no-op relative to what re-fetching the same ref would return. I did not find a path where an attacker-controlled lockfile or package.json widens what gets reused beyond what was already trusted, but this is exactly the kind of invariant a maintainer should confirm.

Level of scrutiny

High. This is core bun install resolution logic; the failure mode is silent (wrong commit installed, lockfile looks fine). The first revision of this PR did carry a real regression (committish-only override change ignored when ≥2 slots share the name), now fixed with a covering test. The scp-form ssh:// normalization has no automated test (needs a real ssh transport, noted in the PR body). It also composes with two adjacent PRs (#37142, #36689) whose interaction the description asserts was verified locally.

Other factors

All prior review threads (mine and CodeRabbit's) are resolved; the author responded to each with a specific fix or a stated reason. Tests are hermetic (local Bun.serve for git dumb-HTTP and fake codeload; per-test cache dir; SIGKILL-of-git-child retry that restores counters). CI on the last two pushes hit prebuilt-download outages unrelated to this diff; build #90149 was in progress at review time. Given the subtlety of the reuse-authority argument and the two-pass ordering change to existing override/catalog handling, a human sign-off is warranted.

[robobun]

A deeper review pass surfaced one more regression, fixed in 3b5a140: with a cold cache under the isolated linker, the store entry for a git dependency waits on the locked commit's checkout id, but the resolve-phase re-enqueue after a clone derived the checkout from find_commit on the dependency's committish, which follows the ref. With a moved branch head the locked checkout never ran and the install hung forever (without the reuse the same flow floated the pin to the new head instead, and a clean-lockfile cold-cache install already hung before this PR). The re-enqueue now checks out the bound package's resolved commit, matching how the clone-failure drain and the hoisted installer key it.

New coverage in the same push: a cold-cache leg (moved head, wiped cache and node_modules, isolated linker; asserts termination, locked sha, locked content), the catalog variant of the changed-entry test, and the must-not-reuse boundary (changed ref literal consults the remote again).

[claude]

No new issues found; both concerns from the earlier pass (interleaved override/catalog invalidation, and the scp-form ssh:// prefix mismatch) are addressed in e91879a, and 3b5a140 adds the locked-commit checkout keying with a cold-cache test leg. This is still a non-trivial change to git/github resolution semantics in the package manager — it introduces a new lockfile-binding reuse path, restructures override/catalog invalidation into two passes, and changes how the post-clone re-enqueue derives its checkout commit — so it's worth a human look before landing.

What was reviewed:

• find_locked_git_package: literal/name-hash/tag match + repo/owner guard + ssh:// normalization mirrors the lockfile Formatter's is_scp_like_path branch; to_update skip matches Diff::generate's update-target test.
• Two-pass override/catalog loops in install_with_manager.rs: all affected slots cleared before any re-enqueue, so the reuse scan cannot see a stale sibling binding.
• bound_resolved in the post-clone path: reads resolutions[id], bounds-checks against packages.len(), and only substitutes when the bound resolution is Git with a non-empty resolved — falls through to find_commit otherwise.
• Test file: hermetic (local dumb-http git + fake codeload server), pinned per-test cache dir, staged installs to bound concurrent git children, SIGKILL retry restores counters to the pre-attempt baseline.

Extended reasoning...

Overview

Three files: PackageManagerEnqueue.rs gains find_locked_git_package (called from both the Tag::Git and Tag::Github arms after get_package_id misses) and a bound_resolved short-circuit in the post-clone re-enqueue path that checks out the bound package's locked commit rather than following the ref via find_commit. install_with_manager.rs restructures the overrides_changed and catalogs_changed loops into two passes (invalidate all matching slots, then enqueue). bun-lock.test.ts adds ~410 lines: four new tests plus makeTarball/makeGitFixture helpers.

Security risks

None identified. The change is a lockfile-reuse optimization gated by strict equality on the dependency's stored name hash, version tag, and version literal, plus the bound package's resolution tag and repo/owner. It cannot bind a dependency to a package from a different repository. The two-pass invalidation is strictly more conservative than before (it clears more state earlier). Tests use only local servers.

Level of scrutiny

High. This is core bun install resolution: a wrong reuse silently pins a git dependency to the wrong commit, and the checkout-keying change in 3b5a140 fixes what would otherwise be a hang under the isolated linker. Three real regressions were found and fixed across the review iterations (interleaved invalidation, scp-form repo mismatch, cold-cache checkout starvation), which suggests the interaction surface is subtle enough to merit a maintainer's read of the final shape — particularly the bound_resolved branch and its coupling to the isolated store's checkout-id keying in runTasks.rs.

Other factors

All prior review threads (mine, CodeRabbit's, and the comment-cop advisories) are resolved. Test coverage is thorough: reuse for bare/branch/github refs, the must-not-reuse boundary (changed literal), bun update still contacting the remote, override and catalog committish changes with two dependents each, and a cold-cache moved-head leg under the isolated linker. CI on the latest commit was still building when last reported. Not approving because the change is neither small nor mechanical and touches install-correctness paths.

[claude]

I reviewed this revision and found no new issues — all three earlier rounds of findings (the interleaved override/catalog invalidation, the scp ssh:// acceptance hang, the test timeouts and cold-cache staging) are addressed in the current diff. Because this changes core git-dependency resolution behavior (the literal-match reuse, the bound_resolved checkout keying in the post-clone re-enqueue, and the two-pass override/catalog invalidation), a human look at the final shape would still be worthwhile.

What was reviewed:

• find_locked_git_package: literal/name-hash match, repo/owner byte-equality guard (scp intentionally excluded), to_update gate matching Diff::generate.
• The bound_resolved branch: checked it keys the checkout on the bound package's locked sha while the URL still comes from dep.repo, so task ids stay consistent with the isolated store's waiter for byte-equal repos.
• Two-pass invalidation in install_with_manager.rs: confirmed both loops clear every affected slot before any enqueue, covered by the override and catalog tests.
• Tests: hermetic (local dumb-HTTP git + fake codeload), per-test timeouts on all four git-spawning tests, cold-cache stage now single-clone.

Extended reasoning...

Overview

The PR adds find_locked_git_package in PackageManagerEnqueue.rs so that on re-resolution (typically a workspace member edit), git and github dependencies whose committish is a branch/tag/bare ref reuse the package an identical dependency literal is already bound to in the loaded lockfile, instead of re-fetching. It also adds a bound_resolved branch in the post-clone re-enqueue that checks out the bound package's locked commit rather than following the ref via find_commit, and reshapes the override/catalog re-resolution loops in install_with_manager.rs to invalidate all affected slots before enqueueing any (so the reuse cannot rebind to a stale sibling). ~420 lines of new tests in bun-lock.test.ts cover the reuse, the bun update skip, the changed-override/catalog re-resolution, and a cold-cache isolated-linker hang.

Security risks

None identified. The change is an optimization on when to reuse an existing lockfile binding vs. contact the remote; it does not introduce new trust boundaries, and the byte-equal repo/owner guard prevents binding across redirected resolutions. Tests are hermetic (local servers only).

Level of scrutiny

High. This is core package manager resolution logic with cross-file invariants (task-id keying between enqueue_git_for_checkout, the isolated store waiter, and the post-clone re-enqueue; the lockfile writer's scp ssh:// normalization). The review history bears this out: two real regressions were surfaced and fixed during review (the interleaved invalidation letting a changed override silently reuse the old pin, and the scp ssh:// acceptance stranding the isolated store's checkout waiter). The final shape is defensible and covered by tests, but the design choices — literal-match as the reuse authority, byte-equal repo exclusion of scp, checking out the bound package's commit on re-enqueue — deserve a human maintainer's confirmation.

Other factors

All earlier automated findings are resolved and the corresponding threads are marked resolved. The four new tests carry explicit per-test timeouts (90s/60s), stage installs to keep concurrent git clones to one, and retry once on SIGKILL from a clean cache. CI status per the thread has been infra-flaky (WebKit prebuilt download failures), not test failures on this diff. Given the complexity and the iteration history, deferring to a human reviewer rather than auto-approving.