Conversation
|
Updated 2:44 AM PT - Aug 27th, 2026
❌ @robobun, your commit 3835677 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33156That installs a local version of the PR into your bun-33156 --bun |
|
Found 6 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPeer resolution now accepts non-registry package sources, folder packages can be reused, unresolved peer ranges remain loadable in lockfiles, and install tests cover the updated behavior. ChangesPeer Dependency and Folder Resolution Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/PackageManagerEnqueue.rs`:
- Around line 2311-2323: The fallback in PackageManagerEnqueue’s
`PackageIndexEntry::Ids` handling is running too early, so `list[0]` can be
returned before `resolution_provides_peer` has a chance to match a valid peer
provider. Update the peer-resolution flow around the `resolution_provides_peer`
scan and the `success_fn`/`ResolvedPackageResult` return so the helper is
checked before any fallback to the first registry entry, ensuring `file:`,
`git:`, or tarball providers can win when they satisfy the peer.
🪄 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: 57d1b9fa-b460-4a78-8aac-9adbe12cb0d0
📒 Files selected for processing (3)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/lockfile/Tree.rstest/cli/install/bun-lock.test.ts
There was a problem hiding this comment.
I didn't find any bugs, but this changes core peer-resolution semantics (accepting any non-registry resolution as satisfying an npm peer range) and reroutes folder placement through hoist_dependency, so it's worth a human look at the install/lockfile behavior.
Extended reasoning...
Overview
This PR touches three files: src/install/PackageManager/PackageManagerEnqueue.rs (adds resolution_provides_peer and calls it in two places inside get_or_put_resolved_package; adds a dedup-by-resolution check before appending a transitive folder package), src/install/lockfile/Tree.rs (routes Folder-resolved packages through hoist_dependency with the current node as its own hoist root instead of placing directly), and two new tests in test/cli/install/bun-lock.test.ts.
Security risks
None apparent. The change does not touch auth, network, or path handling; it only affects which already-resolved package entry a peer dependency binds to and how folder packages are deduped in the tree.
Level of scrutiny
High. This is core bun install resolution and lockfile-tree logic that runs on every install. The new resolution_provides_peer helper introduces a semantic rule — any Folder/Symlink/tarball/Git/Github resolution satisfies any npm-range or dist-tag peer, without version comparison — which is a deliberate policy choice (matching npm/pnpm behavior per the description) rather than a mechanical fix. The Tree.rs change replaces an unconditional Placement with a call into hoist_dependency, which now exposes folder packages to the Resolve/ResolveReplace/ResolveLater/DependencyLoop paths at the same node; the next_id == hoist_root_id argument should keep them from hoisting to a parent, but the additional code paths deserve a maintainer's eye.
Other factors
The PR is well-described with clear repros, root-cause analysis, and two focused tests that exercise offline resolution, --frozen-lockfile round-tripping, and second-install idempotence. The author reports the broader install/lock/workspace/peer suites are unchanged. The bug-hunting pass found nothing. Still, given how many edge cases peer resolution has (optional peers, workspace-provided peers, git peers, the "incorrect peer dependency" warn-and-use fallback ordering), I'd rather a human confirm the new acceptance rule and the folder→hoist_dependency reroute are the intended shape before this lands.
|
I checked all six against this branch. #26046: fixed, and now part of this PR. Reproduced on main with the issue's exact steps: The other five are different mechanisms and are not fixed here:
So only |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/install/lockfile/bun.lock.rs (2)
2818-2835: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoot-dependency peer unresolved handling not updated to match the new fix.
This loop resolving the root package's dependencies still only tolerates
Behavior::OPTIONALon lookup miss; it doesn't skip fordep.behavior.is_peer()the way the fix at line 2971 now does for nested package dependencies. If the rootpackage.jsondeclares a peer dependency that Bun already tolerated (unresolved) at install time, loading that lockfile will still hitdependency_resolution_failureand fail to parse here — the exact symptom this PR is fixing, just at the root level instead of nested packages.🐛 Proposed fix
let Some(&res_id) = pkg_map.get(dep.name.slice(string_buf)) else { - if dep.behavior.contains(Behavior::OPTIONAL) { + if dep.behavior.contains(Behavior::OPTIONAL) || dep.behavior.is_peer() { continue; }Since
verify_resolutionsinPackageManagerResolution.rsalready treats unresolved peers this way (failed_dep.behavior.is_peer()), this appears to be an existing, established tolerance pattern that this loop is simply missing.🤖 Prompt for 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. In `@src/install/lockfile/bun.lock.rs` around lines 2818 - 2835, The root dependency resolution loop still only skips unresolved OPTIONAL deps, so it must be updated to also tolerate unresolved peer deps like the nested-package fix does. In the dependency lookup miss branch in the root package dependency walk, add the same dep.behavior.is_peer() check used elsewhere (for example in verify_resolutions and the newer nested dependency handling) before calling dependency_resolution_failure, so unresolved peer dependencies are skipped instead of failing lockfile parsing.
2894-2910: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSame gap for workspace-package dependency resolution.
This workspace-dependency loop has the identical pattern as the root-dependency loop above: it only skips
dependency_resolution_failureforBehavior::OPTIONAL, not foris_peer(). A workspace package declaring an unresolved peer dependency (which the installer already tolerates at install time per the PR's stated behavior) would still fail to parse back from the lockfile.🐛 Proposed fix
else { - if dep.behavior.contains(Behavior::OPTIONAL) { + if dep.behavior.contains(Behavior::OPTIONAL) || dep.behavior.is_peer() { continue; }This is the same bug class fixed at line 2971 for the general package dependency loop; per the "fix the whole class in the same PR" guideline, sibling sites sharing this pattern should be updated together.
🤖 Prompt for 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. In `@src/install/lockfile/bun.lock.rs` around lines 2894 - 2910, The workspace dependency resolution path in the lockfile parser still rejects unresolved peer dependencies because it only skips failure for Behavior::OPTIONAL. Update the dependency loop that uses pkg_map and dependency_resolution_failure to also bypass the error when dep.is_peer() is true, matching the fix already applied in the general package dependency loop. Keep the behavior consistent for workspace packages so unresolved peers round-trip without ParseError::InvalidPackageInfo.
🤖 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.
Outside diff comments:
In `@src/install/lockfile/bun.lock.rs`:
- Around line 2818-2835: The root dependency resolution loop still only skips
unresolved OPTIONAL deps, so it must be updated to also tolerate unresolved peer
deps like the nested-package fix does. In the dependency lookup miss branch in
the root package dependency walk, add the same dep.behavior.is_peer() check used
elsewhere (for example in verify_resolutions and the newer nested dependency
handling) before calling dependency_resolution_failure, so unresolved peer
dependencies are skipped instead of failing lockfile parsing.
- Around line 2894-2910: The workspace dependency resolution path in the
lockfile parser still rejects unresolved peer dependencies because it only skips
failure for Behavior::OPTIONAL. Update the dependency loop that uses pkg_map and
dependency_resolution_failure to also bypass the error when dep.is_peer() is
true, matching the fix already applied in the general package dependency loop.
Keep the behavior consistent for workspace packages so unresolved peers
round-trip without ParseError::InvalidPackageInfo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c097e87a-ce0e-4116-b958-2acfac4f697f
📒 Files selected for processing (2)
src/install/lockfile/bun.lock.rstest/cli/install/bun-install.test.ts
|
8a00881 addresses the review feedback and the Windows CI failure from build 67393:
Locally: |
19286ae to
42f5599
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-install.test.ts`:
- Around line 1478-1481: The test in bun-install.test.ts only verifies that one
unresolved peer entry exists, so it does not prove both root and workspace peers
survived the round-trip. Tighten the assertions around the bun.lock contents by
checking for both unresolved peer records using the existing lockfile read in
the test, and keep the negative check for any resolved "bar@" entry so the test
fails if either peer entry is dropped. Use the surrounding install test case and
the lockfile assertions near the current expect(lockfile) calls to locate the
update.
🪄 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: 7a3fa23b-d6e9-4735-8135-78598954723d
📒 Files selected for processing (5)
src/install/lockfile/Package.rssrc/install/lockfile/bun.lock.rstest/cli/install/bun-add.test.tstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.ts
There was a problem hiding this comment.
No new issues found after the fixes in 8a00881/42f5599c — but this changes peer-resolution semantics, folder-package deduplication, and lockfile-parse tolerance across several coupled sites, plus updates existing test expectations, so it warrants a human pass (and CI build #67420 still shows musl build failures and Windows bun-add.test.ts failures).
Extended reasoning...
Overview
This PR touches four distinct pieces of the installer:
PackageManagerEnqueue.rs: newresolution_provides_peerhelper accepting non-registry resolutions (folder/symlink/tarball/git/github) as peer providers, plus dedup of transitive folder packages by (name, resolution) before appending.Tree.rs: folder-resolved packages now route throughhoist_dependency(withnext_idas the hoist root) instead of direct placement, so same-node dedup runs.bun.lock.rs: three dependency-resolution loops (root, workspace, package) now tolerate unresolved peers via a shareddependency_can_be_unresolvedhelper.Package.rs: Windows-only posix normalization of the relative folder path stored for transitivefile:deps.
Plus four new tests and updates to nine existing test expectations (N packages installed counts in bun-install-registry.test.ts and a Windows path assertion in bun-add.test.ts).
Security risks
None identified. No auth, crypto, network trust, or path-traversal surface is touched; the changes are confined to internal resolution/hoisting bookkeeping and lockfile round-tripping.
Level of scrutiny
High. This is core package-manager resolution logic where subtle ordering matters — e.g., resolution_provides_peer deliberately runs after the existing "incorrect peer dependency" fallback, and the Tree.rs change replaces an unconditional placement with a call that can now return Hoisted/Resolve/ResolveReplace/etc. The folder-dedup in enqueue_dependency changes when a new package entry is created vs. reused, which is why existing package-count assertions moved. Each piece looks correct in isolation and is well-tested, but the interaction surface (peer + folder + hoisting + lockfile round-trip) is exactly where installer regressions hide.
Other factors
- My two prior inline comments (unresolved-peer tolerance at only 1 of 3 parse sites; Windows backslash in folder resolutions) were both addressed with code fixes and new tests, and the threads are resolved.
- The robobun CI status for the latest reported build (#67420) shows musl build failures and Windows
bun-add.test.tsfailures — worth confirming those are green on the current head before merge. - Existing test expectations were updated (package counts 14→13, 13→12); the PR description explains why, and the on-disk-tree assertions in those tests are unchanged, but a maintainer should confirm the count change is expected and not masking a dropped install.
|
@robobun are you sure this is correct? peers are required and only optional when they have optional: true in peerDependenciesMeta right? (looking at the changes in bun.lock.rs) |
|
Right, and that part is unchanged: a peer is only optional via The bun.lock.rs change is only about what the parser does with a lockfile in which a required peer recorded no resolution. The installer already allows that state on purpose: That is #26046, and the same thing happens for any dependency carrying a peer range that matches nothing published ( So the parser now tolerates exactly what the resolver and the writer already tolerate, nothing more. If you would rather a required peer that resolves to nothing fail the install itself (what that TODO describes), I can do that instead or as a follow-up, but it is a behavior change (the |
290c7af to
0c54917
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/lockfile/Package.rs`:
- Around line 1820-1839: The Windows path append logic in the folder assignment
block is duplicated from the `Tag::Workspace | Folder | Symlink | Tarball`
handling in `parse_dependency`, so extract that inline/non-inline POSIX
normalization into a shared helper. Add a helper near
`Package`/`parse_dependency` (for example one that takes `StringBuilder` and
`folder_path` and returns the appended `String`) and update both call sites to
use it, keeping the existing `String::can_inline` and
`dangerously_convert_path_to_posix_in_place` behavior unchanged.
🪄 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: c01e5c17-b59c-4aa6-b95e-e074d8c79197
📒 Files selected for processing (8)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/lockfile/Package.rssrc/install/lockfile/Tree.rssrc/install/lockfile/bun.lock.rstest/cli/install/bun-add.test.tstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.tstest/cli/install/bun-lock.test.ts
|
CI status for the current head (6ce9858, rebased on main): 282 jobs pass, 2 fail, both unrelated to this diff.
The Every install suite is green on this head, in CI and locally: |
|
Another trigger for the duplicate // package.json
{ "dependencies": { "a": "file:./vendor/a" },
"devDependencies": { "a": "file:./vendor/a" } }The
Not opening a separate PR since the src fix is identical. |
0c54917 to
6ce9858
Compare
|
Rebased onto main (6ce9858) to clear the conflict with #32182, which rewrote the three dependency-resolution loops in Resolution: kept #32182's new Verified on the rebase: |
|
Still reproduces on 1.4.0-canary.1 (b66764f): the first test in this PR (peer provided by a root file: dependency) fails against that build with |
|
While working on #37289 (the migration half of the "install then --frozen-lockfile fails on an unchanged tree" problem) I independently reproduced two more user-visible failures that the Shape, no foreign lockfile involved: mkdir -p vendor/gamma vendor/delta packages/ws-a
cat > vendor/gamma/package.json <<'EOF'
{"name":"gamma","version":"1.0.0","peerDependencies":{"delta":"*"},"peerDependenciesMeta":{"delta":{"optional":true}},"optionalDependencies":{"delta":"file:../delta"}}
EOF
echo '{"name":"delta","version":"2.0.0"}' > vendor/delta/package.json
echo '{"name":"ws-a","version":"0.1.0","dependencies":{"delta":"file:../../vendor/delta"}}' > packages/ws-a/package.json
echo '{"name":"sandbox","version":"1.0.0","workspaces":["packages/ws-a"],"dependencies":{"gamma":"file:vendor/gamma"}}' > package.json
bun install && bun install --frozen-lockfileOn current main (and in a 60-iteration loop):
With the A regression test for this shape, in case it is useful here: install once, then assert |
alii
left a comment
There was a problem hiding this comment.
Requesting changes. The parser change and the posix folder path are fine. The tree half is not: every Folder-resolved edge is still placed under the package that depends on it, and the new peer binding now routes peers through that path.
- Tree.rs: a peer bound to a root file: package gets a second placement under the consumer. The PR's own root-provides case installs the host twice, an npm consumer gets a placement that never installs, and a root provider at file:../host makes install exit 1. The last two work on main.
- resolution_provides_peer binds by name to any Folder entry, including the stubs created for file: deps declared inside npm packages.
- The new bun-lock test dedupes its lockfile matches with new Set, which is what hides the extra placement.
6ce9858 to
ed8a997
Compare
00f1fda to
d5eb5e9
Compare
|
@alii and one more small one, d5eb5e9: the |
d5eb5e9 to
99fc3c4
Compare
|
@alii one more narrowing of the provider rule since your approval, now at 99fc3c4 (single commit, rebased on current main). Two more holes were found in the folder arm of
Each clause has a test in bun-install.test.ts that fails on the previous head (the |
99fc3c4 to
2d9c749
Compare
2d9c749 to
c172f8f
Compare
|
@alii rebased again, now c172f8f on current main (one commit, mergeable). What changed on the way, beyond the conflicts:
The description's Fix and Verification sections describe the PR as it stands now. CI on this head (build 97943): 177 jobs passed, no test failures; the two ":darwin: 14 aarch64 - test-bun" shards expired after waiting an hour for an agent, which is hitting about a third of today's builds on that lane. I have asked for those two jobs to be retried rather than pushing again. All review threads are resolved (your round-two ones were addressed before your later sign-off; I resolved them now so the list is clean). GitHub still shows your request-changes review from the 13th as the standing one, so a fresh approval is what it needs once you are happy with the final rule. |
There was a problem hiding this comment.
I reviewed c172f8f and the automated bug-hunting pass found no issues. Since the find_peer_provider / dedupes_onto_root_dependency rule has been narrowed several times after the earlier human approval, a fresh look at the final shape of that rule would be worthwhile before merge.
What was checked on this head:
- The two
dedupes_onto_root_dependencygaps flagged on the previous head — the missingis_enabled(root_features)on the root edge, and the same-name/different-package blocker at an intermediate — are both addressed in the current diff, with matching tests in bun-install.test.ts. - A candidate concern that
dedupes_onto_root_dependencyreads incomplete resolution state during concurrent enqueue was examined and ruled out. - The Tree.rs Folder-peer walk-up now aligns with the invariant
dedupes_onto_root_dependencyenforces (single install name, root places it, nothing else under that name).
Extended reasoning...
Overview
This PR changes peer-dependency resolution and tree hoisting in bun install so that a peer satisfied by a file: (folder/link) package binds to it instead of hitting the registry, and so that two edges of one package pointing at the same folder produce a single lockfile key. It touches PackageManagerEnqueue.rs (new find_peer_provider fallback and a file:-peer reuse block), lockfile.rs (three new helpers: declaring_package, resolution_of_dependency_named, dedupes_onto_root_dependency), Tree.rs (Folder edges now go through hoist_dependency with peers walking up to the hoist root and non-peers clamped to their own node), plus ~900 lines of tests across bun-install/bun-lock and one inverted expectation in the pnpm migration suite.
Security risks
None identified. The change is confined to how already-resolved local packages are matched against peer edges and where they land in the tree; it does not touch network fetching, tarball extraction, path validation, or credential handling. The only user-controlled input newly consulted is the dependency name hash and behavior bits already parsed from package.json.
Level of scrutiny
High. This is core install correctness logic — the exact path that determines which package satisfies a peer and where it is placed. The PR's own review history (14+ iterations, at least eight distinct correctness gaps found and fixed in the folder-provider rule alone: aliased providers, workspace-only providers, omitted root providers, nested declarers, aliased declarers, filtered sibling edges, filtered root declarer edges, same-name blockers) shows how sensitive the invariant is: find_peer_provider decides at enqueue time that the tree builder will later dedupe the peer onto a specific placement, and every case where that prediction is wrong either writes an uninstallable folder path or errors. That coupling between two phases is inherently fragile and warrants a maintainer's eyes on the final rule.
Other factors
- alii approved an earlier revision (ef3a32b), but the
find_peer_providerfolder arm and the entirety ofdedupes_onto_root_dependencyhave been rewritten several times since. robobun has flagged each change to alii, so a re-review is already expected. - No bugs were found on c172f8f. My previous findings on the prior head are verifiably addressed in the diff (the
root_featuresgate and theelse if dep.name_hash == install_name && resolutions[dep_id] != invalid_package_idclause are both present), and one candidate about reading resolutions during concurrent enqueue was ruled out by the verifier. - Test coverage is thorough: each clause of the provider rule has a positive and a negative test, and every lockfile-writing test round-trips through
--frozen-lockfile. The one changed existing expectation (pnpm-lock-v9) is explained and looks correct — the redundantwith/has-peer/peercopy is now deduped ontowith/peer. - The tarball/git arm of
find_peer_provider(accepted anywhere, no range check) was discussed and intentionally kept as-is for a follow-up; that is a design call the maintainer should confirm.
### Problem - `bun pm ls` without `--all` prints a root dependency once per entry the root has for it. A workspace `a` that is also in the root's `devDependencies` prints twice under a `node_modules (1)` header. So does a name declared in two groups (`typescript` in #8283's example). - Cause: `src/runtime/cli/package_manager_command.rs:644` prints the root's dependency list as is: one entry per declaration plus one per workspace member. `node_modules` has one folder per name. ### Fix - Stable sort the root's entries by name and drop the later entries of a name (`dedup_by`). `--trusted` filters the result. - Correct because one name is one `node_modules/<name>` folder, the same collapse `Tree::hoist_dependency` applies. The list is in the tree builder's order, so the entry kept is the one the tree kept. An alias has its own name and keeps its row. The listing does not read the stored tree, so an older `bun.lockb` prints as before (Notes). - Verified: `test/cli/install/bun-pm.test.ts`. Three new tests (bun.lock, bun.lockb, `--trusted`) fail on the released bun. One more pins that a root optional peer bound to a dependency's copy stays listed. On this repo's lockfile the only change is the removed `@types/bun` row. ### Background - Each package owns one range of `buffers.dependencies`. The root's range holds its declared dependencies plus one `Behavior::WORKSPACE` entry per workspace member, sorted by group and name. Entries of one name share one folder. - The hoisted tree (`buffers.trees`, `buffers.hoisted_dependencies`) models `node_modules`: one tree per folder, one entry per name. `--all` prints it and the header `(N)` counts its entries. The default listing prints only the root's range. <details><summary>Notes</summary> Repro without a registry: ```sh mkdir -p d/packages/a && cd d echo '{"name":"root","workspaces":["packages/*"],"devDependencies":{"a":"workspace:*"}}' > package.json echo '{"name":"a","version":"1.0.0"}' > packages/a/package.json bun install && bun pm ls ``` Released bun prints `a@workspace:packages/a` on two lines, three when `a` is also in `dependencies`. `bun pm ls --all` prints it once. `bun pm ls --trusted` with `trustedDependencies: ["a"]` prints it twice. In this repo `bun pm ls` prints `@types/bun@workspace:packages/@types/bun` twice. - The first version of this PR printed the tree's root folder filtered to the root's id range. A self-review found a case it drops: a root optional peer that a dependency provides. When the tree builder binds that peer late, it stores the folder entry under the provider's dependency id (`Tree.rs`, `ResolveReplace`). bun 1.4.0 re-hoists on load and on a second pass stores the root's own id, but a `bun.lockb` written by bun 1.3.x keeps the provider's id, and bun does not rewrite a lockfile that needs no changes. Checked with a `bun.lockb` written by bun 1.3.14 from the `optional-peer-hoist-provider` and `-target` registry fixtures: the tree version listed only the provider, the current version lists both, as 1.3.14 and the released 1.4.0 do. The new optional peer test covers the fresh lockfile shape of this case. - Dedupe by package id was rejected: `bar` and `bar-alias: npm:bar` share one package id but are two folders. The test keeps both rows. - The `--trusted` test uses bun.lock only. bun.lockb stores only hashes of `trustedDependencies`, and `has_trusted_dependency` does not trust a hash alone since #31339, so `--trusted` lists nothing from a bun.lockb. Released bun does the same. Not related to this change. - A real conflict (workspace `a` plus `"a": "file:..."` at the root) does not reach `pm ls` today: `bun install` writes a duplicate package path and the lockfile fails to load (the area of #33156). - #8283 asks for grouped output and `--json`. This PR removes the duplicate row in its example and nothing else, so it does not close that issue. - Open PRs that touch this branch and do not fix this: #38923 (filter by what is on disk), #38952 (header wording). The fold branch of #39403 has the same loop. - Suites run with the debug build: `test/cli/install/bun-pm.test.ts` (22 pass), `bun-install-lifecycle-scripts.test.ts -t trusted` (55 pass), `bun-install-registry.test.ts -t "migration is out of sync"`, `test/regression/issue/24502`. `cargo fmt --check` is clean. `bun pm ls --all` on this repo is unchanged byte for byte. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/install/bun-pm.test.ts bun test v1.4.0 (6e906e4) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [386.82ms] (pass) should list all dependencies [333.90ms] (pass) should list top-level aliased dependency [329.02ms] (pass) should list aliased dependencies [346.24ms] (pass) should list only trusted dependencies with --trusted [464.07ms] (pass) should list only trusted dependencies with --all --trusted [339.68ms] (pass) should list trusted transitive dependencies under untrusted parents with --all --trusted (isolated) [356.01ms] (pass) should list nothing with --trusted when no dependencies are trusted [346.58ms] 597 | expect(await exists(join(package_dir, lockfile))).toBeTrue(); 598 | urls.length = 0; 599 | 600 | const [stdout, stderr, exitCode] = await spawnAndCollect("pm", "ls"); 601 | expect(stderr).toBe(""); 602 | expect(normalizeBunSnapshot(stdout, package_dir)).toMatchInlineSnapshot(` ^ error: expect(received).toMatchInlineSnaps ... (truncated) release without fix: 3 FAILED bun test v1.4.0-canary.1 (6e906e4) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [17.59ms] (pass) should list all dependencies [15.04ms] (pass) should list top-level aliased dependency [18.13ms] (pass) should list aliased dependencies [26.89ms] (pass) should list only trusted dependencies with --trusted [29.53ms] (pass) should list only trusted dependencies with --all --trusted [30.36ms] (pass) should list trusted transitive dependencies under untrusted parents with --all --trusted (isolated) [19.13ms] (pass) should list nothing with --trusted when no dependencies are trusted [20.57ms] 597 | expect(await exists(join(package_dir, lockfile))).toBeTrue(); 598 | urls.length = 0; 599 | 600 | const [stdout, stderr, exitCode] = await spawnAndCollect("pm", "ls"); 601 | expect(stderr).toBe(""); 602 | expect(normalizeBunSnapshot(stdout, package_dir)).toMatchInlineSnapshot(` ^ error: expect(received).toMatchInlineSnapshot(expected) "<dir> node_modules (5) ├── bar@0.0.2 + ├── bar@0.0.2 ├── bar-alias@0.0.2 ├── ws-once@workspace:packages/ws-once + ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/cli/install/bun-pm.test.ts bun test v1.4.0 (6e906e4) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [381.86ms] (pass) should list all dependencies [333.18ms] (pass) should list top-level aliased dependency [351.49ms] (pass) should list aliased dependencies [347.67ms] (pass) should list only trusted dependencies with --trusted [463.21ms] (pass) should list only trusted dependencies with --all --trusted [348.99ms] (pass) should list trusted transitive dependencies under untrusted parents with --all --trusted (isolated) [321.16ms] (pass) should list nothing with --trusted when no dependencies are trusted [309.86ms] (pass) should list a workspace the root also depends on once (bun.lock) [541.88ms] (pass) should list a workspace the root also depends on once (bun.lockb) [314.14ms] (pass) should list a trusted workspace the root also depends on once with --trusted [315.39ms] (pass) should list a root optional peer that a dependency provides [328.74ms] (pass) should remove all cache [465.82ms] (pass) bun pm m ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 683ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/5] gen generated_host_exports.rs generated_host_exports.rs: 92 exports (host=3, lazy=10, generic=79, rust=0); 240 extern-C blocks audited [1/5] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19) �[1m�[92m Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) �[1m�[92m Compiling�[0m bun_bin v0.0.0 (/workspace/bun/src/bun_bin) �[1m�[92m Finished�[0m `release` profile [optimized + debuginfo] target(s) in 4m 36s [2/5] link bun-profile [4/5] strip bun [4/5] bun-profile --revision 1.4.0-canary.1+b263001b0 [build] done bun test v1.4.0-canary.1 (b263001) test/cli/install/bun-pm.test.ts: (pass) should list top-level dependency [15.40ms] (pass) should list all dependencies [11.19ms] (pass) should list top-level aliased dependency [9.40ms] (pass) should list aliased dependencies [9.71ms] (pass) should list only trusted dependencies with --trusted [10.18ms] (pass) should list on ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/cli/package_manager_command.rs | 8 +- test/cli/install/bun-pm.test.ts | 150 ++++++++++++++++++++++++++++- 2 files changed, 152 insertions(+), 6 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/cli/package_manager_command.rs 5 4 0 test/cli/install/bun-pm.test.ts 10 9 0 ``` </details> **root cause** · written by the author bot The root package's dependency list receives one entry per workspace member plus one per declared dependency, so a workspace that the root also declares as a dependency has two entries resolving to the same package, and the default bun pm ls loop printed each of them. The fix sorts the root's entries by name with a stable sort and then collapses adjacent entries with the same name, so each package is printed once, while the --trusted filtering in the same branch is unaffected. <!-- robobun:evidence:end -->
c172f8f to
4cb962d
Compare
|
@alii rebased once more onto today's main (4cb962d, one commit, mergeable). One conflict: #40014 (self-contained workspaces) adds a hoisting barrier the provider rule has to respect: a package that a self-contained workspace depends on gets its own copy inside that workspace's Everything else is unchanged. Locally, every install suite passes except four |
4cb962d to
213a32d
Compare
213a32d to
f09b280
Compare
|
@alii one more change since this morning's rebase, now f09b280 (rebased onto the newest main, still one commit, mergeable). Review found two more cases where the root-folder arm could bind something the installed tree cannot reach, and one of them needed a structural fix rather than another clause:
Both have tests in bun-install.test.ts that fail on the previous head. All install suites pass locally apart from main's own |
There was a problem hiding this comment.
I reviewed f09b280 and found no bugs. Given the new deferred-peer resolution loop introduced in this revision and the number of times the provider rule has needed narrowing, a human sign-off on the final shape is still warranted.
What was reviewed:
resolve_deferred_folder_peersloop termination — each round either binds all remaining peers or sends a strict subset to the registry;dedupes_onto_root_dependencyis monotone (true→false only) as edges arrive, and the package set is finite.dedupes_onto_root_dependencywhen the declarer is a self-contained workspace — workspace packages are Resolution::Workspace, sofind_peer_providernever reaches therootsarm for their own peers; the predicate is only queried for the workspace's dependents.peer_graph_completeis heldtrueacross theenqueue_dependency_with_maincalls in the final round so bound peers actually resolve to the folder rather than re-deferring.
Extended reasoning...
Overview
This PR changes how bun install resolves peer dependencies that a file: package could satisfy, and how folder-resolved edges are placed in the lockfile tree. It touches PackageManager state (two new fields), the enqueue path (get_or_put_resolved_package, new find_peer_provider/bind_peer_provider), the resolution wait loop (wait_for_resolution now iterates until deferred folder peers settle), three new Lockfile helpers (declaring_package, resolution_of_dependency_named, dedupes_onto_root_dependency, is_dependency_of_self_contained_workspace), and Tree::process_subtree (folder edges now go through hoist_dependency). ~1,200 lines of tests across three files pin each clause of the provider rule.
Security risks
None identified. The change is confined to dependency-graph resolution and tree placement; it does not touch network trust, integrity verification, path validation, or archive extraction. The new code reads only lockfile buffers already populated by existing parsing.
Level of scrutiny
High. This is core package-manager resolution logic where a wrong binding silently installs the wrong package (or nothing) under a consumer. The provider rule has been narrowed through eight rounds of review on this PR, each finding a shape where the previous revision bound a folder the installed tree could not reach. The latest revision introduces a new structural mechanism (deferred_folder_peers + the resolve_deferred_folder_peers fixpoint loop in wait_for_resolution) that had not been through prior review rounds. That mechanism's correctness depends on dedupes_onto_root_dependency being monotone under graph growth and on the loop terminating, both of which I traced but which deserve human confirmation.
Other factors
alii previously approved and then the rule was narrowed three more times; robobun has already asked for a fresh approval on f09b280. All prior automated-review threads are resolved with tests that fail on the previous head. The one existing test expectation that changed (pnpm-lock-v9.test.ts) is explained in the description and looks correct — a peer satisfied by an ancestor workspace's copy no longer gets a redundant nested placement. Test coverage is thorough (each clause of the rule has a positive and a negative test), and every install suite is reported passing locally. Deferring rather than approving because the resolution-loop change is new enough that a maintainer familiar with the peer-pass ordering should confirm the fixpoint is sound.
Squash of the reviewed commits, rebased on main. Parts of the original change landed separately while it was in review and are no longer in this diff: the bun.lock parser change (#38851) and the tree dedupe of a peer bound to a folder package (#40564). A self-contained workspace (#40014) is a hoisting barrier, so a package in its dependency closure does not take a root folder as its peer provider. Peers a root folder could satisfy are decided only once the dependency graph is complete. See the PR description for the full write-up.
f09b280 to
3835677
Compare
|
@alii rebased onto today's main (3835677, one commit, mergeable). Two more pieces of this PR landed independently while it waited, so the diff got smaller:
What is left is the |
|
Follow-up to my comment above: this PR no longer carries the |
Problem
A peer dependency that a
file:package provides is fetched from the registry anyway.npm and pnpm resolve this offline. The same happens for the optional-peer idiom where the consumer vendors its own copy (
peerDependencies+optionalDependencies: { "host": "file:../host" }), which is the original report.(Two other symptoms this PR originally covered landed as their own fixes while it was in review and are no longer in this diff: a required peer matching no published version made
bun.lockunloadable, #26046, fixed in #38851; and a dependency plus afile:peer on the same folder wrote the same package path twice, fixed by the tree dedupe in #40564.)Cause
get_or_put_resolved_packageonly accepts an existing package for a peer when the resolution kinds are comparable (npm/npm, git/git, github/github), so a folder package never satisfies an npm range and bun falls through to the registry.Fix
find_peer_provider: after every existing acceptance path has declined, an npm-range or dist-tag peer can be bound to a same-named package that came from a non-registry source:--omit, since the lockfile tree contains the edge either way (Lockfile::resolution_of_dependency_named).devDependenciesfolder is not a provider under--omit=dev/--production), and only when the peer's walk up the tree is certain to end at that copy: the install is not filtered (--filtercan leave the root's dependencies out), it places the declarer directly under the root, nothing installs any other package under the declarer's name, and no self-contained workspace (install: self-contained workspaces for the hoisted linker (workspaces.selfContained/installConfig.hoistingLimits) #40014, a hoisting barrier) depends on the declarer (Lockfile::dedupes_onto_root_dependency). Every dependency on the declarer then dedupes onto the root's copy, one level below the provider (the tree dedupe from install: dedupe a peer bound to a folder package instead of nesting a second copy #40564).find_peer_providerreturnsDeferred, the peer is parked instead of going to the registry, andwait_for_resolutiondecides the parked peers once the queue is empty and nothing is in flight (resolve_deferred_folder_peers). The check can only turn from true to false as edges arrive, so the peers it rejects go to the registry first, the rest stay parked while the graph settles, and the loop ends when a round binds everything.file:dependencies declared inside registry packages.Verification
test/cli/install/bun-lock.test.ts(each asserts the exactpackageskeys for the host, what is and is not nested, and that the lockfile round-trips:--frozen-lockfilepasses and a reinstall is a no-op; none of the hosts exist in the registry). Failing with main'ssrc/:file:dependency: one key, nothing under the consumerfile:../hostoutside the projectpeerDependenciesplusoptionalDependencies/dependencies: file:on the same name inside the plugin: one key under the pluginPassing with main's
src/as well (they pin thefile:-peer shapes #40564 now dedupes):dependenciesplus afile:peer on one folder gives one key; two packages depending on the same folder, one of them alsofile:-peering on it, each keep their own copy.test/cli/install/bun-install.test.ts, failing with main'ssrc/:file:dependency: only the consumer is requested, one key, nothing nested, round-tripsfile:devDependency satisfies a registry package's peer on a normal install (no registry lookup); under--omit=devthe same peer is resolved from the registry instead--omit=optionalwhile the root also provides a copy: the peer binds to the plugin's own vendored copy and the registry is not askedtest/cli/install/bun-install.test.ts, passing with main'ssrc/as well: each pins one clause of the rule above and fails when that clause is removed (in earlier revisions of this PR the last six bound the folder at a path where it does not install):optionalDependenciesplus afile:peer on one folder, installed with--omit=optional: one keyfile:fork installed under a different name than its manifest does not satisfy a peer on the manifest namefile:copy: the peer is reported unmet exactly as without the folder, and the plugin's own copy is what gets nested under it--omit=devso that another version takes the root slot and it gets nested: samebun install --filter=<member>, which leaves the root's dependencies out of node_modules: same, the registry copy lands at the rootbun-install-registry,bun-workspaces,bun-workspaces-self-contained,bun-add,bun-add-filter,bun-lockb,isolated-install,hoist,bun-dedupe,bun-update,bun-update-transitive,catalogs,bun-pm,bun-prune,frozen-lockfile-pruned,regression/issue/40561and themigration/suites pass unmodified.Related: #26046 (its lockfile symptom is fixed by #38851; the
file:shapes above are this PR).Earlier revisions
The first version deduped folder peers only at the consumer's own node and bound peers to any same-named folder entry, which review showed still placed a second copy of a root-provided host under the consumer (hidden by a test that deduplicated its lockfile matches) and merged unrelated
file:stubs declared by different registry packages. Later rounds narrowed the root arm offind_peer_providerstep by step (aliased providers, providers only a workspace member declares, omitted root providers, declarers that get nested for one reason or another, the incomplete graph during the peer pass) until it became the rule stated above; the constraint-pinning tests in the last list are the shapes found on the way. Three pieces this PR carried were landed independently and dropped here on rebase: the bun.lock parser tolerance for unresolved peers (#38851), the Windows path normalization for folder dependencies (#38333), and theTree.rschange that dedupes a peer bound to a folder package against the copy an ancestor already provides (#40564), together with the entry reuse forfile:peers it made unnecessary.no test proof · iteration 19 · 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, test/cli/install/bun-install.test.ts