fix(pacquet): close four lockfile-parity gaps (importer peer+dev merge, peer-vs-own-dep, peer overrides, optional-peer hoist)#12345
Conversation
Code Review by Qodo
Context used 1. Optional peers hoisted as direct deps
|
|
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 (14)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (12)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (5)📚 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-06-06T18:58:37.156ZApplied to files:
📚 Learning: 2026-06-12T07:04:03.703ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughThis PR adds ChangesAuto-install peers and meta-only peer dependency feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
PR Summary by QodoFix pacquet lockfile parity for peer merge/shadowing, peer overrides, optional-peer hoist WalkthroughsDescription• Align importer wanted-spec merging with pnpm so regular deps override same-name peers. • Match pnpm peer/own-dep shadowing and optional-peer hoist behavior (incl. meta-only peers). • Apply pnpm-style overrides to peerDependencies and expand regression coverage. Diagramgraph TD
A["package-manager"] --> D["resolve_workspace"] --> E["resolve_importer"] --> F["resolve_dependency_tree"] --> G["resolve_peers & hoist"] --> H[("lockfile output")]
A --> B["VersionsOverrider"] --> C["manifest rewrite"]
High-Level AssessmentThe following are alternative approaches to this PR: 1. Thread parent-scope context to fully port non-autoInstallPeers peer omission
2. Port a real semver-range intersection library for peer range merges
3. Reuse upstream peer-range validation logic via shared crate/module
Recommendation: The current approach is appropriate for lockfile-parity work: it directly ports pnpm’s decision points where pacquet has equivalent context (importer wanted-spec merge order, autoInstallPeers shadowing, optional-peer hoist inputs, peer overrides) and explicitly documents the one upstream arm that cannot be faithfully replicated without redesign (non-autoInstallPeers parent-scope omission). Consider the parent-context threading alternative only if remaining parity gaps prove material and justify the additional architectural complexity. File ChangesEnhancement (2)
Bug fix (6)
Tests (6)
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
252-300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
WorkspaceTreeCtx.auto_install_peersandResolveImporterOptions.auto_install_peersin sync here.
resolve_importer()stamps a freshWorkspaceTreeCtxwithopts.auto_install_peers, but this overload never reconciles the same flag on the caller-supplied workspace. That lets one call passauto_install_peers: truewhile the shared workspace still saysfalse, soimporter_direct_wanted_specs()and theTreeCtx-driven walk can run under different peer-install settings and build the wrong tree.Based on learnings, "follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly."
🤖 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 `@pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs` around lines 252 - 300, The workspace's auto_install_peers flag must be synchronized with the options passed into resolve_importer_with_workspace: before creating the TreeCtx (and before calling importer_direct_wanted_specs), set the caller-supplied WorkspaceTreeCtx.auto_install_peers to the ResolveImporterOptions.auto_install_peers value so the shared workspace and the TreeCtx/walk use the same peer-install behavior; update resolve_importer_with_workspace to apply that change (or call the workspace's setter) right after destructuring opts and before TreeCtx::with_workspace / importer_direct_wanted_specs, ensuring resolve_importer and resolve_importer_with_workspace behave consistently.Source: Learnings
🧹 Nitpick comments (1)
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
78-92: ⚡ Quick winUpdate the
all_preferred_versionscontract comment.This doc block still says optional-peer hoisting reads a pre-run snapshot, but Lines 302-310 and 383-384 now intentionally consult the run-extended map. Leaving the old text here makes the public API contract wrong.
As per coding guidelines, "Doc comments (
///,//!) document the contract — preconditions, postconditions, panics, and the reason the function exists; they are not a re-narration of the body."🤖 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 `@pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs` around lines 78 - 92, The doc comment for the struct field `all_preferred_versions` is outdated and must be rewritten to reflect the current contract: explain that the orchestrator extends the preferred-versions map in place as packages are walked (so run-time extensions are visible), and that both the `hoist_peers` (required-peer) picker and the `get_hoistable_optional_peers` logic consult this run-extended map rather than a pre-run snapshot; state the caller should pass the result of `get_preferred_versions_from_lockfile_and_manifests` (or an empty map when none) and document this precondition/intent clearly on `all_preferred_versions`.Source: Coding guidelines
🤖 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_dependency_tree.rs`:
- Around line 1035-1039: Reused manifests are not normalized like fresh
resolves, causing peers listed also as dependencies to be dropped when
ctx.workspace.auto_install_peers is enabled; update resolve_reused_node() to run
the same normalization as the fresh-path by applying
omit_peer_shadowed_dependencies(...) to result_inner.manifest when
ctx.workspace.auto_install_peers is true and a manifest exists (mirror the
pattern used where result_inner.manifest is taken and then replaced with
Some(omit_peer_shadowed_dependencies(manifest))). This ensures
extract_peer_dependencies(...) and snapshot_child_refs(...) see the normalized
manifest and prevents reintroducing peer-vs-own-dep mismatches.
In `@pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs`:
- Around line 507-508: The meta_only flag on MissingPeerInfo should be combined
with AND semantics when merging multiple occurrences (so meta_only stays false
if any occurrence was false) instead of last-writer-wins; locate where
MissingPeerInfo entries are merged (places using HashMap::insert or the map
merge for peer names) and change the merge to set meta_only = existing.meta_only
&& new.meta_only (or equivalent) so that resolve_node and
partition_missing_peers see meta_only==true only when all occurrences were
meta-only.
In `@pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs`:
- Around line 163-170: The workspace-level auto_install_peers value in
resolve_workspace is the single source of truth but importer_opts still keep
their callback-returned values, which can cause mixed policies; after building
each ResolveImporterOptions (the importer_opts instances) and before calling
compute_time_based_cutoff or entering the resolve loop (and before
resolve_importer_with_workspace/shadow pruning), assign
importer_opts.auto_install_peers = auto_install_peers so every importer uses the
WorkspaceTreeCtx auto_install_peers; update references where importer_opts are
constructed in resolve_workspace to normalize this field immediately after
construction.
---
Outside diff comments:
In `@pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs`:
- Around line 252-300: The workspace's auto_install_peers flag must be
synchronized with the options passed into resolve_importer_with_workspace:
before creating the TreeCtx (and before calling importer_direct_wanted_specs),
set the caller-supplied WorkspaceTreeCtx.auto_install_peers to the
ResolveImporterOptions.auto_install_peers value so the shared workspace and the
TreeCtx/walk use the same peer-install behavior; update
resolve_importer_with_workspace to apply that change (or call the workspace's
setter) right after destructuring opts and before TreeCtx::with_workspace /
importer_direct_wanted_specs, ensuring resolve_importer and
resolve_importer_with_workspace behave consistently.
---
Nitpick comments:
In `@pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs`:
- Around line 78-92: The doc comment for the struct field
`all_preferred_versions` is outdated and must be rewritten to reflect the
current contract: explain that the orchestrator extends the preferred-versions
map in place as packages are walked (so run-time extensions are visible), and
that both the `hoist_peers` (required-peer) picker and the
`get_hoistable_optional_peers` logic consult this run-extended map rather than a
pre-run snapshot; state the caller should pass the result of
`get_preferred_versions_from_lockfile_and_manifests` (or an empty map when none)
and document this precondition/intent clearly on `all_preferred_versions`.
🪄 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: d5de30b4-8e47-41a7-abce-676d28ff393b
📒 Files selected for processing (14)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/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/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📜 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 (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: ubuntu-latest / Node.js 24 / Test
🧰 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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.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/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/overrides/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/package-manager/src/overrides.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/tests.rs
🔇 Additional comments (5)
pacquet/crates/package-manager/src/overrides.rs (1)
119-119: LGTM!Also applies to: 156-165, 219-280, 321-327
pacquet/crates/package-manager/src/overrides/tests.rs (1)
279-348: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
1030-1030: LGTM!pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs (1)
175-181: LGTM!pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
557-560: LGTM!
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12345 +/- ##
==========================================
+ Coverage 87.85% 87.97% +0.11%
==========================================
Files 291 292 +1
Lines 36246 36752 +506
==========================================
+ Hits 31844 32332 +488
- Misses 4402 4420 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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": 4.0032156271,
"stddev": 0.1369877319390789,
"median": 4.0733646266,
"user": 3.838639379999999,
"system": 2.0902702999999994,
"min": 3.7863705106000003,
"max": 4.1435850846,
"times": [
4.1302483476,
3.8909674806,
3.8342614256000003,
4.1435850846,
4.0889988396,
4.1231176896,
4.0721401406,
3.8878776396,
3.7863705106000003,
4.0745891126
]
},
{
"command": "pacquet@main",
"mean": 3.8611097349999994,
"stddev": 0.1384283523961935,
"median": 3.8353241250999996,
"user": 3.8672411799999997,
"system": 2.1010458,
"min": 3.6647599186,
"max": 4.1919642276,
"times": [
3.9265130986,
3.9198103726,
3.7763333976,
3.8609228236,
4.1919642276,
3.6647599186,
3.7811864906,
3.8397446316,
3.8189587706,
3.8309036186
]
},
{
"command": "pnpr@HEAD",
"mean": 2.1107412765,
"stddev": 0.20663987806188877,
"median": 2.1139132441,
"user": 2.8714842800000002,
"system": 1.8099260999999995,
"min": 1.8954830076000002,
"max": 2.4543430156,
"times": [
2.2509941196,
2.2265077626,
2.4543430156,
1.8998095266,
1.8997653826,
2.2450662326,
2.0013187256,
2.3010868166,
1.8954830076000002,
1.9330381756000001
]
},
{
"command": "pnpr@main",
"mean": 2.0166318477000003,
"stddev": 0.1344524853793379,
"median": 1.9710337966,
"user": 2.8846097800000003,
"system": 1.7942183,
"min": 1.8448647026,
"max": 2.2431163486,
"times": [
2.2431163486,
1.9083000826,
2.2044482156,
2.0808328456,
1.8448647026,
2.0954881516,
1.9328903156,
2.0091772776,
1.9144011456,
1.9327993916000001
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.47601746240000004,
"stddev": 0.010360026211877808,
"median": 0.48038782860000007,
"user": 0.36009028,
"system": 0.77886516,
"min": 0.4601961411,
"max": 0.48852341610000005,
"times": [
0.48556262710000003,
0.4601961411,
0.48852341610000005,
0.46136778910000004,
0.47051558610000005,
0.4811633551,
0.48193042610000003,
0.47961230210000005,
0.4670308651,
0.48427211610000004
]
},
{
"command": "pacquet@main",
"mean": 0.4664263024000001,
"stddev": 0.00506208457728723,
"median": 0.4663320166,
"user": 0.36435867999999993,
"system": 0.7662863599999999,
"min": 0.4568073931,
"max": 0.47614995810000005,
"times": [
0.46663088710000006,
0.46844601310000006,
0.4628476771,
0.4633628281,
0.4568073931,
0.46550580910000006,
0.47614995810000005,
0.46878960710000006,
0.4696897051,
0.4660331461
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6311199821000001,
"stddev": 0.11442703337121747,
"median": 0.5664890836,
"user": 0.3831332800000001,
"system": 0.7808810599999999,
"min": 0.5541722770999999,
"max": 0.8665066641,
"times": [
0.5612345680999999,
0.5826301101,
0.5544405481,
0.5576472621,
0.7284599991,
0.7731302251,
0.8665066641,
0.5649782461,
0.5679999211,
0.5541722770999999
]
},
{
"command": "pnpr@main",
"mean": 0.5694930658999999,
"stddev": 0.010072859598796656,
"median": 0.5672568306,
"user": 0.38128038000000003,
"system": 0.77477356,
"min": 0.5575124571,
"max": 0.5879829911,
"times": [
0.5622547211,
0.5575124571,
0.5653931641,
0.5691204971,
0.5635468321,
0.5724727950999999,
0.5582816701,
0.5797727261,
0.5785928051,
0.5879829911
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 3.6419463663399996,
"stddev": 0.05406740713885515,
"median": 3.6298304312400003,
"user": 3.81648676,
"system": 2.05583934,
"min": 3.59034103174,
"max": 3.75222100674,
"times": [
3.6438254547400004,
3.59042868674,
3.63686217974,
3.70606413074,
3.6093862957400003,
3.75222100674,
3.59034103174,
3.59420261574,
3.6733335787400003,
3.62279868274
]
},
{
"command": "pacquet@main",
"mean": 3.58143873464,
"stddev": 0.04453026798341445,
"median": 3.5849252327400003,
"user": 3.7923357599999994,
"system": 2.02559684,
"min": 3.50046543174,
"max": 3.6589476707400004,
"times": [
3.6085614197400004,
3.5823790797400004,
3.5874713857400002,
3.54172386974,
3.6589476707400004,
3.61795314674,
3.55444701974,
3.5603618737400002,
3.60207644874,
3.50046543174
]
},
{
"command": "pnpr@HEAD",
"mean": 1.9485671475399997,
"stddev": 0.14763820363675065,
"median": 1.96361353874,
"user": 2.6657021600000004,
"system": 1.74973834,
"min": 1.77588141574,
"max": 2.16587809874,
"times": [
2.06521083174,
1.77588141574,
2.02676152774,
1.78060047274,
1.8229414387399998,
1.90933467274,
2.01789240474,
2.11675853974,
1.80441207274,
2.16587809874
]
},
{
"command": "pnpr@main",
"mean": 1.93427089934,
"stddev": 0.14426032720175566,
"median": 1.92837602174,
"user": 2.68368836,
"system": 1.73297144,
"min": 1.68678222774,
"max": 2.17682233874,
"times": [
1.96871603374,
1.9202637487399998,
1.93648829474,
2.12418499874,
1.91988350974,
1.82415275374,
1.9762974867399998,
1.8091176007399998,
1.68678222774,
2.17682233874
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.91487379036,
"stddev": 0.009438542323202431,
"median": 0.9142862350600001,
"user": 1.0866176199999997,
"system": 0.98614426,
"min": 0.90344718806,
"max": 0.9277659050600001,
"times": [
0.9277659050600001,
0.90633293406,
0.9159467330600001,
0.9265878040600001,
0.91262573706,
0.92270929406,
0.90789683606,
0.90344718806,
0.9035144140600001,
0.9219110580600001
]
},
{
"command": "pacquet@main",
"mean": 0.8739124996600001,
"stddev": 0.006188935529207131,
"median": 0.87358729106,
"user": 1.04246792,
"system": 0.9938700599999999,
"min": 0.86151902006,
"max": 0.8827077400600001,
"times": [
0.8728278700600001,
0.86151902006,
0.8678856740600001,
0.8716124340600001,
0.8798774450600001,
0.8724283450600001,
0.8827077400600001,
0.87831201006,
0.87434671206,
0.87760774606
]
},
{
"command": "pnpr@HEAD",
"mean": 0.4916676650599999,
"stddev": 0.008621818651969567,
"median": 0.49111128706000007,
"user": 0.32034931999999994,
"system": 0.74975846,
"min": 0.47911009506,
"max": 0.51282091606,
"times": [
0.49464101906,
0.51282091606,
0.49155798006000007,
0.49198797606000005,
0.47911009506,
0.48828812406000005,
0.49236606806000005,
0.48547164106000007,
0.49066459406000007,
0.48976823706000006
]
},
{
"command": "pnpr@main",
"mean": 0.55349647646,
"stddev": 0.08720245377931576,
"median": 0.49749953856000007,
"user": 0.33026131999999997,
"system": 0.74952416,
"min": 0.48946253706000004,
"max": 0.6903189160600001,
"times": [
0.49374843706000004,
0.49199395206,
0.49065125906000007,
0.49852962106000004,
0.48946253706000004,
0.67957237806,
0.6630222870600001,
0.6903189160600001,
0.54119592106,
0.49646945606000004
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.4246799796199996,
"stddev": 0.021978469821676797,
"median": 2.4203496838199996,
"user": 1.56368774,
"system": 1.19268896,
"min": 2.38818841332,
"max": 2.46851744032,
"times": [
2.4272266443199997,
2.42126454232,
2.38818841332,
2.4194348253199998,
2.41699774132,
2.43350069332,
2.4476892923199998,
2.46851744032,
2.40650638732,
2.41747381632
]
},
{
"command": "pacquet@main",
"mean": 2.38581368792,
"stddev": 0.025802242155046836,
"median": 2.38450042882,
"user": 1.53637414,
"system": 1.18744416,
"min": 2.3476503523199996,
"max": 2.41859792632,
"times": [
2.38684620632,
2.40638802932,
2.3821546513199996,
2.4173694383199997,
2.40696484932,
2.3476503523199996,
2.37006554732,
2.35379841432,
2.41859792632,
2.36830146432
]
},
{
"command": "pnpr@HEAD",
"mean": 0.49571366382000004,
"stddev": 0.0063968860810277965,
"median": 0.49488380332000004,
"user": 0.33234144,
"system": 0.7574323599999999,
"min": 0.48771032632,
"max": 0.5052212853200001,
"times": [
0.5052212853200001,
0.49400543532,
0.49065084932,
0.48982059932,
0.50270862232,
0.49028211132,
0.49576217132,
0.48771032632,
0.5041304123200001,
0.49684482532
]
},
{
"command": "pnpr@main",
"mean": 0.49493324432,
"stddev": 0.005344274226303749,
"median": 0.49644137832,
"user": 0.32577284,
"system": 0.7506447600000001,
"min": 0.48536882732000003,
"max": 0.5024571103200001,
"times": [
0.49865738232,
0.5024571103200001,
0.49504026932,
0.49905155132,
0.48536882732000003,
0.49851078532000004,
0.49118391632,
0.49784248732,
0.49225684232,
0.48896327132
]
}
]
} |
|
| Branch | pr/12345 |
| 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 | 3,641.95 ms(-56.22%)Baseline: 8,319.07 ms | 9,982.88 ms (36.48%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 2,424.68 ms(-47.26%)Baseline: 4,597.06 ms | 5,516.48 ms (43.95%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 914.87 ms(-32.63%)Baseline: 1,357.97 ms | 1,629.57 ms (56.14%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,003.22 ms(-56.45%)Baseline: 9,192.32 ms | 11,030.79 ms (36.29%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 476.02 ms(-28.36%)Baseline: 664.44 ms | 797.33 ms (59.70%) |
|
| Branch | pr/12345 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 1,948.57 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 495.71 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 491.67 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,110.74 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 631.12 ms |
|
Code review by qodo was updated up to the latest commit 5327f7b |
|
Code review by qodo was updated up to the latest commit 43fd70f |
|
Code review by qodo was updated up to the latest commit 69535c8 |
…er range An importer that declares the same alias in both peerDependencies and a regular group (e.g. a workspace:* devDependency plus a catalog: peer) resolved both specs, and the peer's registry resolution overwrote the workspace link in the importers lockfile section. Merge the groups the way pnpm spreads them in getWantedDependencies (https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/getWantedDependencies.ts#L32-L43): peerDependencies first (when autoInstallPeers), then dependencies < devDependencies < optionalDependencies, a later group's range replacing an earlier one, so each alias yields a single wanted spec. Part of #12266 (the @pnpm/logger link-vs-registry divergence, 182 lines of the monorepo lockfile diff).
… as a dependency Under autoInstallPeers, pnpm removes peer-shadowed names from a resolved package's dependencies before extracting its peers (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 peer suffix. pacquet did the inverse — peerDependenciesWithoutOwn-style filtering dropped the peer and walked the own dependency — so e.g. @babel/parser (whose packageExtensions-added @babel/types peer is also its regular dependency) lost its (@babel/types@x) suffix and the peerDependencies block in the lockfile. Also align extract_peer_dependencies with upstream's peerDependenciesWithoutOwn: the package's own name counts as an own dep, and a peerDependenciesMeta entry without a matching peerDependencies entry only becomes a peer 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, and the own dependency keeps winning there, which matches upstream whenever the peer is not in scope. Part of #12266 (~80 lines of the monorepo lockfile diff).
Port the peerDependencies arm of upstream's 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 (semver, or a workspace:/catalog: expression), and otherwise written into dependencies while the declared peer range stays. The apply_to_arc clone gate now also fires for manifests whose only match is a peer. Without this, e.g. the ajv@<8.18.0 → >=8.18.0 override kept a package's peerDependencies at the stale range and @yarnpkg/libzip lost its (@yarnpkg/fslib@3.x) suffix relative to pnpm's lockfile. Part of #12266.
…s, except meta-only peers pnpm folds every run-resolved package version into ctx.allPreferredVersions (https://github.com/pnpm/pnpm/blob/01b3d45ddb/installing/deps-resolver/src/resolveDependencies.ts#L1483-L1488) and getHoistableOptionalPeers reads that map after each wave — so an optional peer with a real peerDependencies entry (e.g. eslint's jiti) resolves against a provider anywhere in the freshly resolved tree. What keeps a peer like debug's supports-color bare is not the 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 peer declared solely via peerDependenciesMeta never feeds the hoist. Both behaviors verified against pnpm 11.6.0 (eslint + cosmiconfig-typescript-loader hoists jiti; debug + concurrently stays bare). pacquet modeled this with a static lockfile-seeded map, which got the meta-only case right by the wrong mechanism and under-hoisted every real-entry optional peer (jiti, typanion, conventional-commits-parser, @types/node suffixes on the monorepo). Feed the run-extended map to the optional hoist and track meta-only-ness on PeerDep/MissingPeer so partition_missing_peers can exclude meta-only peers from the hoist bucket. Part of #12266.
… for auto_install_peers The setting is workspace-wide (pnpm's autoInstallPeers is one config value per install), so the workspace options now override each per-importer value instead of trusting the callback to agree — the importer hoist loop and the tree walk's shadow pruning can no longer diverge. Addresses a review comment on the PR.
69535c8 to
46d2cc9
Compare
|
Code review by qodo was updated up to the latest commit 46d2cc9 |
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
peerDependenciesand 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.1instead oflink:../../core/logger, 182 diff lines).importer_direct_wanted_specsnow merges the groups the way pnpm spreads them ingetWantedDependencies: peers first, thendependencies<devDependencies<optionalDependencies, one wanted spec per alias. The@pnpm/loggerresolution 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'sdependenciesbefore extracting peers (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/typespeer is also its regular dependency — lost its(@babel/types@7.29.7)suffix and itspeerDependencies:block. Also alignsextract_peer_dependencieswithpeerDependenciesWithoutOwn: the package's own name counts as an own dep, and apeerDependenciesMeta-only entry becomes a peer only whenoptional: true. The non-autoInstallPeersarm (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: a matched peer is deleted on-, rewritten in place when the override value is a valid peer range, otherwise written intodependencieswhile the declared peer stays. Fixes e.g.ajv@>=7.0.0-alpha.0 <8.18.0 → >=8.18.0not rewriting peer ranges and@yarnpkg/libziplosing 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, since #7812) andgetHoistableOptionalPeersreads that map after each wave — so an optional peer with a realpeerDependenciesentry (eslint'sjiti) resolves against a provider anywhere in the freshly resolved tree. What keepsdebug'ssupports-colorbare is not a static map but the missing-peer set:getMissingPeersiteratespeerDependenciesonly, so a meta-only peer never feeds the hoist. Both behaviors verified empirically against pnpm 11.6.0 (eslint+cosmiconfig-typescript-loaderhoistsjiti@2.6.1;debug+concurrentlystays 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
pacquet-resolving-deps-resolver(importer group merge ×4, peer-vs-own-dep shadowing ×3, optional-peer hoist real-entry/meta-only ×2) andpacquet-package-manager(peer overrides ×4).--deny warningsclean; rustfmt + typos clean.@pnpm/logger,jiti,typanion,@babel/types,ajvcategories eliminated. Two consecutive pacquet runs byte-identical.Remaining gaps (tracked in #12266)
The surviving 461 lines are dominated by a hoisted optional-peer version-pick divergence (
@types/node22.19.21 vs 25.9.3,spdx-expression-parse4.0.0 vs 3.0.1 — pacquet also emits duplicate suffix variants of the same snapshot), plus small items:es-abstract/varintversion picks, the array-formenginesquirk, and the env-lockfile document's missingpackageManagerDependenciesblock andlibc:fields.Written by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
New Features
Improvements