install: fix bun add ignoring explicit version when name is a peerDep#30855
install: fix bun add ignoring explicit version when name is a peerDep#30855ramonclaudio wants to merge 1 commit into
Conversation
f927d29 to
a44a61e
Compare
WalkthroughThis PR refactors peer dependency resolution to use the normal find_best_version flow (removing an order-dependent early-match) and moves deduplication and incorrect-peer warnings into hoisting. It also adds a did_retry_from_manifests flag to bound manifest-cache-control retries and updates tests to match the new behavior. ChangesPeer Dependency Resolution and Hoisting Refactor
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
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 `@src/install/lockfile/Tree.rs`:
- Around line 1104-1123: The current has_satisfying_non_peer does an
O(total_deps) scan over builder.dependencies/builder.resolutions for each peer;
replace this hot-path scan by enumerating package candidates via
lockfile.package_index for dependency.name_hash and checking only those package
IDs' resolutions in pkg_resolutions for Tag::Npm and range.satisfies, thereby
bounding work to the number of versions for that package; additionally add a
short-circuit so you only run the candidate check when
pkg_resolutions[package_id] itself does not already satisfy the range (avoid
work on the happy path); update Builder construction if needed to ensure access
to lockfile.package_index or maintain a name_hash → package_id list to reuse
across checks.
- Around line 1134-1144: The warning message currently logged by
builder.log.add_warning_fmt only shows the resolved package (names[res_id],
resolutions[res_id]) and omits the peer's requested range; update the call to
include the peer dependency's requested name and version/range (use
dependency.name.fmt(buf) and dependency.version.literal.fmt(buf) or
dependency.version.npm().version.fmt(buf)) so the message reads like "<pkg>
requires a peer of <requested-range> but <resolved>@<version> is installed";
keep the same logging call (builder.log.add_warning_fmt) and append the
dependency fields into the format_args invocation.
- Around line 1088-1147: The outer guard lets Git/Github peers through but the
has_satisfying_non_peer check only runs for npm tags, which makes the intent
unclear; update the logic in the peer-handling block (the if using
existing_tag/target_tag and crate::resolution::Tag) so the behavior is explicit:
either restrict the outer guard to Tag::Npm only (remove Git/Github from that
condition) or introduce an explicit branch that sets has_satisfying_non_peer =
false for non-Npm tags and only performs the npm satisfiability scan when
dependency.version.tag == VersionTag::Npm; adjust the warning/return path that
uses BuilderMethod::Resolvable, builder.lockfile, and
HoistDependencyResult::Hoisted accordingly.
In `@test/cli/install/bun-install-registry.test.ts`:
- Around line 4045-4066: The test "dependencies + peerDependencies on same name
does not loop on missing version" currently only inspects stderr for "Saved
lockfile" which can miss a silently written lockfile; after the spawn completes
(after awaiting stdout, stderr, exited) add an explicit artifact assertion that
the lockfile was not created by checking that the lockfile file (e.g.,
path.join(packageDir, "bun.lockb") or whatever lockfile name is used in this
suite) does not exist (use fs.existsSync or an async equivalent) and assert
false, e.g., expect(exists).toBe(false), before asserting the exit code and
stderr contents. Ensure you reference packageDir and the lockfile filename used
elsewhere in the tests.
🪄 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: c931d2fd-ee42-483b-9780-998871353e7b
📒 Files selected for processing (6)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/lockfile/Tree.rstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.tstest/cli/install/bunx.test.tstest/cli/install/test-dev-peer-dependency-priority.test.ts
| let _ = builder.log.add_warning_fmt( | ||
| None, | ||
| bun_ast::Loc::EMPTY, | ||
| format_args!( | ||
| "incorrect peer dependency \"{}@{}\"", | ||
| names[res_id as usize].fmt(buf), | ||
| resolutions[res_id as usize] | ||
| .fmt(buf, bun_core::fmt::PathSep::Auto), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
"incorrect peer dependency" warning omits the requested peer range.
The message format only reports the resolved package name and version (names[res_id] / resolutions[res_id]), not the peer's requested version range. npm and pnpm both report something closer to "<pkg> requires a peer of <range> but <resolved> is installed", which makes the mismatch actionable without grepping the lockfile.
Consider including dependency.name.fmt(buf) / dependency.version.literal.fmt(buf) (or dependency.version.npm().version.fmt(...)) so users can see both sides of the mismatch:
📝 Suggested message format
let _ = builder.log.add_warning_fmt(
None,
bun_ast::Loc::EMPTY,
format_args!(
- "incorrect peer dependency \"{}@{}\"",
+ "incorrect peer dependency \"{}@{}\" (requested \"{}\")",
names[res_id as usize].fmt(buf),
resolutions[res_id as usize]
.fmt(buf, bun_core::fmt::PathSep::Auto),
+ dependency.version.literal.fmt(buf),
),
);🤖 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/Tree.rs` around lines 1134 - 1144, The warning message
currently logged by builder.log.add_warning_fmt only shows the resolved package
(names[res_id], resolutions[res_id]) and omits the peer's requested range;
update the call to include the peer dependency's requested name and
version/range (use dependency.name.fmt(buf) and
dependency.version.literal.fmt(buf) or
dependency.version.npm().version.fmt(buf)) so the message reads like "<pkg>
requires a peer of <requested-range> but <resolved>@<version> is installed";
keep the same logging call (builder.log.add_warning_fmt) and append the
dependency fields into the format_args invocation.
| test("dependencies + peerDependencies on same name does not loop on missing version", async () => { | ||
| await writeFile( | ||
| packageJson, | ||
| JSON.stringify({ | ||
| name: "foo", | ||
| dependencies: { "no-deps": "9.9.9" }, | ||
| peerDependencies: { "no-deps": "^1" }, | ||
| }), | ||
| ); | ||
|
|
||
| const { stdout, stderr, exited } = spawn({ | ||
| cmd: [bunExe(), "install"], | ||
| cwd: packageDir, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env, | ||
| }); | ||
| const [out, err, code] = await Promise.all([stdout.text(), stderr.text(), exited]); | ||
| expect(err).toContain("No version matching"); | ||
| expect(err).not.toContain("Saved lockfile"); | ||
| expect(code).not.toBe(0); | ||
| }); |
There was a problem hiding this comment.
Assert lockfile non-creation directly in the missing-version failure case.
Line 4064 only checks stderr text (Saved lockfile), which can miss silent lockfile writes. Add an explicit artifact assertion after failure.
Suggested assertion hardening
const [out, err, code] = await Promise.all([stdout.text(), stderr.text(), exited]);
expect(err).toContain("No version matching");
expect(err).not.toContain("Saved lockfile");
expect(code).not.toBe(0);
+ expect(await exists(join(packageDir, "bun.lock"))).toBeFalse();
+ expect(await exists(join(packageDir, "bun.lockb"))).toBeFalse();🤖 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 `@test/cli/install/bun-install-registry.test.ts` around lines 4045 - 4066, The
test "dependencies + peerDependencies on same name does not loop on missing
version" currently only inspects stderr for "Saved lockfile" which can miss a
silently written lockfile; after the spawn completes (after awaiting stdout,
stderr, exited) add an explicit artifact assertion that the lockfile was not
created by checking that the lockfile file (e.g., path.join(packageDir,
"bun.lockb") or whatever lockfile name is used in this suite) does not exist
(use fs.existsSync or an async equivalent) and assert false, e.g.,
expect(exists).toBe(false), before asserting the exit code and stderr contents.
Ensure you reference packageDir and the lockfile filename used elsewhere in the
tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Tree.rs`:
- Around line 1097-1145: The current condition allowing relaxed peer-collapse
for Tag::Git and Tag::Github (in the block comparing existing_tag and target_tag
and checking Tag::Npm/Tag::Git/Tag::Github) is too permissive; restrict this
path to Npm-only to avoid collapsing different git sources/refs. Change the
if-condition that currently checks (existing_tag == crate::resolution::Tag::Npm
|| crate::resolution::Tag::Git || crate::resolution::Tag::Github) to only allow
crate::resolution::Tag::Npm, or alternatively add an exact-source+ref equality
check for Git/Github before entering the hoist/warning path (refer to symbols
existing_tag, target_tag, pkg_resolutions, dependency, and
has_satisfying_non_peer).
In `@test/cli/install/bun-install-registry.test.ts`:
- Around line 4068-4103: The test "bun add exact version when root also has
peerDependency on same name" currently only verifies node_modules; add an
assertion that the project's package.json (referenced by packageJson) records
the exact dependency string "1.0.0" under dependencies. After the existing
expect that node_modules/no-deps/package.json has version "1.0.0" (i.e., after
the final toMatchObject), load the package.json (using the existing test
helpers, e.g., await file(packageJson).json()) and assert that the dependencies
field contains "no-deps": "1.0.0" to ensure the manifest was pinned. Ensure this
check runs after the exited/err checks so it verifies the post-install state.
🪄 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: e8fd9ade-f74b-4f97-99cf-8d0dc9418cdc
📒 Files selected for processing (6)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/lockfile/Tree.rstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.tstest/cli/install/bunx.test.tstest/cli/install/test-dev-peer-dependency-priority.test.ts
| let existing_tag = pkg_resolutions[res_id as usize].tag; | ||
| let target_tag = pkg_resolutions[package_id as usize].tag; | ||
| if existing_tag == target_tag | ||
| && (existing_tag == crate::resolution::Tag::Npm | ||
| || existing_tag == crate::resolution::Tag::Git | ||
| || existing_tag == crate::resolution::Tag::Github) | ||
| { | ||
| let has_satisfying_non_peer = | ||
| dependency.version.tag == crate::dependency::VersionTag::Npm && { | ||
| let range = &dependency.version.npm().version; | ||
| builder | ||
| .dependencies | ||
| .iter() | ||
| .zip(builder.resolutions.iter()) | ||
| .any(|(other_dep, &other_res_id)| { | ||
| other_dep.name_hash == dependency.name_hash | ||
| && !other_dep.behavior.is_peer() | ||
| && (other_res_id as usize) < pkg_resolutions.len() | ||
| && pkg_resolutions[other_res_id as usize].tag | ||
| == crate::resolution::Tag::Npm | ||
| && range.satisfies( | ||
| pkg_resolutions[other_res_id as usize].npm().version, | ||
| builder.buf(), | ||
| builder.buf(), | ||
| ) | ||
| }) | ||
| }; | ||
| if !has_satisfying_non_peer { | ||
| if METHOD == BuilderMethod::Resolvable { | ||
| // PORT NOTE: mirrors the borrowck reshape below. | ||
| // Copy the ParentRef so the &Lockfile detaches | ||
| // from &builder before taking &mut builder.log. | ||
| let lockfile_ref = builder.lockfile; | ||
| let lockfile: &Lockfile = lockfile_ref.get(); | ||
| let buf = lockfile.buffers.string_bytes.as_slice(); | ||
| let names = lockfile.packages.items_name(); | ||
| let resolutions = lockfile.packages.items_resolution(); | ||
| let _ = builder.log.add_warning_fmt( | ||
| None, | ||
| bun_ast::Loc::EMPTY, | ||
| format_args!( | ||
| "incorrect peer dependency \"{}@{}\"", | ||
| names[res_id as usize].fmt(buf), | ||
| resolutions[res_id as usize] | ||
| .fmt(buf, bun_core::fmt::PathSep::Auto), | ||
| ), | ||
| ); | ||
| } | ||
| return Ok(HoistDependencyResult::Hoisted); // 1 |
There was a problem hiding this comment.
Restrict the relaxed peer-collapse path to npm resolutions.
On Lines 1099-1145, Git/Github peers are treated as compatible on tag equality alone. Because has_satisfying_non_peer is npm-only, any same-name Git/Github mismatch falls into the warning-and-Hoisted path even when res_id != package_id. That can collapse two different git URLs/refs onto the ancestor package. The exact-match case is already handled above by res_id == package_id, so this relaxed mismatch collapse should stay npm-only unless you also compare the Git/Github source+ref before hoisting.
Suggested minimal change
- if existing_tag == target_tag
- && (existing_tag == crate::resolution::Tag::Npm
- || existing_tag == crate::resolution::Tag::Git
- || existing_tag == crate::resolution::Tag::Github)
- {
- let has_satisfying_non_peer =
- dependency.version.tag == crate::dependency::VersionTag::Npm && {
+ if existing_tag == crate::resolution::Tag::Npm
+ && target_tag == crate::resolution::Tag::Npm
+ && dependency.version.tag == crate::dependency::VersionTag::Npm
+ {
+ let has_satisfying_non_peer = {
let range = &dependency.version.npm().version;
builder
.dependencies🤖 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/Tree.rs` around lines 1097 - 1145, The current condition
allowing relaxed peer-collapse for Tag::Git and Tag::Github (in the block
comparing existing_tag and target_tag and checking
Tag::Npm/Tag::Git/Tag::Github) is too permissive; restrict this path to Npm-only
to avoid collapsing different git sources/refs. Change the if-condition that
currently checks (existing_tag == crate::resolution::Tag::Npm ||
crate::resolution::Tag::Git || crate::resolution::Tag::Github) to only allow
crate::resolution::Tag::Npm, or alternatively add an exact-source+ref equality
check for Git/Github before entering the hoist/warning path (refer to symbols
existing_tag, target_tag, pkg_resolutions, dependency, and
has_satisfying_non_peer).
| test("bun add exact version when root also has peerDependency on same name", async () => { | ||
| await writeFile( | ||
| packageJson, | ||
| JSON.stringify({ | ||
| name: "foo", | ||
| peerDependencies: { "no-deps": "^1" }, | ||
| }), | ||
| ); | ||
|
|
||
| let { stderr, exited } = spawn({ | ||
| cmd: [bunExe(), "install"], | ||
| cwd: packageDir, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env, | ||
| }); | ||
| expect(await exited).toBe(0); | ||
| expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toMatchObject({ | ||
| version: "1.1.0", | ||
| }); | ||
|
|
||
| ({ stderr, exited } = spawn({ | ||
| cmd: [bunExe(), "add", "no-deps@1.0.0"], | ||
| cwd: packageDir, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| env, | ||
| })); | ||
| const err = await stderr.text(); | ||
| expect(err).not.toContain("error:"); | ||
| expect(await exited).toBe(0); | ||
| expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toMatchObject({ | ||
| version: "1.0.0", | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Assert package.json is pinned to the exact added version.
The test title claims exact-version add behavior, but it only checks node_modules. Add an assertion that dependencies["no-deps"] === "1.0.0" in package.json.
Suggested assertion addition
const err = await stderr.text();
expect(err).not.toContain("error:");
expect(await exited).toBe(0);
+ expect(await file(packageJson).json()).toMatchObject({
+ dependencies: {
+ "no-deps": "1.0.0",
+ },
+ });
expect(await file(join(packageDir, "node_modules", "no-deps", "package.json")).json()).toMatchObject({
version: "1.0.0",
});🤖 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 `@test/cli/install/bun-install-registry.test.ts` around lines 4068 - 4103, The
test "bun add exact version when root also has peerDependency on same name"
currently only verifies node_modules; add an assertion that the project's
package.json (referenced by packageJson) records the exact dependency string
"1.0.0" under dependencies. After the existing expect that
node_modules/no-deps/package.json has version "1.0.0" (i.e., after the final
toMatchObject), load the package.json (using the existing test helpers, e.g.,
await file(packageJson).json()) and assert that the dependencies field contains
"no-deps": "1.0.0" to ensure the manifest was pinned. Ensure this check runs
after the exited/err checks so it verifies the post-install state.
|
@coderabbitai. All five findings were reviewed against #29804's design. This PR is a 1:1 Rust port of the work @dylan-conway already made.
If maintainers want any of these tightened post-merge, glad to file follow-ups. |
|
Tip For best results, initiate chat on the files or code changes.
If you'd like to track any of them for future work, I can open follow-up GitHub issues now. The candidates with the clearest improvement path seem to be:
Just say the word and I'll file them. Otherwise, happy to leave the PR as-is. ( ˶ᵔ ᵕ ᵔ˶ ) 🐇 ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes.
If you'd like, I can open follow-up GitHub issues now for the two items with a concrete improvement path:
Just say the word and I'll file them against ( ˶ᵔ ᵕ ᵔ˶ ) 🐇 ✏️ Learnings added
|
a44a61e to
e71026c
Compare
e71026c to
8a066b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/bunx.test.ts`:
- Line 1177: Update the test's bundler install command to use a real published
TypeScript version: replace the string "typescript@5.0.4" in the cmd array (the
line using cmd: [bunExe(), "add", "typescript@5.0.4"]) with a released version
such as "typescript@5.0.3" (or "typescript@5.0.2") so the bunExe() add command
uses an actually available package version.
In `@test/cli/install/test-dev-peer-dependency-priority.test.ts`:
- Around line 221-232: Create a tracked GitHub issue for the TODO in the test
(referencing the regression described around getOrPutResolvedPackage and
findBestVersion) that captures the optimization goal: prefer same-name workspace
members over registry candidates for peer ranges (even when strict-semver does
not match) and the proposed approach to change findBestVersion; then update the
test.todo comment in test-dev-peer-dependency-priority.test.ts to include the
issue number (e.g., // TODO(`#NNNN`): ...) and a one-line summary so the future
change is discoverable and linked to the issue; reference symbols
getOrPutResolvedPackage, findBestVersion, and package_index in the issue body to
preserve context.
🪄 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: 7490b69a-c13e-47c7-93c6-fc42fe0c4cc6
📒 Files selected for processing (6)
src/install/PackageManager/PackageManagerEnqueue.rssrc/install/lockfile/Tree.rstest/cli/install/bun-install-registry.test.tstest/cli/install/bun-install.test.tstest/cli/install/bunx.test.tstest/cli/install/test-dev-peer-dependency-priority.test.ts
|
|
||
| const subprocess1 = spawn({ | ||
| cmd: [bunExe(), "add", "typescript@5.0.0"], | ||
| cmd: [bunExe(), "add", "typescript@5.0.4"], |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does TypeScript version 5.0.4 exist on npm?
💡 Result:
No, TypeScript version 5.0.4 does not exist on npm. While TypeScript 5.0.2 and 5.0.3 were released, the version 5.0.4 was skipped in the official release sequence [1].
Citations:
Change typescript version to an actual released version (5.0.3 or 5.0.2).
The test references typescript@5.0.4, which does not exist on npm. TypeScript versions 5.0.2 and 5.0.3 were released, but 5.0.4 was skipped in the official release sequence. This will cause the test to fail when npm cannot locate the version. Update to typescript@5.0.3 or typescript@5.0.2.
🤖 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 `@test/cli/install/bunx.test.ts` at line 1177, Update the test's bundler
install command to use a real published TypeScript version: replace the string
"typescript@5.0.4" in the cmd array (the line using cmd: [bunExe(), "add",
"typescript@5.0.4"]) with a released version such as "typescript@5.0.3" (or
"typescript@5.0.2") so the bunExe() add command uses an actually available
package version.
| // Known regression after the order-dependent peer-dep early-match was removed | ||
| // from `getOrPutResolvedPackage`. Peers now flow through `findBestVersion`, so | ||
| // when devDep pins to a workspace canary (`next@15.0.0-canary.119`) and the | ||
| // peer range is `^14.0.0 || ^15.0.0`, the canary does not satisfy `^15.0.0` | ||
| // under strict semver, and `findBestVersion` falls back to the registry. The | ||
| // previous deleted block bound the peer to whichever same-name resolution was | ||
| // already in `package_index` (including the workspace member), avoiding the | ||
| // fetch. Restoring this optimisation without re-introducing the order | ||
| // dependence Dylan flagged in #29804 needs a separate change to | ||
| // `findBestVersion` (probably: prefer same-name workspace members over | ||
| // registry candidates for peer ranges, regardless of strict-semver fit). | ||
| test.todo("Next.js monorepo scenario should not make unnecessary network requests", async () => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider creating a tracked issue for the TODO test.
The detailed comment explains the known regression well: peer resolution now flows through findBestVersion, causing workspace canary versions to fall back to the registry when they don't satisfy strict semver. The PR summary confirms this optimization loss is out-of-scope for the determinism fix.
However, the comment states "needs a separate change" without referencing a GitHub issue. Should a tracking issue be created so this optimization work isn't lost? The comment could then reference it (e.g., // TODO(#NNNNN): restore ...).
Would you like me to help draft an issue description based on this comment?
🤖 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 `@test/cli/install/test-dev-peer-dependency-priority.test.ts` around lines 221
- 232, Create a tracked GitHub issue for the TODO in the test (referencing the
regression described around getOrPutResolvedPackage and findBestVersion) that
captures the optimization goal: prefer same-name workspace members over registry
candidates for peer ranges (even when strict-semver does not match) and the
proposed approach to change findBestVersion; then update the test.todo comment
in test-dev-peer-dependency-priority.test.ts to include the issue number (e.g.,
// TODO(`#NNNN`): ...) and a one-line summary so the future change is discoverable
and linked to the issue; reference symbols getOrPutResolvedPackage,
findBestVersion, and package_index in the issue body to preserve context.
0f3f753 to
863e3df
Compare
Removes the install_peer && behavior.is_peer() block from get_or_put_resolved_package (PackageManagerEnqueue.rs) and the resolution_satisfies_dependency helper that only that block called. Peers now flow through findBestVersion. Dedup and the "incorrect peer dependency" warning live in Tree::hoist_dependency where placement is deterministic. The deleted block scanned lockfile.package_index and bound peers to whichever already-resolved version of the same name appeared first. package_index is populated as manifest download tasks complete, which is whatever order the thread pool finishes them, so the same install produced different bun.lock contents run to run. Also bounds the 'retry_from_manifests_ptr cache-control retry to a single trip so a stale-but-fresh manifest paired with a missing resolution can't ping-pong forever after the early-match removal. Tree::hoist_dependency gains a symmetric peer-collapse check (either side may be a peer) and a has_satisfying_non_peer scan that prevents force-collapsing a peer onto a wrong-major resolution when a satisfying non-peer dep exists elsewhere in the lockfile. Rust port of Dylan Conway's oven-sh#29804 (Zig, stuck pre-rewrite).
863e3df to
8c23100
Compare
Fixes a user-visible bug:
bun add X@versionwas silently ignored whenXwas apeerDependencyon the same name.package.jsonupdated to the new version whilebun.lockandnode_modulesstayed pinned to whatever the peer had resolved to first.Repro:
Rust port of Dylan's #29804 with the additional observable fix above. The underlying change removes the order-dependent peer-dep early match in
get_or_put_resolved_packagethat bound peers to whichever same-name resolutionlockfile.package_indexheld first.package_indexis populated by manifest tasks completing in thread-pool order, so the same install can produce differentbun.lockcontents run to run. With the early match gone, peers now flow throughfind_best_versionand dedup happens inTree::hoist_dependencywhere tree-walk order is fixed.Changes
get_or_put_resolved_packagefind_best_versionlike any other depTree::hoist_dependencyso an incoming non-peer collapses into an already-placed peer at the same level when range satisfieshas_satisfying_non_peerguard so an unsatisfied peer reaching a same-name ancestor only collapses when no satisfying version exists elsewhere in the lockfile (yarn v1 and pnpm semantics, but doesn't nestajv@8underschema-utilswhen root hasajv@6)'retry_from_manifests_ptrloop to a single trip viadid_retry_from_manifestsso a stale-but-cache-control-fresh manifest paired with a missing resolution can't ping-pong foreverTest Plan
bun bd test test/cli/install/bun-install-registry.test.ts223 pass / 5 todobun bd test test/cli/install/bun-install.test.ts -t peer8 pass / 1 todobun bd test test/cli/install/semver.test.ts22 passtest/cli/install/sweep: 71 files, 0 new fails. 7 files have pre-existing fails on main, unrelated.peer-deps-fixed + no-deps@2.0.0, diamondprovides-peer-deps-*,forward-peer-deps + no-deps@2.0.0,mismatched-peer-deps) through a 0-500ms jitter proxy: 240/240 byte-identical lockfiles on bothmainand this PR.bun addrepro above against verdaccio reproduces 100% on main and is fixed 100% on this PR.cargo check -p bun_install -p bun_semverclean.bun run rust:check-allgreen across all 10 targets.