Conversation
When a package peer-depends both another package and one of that package's own peer dependencies (e.g. @typescript-eslint/eslint-plugin peer-depends both @typescript-eslint/parser and typescript, and @typescript-eslint/parser peer-depends typescript), pnpm reused a hoisted instance of the shared peer that was resolved against a different version, producing an inconsistent resolution. Close #12079
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
📝 WalkthroughWalkthroughThis PR fixes a regression in pnpm 9.9.0 where peer dependencies were resolved inconsistently across "diamond" dependency graphs (multiple packages sharing the same peer). The fix adds conflict detection to prevent reusing inherited parent packages when doing so would cause sibling peer dependencies to resolve against different versions, plus comprehensive test coverage. ChangesPeer Diamond Conflict Detection and Testing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Add @pnpm.e2e/peer-diamond-* fixtures modeling #12079 (plugin peer-depends both parser and ts; parser peer-depends ts) and integration tests on both stacks. The pnpm test guards the fix; the pacquet test confirms pacquet already resolves the diamond consistently (its merge always prefers the node's own child).
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12081 +/- ##
==========================================
- Coverage 89.38% 87.17% -2.21%
==========================================
Files 243 243
Lines 32536 26798 -5738
==========================================
- Hits 29082 23362 -5720
+ Misses 3454 3436 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Review Summary by QodoFix inconsistent resolution of peer dependencies shared through a diamond
WalkthroughsDescription• Fixes inconsistent peer resolution in diamond dependency patterns • Prevents reusing hoisted peer instances when they break peer-diamond consistency • Adds inheritedParentPkgBreaksPeerDiamond check to detect and handle conflicts • Includes comprehensive test coverage for both pnpm and pacquet implementations Diagramflowchart LR
A["Plugin peer-depends<br/>Parser + TypeScript"] -->|Diamond conflict| B["Inherited Parser<br/>resolved vs ts@2.0.0"]
A -->|Must use| C["Own child Parser<br/>resolved vs ts@1.0.0"]
B -->|Breaks consistency| D["Inconsistent versions"]
C -->|Maintains consistency| E["All peers agree<br/>on ts@1.0.0"]
F["New check:<br/>inheritedParentPkgBreaksPeerDiamond"] -->|Detects| B
F -->|Falls back to| C
File Changes1. installing/deps-resolver/src/resolvePeers.ts
|
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 @.changeset/twelve-peers-diamond.md:
- Line 6: The changelog sentence is missing the preposition "on"; update the
sentence that reads "peer-depends both another package" to "peer-depends on both
another package" so the example line referencing
`@typescript-eslint/eslint-plugin`, `@typescript-eslint/parser` and typescript reads
correctly; locate the sentence in the change description about inconsistent
resolution of a peer dependency shared through a diamond and insert "on" after
"peer-depends".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0122a4f4-1048-4ece-9d18-656aec5bb3a1
📒 Files selected for processing (11)
.changeset/twelve-peers-diamond.mdinstalling/deps-installer/test/install/peerDependencies.tsinstalling/deps-resolver/src/resolvePeers.tsinstalling/deps-resolver/test/resolvePeers.tspacquet/crates/cli/tests/install.rspnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-app/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-bundle/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-parser/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-plugin/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-ts/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-ts/2.0.0/package.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Follow Standard Style with trailing commas, preferring functions over classes, and declaring functions after they are used (relying on hoisting)
Use a single options object instead of multiple parameters when a function needs more than two or three arguments
Follow Import Order: Standard libraries first, then external dependencies (alphabetically), then relative imports
Write self-documenting code where function names, parameters, and types explain what a function does without requiring prose comments
Do not write comments that restate what the code already says; refactor via renaming, splitting helpers, or restructuring instead
Do not repeat documentation at call sites that already exists in JSDoc on the callee; update JSDoc once for all call sites to benefit
Use JSDoc only for a function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the body
Do not record past implementation shape, refactor history, or 'the previous code did X' framing in code; use git log and git blame instead
Write comments only when: the reason for code is non-obvious (hidden invariant, workaround for known bug, deliberate exception), or the right name doesn't fit (temporary technical constraint)
Files:
installing/deps-resolver/test/resolvePeers.tsinstalling/deps-installer/test/install/peerDependencies.tsinstalling/deps-resolver/src/resolvePeers.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/cli/tests/install.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/cli/tests/install.rs
🧠 Learnings (5)
📚 Learning: 2026-05-26T21:01:06.666Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.
Applied to files:
.changeset/twelve-peers-diamond.md
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
installing/deps-resolver/test/resolvePeers.tsinstalling/deps-installer/test/install/peerDependencies.tsinstalling/deps-resolver/src/resolvePeers.ts
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/cli/tests/install.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/cli/tests/install.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/cli/tests/install.rs
🔇 Additional comments (10)
installing/deps-resolver/src/resolvePeers.ts (1)
432-435: LGTM!Also applies to: 670-730
pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-ts/2.0.0/package.json (1)
1-4: LGTM!installing/deps-resolver/test/resolvePeers.ts (1)
928-1053: LGTM!installing/deps-installer/test/install/peerDependencies.ts (1)
2047-2065: LGTM!pacquet/crates/cli/tests/install.rs (1)
368-413: LGTM!pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-ts/1.0.0/package.json (1)
1-4: LGTM!pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-parser/1.0.0/package.json (1)
1-7: LGTM!pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-plugin/1.0.0/package.json (1)
1-8: LGTM!pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-bundle/1.0.0/package.json (1)
1-11: LGTM!pnpr/.fixtures/packages/@pnpm.e2e/peer-diamond-app/1.0.0/package.json (1)
1-8: LGTM!
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0648381463800005,
"stddev": 0.11038590910228807,
"median": 2.0494464322800003,
"user": 2.6838009,
"system": 3.2008603399999997,
"min": 1.94576954878,
"max": 2.2879749047800004,
"times": [
2.2879749047800004,
1.9786638357800002,
1.94576954878,
2.0945266677800003,
1.97243364978,
1.96811799678,
2.1008408337800004,
2.12829483978,
2.0043661967800004,
2.16739298978
]
},
{
"command": "pacquet@main",
"mean": 2.01691803478,
"stddev": 0.06777935514243033,
"median": 2.00378060178,
"user": 2.6897699,
"system": 3.2384550400000003,
"min": 1.94710736578,
"max": 2.18900379478,
"times": [
1.96157096778,
2.18900379478,
1.94710736578,
2.04608123678,
2.0336259617800003,
1.97330928178,
2.01401708278,
2.0060695867800002,
1.99690345278,
2.00149161678
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6677722531600001,
"stddev": 0.06268988416773781,
"median": 0.64426065206,
"user": 0.34319562,
"system": 1.3306007599999998,
"min": 0.6362986945600001,
"max": 0.8409558275600001,
"times": [
0.8409558275600001,
0.6516426515600001,
0.6888305875600002,
0.6396897865600001,
0.6510192615600001,
0.6404474575600001,
0.6422450395600001,
0.6462762645600001,
0.6403169605600001,
0.6362986945600001
]
},
{
"command": "pacquet@main",
"mean": 0.6612753597600001,
"stddev": 0.027964435873740687,
"median": 0.65222701506,
"user": 0.35235832,
"system": 1.33102346,
"min": 0.63648028456,
"max": 0.72560473156,
"times": [
0.72560473156,
0.6570223755600001,
0.63648028456,
0.65132939856,
0.6531246315600001,
0.64319687556,
0.6609602375600001,
0.6446149535600001,
0.6443506145600001,
0.6960694945600001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.30169645442,
"stddev": 0.028855606261801763,
"median": 2.2950089904200004,
"user": 3.8276410199999993,
"system": 3.0455469,
"min": 2.25387737492,
"max": 2.35117324392,
"times": [
2.28248104592,
2.25387737492,
2.2959538789200002,
2.35117324392,
2.28617203692,
2.29376824492,
2.34549050992,
2.31022199692,
2.29406410192,
2.30376210992
]
},
{
"command": "pacquet@main",
"mean": 2.3103861282200002,
"stddev": 0.02396499718740754,
"median": 2.3041284934200004,
"user": 3.834107019999999,
"system": 3.0340455000000004,
"min": 2.28235370592,
"max": 2.35085001892,
"times": [
2.29362859192,
2.33788122892,
2.30223851592,
2.30677293292,
2.28235370592,
2.30001790292,
2.28468758192,
2.33941233192,
2.3060184709200002,
2.35085001892
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.4961440914000002,
"stddev": 0.01718734726490081,
"median": 1.4959880056000001,
"user": 1.6675393800000002,
"system": 1.8621595,
"min": 1.4680171431000002,
"max": 1.5329997951,
"times": [
1.4944029031000001,
1.5329997951,
1.5039386101,
1.4793379091,
1.4975731081,
1.5059945151,
1.4903190761,
1.4680171431000002,
1.4909608801,
1.4978969741000001
]
},
{
"command": "pacquet@main",
"mean": 1.5385936371000002,
"stddev": 0.06326749190731726,
"median": 1.5240105951,
"user": 1.68648558,
"system": 1.8749169999999995,
"min": 1.4922707001,
"max": 1.7066958201,
"times": [
1.5034300081,
1.5228569031,
1.5251642871000002,
1.7066958201,
1.5261775681,
1.5036075671000002,
1.5743413051000001,
1.5044783781000002,
1.4922707001,
1.5269138341000001
]
}
]
} |
|
| Branch | pr/12081 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,301.70 ms(-3.56%)Baseline: 2,386.59 ms | 2,863.91 ms (80.37%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,496.14 ms(-1.41%)Baseline: 1,517.54 ms | 1,821.05 ms (82.16%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,064.84 ms(-2.42%)Baseline: 2,116.01 ms | 2,539.21 ms (81.32%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 667.77 ms(-1.32%)Baseline: 676.72 ms | 812.07 ms (82.23%) |
Benchmark Results
Run 26697942305 · 10 runs per scenario · triggered by @zkochan |
|
This seems to narrowly match the specific problematic scenario of #12079 without addressing the broader algorithmic problem, and is easily fooled: |
Summary
Fixes #12079 — a 9.9.0 regression where a peer dependency shared through a "diamond" resolved inconsistently, crashing
xoat runtime (TypeError: tsutils.unionConstituents is not a function).@typescript-eslint/eslint-pluginpeer-depends on both@typescript-eslint/parserandtypescript, and@typescript-eslint/parseritself peer-dependstypescript. All three must agree on a singletypescript. Withtypescript@6.0.3at the root andtypescript@5.9.3nested underxo, the resolver gaveeslint-pluginatypescript@5.9.3but aparserbound totypescript@6.0.3:Root cause
#8457 (the OOM fix for #8370) made the peer-merge reuse a hoisted instance of a peer-dep package for nested subtrees whenever name+version match, instead of re-resolving the node's own nested child. That reuse is correct for the non-diamond cases it targeted, but when a sibling peer-depends both the package and one of that package's own peers, reusing the hoisted instance silently pulls in the wrong transitive peer version.
The fix
The merge keeps the #8457 reuse except when doing so would break a peer-diamond — i.e. when a sibling peer-depends both the package and one of its peers that resolves differently in the inherited vs. local context. Then it falls back to the node's own child (
inheritedParentPkgBreaksPeerDiamondinresolvePeers.ts).This is the discriminator that separates #12079 from the cases #8457 must keep collapsing (e.g. the
repeat-peers"hoisted" test), where no node peer-depends both — so those are untouched.Performance
No regression — and not slower:
Validation
"5.9.3"/"5.9.3"(was5.9.3/6.0.3).test/resolvePeers.tsmodeling the diamond — verified it fails without the fix.resolvePeersunit (94 incl. the new one),peerDependencies(67),autoInstallPeers/defaultPeerDependencies/cyclicPeerDeterminism(26).pacquet parity
pacquet already resolves this diamond correctly — its
resolve_peers.rsusesbump_occurrence_on_shadow, which always prefers the node's own child over an inherited same-version instance (it never ported #8457's collapse), so it never reuses a root-levelts@2.0.0parser for a nested plugin. No pacquet code change is needed.To lock that in, this PR adds matching coverage on both stacks using shared
@pnpm.e2e/peer-diamond-*fixtures:installing/deps-installer/test/install/peerDependencies.ts(plus the focused unit test ininstalling/deps-resolver/test/resolvePeers.ts).pacquet/crates/cli/tests/install.rs::peer_shared_through_a_diamond_is_resolved_consistently.Both tests were verified to fail when their respective merge logic is broken, so they genuinely guard the behavior.
Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Bug Fixes
Tests