fix(pacquet): match pnpm peer-suffix rendering for walk-ancestor peers#12267
Merged
Conversation
When a node's resolved peer is a walk-ancestor still in progress (e.g.
`@eslint-community/eslint-utils` peer-depends on its parent `eslint`),
`build_peer_id` had no depPath for it yet and fell back to the
`name@version` cycle form, freezing a collapsed suffix like
`eslint-utils@4.9.1(eslint@10.4.1)` instead of pnpm's
`eslint-utils@4.9.1(eslint@10.4.1(supports-color@8.1.1))`. Because the
depPath is the graph key, the truncation cascaded to every reference.
Port pnpm's deferred `calculateDepPath`: leave the walk (and the
provisional depPaths `find_hit` reads) untouched, capture each node's
NodeId-level edges, then recompute depPaths in a post-walk pass that
resolves each peer to its full depPath and collapses to `name@version`
only for genuine cycles — detected as peer-graph SCCs via iterative
Tarjan. The graph is rebuilt from the per-node records keyed by the
corrected depPaths, so wrongly-collapsed nodes split and correctly-
collapsed ones still merge.
Also fixes a related divergence: snapshot `dependencies` now carry only
the node's own resolved peers, mirroring pnpm's
`childrenNodeIds: { ...children, ...resolvedPeers }`, so a descendant's
optional peer (e.g. `debug`'s `supports-color`) is no longer materialized
as the parent's dependency.
Refs #12266.
|
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 (2)
✅ 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). (8)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (6)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
📚 Learning: 2026-05-24T21:10:50.292ZApplied to files:
📚 Learning: 2026-05-24T21:10:50.292ZApplied to files:
📚 Learning: 2026-06-06T18:58:37.156ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThe PR isolates optional-peer hoisting using a pre-update preferred-versions snapshot and rewrites peer resolution to record per-node edges/metadata during the walk, then runs a post-walk SCC-driven pipeline to recompute depPaths and rebuild the dependency graph with correct peer suffixes. ChangesPeer-Resolution Correctness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
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 docstrings
🧪 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 |
Contributor
Micro-Benchmark ResultsLinux |
…ersions `getHoistableOptionalPeers` hoists an optional missing peer only when a preferred version is already in scope. pnpm reads its static `ctx.allPreferredVersions` (wanted lockfile + manifests, empty on a fresh install) for that decision and never folds in versions resolved during the run. pacquet was feeding every freshly-resolved transitive version into the same map, so an optional peer like `debug`'s `supports-color` got hoisted against a deep-tree provider (e.g. `jest-worker`'s `supports-color`) that pnpm never considers — resolving the peer where pnpm leaves it bare, and non-deterministically (the outcome depended on whether the provider landed in the map before the optional pass ran). Snapshot the preferred-versions map before any run-resolved version is folded in and use that snapshot for the optional-peer hoist. The required-peer hoist keeps using the run-extended map: a required peer is auto-installed either way, and reusing an in-tree version matches pnpm's picker dedup (covered by the existing auto_install_* tests). New regression test `optional_peer_only_in_resolved_tree_is_not_hoisted`, verified to fail without the fix. Refs #12266.
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 10.03362013252,
"stddev": 0.17697149871386514,
"median": 9.96147686092,
"user": 3.10059006,
"system": 3.3900307400000003,
"min": 9.848311879919999,
"max": 10.31044957592,
"times": [
10.30415622292,
10.023583126919998,
9.950216147919999,
9.97273757392,
10.31044957592,
9.905952333919998,
9.87581036092,
9.848311879919999,
9.926344793919998,
10.218639308919998
]
},
{
"command": "pacquet@main",
"mean": 9.93030762282,
"stddev": 0.08925889461303839,
"median": 9.90772291742,
"user": 3.1283832599999997,
"system": 3.3659928399999997,
"min": 9.842053525919999,
"max": 10.124273599919999,
"times": [
10.124273599919999,
9.84917607792,
9.88920784392,
9.92794807792,
9.842053525919999,
9.901664968919999,
10.03036581092,
9.91378086592,
9.96825874192,
9.856346714919999
]
},
{
"command": "pnpr@HEAD",
"mean": 5.42166157092,
"stddev": 0.13454503654014097,
"median": 5.38308088592,
"user": 2.52194516,
"system": 3.02792094,
"min": 5.32498338792,
"max": 5.7918298659200005,
"times": [
5.32498338792,
5.43293371692,
5.38795892492,
5.35300817892,
5.40089229392,
5.36368680992,
5.3523136859200005,
5.37820284692,
5.43080599792,
5.7918298659200005
]
},
{
"command": "pnpr@main",
"mean": 5.40166672802,
"stddev": 0.06770565125924924,
"median": 5.38089450292,
"user": 2.5564166599999996,
"system": 3.0759903399999997,
"min": 5.34043288992,
"max": 5.57992065792,
"times": [
5.38238176892,
5.34043288992,
5.41923649992,
5.37994210292,
5.42890790092,
5.36301862292,
5.36857675892,
5.57992065792,
5.3818469029200005,
5.37240317492
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6722418320400001,
"stddev": 0.031263887936108715,
"median": 0.6572251118400001,
"user": 0.38988483999999995,
"system": 1.3558646199999997,
"min": 0.6415978763400001,
"max": 0.72111868434,
"times": [
0.6540776673400001,
0.72111868434,
0.65287429234,
0.6441028013400001,
0.66037255634,
0.6415978763400001,
0.7110982463400001,
0.68980279334,
0.70537874034,
0.64199466234
]
},
{
"command": "pacquet@main",
"mean": 0.7083728300400002,
"stddev": 0.07768414055203837,
"median": 0.6808993388400001,
"user": 0.40313593999999997,
"system": 1.3679984199999997,
"min": 0.66032304734,
"max": 0.9245979583400001,
"times": [
0.6989480743400001,
0.67978800934,
0.6820106683400001,
0.6765744703400001,
0.6735508683400001,
0.7093265053400001,
0.9245979583400001,
0.7079943713400001,
0.67061432734,
0.66032304734
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7884063220400002,
"stddev": 0.03282668276867903,
"median": 0.7801857053400001,
"user": 0.41977224,
"system": 1.34047722,
"min": 0.7514696033400001,
"max": 0.86090147534,
"times": [
0.8021276423400001,
0.81212980834,
0.7599198473400001,
0.8058815093400001,
0.7514696033400001,
0.86090147534,
0.7815842603400001,
0.7615073363400001,
0.7787871503400001,
0.7697545873400001
]
},
{
"command": "pnpr@main",
"mean": 0.7945138165400001,
"stddev": 0.051218275724813585,
"median": 0.7643701623400001,
"user": 0.4171004399999999,
"system": 1.3569610199999995,
"min": 0.7443081313400001,
"max": 0.87741829134,
"times": [
0.7634741973400001,
0.8398124193400001,
0.7612928363400001,
0.7506539453400001,
0.76485242734,
0.8733979053400001,
0.87741829134,
0.7443081313400001,
0.8060401143400001,
0.7638878973400001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.28250992982,
"stddev": 0.05358844304996749,
"median": 9.28284233502,
"user": 3.6759015399999995,
"system": 3.4151439599999995,
"min": 9.205949004019999,
"max": 9.384607384019999,
"times": [
9.29221608202,
9.26855369602,
9.205949004019999,
9.384607384019999,
9.286494442019999,
9.23968049302,
9.27919022802,
9.22058411502,
9.315346736019999,
9.33247711802
]
},
{
"command": "pacquet@main",
"mean": 9.345572853219998,
"stddev": 0.0522205694023228,
"median": 9.350831758519998,
"user": 3.7967256399999996,
"system": 3.4304458600000003,
"min": 9.24961031302,
"max": 9.42014710602,
"times": [
9.26884520602,
9.393590462019999,
9.37271036202,
9.35417845202,
9.332541509019999,
9.369961111019999,
9.24961031302,
9.347485065019999,
9.42014710602,
9.34665894602
]
},
{
"command": "pnpr@HEAD",
"mean": 5.19246907422,
"stddev": 0.14423273322827512,
"median": 5.13168735202,
"user": 2.4032892400000003,
"system": 3.0162446599999995,
"min": 5.10715631202,
"max": 5.55772237302,
"times": [
5.12645004502,
5.10943629202,
5.169811123020001,
5.135512959020001,
5.55772237302,
5.10715631202,
5.11403372902,
5.33052753802,
5.14617862602,
5.127861745020001
]
},
{
"command": "pnpr@main",
"mean": 5.175162444620001,
"stddev": 0.1420021631851869,
"median": 5.1391212825200006,
"user": 2.43599804,
"system": 3.00208436,
"min": 5.03193646302,
"max": 5.53750233202,
"times": [
5.13323723502,
5.137491589020001,
5.10089314702,
5.1407509760200005,
5.53750233202,
5.03193646302,
5.1578933220200005,
5.27674009602,
5.153333609020001,
5.0818456770200005
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.5579600945599998,
"stddev": 0.06935060689140368,
"median": 1.59577351576,
"user": 1.7335039599999997,
"system": 1.866694,
"min": 1.45090061976,
"max": 1.61209767376,
"times": [
1.59866046076,
1.45279836776,
1.5895316647600002,
1.61138520876,
1.45090061976,
1.59288657076,
1.5991993317600002,
1.47107977076,
1.61209767376,
1.60106127676
]
},
{
"command": "pacquet@main",
"mean": 1.5638797351600002,
"stddev": 0.0571648729501066,
"median": 1.56465889276,
"user": 1.72441736,
"system": 1.9090578,
"min": 1.47951735976,
"max": 1.66495031476,
"times": [
1.52265047376,
1.53028543476,
1.58409560176,
1.47951735976,
1.58947739176,
1.56833005576,
1.50561227376,
1.63289071576,
1.66495031476,
1.56098772976
]
},
{
"command": "pnpr@HEAD",
"mean": 0.70853146136,
"stddev": 0.021285240839550124,
"median": 0.70210085826,
"user": 0.37032515999999993,
"system": 1.3097747,
"min": 0.68800537776,
"max": 0.75534562876,
"times": [
0.70071500476,
0.7377684267600001,
0.69411631076,
0.70365872776,
0.70825990876,
0.69345791976,
0.75534562876,
0.68800537776,
0.70348671176,
0.7005005967600001
]
},
{
"command": "pnpr@main",
"mean": 0.7176313418600001,
"stddev": 0.028568365316988645,
"median": 0.7122636037600001,
"user": 0.37433726,
"system": 1.3253733,
"min": 0.6776801707600001,
"max": 0.76308557876,
"times": [
0.7569431657600001,
0.7066836847600001,
0.71595198776,
0.7450977347600001,
0.6917484837600001,
0.7133780807600001,
0.76308557876,
0.69459540476,
0.7111491267600001,
0.6776801707600001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 5.1612119426600005,
"stddev": 0.08286915015513835,
"median": 5.14752920046,
"user": 1.9787632999999996,
"system": 2.02863282,
"min": 5.05973496046,
"max": 5.33063263446,
"times": [
5.08675933446,
5.10636919546,
5.18200329346,
5.22356409246,
5.09620927146,
5.13424517446,
5.2317882434600005,
5.16081322646,
5.33063263446,
5.05973496046
]
},
{
"command": "pacquet@main",
"mean": 5.13366429246,
"stddev": 0.05582556774726276,
"median": 5.14210635646,
"user": 1.8669536999999998,
"system": 2.05867452,
"min": 5.03235212246,
"max": 5.24440629746,
"times": [
5.15343624646,
5.15707471546,
5.24440629746,
5.15979837646,
5.11048306246,
5.10946410146,
5.13077646646,
5.08506226346,
5.15378927246,
5.03235212246
]
},
{
"command": "pnpr@HEAD",
"mean": 0.7030478777600001,
"stddev": 0.018104297171141127,
"median": 0.7054723449600001,
"user": 0.352459,
"system": 1.31580122,
"min": 0.6750006954600001,
"max": 0.7298096424600001,
"times": [
0.7298096424600001,
0.7209545324600001,
0.6888925644600001,
0.6928447264600001,
0.7078735064600001,
0.6750006954600001,
0.7093183884600001,
0.6819454064600001,
0.7030711834600001,
0.7207681314600001
]
},
{
"command": "pnpr@main",
"mean": 0.72280004206,
"stddev": 0.07944069399260235,
"median": 0.6982550329600001,
"user": 0.3556202,
"system": 1.3255331199999998,
"min": 0.6719298944600001,
"max": 0.93555845146,
"times": [
0.7254568774600001,
0.6993475414600001,
0.6818523944600001,
0.70637484246,
0.93555845146,
0.75909433146,
0.6780515414600001,
0.69716252446,
0.6719298944600001,
0.6731720214600001
]
}
]
} |
Contributor
Bencher Report
Code Review by Qodo
1. Unstable graph node selection
|
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 `@pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs`:
- Around line 955-964: The NodeRecord.depth stored in self.node_records can
remain stale when a package is first seen at deeper depth and later revisited at
a shallower depth via pure_pkgs or find_hit, causing build_final_graph to
restore the deeper depth; update the cached NodeRecord.depth on those
early-return paths (where pure_pkgs and find_hit short-circuit) to take the
minimum of the existing record.depth and the new tree_node.depth (or otherwise
propagate the shallower depth), or alternatively adjust build_final_graph to
compute depth from all occurrences for a depPath; locate and modify the code
paths around pure_pkgs handling, find_hit handling, and the places that insert
into self.node_records (NodeRecord, node_records, build_final_graph) to ensure
the stored depth is the minimum seen.
🪄 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: aa38e19f-9dd4-413b-9496-6d971f6d5545
📒 Files selected for processing (4)
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
🧠 Learnings (6)
📚 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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
🔇 Additional comments (3)
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
81-91: LGTM!Also applies to: 301-312, 385-388
pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs (1)
718-781: LGTM!pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
924-1000: LGTM!
`build_final_graph` keyed a graph node's `depth` off the per-NodeId `NodeRecord`, but the `pure_pkgs` and `find_hit` fast paths short-circuit before a `NodeRecord` is created — they only lower the depth on the inline `self.graph`, which the rebuild discards. So a package first walked at a deeper depth and later revisited shallower kept the deeper depth, dropping the `Math.min` tie-break those fast paths' own comments preserve. Recompute the minimum tree depth per final depPath from `node_dep_paths` (which records every walked NodeId, revisits included) and use it for each rebuilt node. No lockfile-visible change today — `DependenciesGraphNode.depth` isn't serialized and the hoister computes its own BFS depth — but it keeps the graph's depth correct for any future consumer. Regression introduced by the deferred depPath rebuild; guarded by a new test. Refs #12266.
|
Code review by qodo was updated up to the latest commit f381e15 |
…ibling" Ports upstream's `resolvePeers.ts` "a peer's own peer is shared with a sibling that peer-depends both" — `plugin`'s `parser` peer must resolve the `typescript@1.0.0` that `plugin` itself uses, not be shadowed by a top-level `parser` that resolved `typescript@2.0.0`. Exercises the depPath suffix machinery reworked for the peer-suffix fixes; pacquet already passes it. Also records the peer-resolution porting status (resolvePeers.ts + hoistPeers.test.ts) in plans/TEST_PORTING.md: what's ported/covered and the remaining gaps (multi-project peer separation, npm-alias peers, the peer-conflict reporting edge cases, and the unported lockedPeerContext series). Refs #12266.
|
Code review by qodo was updated up to the latest commit f48eca6 |
This was referenced Jun 8, 2026
zkochan
added a commit
that referenced
this pull request
Jun 12, 2026
…e, peer-vs-own-dep, peer overrides, optional-peer hoist) (#12345) ## Summary Four lockfile-parity fixes for pacquet, continuing #12266. Measured on the monorepo itself (fresh state, `install --lockfile-only`, back-to-back against the live registry vs **pnpm 11.6.0**), the real-lockfile document diff drops from **806 to 461 changed lines**. Two consecutive pacquet runs are byte-identical before and after. ### 1. Importer's regular dep wins over its own peer range (`fix(deps-resolver)`) An importer that declares the same alias in both `peerDependencies` and a regular group (e.g. `@pnpm/logger`: `workspace:*` devDependency + `catalog:` peer — 67 importers in this repo) resolved both specs, and the peer's registry resolution overwrote the workspace link in the importers section (`version: 1001.0.1` instead of `link:../../core/logger`, 182 diff lines). `importer_direct_wanted_specs` now merges the groups the way pnpm spreads them in [`getWantedDependencies`](https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/getWantedDependencies.ts#L32-L43): peers first, then `dependencies` < `devDependencies` < `optionalDependencies`, one wanted spec per alias. The `@pnpm/logger` resolution tallies now match pnpm's exactly. ### 2. A package's peer survives when it also lists the name as a dependency (`fix(deps-resolver)`) Under `autoInstallPeers`, pnpm removes peer-shadowed names from a resolved package's `dependencies` **before** extracting peers ([resolveDependencies.ts#L1527-L1542](https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/resolveDependencies.ts#L1527-L1542)), so the peer edge supplies the package and the depPath carries the suffix. pacquet did the inverse (dropped the peer, walked the own dep), so `@babel/parser` — whose packageExtensions-added `@babel/types` peer is also its regular dependency — lost its `(@babel/types@7.29.7)` suffix and its `peerDependencies:` block. Also aligns `extract_peer_dependencies` with [`peerDependenciesWithoutOwn`](https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/resolveDependencies.ts#L1840-L1864): the package's own name counts as an own dep, and a `peerDependenciesMeta`-only entry becomes a peer only when `optional: true`. The non-`autoInstallPeers` arm (omit only peers resolvable from the parent scope) is not ported — pacquet's per-package children cache has no parent context; behavior there matches pnpm whenever the peer is not in scope. ### 3. Overrides apply to `peerDependencies` (`fix(package-manager)`) Ports the peer arm of [`overrideDepsOfPkg`](https://github.com/pnpm/pnpm/blob/01b3d45ddb/hooks/read-package-hook/src/createVersionsOverrider.ts#L68-L129): a matched peer is deleted on `-`, rewritten in place when the override value is a valid peer range, otherwise written into `dependencies` while the declared peer stays. Fixes e.g. `ajv@>=7.0.0-alpha.0 <8.18.0 → >=8.18.0` not rewriting peer ranges and `@yarnpkg/libzip` losing its `(@yarnpkg/fslib@3.x)` suffix. ### 4. Optional-peer hoist: run-extended preferred versions, meta-only peers excluded (`fix(deps-resolver)`) Corrects the model from #12267. pnpm folds **every run-resolved version** into `ctx.allPreferredVersions` ([resolveDependencies.ts#L1483-L1488](https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/resolveDependencies.ts#L1483-L1488), since #7812) and `getHoistableOptionalPeers` reads that map after each wave — so an optional peer with a **real `peerDependencies` entry** (eslint's `jiti`) resolves against a provider anywhere in the freshly resolved tree. What keeps `debug`'s `supports-color` bare is not a static map but the missing-peer set: [`getMissingPeers`](https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/resolveDependencies.ts#L1773-L1782) iterates `peerDependencies` only, so a **meta-only** peer never feeds the hoist. Both behaviors verified empirically against pnpm 11.6.0 (`eslint` + `cosmiconfig-typescript-loader` hoists `jiti@2.6.1`; `debug` + `concurrently` stays bare). pacquet's static-snapshot approach got the meta-only case right by the wrong mechanism and under-hoisted every real-entry optional peer — the missing `(jiti@2.6.1)`, `(typanion@3.14.0)`, `(conventional-commits-parser@6.4.0)`, `(@types/node@…)` suffixes and their cascades on the monorepo. The 12267-era regression test asserted the anti-pnpm behavior for the real-entry shape and was inverted; a new test pins the meta-only shape. ## Verification - New unit tests in `pacquet-resolving-deps-resolver` (importer group merge ×4, peer-vs-own-dep shadowing ×3, optional-peer hoist real-entry/meta-only ×2) and `pacquet-package-manager` (peer overrides ×4). - Full workspace suite: 3218 tests pass; clippy `--deny warnings` clean; rustfmt + typos clean. - Whole-monorepo lockfile diff vs fresh pnpm 11.6.0: 806 → 461 changed lines; `@pnpm/logger`, `jiti`, `typanion`, `@babel/types`, `ajv` categories eliminated. Two consecutive pacquet runs byte-identical.
This was referenced Jun 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
First slice of the pacquet lockfile-parity work tracked in #12266. Makes pacquet's peer-suffix rendering match the pnpm CLI for the dominant divergence class.
When a node's resolved peer is a still-in-progress walk-ancestor (e.g.
@eslint-community/eslint-utilspeer-depends on its parenteslint),build_peer_idhad no depPath for it yet and fell back to thename@versioncycle form, freezing a collapsed suffix likeeslint-utils@4.9.1(eslint@10.4.1)instead of pnpm'seslint-utils@4.9.1(eslint@10.4.1(supports-color@8.1.1)). Because the depPath is the graph key, the truncation cascaded to every reference.How
Ports pnpm's deferred
calculateDepPath:find_hitreads) are left untouched, so peer-resolution and cache decisions stay identical.NodeRecord.build_final_dep_paths) recomputes depPaths, resolving each peer to its full depPath and collapsing toname@versiononly for genuine cycles — detected as peer-graph SCCs via iterative Tarjan (peer_sccs).build_final_graphrebuilds the graph keyed by the corrected depPaths: wrongly-collapsed nodes split, correctly-collapsed ones still merge.Also fixes a related divergence: snapshot
dependenciesnow carry only the node's own resolved peers (mirroring pnpm'schildrenNodeIds: { ...children, ...resolvedPeers }), so a descendant's optional peer (e.g.debug'ssupports-color) is no longer materialized as the parent's dependency.Verification
eslint+supports-color;debug+concurrently) are now byte-identical to pnpm 11.5.2.ancestor_peer_carries_its_own_suffix, verified to fail without the fix.Status
Draft — more lockfile-parity work to come on this branch:
@babel/corepeer suffixes; jest resolving a peer to@babel/corewhere pnpm resolves to@babel/types).npm:aliases in catalogs/overrides/snapshots;link:.→link:;runtime:prefix;pnpmfileChecksum;patchedDependencies.resolve_nodeis now redundant (only feeds the discardedself.graph+patch_pending_peer_edges).Part of #12266.
Written by an agent (Claude Code, claude-opus-4-8).
Summary by CodeRabbit
Bug Fixes
Tests
Documentation