install: resolve overrides that point at a tarball with a mismatched internal name#33194
install: resolve overrides that point at a tarball with a mismatched internal name#33194robobun wants to merge 5 commits into
Conversation
…internal name
When an overrides/resolutions entry points a name at an HTTP(S) or local
tarball, the extracted package is indexed under the tarball's own
package.json name. The override edge kept the original name hash when
re-enqueuing, so get_package_id could not find the extracted package and
the dependency (or any transitive peer relying on it) failed to resolve:
error: vite@^5.0.0 failed to resolve
The direct-dependency path already back-propagates the extracted name
onto the stored tarball version; the override path skipped it because the
stored version keeps its original tag. Record the extracted name on the
override entry in that case, and derive the replacement name hash from the
tarball's own name once it is known, so resolution finds the package under
the key the user overrode (matching npm and pnpm).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR updates tarball override name-hash handling, writes extracted package names back to the correct lockfile location for overrides, and adds install tests for mismatched tarball names across remote, local, peer, and regression cases. ChangesTarball Override Name-Hash Fix
Possibly related issues
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/install/PackageManager/runTasks.rs`:
- Around line 1222-1235: The fallback arm in the `target`/`version.tag` match
inside `runTasks` is silently swallowing a state that should not happen. Update
the `if let Some(version) = target` handling for
`bun_install::DependencyVersionTag::Git`, `Github`, and `Tarball` to add a
`debug_assert!` or `scoped_log!` in the `_` branch so mismatched or stale
override entries are surfaced instead of ignored.
In `@test/cli/install/bun-install.test.ts`:
- Around line 9862-10035: These install cases are independent and should run
concurrently instead of serially. Update the surrounding `describe` block in
`bun-install.test.ts` to use `describe.concurrent` (or mark each `test` as
`test.concurrent`) so the four install scenarios can execute in parallel; keep
the existing `tempDir`, `serveTarballs`, `Bun.spawn`, and `npmTarball` setup
unchanged since each test already isolates its own state.
- Around line 9871-9891: The local npmTarball helper in bun-install.test.ts
duplicates tarball construction already used by the install tarball tests. Reuse
or extract the shared tarball-building logic from the existing install suite
helpers, and update this test to call that shared helper instead of maintaining
its own header assembly and gzip packaging.
🪄 Autofix (Beta)
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: 843c2e27-d4a6-40d0-bd88-67bb793f9d2e
📒 Files selected for processing (3)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/PackageManager/runTasks.rstest/cli/install/bun-install.test.ts
|
The diff is green; CI is red only on unrelated flaky tests. This change is install-only ( The telling part is that the two runs failed on different tests, which is the signature of pre-existing flakes rather than a deterministic break:
All of these are GC/leak/timeout/SSR/infra flakes in subsystems an install-resolution change cannot touch. Ready for a maintainer to merge or re-run CI. |
What
Fixes #33192. A dependency or
overrides/resolutionsentry that points a name at an HTTP(S) or local tarball is now installed under the dependency key even when the tarball's ownpackage.jsonnamefield differs, matching npm and pnpm.Repro
In graphs where the only
viteedge is a transitive peer (vitest -> @vitest/mocker),bun installinstead exits 0 and silently dropsviterather than erroring.Cause
When a tarball is extracted, the resulting package is indexed in the lockfile's package index under the tarball's own
package.jsonname (@scope/vite-core). Resolution then looks the package up by name hash.hash("@scope/vite-core")and finds the package._ => {}), andupdate_name_and_name_hash_from_version_replacementreturned the original name hash (hash("vite")).get_package_id(hash("vite"), ...)could not find the package indexed underhash("@scope/vite-core"), so the edge never resolved.Fix
Two changes, both in the hoisted installer's tarball-extract path:
runTasks.rs: when the completed tarball's waiting dependency edge keeps its original (non-tarball) tag, it was redirected byoverrides/resolutions, so record the extracted name on the override entry (keyed by the original name hash) instead of the stored version.PackageManagerEnqueue.rs: once a git/github/tarball replacement's package name is known, derive the replacement's name hash from that name so resolution finds the package under the tarball's own name. While the name is still unknown (pre-extract), the original name hash is kept so the override stays keyed by its original name.The directory name stays the overridden key (
node_modules/vite), sinceassign_resolutiononly overwrites a dependency's name when it is empty or equals the version literal.Catalog replacements are unchanged: their package name is never back-propagated, so the new helper returns the original hash (its existing behavior).
Scope
overridesthat resolve to a git URL (git+..., cloned rather than tarball-extracted) are intentionally left out of this PR. They take a different completion path (Task::Tag::GitCheckout) with a pre-existing wrong-union-arm write for override edges, and git clones cannot be reproduced hermetically in the test harness, so that sibling is a follow-up. GitHub URLs are covered here because they download as tarballs through the same extract path.Tests
test/cli/install/bun-install.test.ts, serving in-process tarballs over HTTP and from a local path:The three mismatched-name tests fail on the current binary (two with
vite@^5.0.0 failed to resolve, the transitive-peer one by silently not installingvite) and pass with the fix; the matching-name guard passes both ways.