fix(deps-resolver): prefer locked peer contexts during resolution by default#12083
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds an opt-in ChangesPeer Resolution Mode Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
✅ Actions performedReview triggered.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12083 +/- ##
==========================================
+ Coverage 87.47% 87.60% +0.12%
==========================================
Files 262 267 +5
Lines 29903 30661 +758
==========================================
+ Hits 26158 26860 +702
- Misses 3745 3801 +56 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0772624199800003,
"stddev": 0.11849315197522589,
"median": 2.01989419918,
"user": 2.5687213399999993,
"system": 3.39235668,
"min": 1.96733244718,
"max": 2.28191351218,
"times": [
2.08880134218,
2.13602444518,
2.27886497718,
1.96733244718,
2.00344372518,
2.02562534518,
2.28191351218,
2.01416305318,
1.9986625511800002,
1.9777928011800001
]
},
{
"command": "pacquet@main",
"mean": 2.0473134317799997,
"stddev": 0.046681763003528345,
"median": 2.04260490168,
"user": 2.5511548399999997,
"system": 3.375646379999999,
"min": 1.98828289018,
"max": 2.14333927618,
"times": [
2.00897739418,
2.02926991718,
1.99959600118,
2.03834110418,
2.0838893181800002,
2.08487363718,
2.14333927618,
2.04969608018,
2.04686869918,
1.98828289018
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6596570533,
"stddev": 0.02909180236721938,
"median": 0.6503970971999999,
"user": 0.3627385799999999,
"system": 1.33001574,
"min": 0.6372703867,
"max": 0.7343900587000001,
"times": [
0.7343900587000001,
0.6603160477000001,
0.6677771487,
0.6552617207,
0.6732703137,
0.6388227587,
0.6416491017,
0.6455324737,
0.6422805227,
0.6372703867
]
},
{
"command": "pacquet@main",
"mean": 0.6664036192,
"stddev": 0.03315757926977948,
"median": 0.6517157587,
"user": 0.35928408,
"system": 1.3379215399999997,
"min": 0.6408113187000001,
"max": 0.7403821037,
"times": [
0.7134042357,
0.6466469097,
0.6519496767,
0.6544466597,
0.6514818407,
0.6665245317,
0.6498222017,
0.6485667137000001,
0.6408113187000001,
0.7403821037
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.316507052200001,
"stddev": 0.028907013830705815,
"median": 2.3112749595000004,
"user": 3.7125234,
"system": 3.1484871199999995,
"min": 2.2869352600000004,
"max": 2.3665862950000003,
"times": [
2.2869352600000004,
2.2914401250000003,
2.2927635320000004,
2.3355269560000003,
2.3665862950000003,
2.2963666860000003,
2.342886548,
2.2870459490000004,
2.339335938,
2.326183233
]
},
{
"command": "pacquet@main",
"mean": 2.3175844057000003,
"stddev": 0.027356044952991383,
"median": 2.3203221660000004,
"user": 3.7167439,
"system": 3.14601492,
"min": 2.281446829,
"max": 2.3569658930000004,
"times": [
2.308186661,
2.336341378,
2.3569658930000004,
2.2869611450000003,
2.281446829,
2.2832035100000003,
2.3302543260000004,
2.310390006,
2.3394512790000004,
2.34264303
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.49499924244,
"stddev": 0.020907610781396545,
"median": 1.4892796276400002,
"user": 1.7020975799999998,
"system": 1.84631832,
"min": 1.47438009714,
"max": 1.5434113091400001,
"times": [
1.49108247814,
1.48238134714,
1.49166677614,
1.5189466361400001,
1.5434113091400001,
1.4964384211400001,
1.4874767771400002,
1.48483584414,
1.47438009714,
1.4793727381400001
]
},
{
"command": "pacquet@main",
"mean": 1.54313268774,
"stddev": 0.08301783201421883,
"median": 1.52032892664,
"user": 1.70889858,
"system": 1.8954199200000001,
"min": 1.4902726701400002,
"max": 1.77396015514,
"times": [
1.51277412114,
1.4904336771400002,
1.77396015514,
1.5280677521400001,
1.5473707311400002,
1.51818151614,
1.53435858414,
1.52247633714,
1.4902726701400002,
1.51343133314
]
}
]
} |
|
Bencher Report| Branch | pr/12083 |
| 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,316.51 ms(-1.41%)Baseline: 2,349.67 ms | 2,819.60 ms (82.16%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,495.00 ms(-1.88%)Baseline: 1,523.59 ms | 1,828.31 ms (81.77%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,077.26 ms(+0.41%)Baseline: 2,068.82 ms | 2,482.59 ms (83.67%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 659.66 ms(+0.78%)Baseline: 654.52 ms | 785.43 ms (83.99%) |
|
✅ Actions performedReview triggered.
|
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 (1)
installing/deps-resolver/src/resolvePeers.ts (1)
705-723:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep aliased direct providers authoritative during locked-context reuse.
This check only protects explicit requests and brand-new declared providers. An already-installed direct alias still falls through, so
prefer-lockedcan reuse the locked provider and override the current direct alias on a later writable install. That regresses the aliased-direct-provider precedence this resolver already relies on.🩹 Suggested fix
for (const source of ctx.currentProviderSources) { if ([...source.directNodeIdsByAlias].some(([alias, directNodeId]) => directNodeId === peerNodeId && - (source.explicitlyRequestedDirectDependencies.has(alias) || + (alias !== peerName || + source.explicitlyRequestedDirectDependencies.has(alias) || (source.declaredDirectDependencies.has(alias) && ctx.dependenciesTree.get(peerNodeId)?.previousDepPath == null) ))) return true }🤖 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 `@installing/deps-resolver/src/resolvePeers.ts` around lines 705 - 723, The check in hasCurrentPeerProviderThatMustWin wrongly ignores already-installed direct aliases by only treating declared direct providers as authoritative when previousDepPath == null; to fix, remove the previousDepPath == null guard so that any alias present in source.declaredDirectDependencies (or source.explicitlyRequestedDirectDependencies) wins; update the condition inside the loop that currently reads (source.explicitlyRequestedDirectDependencies.has(alias) || (source.declaredDirectDependencies.has(alias) && ctx.dependenciesTree.get(peerNodeId)?.previousDepPath == null)) to instead be (source.explicitlyRequestedDirectDependencies.has(alias) || source.declaredDirectDependencies.has(alias)), keeping the rest of the loop and function logic unchanged so installed aliased direct providers remain authoritative.
🤖 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 `@installing/deps-resolver/src/resolvePeers.ts`:
- Around line 705-723: The check in hasCurrentPeerProviderThatMustWin wrongly
ignores already-installed direct aliases by only treating declared direct
providers as authoritative when previousDepPath == null; to fix, remove the
previousDepPath == null guard so that any alias present in
source.declaredDirectDependencies (or
source.explicitlyRequestedDirectDependencies) wins; update the condition inside
the loop that currently reads
(source.explicitlyRequestedDirectDependencies.has(alias) ||
(source.declaredDirectDependencies.has(alias) &&
ctx.dependenciesTree.get(peerNodeId)?.previousDepPath == null)) to instead be
(source.explicitlyRequestedDirectDependencies.has(alias) ||
source.declaredDirectDependencies.has(alias)), keeping the rest of the loop and
function logic unchanged so installed aliased direct providers remain
authoritative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 11ad61d6-80f8-45c6-8176-0ef8e2d99b0d
📒 Files selected for processing (3)
installing/deps-installer/test/install/peerDependencies.tsinstalling/deps-resolver/src/resolvePeers.tsinstalling/deps-resolver/test/resolvePeers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- installing/deps-resolver/test/resolvePeers.ts
📜 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). (7)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Compile & Lint
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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-installer/test/install/peerDependencies.tsinstalling/deps-resolver/src/resolvePeers.ts
🧠 Learnings (1)
📚 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-installer/test/install/peerDependencies.tsinstalling/deps-resolver/src/resolvePeers.ts
🔇 Additional comments (2)
installing/deps-installer/test/install/peerDependencies.ts (1)
2118-2119: LGTM!Also applies to: 2133-2134, 2137-2159
installing/deps-resolver/src/resolvePeers.ts (1)
114-161: LGTM!Also applies to: 382-389
|
@codex review The latest follow-up commit Written by an agent (Codex, GPT-5). Sent by Codex |
|
✅ Actions performedReview triggered.
|
c17c613 to
38bbb6d
Compare
Review Summary by QodoAdd opt-in prefer-locked peer resolution mode to reduce lockfile churn
WalkthroughsDescription• Add opt-in peerResolutionMode: prefer-locked setting for lockfile stability • Preserve compatible locked optional peer versions during re-resolution • Reuse existing peer contexts when providers remain available and valid • Respect current manifest choices: new/updated/aliased providers take precedence • Record peer resolution mode in lockfile and workspace state for consistency Diagramflowchart LR
A["Config Setting<br/>peerResolutionMode"] --> B["Lockfile Settings<br/>& Workspace State"]
C["Initial Peer<br/>Resolution"] --> D["Second Pass:<br/>Reuse Compatible<br/>Locked Contexts"]
D --> E["Respect Current<br/>Manifest Choices"]
E --> F["Stable Lockfile<br/>with Multiple<br/>Peer Contexts"]
B --> D
File Changes1. pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
|
|
@zkochan this follow-up is ready for review. It is rebased onto the latest This adds the opt-in Known landing requirement: pacquet still needs prior lockfile @codex review Written by an agent (Codex, GPT-5). Sent by Codex |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in peerResolutionMode: prefer-locked setting that reduces lockfile churn by reusing previously locked, still-compatible peer provider contexts during writable resolution, while preserving precedence for newly introduced/updated providers. The branch also includes the prerequisite fix to preserve weighted (lockfile-seeded) optional peer candidates during hoisting.
Changes:
- Introduce
peerResolutionMode: prefer-lockedplumbing across config, workspace state, lockfile settings, installer, and resolver, including frozen-lockfile mismatch detection. - Add a second “prefer locked providers” peer-resolution pass that can reuse compatible locked peer provider paths when still present in the current graph.
- Fix/port optional-peer hoisting to consider weighted preferred-version selectors (lockfile-seeded), with regression tests in both TypeScript and pacquet (Rust).
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| workspace/state/test/createWorkspaceState.test.ts | Verifies workspace state persists peerResolutionMode. |
| workspace/state/src/types.ts | Adds peerResolutionMode to the set of lockfile-affecting workspace-state keys. |
| pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs | Adds Rust regression test for preserving weighted optional-peer preferred versions. |
| pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs | Adds Rust unit test for weighted version selectors in optional-peer hoisting. |
| pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs | Updates Rust optional-peer hoisting to handle weighted selectors. |
| lockfile/types/src/index.ts | Extends lockfile settings type with peerResolutionMode. |
| lockfile/settings-checker/src/getOutdatedLockfileSetting.ts | Treats settings.peerResolutionMode changes as lockfile-setting mismatches. |
| installing/deps-resolver/test/resolvePeers.ts | Adds resolver tests covering locked peer provider reuse/precedence rules. |
| installing/deps-resolver/test/hoistPeers.test.ts | Adds unit test for weighted selector handling in optional-peer hoisting. |
| installing/deps-resolver/src/resolvePeers.ts | Implements locked peer provider reuse logic + tracks pathsByNodeId for the second pass. |
| installing/deps-resolver/src/resolveDependencyTree.ts | Propagates lockedPeerContext and previousDepPath into the dependency tree for peer resolution. |
| installing/deps-resolver/src/resolveDependencies.ts | Records prior depPath/peer context from lockfile when peerResolutionMode is enabled; tracks “must-win” providers. |
| installing/deps-resolver/src/index.ts | Wires peerResolutionMode, computes declared/explicit direct deps, and performs 2-pass peer resolution in prefer-locked mode. |
| installing/deps-resolver/src/hoistPeers.ts | Fixes optional-peer hoisting to consider weighted preferred-version selectors. |
| installing/deps-installer/test/install/peerDependencies.ts | Adds installer-level tests for lockfile setting persistence and provider-precedence behavior. |
| installing/deps-installer/test/install/frozenLockfile.ts | Adds frozen-lockfile mismatch test for settings.peerResolutionMode. |
| installing/deps-installer/test/install/autoInstallPeers.ts | Adds end-to-end regression test for preserving weighted optional-peer versions. |
| installing/deps-installer/src/install/index.ts | Records peerResolutionMode in lockfile settings and participates in lockfile mismatch checking. |
| installing/deps-installer/src/install/extendInstallOptions.ts | Adds peerResolutionMode to install options defaults/types. |
| installing/deps-installer/src/getPeerDependencyIssues.ts | Plumbs peerResolutionMode into peer-issue computation context. |
| installing/commands/src/recursive.ts | Exposes peerResolutionMode through recursive command options. |
| installing/commands/src/installDeps.ts | Exposes peerResolutionMode through install-deps options. |
| installing/commands/src/install.ts | Exposes peerResolutionMode through install command options. |
| deps/status/test/checkDepsStatus.test.ts | Adds deps-status test for detecting peerResolutionMode workspace-state changes. |
| deps/status/src/checkDepsStatus.ts | Includes peerResolutionMode in lockfile up-to-date assertions. |
| config/reader/test/index.ts | Asserts peerResolutionMode is read from workspace YAML settings. |
| config/reader/test/fixtures/settings-in-workspace-yaml/pnpm-workspace.yaml | Fixture adds peerResolutionMode: prefer-locked. |
| config/reader/src/types.ts | Registers peer-resolution-mode as a valid config key/value. |
| config/reader/src/localConfig.ts | Documents peer-resolution-mode among resolution-strategy keys (comment table). |
| config/reader/src/configFileKey.ts | Adds peer-resolution-mode to excluded pnpm keys list. |
| config/reader/src/Config.ts | Extends Config type with peerResolutionMode. |
| .changeset/steady-optional-peers.md | Changeset for optional-peer preservation fix (prerequisite commit). |
| .changeset/prefer-locked-peer-contexts.md | Changeset describing the new opt-in prefer-locked peer resolution mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
38bbb6d to
e34262c
Compare
… default Peer dependency resolution now reuses the peer contexts already recorded in the lockfile when those providers remain present in the dependency graph and still satisfy the peer ranges, instead of collapsing compatible contexts on a writable re-resolution. Current manifest choices stay authoritative: newly added, explicitly updated, or aliased direct providers, changed nested providers, and locked versions that no longer satisfy the range still win. The pacquet lockfile-reuse path is fixed to match: a reused node now derives its children from the package manifest, skipping resolved peers recorded in the snapshot, so the peer pass re-derives each occurrence's context instead of baking in the first occurrence's. Adds end-to-end coverage to both the pnpm CLI and pacquet.
e34262c to
27c2d4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (2)
installing/deps-installer/test/install/peerDependencies.ts:2133
- This installer test is about precedence in the “locked peer context reuse” mode, but it doesn’t enable the opt-in setting. If the resolver behavior is meant to be opt-in, pass
peerResolutionMode: 'prefer-locked'in the options here so the test remains aligned with the intended contract.
const project = prepareEmpty()
const opts = testDefaults({ strictPeerDependencies: false })
installing/deps-installer/test/install/peerDependencies.ts:2150
- This installer test expects preservation of multiple compatible peer contexts, but it doesn’t enable the PR’s opt-in “prefer locked” mode. If the feature is intended to be opt-in, set
peerResolutionMode: 'prefer-locked'in the install options so the test is specifically exercising that mode rather than implicitly changing default behavior expectations.
const project = prepareEmpty()
const opts = testDefaults({ strictPeerDependencies: false })
…locked-context reuse Limit locked peer-context reuse to stable leaf providers (those whose locked depPath has no peer suffix of their own). A provider that carries its own peer suffix is context-dependent — and self-referential for cyclic transitive peers — so reusing it could re-expand cyclic suffixes across installs and break the deterministic resolution from pnpm#8155. Also skip a provider that has already resolved to a different path in the current pass, which would otherwise leave pathsByNodeId pointing at a depPath absent from the dependency graph.
…des (#12320) * fix: prevent a pinned locked peer provider from leaking to sibling nodes When the locked-peer-context pinning introduced in #12083 runs for a node that has no child dependencies, parentPkgs aliases the parent's object, so writing the pinned provider into it exposed the provider to every sibling resolved afterwards. Sibling order follows resolution completion order, so optional peers of siblings resolved nondeterministically and "pnpm dedupe --check" failed intermittently in CI. Copy parentPkgs before pinning so the pin stays scoped to the node and its own subtree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * perf: copy parentPkgs only before the first pin write --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Zoltan Kochan <z@kochan.io>
Summary
Add an opt-in workspace setting in pnpm-workspace.yaml:
During writable resolution, pnpm reuses each dependency instance's previous
compatible peer providers when those providers are still present in the
current dependency graph.
The default resolution behavior is unchanged.
Why
#12075 fixed optional-peer candidate
selection: pnpm no longer discards a compatible optional-peer version merely
because it came from the lockfile.
This PR addresses a separate source of lockfile churn. A writable install could
still replace one valid peer context with another valid peer context even when
the existing provider remained present and satisfied the peer range.
Public reproduction:
https://github.com/sharmila-oai/pnpm-optional-peer-lockfile-repro
The nested reproduction starts with two valid
vitest@3.2.4contexts:Running a writable lockfile regeneration should retain both contexts:
Behavior
pnpm reuses a locked peer provider only when:
Current manifest choices remain authoritative. In particular, pnpm does not
replace:
The reuse pass runs only when the dependency tree contains locked peer contexts,
so fresh installs do not pay for a second peer-resolution pass.
Tradeoff
This change favors lockfile stability over reducing the number of peer
contexts. A writable install may retain multiple compatible peer contexts where
a fresh install would select one.
Implementation
The resolver performs its normal peer-resolution pass first. When the
dependency tree contains locked peer contexts, it performs a second pass that
may reuse compatible provider paths from the lockfile while respecting current
manifest choices.
pacquet now mirrors this behavior. Its lockfile-reuse path rebuilds child
dependencies from the package manifest and skips peer dependencies recorded in
the snapshot, so the peer pass derives each dependency instance's peer context.
Validation
newly declared providers, explicitly updated providers, changed nested
providers, workspace-root providers, and aliases.
peer contexts during writable lockfile regeneration.
cargo fmt --check.cargo test -p pacquet-resolving-deps-resolver.cargo clippy -p pacquet-resolving-deps-resolver --all-targets -- --deny warnings.Written by an agent (Codex, GPT-5).
Sent by Codex