Skip to content

fix(pacquet): match pnpm lockfile resolution#12372

Merged
zkochan merged 2 commits into
mainfrom
codex/fix-12330-lockfile-parity
Jun 13, 2026
Merged

fix(pacquet): match pnpm lockfile resolution#12372
zkochan merged 2 commits into
mainfrom
codex/fix-12330-lockfile-parity

Conversation

@zkochan

@zkochan zkochan commented Jun 13, 2026

Copy link
Copy Markdown
Member

Summary

  • Match pacquet peer-resolution and lockfile output to pnpm for transitive optional peer variants.
  • Apply pnpm's Yarn compatibility package extensions in pacquet, with ignoreCompatibilityDb support.
  • Add regression coverage on both the pnpm resolver test and pacquet resolver/install paths.

Fixes #12330.

Verification

  • pnpm --filter @pnpm/installing.deps-resolver test test/resolvePeers.ts -t "transitive pending peer uses provider final suffix"
  • cargo test -p pacquet-resolving-deps-resolver
  • cargo test -p pacquet-config parses_ignore_compatibility_db_from_yaml_and_applies
  • cargo test -p pacquet-package-manager compatibility_db
  • cargo test -p pacquet-package-manager duplicate_manifest_alias_uses_pnpm_dependency_field_precedence
  • cargo test -p pacquet-resolving-npm-resolver version_range_lte_partial_allows_entire_major
  • cargo build --release -p pacquet-cli
  • Babylon fixture lockfile SHA-256 matched pnpm: 73db2a62f695f50a3816575c84b8bc82f9106051cb3d3698ccde9754aa890b26
  • Pre-push hook passed for codex/fix-12330-lockfile-parity

Note: just ready was attempted separately, but its repo-wide typos step currently fails on existing changelog/hash false positives outside this PR.


Written by an agent (Codex, GPT-5).

Summary by CodeRabbit

  • New Features

    • Added ignore_compatibility_db config option
    • Added resolve_peers_from_workspace_root workspace option
    • Integrated built-in compatibility package-extensions DB and exposed getEffectivePackageExtensions
  • Improvements

    • More accurate and deterministic peer dependency resolution, deduplication, and depPath finalization
    • Lockfile stability and checksum handling improvements when package-extensions are applied
    • Enhanced semver range handling for partial "<=" comparators
  • Tests

    • Many new tests and fixtures covering peer-resolution, lockfile, workspace, and transitive peer scenarios

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds compatibility-package extensions and config wiring; applies compat extender at install-time; refactors resolve_peers for SCC-aware deterministic final depPath and graph merging; adjusts workspace cache scoping and lockfile dependency-field precedence; normalizes partial <= semver; and adds tests and fixtures.

Changes

All-in-one review checkpoint

Layer / File(s) Summary
Read-package hook & tests
hooks/read-package-hook/...
Exports getEffectivePackageExtensions, merges compatibility DB + user packageExtensions, conditionally registers package extender hook, and adds unit tests.
Config/env workspace wiring
pacquet/crates/config/src/{lib.rs,env_overlay.rs,workspace_yaml.rs}, tests
Adds Config.ignore_compatibility_db, workspace YAML field, env var mapping, and tests validating YAML/env propagation.
Compat DB dataset & loader
pacquet/crates/package-manager/src/compat_package_extensions.{rs,json}, pacquet/crates/package-manager/src/lib.rs
Embeds compat_package_extensions.json, adds lazy parsing and merging, exposes an Arc for install-time use, and merges selector collisions.
Install-time compat wiring & checksum
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs, .../install.rs, tests
Applies compat extender to cloned importer manifests, composes manifest hooks with compat first, precomputes package_extensions_checksum, and updates lockfile reuse/drift checks; adds fresh-lockfile tests.
Lockfile alias precedence
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs, tests
Changes manifest alias precedence to prefer optionalDependencies over dependencies/devDependencies and updates lookup order; adds test coverage.
Workspace/cache scoping & resolve-peers option
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs, resolve_workspace.rs, tests
Adds project_relative_cache_scope including project_dir for workspace semver cases, threads resolve_peers_from_workspace_root option, and adds workspace-link caching and root-driven peer resolution tests.
Resolve-peers finalization & graph rebuild
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs, tests
Major refactor: walker state, auto_install_resolved_peers propagation, provisional/final depPath helpers, SCC/cyclic-name-driven final_peer_id collapse, deterministic ordering, build_final_graph rewrite, and many new tests.
Importer peer injection & dedupe gating
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs, dedupe_peer_dependents.rs, tests
Appends resolved peer providers into importer direct deps when appropriate, and gates dedupe collapsing on peer-compatibility checks; adds tests for preserved incompatible variants.
Transitive pending peer regressions & fixtures
pnpr/.fixtures/*, pnpm/test/*, installer/resolver/CLI tests
Adds fixtures and multiple tests asserting that transitive pending peers use provider final suffix in lockfiles and resolver outputs.
Semver partial-<= normalization & misc
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs, tests, .typos.toml
Normalizes partial <= ranges before parsing, adds overflow tests, and adds one typos mapping.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • pnpm/pnpm#12273: Closely related peer parent-context and resolve_peers refactor.
  • pnpm/pnpm#12365: Overlapping resolve_peers cycle/finalization work and tests.
  • pnpm/pnpm#12128: Related changes to WantedKey cache scoping for project-relative resolutions.

"I'm a rabbit in a resolver tree,
I hop through peers and chase the key,
I stitch a compat DB with care,
I chase cyclic names until they're fair,
Lockfiles hum — deterministic and free!"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(pacquet): match pnpm lockfile resolution' is concise and directly relevant to the main change in the changeset, which focuses on aligning pacquet's peer-resolution and lockfile output with pnpm for transitive optional peer variants.
Linked Issues check ✅ Passed The PR comprehensively addresses the linked issue #12330 by implementing deterministic peer-resolution matching to pnpm, adding optional-peer handling fixes, compatibility DB support, and extensive test coverage across resolver and install paths as specified.
Out of Scope Changes check ✅ Passed All changes are within scope of the linked objectives: peer-resolution determinism fixes, pnpm compatibility DB integration, configuration support for ignoreCompatibilityDb, related test fixtures, and supporting infrastructure changes are all directly aligned with addressing issue #12330.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-12330-lockfile-parity

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      5.1±0.09ms   856.1 KB/sec    1.00      5.1±0.17ms   855.7 KB/sec

@codecov-commenter

codecov-commenter commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.22857% with 68 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.27%. Comparing base (f20ad8f) to head (2f3c4a3).

Files with missing lines Patch % Lines
...s/package-manager/src/compat_package_extensions.rs 50.00% 29 Missing ⚠️
...rates/resolving-deps-resolver/src/resolve_peers.rs 96.59% 20 Missing ⚠️
...olving-deps-resolver/src/dedupe_peer_dependents.rs 91.89% 9 Missing ⚠️
...solving-npm-resolver/src/pick_package_from_meta.rs 81.81% 8 Missing ⚠️
...es/resolving-deps-resolver/src/resolve_importer.rs 94.44% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12372      +/-   ##
==========================================
+ Coverage   88.19%   88.27%   +0.07%     
==========================================
  Files         296      297       +1     
  Lines       37951    38705     +754     
==========================================
+ Hits        33471    34165     +694     
- Misses       4480     4540      +60     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zkochan zkochan force-pushed the codex/fix-12330-lockfile-parity branch from 8e34de7 to 05636c4 Compare June 13, 2026 10:26
@zkochan zkochan marked this pull request as ready for review June 13, 2026 10:30
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (6) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

Grey Divider


Action required

1. Peer provider merge mismatch 🐞 Bug ≡ Correctness
Description
In Walker::walk, resolved_peer_providers_by_alias uses or_insert, making the *first* seen
provider for a peer alias win, which diverges from pnpm’s Object.assign-based merging (last write
wins) and from pacquet’s own overwrite-merge used when bubbling providers from children. This can
select a different provider NodeId to append as a hidden direct dep, changing auto-install-peers
behavior and potentially lockfile parity.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R665-666]

+            for (peer_alias, peer_node_id) in output.auto_install_resolved_peers {
+                self.resolved_peer_providers_by_alias.entry(peer_alias).or_insert(peer_node_id);
Evidence
The PR introduces a new aggregation of auto_install_resolved_peers where the top-level merge is
first-write-wins, while pacquet’s child merge and pnpm’s resolvedPeers merge are overwrite-based;
since the aggregated map drives hidden direct-dep appends, a different merge policy can change which
provider is appended.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[641-667]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[999-1015]
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs[508-535]
installing/deps-resolver/src/resolveDependencies.ts[594-612]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Walker::walk` aggregates `output.auto_install_resolved_peers` into `resolved_peer_providers_by_alias` via `entry(...).or_insert(...)`, so the first provider encountered for a given peer alias is kept and later providers are dropped. pnpm’s equivalent `resolvedPeers` aggregation uses `Object.assign`, which overwrites earlier values (last write wins), and pacquet already uses overwrite semantics when merging these maps from child outputs.
## Issue Context
This map is consumed by `ResolveImporter::append_resolved_peer_providers` to append hidden direct dependencies used by the auto-install-peers loop.
## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[655-667]
## Suggested change
Replace `entry(...).or_insert(...)` with an overwrite insert (e.g. `self.resolved_peer_providers_by_alias.insert(peer_alias, peer_node_id);`) so duplicate aliases resolve consistently with pnpm-style merging and with the overwrite behavior already used when combining child outputs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Alias edges deduped away 🐞 Bug ≡ Correctness
Description
ordered_child_entries de-duplicates child edges by NodeId, so when multiple aliases map to the
same leaf NodeId only the first alias is walked and inserted into child_dep_paths. This can
silently omit dependency edges from the final DependenciesGraph, leading to missing alias entries
in install/lockfile output.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R2550-2564]

+fn ordered_child_entries(
+    children: &BTreeMap<String, NodeId>,
+    parent_refs: &ParentRefs,
+) -> Vec<(String, NodeId)> {
+    let mut out = Vec::with_capacity(children.len());
+    let mut seen = HashSet::new();
+    for repeated in [true, false] {
+        for (alias, node_id) in children {
+            if parent_refs.contains_key(alias) != repeated {
+                continue;
+            }
+            if seen.insert(node_id.clone()) {
+                out.push((alias.clone(), node_id.clone()));
+            }
+        }
Evidence
The helper drops distinct alias edges whenever the same NodeId repeats, but leaf NodeIds are
intentionally reused (derived from pkgIdWithPatchHash, not alias). Since the child-walk loop only
inserts child_dep_paths for aliases returned by ordered_child_entries, any dropped alias is
omitted from the node’s recorded edges and thus from the rebuilt DependenciesGraph.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2564]
pacquet/crates/resolving-deps-resolver/src/node_id.rs[15-43]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1424-1427]
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs[184-219]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ordered_child_entries()` currently filters children using a `seen: HashSet<NodeId>`, which drops later `(alias, node_id)` pairs when the same `NodeId` appears under multiple aliases. Because `resolve_node()` later builds `child_dep_paths` only from the returned list, dropped aliases disappear from the graph.
This is incorrect when a parent legitimately has two aliases pointing at the same leaf node (leaf `NodeId`s are derived from package id and are reused), because both alias edges must remain in the dependency graph.
## Issue Context
- Leaf nodes reuse `NodeId::Leaf(pkg_id)` across occurrences, so different aliases can share the same `NodeId`.
- `DependenciesTreeNode` children are keyed by alias and can contain multiple aliases pointing to the same `NodeId`.
## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2566]
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
## Suggested fix
Option A (simplest, safest):
- Remove the `seen` de-duplication from `ordered_child_entries()` so it returns **all** `(alias, node_id)` pairs in deterministic order.
Option B (keep perf optimization without losing edges):
- Keep `ordered_child_entries()` returning all pairs.
- In the loop that processes `child_entries`, memoize `child_output` per `NodeId` (e.g., `HashMap<NodeId, NodeOutput>`) so each unique `NodeId` is resolved once, but `child_dep_paths.insert(alias, dep_path)` is still performed for every alias.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

3. Recursive Tarjan stack overflow 🐞 Bug ☼ Reliability ⭐ New
Description
cyclic_peer_names() implements Tarjan SCC via a recursive strongconnect() which can overflow the
process stack on sufficiently deep peer-name graphs, crashing peer resolution. This file already
uses an iterative Tarjan for node SCCs, so this recursive path is an outlier on a graph-shaped
input.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R1516-1523]

+                if let Some(neighbors) = self.graph.get(name) {
+                    for child in neighbors {
+                        if !self.index_of.contains_key(child) {
+                            self.strongconnect(child);
+                            let name_low = self.low_of[name];
+                            let child_low = self.low_of[child];
+                            self.low_of.insert(name.to_string(), name_low.min(child_low));
+                        } else if self.on_stack.contains(child) {
Evidence
The code shows a direct recursive call in the Tarjan walk for peer names, which can recurse once per
edge on long chains; nearby code documents the need for an iterative Tarjan to avoid deep-recursion
issues.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[1478-1566]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[1568-1572]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`cyclic_peer_names()` uses a recursive Tarjan DFS (`self.strongconnect(child)`), which can stack overflow on large/deep peer-name graphs.

## Issue Context
`peer_sccs()` in the same file explicitly uses an iterative Tarjan to avoid deep recursion; `cyclic_peer_names()` should follow the same robustness approach.

## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[1478-1566]
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[1568-1572]

## Implementation notes
- Replace the recursive `PeerNameTarjan::strongconnect` with an explicit work stack (similar to the iterative Tarjan in `peer_sccs`).
- Preserve determinism (the graph is a `BTreeMap<String, BTreeSet<String>>`, so neighbor iteration order is already stable).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Peerless trimming unreachable 🐞 Bug ≡ Correctness ⭐ New
Description
dep_path_with_allowed_peer_segments() returns None when filtering removes all peer segments
(kept.is_empty()), so final_peer_edge_dep_path() cannot rewrite a peer edge to a peerless
depPath (pkgIdWithPatchHash) unless that peerless variant already exists in variants_by_pkg_id. In
that case the logic falls back to selecting an existing subset variant or returning the original
depPath.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R2439-2441]

+    if kept.is_empty() || kept.len() == segments.len() {
+        return None;
+    }
Evidence
The helper explicitly returns None when all segments would be removed, and the caller only
performs trimming when it receives Some(trimmed), otherwise it falls through to existing-variant
selection or returning the original depPath.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[1806-1875]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2423-2452]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`dep_path_with_allowed_peer_segments()` treats `kept.is_empty()` as "no rewrite" and returns `None`, which prevents `final_peer_edge_dep_path()` from ever producing a peerless depPath via trimming when *all* peer segments are disallowed.

## Issue Context
`final_peer_edge_dep_path()` only attempts a rewrite/synthetic node when `dep_path_with_allowed_peer_segments()` returns `Some(trimmed)`. When it returns `None`, the function can only choose a pre-existing subset variant (including a peerless one) or fall back to `original`.

## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[1806-1875]
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2423-2452]

## Implementation notes
- Change `dep_path_with_allowed_peer_segments()` so that when `kept.is_empty()` it returns `Some(DepPath::from(raw[..peers_index] + patch_hash_if_any))` (i.e. remove the entire peer suffix), rather than `None`.
- Ensure caller behavior remains correct when the trimmed depPath is peerless:
 - If the peerless variant already exists, return it.
 - Otherwise, allow synthesizing a peerless variant node when needed (consistent with existing synthetic-node handling for partially-trimmed cases).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Bad package name parsing 🐞 Bug ≡ Correctness
Description
dedupe_peer_dependents derives a package name by slicing resolved_package_id at the last '@',
but resolved_package_id can be a raw non-registry identifier that contains '@' (e.g. git SSH IDs).
This produces bogus names that can make the peer-compatibility check incorrectly skip (or
mis-evaluate) edge rewrites, leaving extra peer variants in the graph/lockfile.
Code

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs[R282-284]

+fn package_name_from_pkg_id(pkg_id: &str) -> String {
+    pkg_id.rfind('@').map_or_else(|| pkg_id.to_string(), |index| pkg_id[..index].to_string())
+}
Evidence
The new compatibility logic adds package_name_from_pkg_id() and uses it to augment the peer-name
sets used for collapse/rewrite decisions. However, the resolver pipeline can produce
resolved_package_id values that are not simple name@version strings (notably git IDs, which can
include git@…), so splitting on the last @ can yield a non-package name and skew the
compatibility predicate.

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs[276-284]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[2386-2417]
pacquet/crates/resolving-git-resolver/src/git_resolver.rs[114-140]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`package_name_from_pkg_id()` assumes `resolved_package_id` is always in `name@version` form and extracts the name by taking everything before the last `@`. This breaks for non-registry IDs that legitimately contain `@` (notably git SSH IDs like `git@github.com:…`, and potentially other raw IDs).
The extracted value is then used in `collapsed_target_matches_parent()` compatibility checks, which can change whether dep-path variants are rewritten/collapsed.
## Issue Context
`resolved_package_id` is not guaranteed to be `name@version`:
- For git resolutions, `ResolveResult.id` can be a git identifier containing `@` (e.g. `git@…`) and `name_ver` may be `None`.
- `build_pkg_id_with_patch_hash()` can also return the raw id in some cases (no `name_ver` and no manifest name).
So name extraction must not depend on `rfind('@')` over the whole id string.
## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs[276-284]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
6. Compat extender cloned repeatedly 🐞 Bug ➹ Performance
Description
compat_package_extender() returns a .clone() of a cached PackageExtender, which deep-clones
the full in-memory compatibility DB structure each time it’s requested. This is avoidable overhead
because the compatibility DB is immutable and can be shared via Arc/static reference.
Code

pacquet/crates/package-manager/src/compat_package_extensions.rs[R9-16]

+pub(crate) fn compat_package_extender() -> PackageExtender {
+    COMPAT_PACKAGE_EXTENDER
+        .get_or_init(|| {
+            PackageExtender::new(compat_package_extensions())
+                .expect("@yarnpkg/extensions compatibility DB selectors are valid")
+        })
+        .clone()
+}
Evidence
The compat extender accessor explicitly clones the cached PackageExtender, and PackageExtender
is a Cloned struct containing an owned HashMap, making the clone a deep copy rather than a cheap
handle clone.

pacquet/crates/package-manager/src/compat_package_extensions.rs[6-16]
pacquet/crates/package-manager/src/package_extender.rs[64-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`compat_package_extender()` clones a `PackageExtender` retrieved from a `OnceLock`, which deep-clones a `HashMap<String, Vec<...>>` of all compat selectors on each call. The database is immutable and should be shared.
## Issue Context
Callers already wrap the returned extender in an `Arc`, but the expensive clone happens before that.
## Fix Focus Areas
- pacquet/crates/package-manager/src/compat_package_extensions.rs[6-16]
## Suggested change
Change the cache to store an `Arc<PackageExtender>` (e.g. `OnceLock<Arc<PackageExtender>>`) and have `compat_package_extender()` return `Arc<PackageExtender>` (or a `&'static PackageExtender`) so callers can clone the `Arc` cheaply rather than cloning the whole structure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 2f3c4a3

Results up to commit 56c6a45


🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

Action required
1. Peer provider merge mismatch 🐞 Bug ≡ Correctness
Description
In Walker::walk, resolved_peer_providers_by_alias uses or_insert, making the *first* seen
provider for a peer alias win, which diverges from pnpm’s Object.assign-based merging (last write
wins) and from pacquet’s own overwrite-merge used when bubbling providers from children. This can
select a different provider NodeId to append as a hidden direct dep, changing auto-install-peers
behavior and potentially lockfile parity.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R665-666]

+            for (peer_alias, peer_node_id) in output.auto_install_resolved_peers {
+                self.resolved_peer_providers_by_alias.entry(peer_alias).or_insert(peer_node_id);
Evidence
The PR introduces a new aggregation of auto_install_resolved_peers where the top-level merge is
first-write-wins, while pacquet’s child merge and pnpm’s resolvedPeers merge are overwrite-based;
since the aggregated map drives hidden direct-dep appends, a different merge policy can change which
provider is appended.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[641-667]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[999-1015]
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs[508-535]
installing/deps-resolver/src/resolveDependencies.ts[594-612]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Walker::walk` aggregates `output.auto_install_resolved_peers` into `resolved_peer_providers_by_alias` via `entry(...).or_insert(...)`, so the first provider encountered for a given peer alias is kept and later providers are dropped. pnpm’s equivalent `resolvedPeers` aggregation uses `Object.assign`, which overwrites earlier values (last write wins), and pacquet already uses overwrite semantics when merging these maps from child outputs.
## Issue Context
This map is consumed by `ResolveImporter::append_resolved_peer_providers` to append hidden direct dependencies used by the auto-install-peers loop.
## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[655-667]
## Suggested change
Replace `entry(...).or_insert(...)` with an overwrite insert (e.g. `self.resolved_peer_providers_by_alias.insert(peer_alias, peer_node_id);`) so duplicate aliases resolve consistently with pnpm-style merging and with the overwrite behavior already used when combining child outputs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Alias edges deduped away 🐞 Bug ≡ Correctness
Description
ordered_child_entries de-duplicates child edges by NodeId, so when multiple aliases map to the
same leaf NodeId only the first alias is walked and inserted into child_dep_paths. This can
silently omit dependency edges from the final DependenciesGraph, leading to missing alias entries
in install/lockfile output.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R2550-2564]

+fn ordered_child_entries(
+    children: &BTreeMap<String, NodeId>,
+    parent_refs: &ParentRefs,
+) -> Vec<(String, NodeId)> {
+    let mut out = Vec::with_capacity(children.len());
+    let mut seen = HashSet::new();
+    for repeated in [true, false] {
+        for (alias, node_id) in children {
+            if parent_refs.contains_key(alias) != repeated {
+                continue;
+            }
+            if seen.insert(node_id.clone()) {
+                out.push((alias.clone(), node_id.clone()));
+            }
+        }
Evidence
The helper drops distinct alias edges whenever the same NodeId repeats, but leaf NodeIds are
intentionally reused (derived from pkgIdWithPatchHash, not alias). Since the child-walk loop only
inserts child_dep_paths for aliases returned by ordered_child_entries, any dropped alias is
omitted from the node’s recorded edges and thus from the rebuilt DependenciesGraph.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2564]
pacquet/crates/resolving-deps-resolver/src/node_id.rs[15-43]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1424-1427]
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs[184-219]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ordered_child_entries()` currently filters children using a `seen: HashSet<NodeId>`, which drops later `(alias, node_id)` pairs when the same `NodeId` appears under multiple aliases. Because `resolve_node()` later builds `child_dep_paths` only from the returned list, dropped aliases disappear from the graph.
This is incorrect when a parent legitimately has two aliases pointing at the same leaf node (leaf `NodeId`s are derived from package id and are reused), because both alias edges must remain in the dependency graph.
## Issue Context
- Leaf nodes reuse `NodeId::Leaf(pkg_id)` across occurrences, so different aliases can share the same `NodeId`.
- `DependenciesTreeNode` children are keyed by alias and can contain multiple aliases pointing to the same `NodeId`.
## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2566]
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
## Suggested fix
Option A (simplest, safest):
- Remove the `seen` de-duplication from `ordered_child_entries()` so it returns **all** `(alias, node_id)` pairs in deterministic order.
Option B (keep perf optimization without losing edges):
- Keep `ordered_child_entries()` returning all pairs.
- In the loop that processes `child_entries`, memoize `child_output` per `NodeId` (e.g., `HashMap<NodeId, NodeOutput>`) so each unique `NodeId` is resolved once, but `child_dep_paths.insert(alias, dep_path)` is still performed for every alias.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
3. Bad package name parsing 🐞 Bug ≡ Correctness ⭐ New
Description
dedupe_peer_dependents derives a package name by slicing resolved_package_id at the last '@',
but resolved_package_id can be a raw non-registry identifier that contains '@' (e.g. git SSH IDs).
This produces bogus names that can make the peer-compatibility check incorrectly skip (or
mis-evaluate) edge rewrites, leaving extra peer variants in the graph/lockfile.
Code

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs[R282-284]

+fn package_name_from_pkg_id(pkg_id: &str) -> String {
+    pkg_id.rfind('@').map_or_else(|| pkg_id.to_string(), |index| pkg_id[..index].to_string())
+}
Evidence
The new compatibility logic adds package_name_from_pkg_id() and uses it to augment the peer-name
sets used for collapse/rewrite decisions. However, the resolver pipeline can produce
resolved_package_id values that are not simple name@version strings (notably git IDs, which can
include git@…), so splitting on the last @ can yield a non-package name and skew the
compatibility predicate.

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs[276-284]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[2386-2417]
pacquet/crates/resolving-git-resolver/src/git_resolver.rs[114-140]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`package_name_from_pkg_id()` assumes `resolved_package_id` is always in `name@version` form and extracts the name by taking everything before the last `@`. This breaks for non-registry IDs that legitimately contain `@` (notably git SSH IDs like `git@github.com:…`, and potentially other raw IDs).

The extracted value is then used in `collapsed_target_matches_parent()` compatibility checks, which can change whether dep-path variants are rewritten/collapsed.

## Issue Context
`resolved_package_id` is not guaranteed to be `name@version`:
- For git resolutions, `ResolveResult.id` can be a git identifier containing `@` (e.g. `git@…`) and `name_ver` may be `None`.
- `build_pkg_id_with_patch_hash()` can also return the raw id in some cases (no `name_ver` and no manifest name).

So name extraction must not depend on `rfind('@')` over the whole id string.

## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs[276-284]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


4. Compat extender cloned repeatedly 🐞 Bug ➹ Performance
Description
compat_package_extender() returns a .clone() of a cached PackageExtender, which deep-clones
the full in-memory compatibility DB structure each time it’s requested. This is avoidable overhead
because the compatibility DB is immutable and can be shared via Arc/static reference.
Code

pacquet/crates/package-manager/src/compat_package_extensions.rs[R9-16]

+pub(crate) fn compat_package_extender() -> PackageExtender {
+    COMPAT_PACKAGE_EXTENDER
+        .get_or_init(|| {
+            PackageExtender::new(compat_package_extensions())
+                .expect("@yarnpkg/extensions compatibility DB selectors are valid")
+        })
+        .clone()
+}
Evidence
The compat extender accessor explicitly clones the cached PackageExtender, and PackageExtender
is a Cloned struct containing an owned HashMap, making the clone a deep copy rather than a cheap
handle clone.

pacquet/crates/package-manager/src/compat_package_extensions.rs[6-16]
pacquet/crates/package-manager/src/package_extender.rs[64-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`compat_package_extender()` clones a `PackageExtender` retrieved from a `OnceLock`, which deep-clones a `HashMap<String, Vec<...>>` of all compat selectors on each call. The database is immutable and should be shared.
## Issue Context
Callers already wrap the returned extender in an `Arc`, but the expensive clone happens before that.
## Fix Focus Areas
- pacquet/crates/package-manager/src/compat_package_extensions.rs[6-16]
## Suggested change
Change the cache to store an `Arc<PackageExtender>` (e.g. `OnceLock<Arc<PackageExtender>>`) and have `compat_package_extender()` return `Arc<PackageExtender>` (or a `&'static PackageExtender`) so callers can clone the `Arc` cheaply rather than cloning the whole structure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 56c6a45


🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

Action required
1. Peer provider merge mismatch 🐞 Bug ≡ Correctness ⭐ New
Description
In Walker::walk, resolved_peer_providers_by_alias uses or_insert, making the *first* seen
provider for a peer alias win, which diverges from pnpm’s Object.assign-based merging (last write
wins) and from pacquet’s own overwrite-merge used when bubbling providers from children. This can
select a different provider NodeId to append as a hidden direct dep, changing auto-install-peers
behavior and potentially lockfile parity.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R665-666]

+            for (peer_alias, peer_node_id) in output.auto_install_resolved_peers {
+                self.resolved_peer_providers_by_alias.entry(peer_alias).or_insert(peer_node_id);
Evidence
The PR introduces a new aggregation of auto_install_resolved_peers where the top-level merge is
first-write-wins, while pacquet’s child merge and pnpm’s resolvedPeers merge are overwrite-based;
since the aggregated map drives hidden direct-dep appends, a different merge policy can change which
provider is appended.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[641-667]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[999-1015]
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs[508-535]
installing/deps-resolver/src/resolveDependencies.ts[594-612]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`Walker::walk` aggregates `output.auto_install_resolved_peers` into `resolved_peer_providers_by_alias` via `entry(...).or_insert(...)`, so the first provider encountered for a given peer alias is kept and later providers are dropped. pnpm’s equivalent `resolvedPeers` aggregation uses `Object.assign`, which overwrites earlier values (last write wins), and pacquet already uses overwrite semantics when merging these maps from child outputs.

## Issue Context
This map is consumed by `ResolveImporter::append_resolved_peer_providers` to append hidden direct dependencies used by the auto-install-peers loop.

## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[655-667]

## Suggested change
Replace `entry(...).or_insert(...)` with an overwrite insert (e.g. `self.resolved_peer_providers_by_alias.insert(peer_alias, peer_node_id);`) so duplicate aliases resolve consistently with pnpm-style merging and with the overwrite behavior already used when combining child outputs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Alias edges deduped away 🐞 Bug ≡ Correctness
Description
ordered_child_entries de-duplicates child edges by NodeId, so when multiple aliases map to the
same leaf NodeId only the first alias is walked and inserted into child_dep_paths. This can
silently omit dependency edges from the final DependenciesGraph, leading to missing alias entries
in install/lockfile output.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R2550-2564]

+fn ordered_child_entries(
+    children: &BTreeMap<String, NodeId>,
+    parent_refs: &ParentRefs,
+) -> Vec<(String, NodeId)> {
+    let mut out = Vec::with_capacity(children.len());
+    let mut seen = HashSet::new();
+    for repeated in [true, false] {
+        for (alias, node_id) in children {
+            if parent_refs.contains_key(alias) != repeated {
+                continue;
+            }
+            if seen.insert(node_id.clone()) {
+                out.push((alias.clone(), node_id.clone()));
+            }
+        }
Evidence
The helper drops distinct alias edges whenever the same NodeId repeats, but leaf NodeIds are
intentionally reused (derived from pkgIdWithPatchHash, not alias). Since the child-walk loop only
inserts child_dep_paths for aliases returned by ordered_child_entries, any dropped alias is
omitted from the node’s recorded edges and thus from the rebuilt DependenciesGraph.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2564]
pacquet/crates/resolving-deps-resolver/src/node_id.rs[15-43]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1424-1427]
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs[184-219]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ordered_child_entries()` currently filters children using a `seen: HashSet<NodeId>`, which drops later `(alias, node_id)` pairs when the same `NodeId` appears under multiple aliases. Because `resolve_node()` later builds `child_dep_paths` only from the returned list, dropped aliases disappear from the graph.
This is incorrect when a parent legitimately has two aliases pointing at the same leaf node (leaf `NodeId`s are derived from package id and are reused), because both alias edges must remain in the dependency graph.
## Issue Context
- Leaf nodes reuse `NodeId::Leaf(pkg_id)` across occurrences, so different aliases can share the same `NodeId`.
- `DependenciesTreeNode` children are keyed by alias and can contain multiple aliases pointing to the same `NodeId`.
## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2566]
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
## Suggested fix
Option A (simplest, safest):
- Remove the `seen` de-duplication from `ordered_child_entries()` so it returns **all** `(alias, node_id)` pairs in deterministic order.
Option B (keep perf optimization without losing edges):
- Keep `ordered_child_entries()` returning all pairs.
- In the loop that processes `child_entries`, memoize `child_output` per `NodeId` (e.g., `HashMap<NodeId, NodeOutput>`) so each unique `NodeId` is resolved once, but `child_dep_paths.insert(alias, dep_path)` is still performed for every alias.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
3. Compat extender cloned repeatedly 🐞 Bug ➹ Performance ⭐ New
Description
compat_package_extender() returns a .clone() of a cached PackageExtender, which deep-clones
the full in-memory compatibility DB structure each time it’s requested. This is avoidable overhead
because the compatibility DB is immutable and can be shared via Arc/static reference.
Code

pacquet/crates/package-manager/src/compat_package_extensions.rs[R9-16]

+pub(crate) fn compat_package_extender() -> PackageExtender {
+    COMPAT_PACKAGE_EXTENDER
+        .get_or_init(|| {
+            PackageExtender::new(compat_package_extensions())
+                .expect("@yarnpkg/extensions compatibility DB selectors are valid")
+        })
+        .clone()
+}
Evidence
The compat extender accessor explicitly clones the cached PackageExtender, and PackageExtender
is a Cloned struct containing an owned HashMap, making the clone a deep copy rather than a cheap
handle clone.

pacquet/crates/package-manager/src/compat_package_extensions.rs[6-16]
pacquet/crates/package-manager/src/package_extender.rs[64-67]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`compat_package_extender()` clones a `PackageExtender` retrieved from a `OnceLock`, which deep-clones a `HashMap<String, Vec<...>>` of all compat selectors on each call. The database is immutable and should be shared.

## Issue Context
Callers already wrap the returned extender in an `Arc`, but the expensive clone happens before that.

## Fix Focus Areas
- pacquet/crates/package-manager/src/compat_package_extensions.rs[6-16]

## Suggested change
Change the cache to store an `Arc<PackageExtender>` (e.g. `OnceLock<Arc<PackageExtender>>`) and have `compat_package_extender()` return `Arc<PackageExtender>` (or a `&'static PackageExtender`) so callers can clone the `Arc` cheaply rather than cloning the whole structure.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 05636c4


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used

Action required
1. Alias edges deduped away 🐞 Bug ≡ Correctness
Description
ordered_child_entries de-duplicates child edges by NodeId, so when multiple aliases map to the
same leaf NodeId only the first alias is walked and inserted into child_dep_paths. This can
silently omit dependency edges from the final DependenciesGraph, leading to missing alias entries
in install/lockfile output.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[R2550-2564]

+fn ordered_child_entries(
+    children: &BTreeMap<String, NodeId>,
+    parent_refs: &ParentRefs,
+) -> Vec<(String, NodeId)> {
+    let mut out = Vec::with_capacity(children.len());
+    let mut seen = HashSet::new();
+    for repeated in [true, false] {
+        for (alias, node_id) in children {
+            if parent_refs.contains_key(alias) != repeated {
+                continue;
+            }
+            if seen.insert(node_id.clone()) {
+                out.push((alias.clone(), node_id.clone()));
+            }
+        }
Evidence
The helper drops distinct alias edges whenever the same NodeId repeats, but leaf NodeIds are
intentionally reused (derived from pkgIdWithPatchHash, not alias). Since the child-walk loop only
inserts child_dep_paths for aliases returned by ordered_child_entries, any dropped alias is
omitted from the node’s recorded edges and thus from the rebuilt DependenciesGraph.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2564]
pacquet/crates/resolving-deps-resolver/src/node_id.rs[15-43]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1424-1427]
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs[184-219]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ordered_child_entries()` currently filters children using a `seen: HashSet<NodeId>`, which drops later `(alias, node_id)` pairs when the same `NodeId` appears under multiple aliases. Because `resolve_node()` later builds `child_dep_paths` only from the returned list, dropped aliases disappear from the graph.

This is incorrect when a parent legitimately has two aliases pointing at the same leaf node (leaf `NodeId`s are derived from package id and are reused), because both alias edges must remain in the dependency graph.

## Issue Context
- Leaf nodes reuse `NodeId::Leaf(pkg_id)` across occurrences, so different aliases can share the same `NodeId`.
- `DependenciesTreeNode` children are keyed by alias and can contain multiple aliases pointing to the same `NodeId`.

## Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[2550-2566]
- pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs[994-1010]

## Suggested fix
Option A (simplest, safest):
- Remove the `seen` de-duplication from `ordered_child_entries()` so it returns **all** `(alias, node_id)` pairs in deterministic order.

Option B (keep perf optimization without losing edges):
- Keep `ordered_child_entries()` returning all pairs.
- In the loop that processes `child_entries`, memoize `child_output` per `NodeId` (e.g., `HashMap<NodeId, NodeOutput>`) so each unique `NodeId` is resolved once, but `child_dep_paths.insert(alias, dep_path)` is still performed for every alias.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Match pacquet peer resolution and lockfile output to pnpm (compat DB + pending peers)
🐞 Bug fix ✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

Grey Divider

Walkthroughs

Description
• Fix transitive peer depPaths to use providers' final peer suffixes, matching pnpm lockfile
  snapshots.
• Add pnpm/Yarn compatibility package extensions to pacquet with an ignoreCompatibilityDb switch.
• Align workspace peer resolution and caching behavior; expand regression coverage across TS and
  Rust.
Diagram
graph TD
  A["Installer / CLI"] --> B["Resolve workspace/importer"] --> C["resolve_peers"] --> D["DependenciesGraph"] --> E["Lockfile writer"]
  A --> F["Manifest hook"] --> G["Compat extensions DB"]
  B --> H["NPM meta picker"]
  subgraph Legend
    direction LR
    _svc([Service/Module]) ~~~ _data[(Data/Artifact)]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Port pnpm depPath/analyzeGraph logic verbatim
  • ➕ Lower divergence risk by matching upstream control flow closely
  • ➕ Potentially easier future parity fixes (diff against upstream)
  • ➖ More intrusive refactor; harder to integrate with existing pacquet data structures
  • ➖ Risk of regressions in non-peer resolution paths
2. Store explicit 'final depPath' on nodes instead of recompute pass
  • ➕ Avoids recursive finalization and visiting/cycle handling during rebuild
  • ➕ May simplify lockfile edge rewriting and pending peer patching
  • ➖ Requires broader structural changes across graph building and caching
  • ➖ Harder to keep provisional vs final semantics correct during walk
3. Fetch compat DB at build/install time rather than vendoring JSON
  • ➕ Always up-to-date with upstream @yarnpkg/extensions database
  • ➕ Smaller repo footprint
  • ➖ Introduces network/build reproducibility concerns
  • ➖ More moving parts and version pinning to preserve lockfile determinism

Recommendation: The PR’s approach (track resolver-stage resolved peer providers, finalize depPaths with full peer depPaths, and vendor a deterministic compat-extension DB behind a config flag) is the best balance for pnpm lockfile parity and reproducibility. The added safeguards (cycle-name handling, deterministic node ordering, and extensive regression tests) reduce the risk inherent in peer/depPath changes; a verbatim upstream port or structural final-depPath redesign would be higher blast-radius.

Grey Divider

File Changes

Enhancement (7)
createReadPackageHook.ts Merge compat DB + user packageExtensions into one effective extender +68/-5

Merge compat DB + user packageExtensions into one effective extender

• Introduces getEffectivePackageExtensions() to merge @yarnpkg/extensions compatibility entries with user-defined packageExtensions and honor ignoreCompatibilityDb. Adds deterministic field-wise merging to handle duplicate selectors in the compat database.

hooks/read-package-hook/src/createReadPackageHook.ts


index.ts Export getEffectivePackageExtensions helper +1/-1

Export getEffectivePackageExtensions helper

• Re-exports getEffectivePackageExtensions alongside createReadPackageHook for external use/tests.

hooks/read-package-hook/src/index.ts


compat_package_extensions.json Vendor @yarnpkg/extensions compatibility package-extensions database +1602/-0

Vendor @yarnpkg/extensions compatibility package-extensions database

• Adds a bundled JSON database of compatibility package extensions used to patch known-broken manifests during resolution.

pacquet/crates/package-manager/src/compat_package_extensions.json


compat_package_extensions.rs Load and merge compat extensions into a reusable PackageExtender +73/-0

Load and merge compat extensions into a reusable PackageExtender

• Implements a OnceLock-cached loader for the JSON DB and merges duplicate selectors deterministically into a PackageExtender.

pacquet/crates/package-manager/src/compat_package_extensions.rs


install_with_fresh_lockfile.rs Apply built-in compat extensions and support resolvePeersFromWorkspaceRoot +23/-13

Apply built-in compat extensions and support resolvePeersFromWorkspaceRoot

• Applies the bundled compatibility package extender ahead of user packageExtensions/overrides unless ignore_compatibility_db is set. Computes packageExtensionsChecksum once and threads resolve_peers_from_workspace_root into workspace resolve options.

pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs


lib.rs Wire compat_package_extensions module into package-manager crate +1/-0

Wire compat_package_extensions module into package-manager crate

• Adds the compat_package_extensions module to the crate module list so it can be used by install paths.

pacquet/crates/package-manager/src/lib.rs


resolve_workspace.rs Support resolvePeersFromWorkspaceRoot option +6/-0

Support resolvePeersFromWorkspaceRoot option

• Adds workspace resolve option resolve_peers_from_workspace_root and threads it into the workspace peer-resolution pass.

pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs


Bug fix (6)
dependencies_graph_to_lockfile.rs Match pnpm dependency field precedence for duplicate aliases +5/-5

Match pnpm dependency field precedence for duplicate aliases

• Adjusts direct-dependency group precedence so optionalDependencies wins over dependencies wins over devDependencies, matching pnpm's field ordering for specifiers/importer sections.

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs


dedupe_peer_dependents.rs Prevent collapsing peer variants when parent edges require incompatible suffixes +141/-2

Prevent collapsing peer variants when parent edges require incompatible suffixes

• Enhances dedupe_peer_dependents to only remap child depPaths when the collapsed target’s peer suffix requirements are compatible with the parent edge context, parsing peer names from depPath suffixes.

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs


resolve_dependency_tree.rs Scope wanted-resolution cache by project_dir for workspace-semver cases +21/-21

Scope wanted-resolution cache by project_dir for workspace-semver cases

• Expands cache key scoping so workspace mode semver specs (which may resolve to importer-relative link: paths) include project_dir, preventing cross-importer reuse of relative directory resolutions.

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs


resolve_importer.rs Append resolver-stage peer providers as hidden direct deps for auto-install +40/-2

Append resolver-stage peer providers as hidden direct deps for auto-install

• Captures resolved peer providers from resolve_peers and, when auto-install-peers is enabled, appends eligible providers into the importer's hidden direct dependencies to match pnpm’s resolver-stage resolvedPeers behavior.

pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs


resolve_peers.rs Finalize peer depPaths using provider final suffixes + track resolved providers +704/-106

Finalize peer depPaths using provider final suffixes + track resolved providers

• Reworks final depPath computation so peer IDs for transitive edges use providers’ fully-finalized depPaths, avoiding provisional suffix leakage into lockfile snapshots. Tracks resolver-stage resolved peer providers for auto-install-peers and adds deterministic ordering/cycle guards in finalization.

pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs


pick_package_from_meta.rs Normalize partial '<=' comparators to pnpm-like upper bounds +53/-1

Normalize partial '<=' comparators to pnpm-like upper bounds

• Normalizes ranges like "<=27" or "<= 27" to an exclusive prerelease upper bound (e.g., "<28.0.0-0") before parsing, matching expected selection behavior for partial comparators.

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs


Refactor (1)
install.rs Reuse shared packageExtensionsChecksum computation +2/-7

Reuse shared packageExtensionsChecksum computation

• Refactors lockfile drift checking to call the shared compute_package_extensions_checksum helper from install_with_fresh_lockfile.

pacquet/crates/package-manager/src/install.rs


Tests (18)
createReadPackageHook.ts Add test for merging duplicate compat selectors +10/-1

Add test for merging duplicate compat selectors

• Adds regression coverage ensuring duplicate selectors in the compatibility DB are merged into a single effective extension map.

hooks/read-package-hook/test/createReadPackageHook.ts


peerDependencies.ts Add pnpm installer test for pending transitive peer final suffix +19/-0

Add pnpm installer test for pending transitive peer final suffix

• Adds a lockfile snapshot assertion that transitive pending peers use the provider's final peer suffix, not a provisional one.

installing/deps-installer/test/install/peerDependencies.ts


resolvePeers.ts Add pnpm resolver test for provider final suffix in transitive pending peer +90/-0

Add pnpm resolver test for provider final suffix in transitive pending peer

• Adds a minimal resolvePeers graph test covering the pending-peer scenario where the provider’s final suffix must be used transitively.

installing/deps-resolver/test/resolvePeers.ts


install.rs CLI regression test for final peer suffix in pnpm-lock.yaml +35/-0

CLI regression test for final peer suffix in pnpm-lock.yaml

• Adds an end-to-end pacquet CLI install test asserting the produced lockfile snapshot contains the final peer suffix variant and not the provisional one.

pacquet/crates/cli/tests/install.rs


tests.rs Test ignoreCompatibilityDb parsing and application +12/-0

Test ignoreCompatibilityDb parsing and application

• Adds a unit test verifying the YAML key is parsed and correctly sets Config::ignore_compatibility_db.

pacquet/crates/config/src/workspace_yaml/tests.rs


tests.rs Test pnpm precedence for duplicated manifest aliases +33/-0

Test pnpm precedence for duplicated manifest aliases

• Adds a regression test ensuring a duplicated alias across groups is recorded in optionalDependencies per pnpm precedence.

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs


tests.rs Add lockfile parity tests for final peer suffix and compat DB +157/-0

Add lockfile parity tests for final peer suffix and compat DB

• Adds tests asserting transitive pending peers produce final-suffix snapshots and that the built-in compatibility DB is applied (or skipped via ignore flag) without writing packageExtensionsChecksum.

pacquet/crates/package-manager/src/install/tests.rs


tests.rs Update test config struct for ignore_compatibility_db default +1/-0

Update test config struct for ignore_compatibility_db default

• Extends test Config initialization to include ignore_compatibility_db with default false.

pacquet/crates/package-manager/src/install_package_from_registry/tests.rs


tests.rs Add regression test for incompatible peer variant retention +44/-0

Add regression test for incompatible peer variant retention

• Adds a scenario where direct deps collapse to a larger variant but a consumer edge retains the smaller incompatible peer variant, ensuring both remain in the graph.

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs


tests.rs Test hidden-direct peer provider appending rules +125/-6

Test hidden-direct peer provider appending rules

• Adds tests verifying real peerDependencies providers are appended as hidden direct deps, while meta-only peerDependenciesMeta providers are not.

pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs


tests.rs Add peer-resolution regression tests (final suffix, alias filtering) +606/-13

Add peer-resolution regression tests (final suffix, alias filtering)

• Adds tests for transitive pending peer final suffix behavior and ensures non-peer-relevant child aliases do not satisfy peers via real package name heuristics.

pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs


tests.rs Add workspace tests for per-importer link caching and root peer resolution +184/-13

Add workspace tests for per-importer link caching and root peer resolution

• Adds tests asserting importer-relative link: resolutions are cached per project_dir and that root workspace direct deps can be used to resolve peers for child importers when enabled.

pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs


tests.rs Add tests for partial '<=' normalization behavior +23/-0

Add tests for partial '<=' normalization behavior

• Adds regression tests ensuring partial "<=27" admits all 27.x versions and that overflow in bound calculation returns None.

pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs


peerDependencies.ts Add pnpm E2E test for pending transitive peer final suffix +23/-0

Add pnpm E2E test for pending transitive peer final suffix

• Adds a pnpm test asserting lockfile snapshots contain the final peer suffix form for the transitive pending peer scenario.

pnpm/test/install/peerDependencies.ts


package.json Add fixture package final-peer-a +11/-0

Add fixture package final-peer-a

• Introduces an e2e fixture package with dependencies and a peerDependency used to reproduce the pending transitive peer suffix scenario.

pnpr/.fixtures/packages/@pnpm.e2e/final-peer-a/1.0.0/package.json


package.json Add fixture package final-peer-b +7/-0

Add fixture package final-peer-b

• Introduces an e2e fixture package declaring a peerDependency on final-peer-a.

pnpr/.fixtures/packages/@pnpm.e2e/final-peer-b/1.0.0/package.json


package.json Add fixture package final-peer-c +4/-0

Add fixture package final-peer-c

• Introduces an e2e fixture package acting as the peer provider in the scenario.

pnpr/.fixtures/packages/@pnpm.e2e/final-peer-c/1.0.0/package.json


package.json Add fixture package final-peer-x +7/-0

Add fixture package final-peer-x

• Introduces an e2e fixture package that peers on final-peer-b, forming the transitive pending peer chain.

pnpr/.fixtures/packages/@pnpm.e2e/final-peer-x/1.0.0/package.json


Documentation (1)
TEST_PORTING.md Document newly-ported parity tests +6/-0

Document newly-ported parity tests

• Updates the test porting plan to mark the transitive pending peer final-suffix cases as ported and notes added install/CLI coverage.

pacquet/plans/TEST_PORTING.md


Other (4)
.typos.toml Whitelist 'ect' token for repo typos checks +1/-0

Whitelist 'ect' token for repo typos checks

• Adds an allowlisted word entry so existing content isn't flagged by typos tooling.

.typos.toml


env_overlay.rs Add IGNORE_COMPATIBILITY_DB env overlay field +1/-0

Add IGNORE_COMPATIBILITY_DB env overlay field

• Extends workspace settings env overlay to accept IGNORE_COMPATIBILITY_DB and map it into settings.

pacquet/crates/config/src/env_overlay.rs


lib.rs Introduce ignore_compatibility_db config flag +5/-0

Introduce ignore_compatibility_db config flag

• Adds Config::ignore_compatibility_db, defaulting to false, to control whether the built-in compatibility DB is applied.

pacquet/crates/config/src/lib.rs


workspace_yaml.rs Parse/apply ignoreCompatibilityDb workspace setting +3/-1

Parse/apply ignoreCompatibilityDb workspace setting

• Adds ignoreCompatibilityDb to workspace YAML settings, includes it in reset/apply plumbing, and threads it through config application.

pacquet/crates/config/src/workspace_yaml.rs


Grey Divider

Qodo Logo

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (2)
.typos.toml (1)

12-12: ⚡ Quick win

Avoid whitelisting ect globally if this is a one-off.

[default.extend-words] suppresses ect everywhere the typos job scans, which will also hide real etc misspellings. If this came from a single identifier or fixture, prefer a narrower identifier- or file-scoped exception instead of teaching the linter that ect is generally valid.

🤖 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 @.typos.toml at line 12, Remove "ect" from the global whitelist in
[default.extend-words] (don't teach the linter that "ect" is valid everywhere).
Instead, add a narrower exception: either remove the entry entirely and fix the
one-off typo, or add "ect" only in a file- or identifier-scoped whitelist (e.g.,
a per-file pattern or project-specific exception) or use an inline
suppression/comment at the specific identifier/fixture location so only that
occurrence is allowed.
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs (1)

116-130: ⚡ Quick win

Cover the other new <= normalization forms too.

This only locks in the spaced <=27 path. normalize_partial_lte_comparators() also rewrites inline <=27 and partial-minor <=27.5, so a regression there would still change picks without tripping this test. A small table-driven follow-up here would close out the new branches cheaply.

🤖 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-npm-resolver/src/pick_package_from_meta/tests.rs`
around lines 116 - 130, The test version_range_lte_partial_allows_entire_major
only covers the spaced form ">=24 <=27" but normalize_partial_lte_comparators
also rewrites inline "<=27" and partial-minor "<=27.5"; update the test to be
table-driven: in version_range_lte_partial_allows_entire_major (or a new test)
iterate over a small list of variant strings (e.g. ">=24 <=27", ">=24<=27",
">=24 <=27.5", ">=24<=27.5") constructing the same
PickVersionByVersionRangeOptions (using make_package) and assert
pick_version_by_version_range(&opts).as_deref() == Some("27.5.1") for each
variant so all normalization branches in normalize_partial_lte_comparators are
exercised.
🤖 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/package-manager/src/compat_package_extensions.json`:
- Around line 977-980: The JSON contains a duplicate selector
"gatsby-core-utils@<2.14.0-next.1" which causes the loader
(compat_package_extensions.rs) to let the latter IndexMap entry overwrite the
former; merge the two entries so there is a single
"gatsby-core-utils@<2.14.0-next.1" object whose "dependencies" contains the
union of both dependency maps (deduplicating version strings if needed), or
remove the redundant entry and add any missing deps to the remaining one; update
compat_package_extensions.json accordingly so compat_package_extensions.rs no
longer loses a dependency at load time.

In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 843-847: The lockfile reuse checksum omits compatibility-DB state
causing stale reuse; update the checksum computation and GraphToLockfileOptions
to include the compatibility DB (the ignore_compatibility_db flag and/or the
compat_package_extender contents). Concretely: when you build
compat_package_extender (the Arc from compat_package_extender()), incorporate
that state into compute_package_extensions_checksum (or its caller) so the
returned checksum reflects ignore_compatibility_db and any embedded DB-derived
extensions, and pass that computed value into
GraphToLockfileOptions.package_extensions_checksum so persisted metadata and
reuse checks align with the runtime compat behavior; adjust callers that compute
or pass package_extensions_checksum accordingly. Ensure any places that
previously hashed only config.package_extensions now include the compat
flag/extender fingerprint so toggling ignore_compatibility_db or DB updates
invalidates reuse.

In `@pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs`:
- Around line 254-269: The peer-child compatibility check in
CollapsedTargetPeerInfo::peer_child_is_compatible_with_parent currently treats a
missing entry in self.child_peer_names as compatible; change the logic so a
missing child_peer_names.get(name) returns false (i.e., reject unknown peer
names) instead of defaulting to true. Concretely, replace the is_none_or(...)
usage with an explicit map_or(false, |peer_names| { ... }) or an if let
Some(peer_names) { ... } else { return false; } so the inner conditions (using
self.package_name, parent_peer_names, and self.transitive_peer_dependencies)
only run when peer_names exist, preventing collapsed_target_matches_parent from
accepting unresolved peer names.

In `@pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs`:
- Around line 333-350: The root_parents are built before the per-importer
modules_dir swap, causing link: remaps to use the current (child) modules_dir;
change the order so that when computing root_parents you temporarily set
walker.opts.modules_dir to the root importer's modules_dir (from the importer
with id ".") so build_importer_parents_from and remap_link_node_id run in the
root context; then proceed with the existing per-importer swap inside the for
importer loop (or restore the previous modules_dir after building root_parents)
— adjust the logic around resolve_peers_from_workspace_root, root_importer,
root_parents, walker.opts.modules_dir, and build_importer_parents_from
accordingly.
- Around line 1820-1829: available_peer_names is only populated with
consumer_record.peer_edges and aliases present in
consumer_pkg.peer_dependencies, which omits sibling peer providers referenced by
consumer_record.edges; update the construction in resolve_peers.rs so that you
also insert every alias from consumer_record.edges (unconditionally) into
available_peer_names (in addition to the existing peer_edges and the
pkg_name_version insert) so that final_peer_edge_dep_path() can consider sibling
providers when choosing a provider variant.

In `@pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs`:
- Around line 651-657: The match arms in partial_lte_upper_bound() perform
unchecked arithmetic (major + 1 / minor + 1) which can overflow; change them to
use checked_add(1) on the parsed u64 values and return None if checked_add fails
so the function remains fail-closed. Specifically, in the [major] arm parse
major to u64, call major.checked_add(1).ok_or(None)/or map to produce the
incremented value and only then format "<{}.0.0-0"; do the same in the [major,
minor] arm for minor (i.e., parse minor to u64, use minor.checked_add(1).ok()?
before formatting "<{major}.{}.0-0"). Ensure both arms propagate None on
overflow instead of performing wrapping arithmetic.

---

Nitpick comments:
In @.typos.toml:
- Line 12: Remove "ect" from the global whitelist in [default.extend-words]
(don't teach the linter that "ect" is valid everywhere). Instead, add a narrower
exception: either remove the entry entirely and fix the one-off typo, or add
"ect" only in a file- or identifier-scoped whitelist (e.g., a per-file pattern
or project-specific exception) or use an inline suppression/comment at the
specific identifier/fixture location so only that occurrence is allowed.

In `@pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs`:
- Around line 116-130: The test version_range_lte_partial_allows_entire_major
only covers the spaced form ">=24 <=27" but normalize_partial_lte_comparators
also rewrites inline "<=27" and partial-minor "<=27.5"; update the test to be
table-driven: in version_range_lte_partial_allows_entire_major (or a new test)
iterate over a small list of variant strings (e.g. ">=24 <=27", ">=24<=27",
">=24 <=27.5", ">=24<=27.5") constructing the same
PickVersionByVersionRangeOptions (using make_package) and assert
pick_version_by_version_range(&opts).as_deref() == Some("27.5.1") for each
variant so all normalization branches in normalize_partial_lte_comparators are
exercised.
🪄 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: ee3a7e23-b449-4b69-9e3d-cec2efa2eb35

📥 Commits

Reviewing files that changed from the base of the PR and between 7f2ac5d and 05636c4.

📒 Files selected for processing (25)
  • .typos.toml
  • installing/deps-resolver/test/resolvePeers.ts
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.json
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/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). (5)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate

Files:

  • installing/deps-resolver/test/resolvePeers.ts
🧠 Learnings (13)
📚 Learning: 2026-05-20T20:41:26.759Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11773
File: pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs:115-117
Timestamp: 2026-05-20T20:41:26.759Z
Learning: In pacquet's Rust pnpm port, keep the `is_http_url` behavior exactly aligned with upstream pnpm: when checking whether an input is an HTTP(S) URL, use `bare.starts_with("http:") || bare.starts_with("https:")` rather than `"http://"` / `"https://"`. Do not “fix” this to a more typical `http://` form; pacquet’s cardinal rule is to match pnpm semantics byte-for-byte. Non-URL/malformed inputs are later rejected downstream by `reqwest::Url::parse` (surfacing as a `ResolveError`), so this intentional prefix check is required for compatibility.

Applied to files:

  • .typos.toml
📚 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/dedupe_peer_dependents/tests.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/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/dedupe_peer_dependents/tests.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/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/dedupe_peer_dependents/tests.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/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/dedupe_peer_dependents/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-06-12T14:51:20.984Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12361
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:1338-1341
Timestamp: 2026-06-12T14:51:20.984Z
Learning: When reviewing pacquet Rust dependency-tree resolution logic that performs `landed_on_prior_entry`-style comparisons, avoid comparing raw ids that may include a `patch_hash=(...)` suffix and/or peer suffixes. `PkgNameVerPeer`’s peer-suffix handling can already absorb the patch-hash token such that `prior_key.without_peer()` represents bare `nameversion` for registry packages. The id-mismatch risk is on the normalized-id side: `build_pkg_id_with_patch_hash` may add `file:`/`git:`/`tarball:` prefixes before `name@...`. For correctness, ensure both sides are normalized by stripping patch/peer suffixes—specifically compare `prior_key.without_peer()` with `pacquet_deps_path::remove_suffix(&id)` (both stripped), ideally via the `landed_on_prior_entry` helper so the normalization rules stay consistent.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/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/dedupe_peer_dependents/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/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/dedupe_peer_dependents/tests.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-06-12T20:41:57.558Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12364
File: pacquet/crates/package-manager/src/install/tests.rs:5717-5717
Timestamp: 2026-06-12T20:41:57.558Z
Learning: In the pnpm/pnpm Rust workspace (toolchain Rust 1.95.0), keep using `std::time::Duration::from_mins(...)` and `std::time::Duration::from_hours(...)` as-is. Do not flag these as invalid or suggest replacing them with `Duration::from_secs(...)`, because Clippy’s `clippy::duration_suboptimal_units` lint is denied by `-D warnings` in CI, and changing to seconds may trigger CI failures.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-06-12T07:04:03.703Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12227
File: pacquet/crates/package-manager/src/install.rs:708-719
Timestamp: 2026-06-12T07:04:03.703Z
Learning: In this pnpm-based repo’s package-manager Rust code, `ThrottledClient` uses a two-class semaphore scheduling policy (“reserved half” for tarball/download work and a higher-priority “latency class” for lockfile-verification metadata fetches). Therefore, when reviewing, do NOT flag reuse of the same `Arc<ThrottledClient>` for both downloads and verification as a budget/contention issue—this reuse is intentional and handled by the internal two-class “reserved permits” design. Creating a separate client instance would break the EMFILE guard behavior that relies on shared throttling.

Applied to files:

  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.

Applied to files:

  • installing/deps-resolver/test/resolvePeers.ts
📚 Learning: 2026-06-05T13:47:05.929Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/test/install/frozenStore.ts:2-17
Timestamp: 2026-06-05T13:47:05.929Z
Learning: In the pnpm/pnpm repository, the shared Jest preset keeps `injectGlobals` at its default (`true`), so `test` and `expect` are available as Jest globals. Therefore, reviewers should not flag (or treat as TypeScript/compilation errors) missing `import { test, expect } from 'jest/globals'` when a test file uses `test`/`expect` without importing them. Importing from `jest/globals` may still be used for consistency with sibling files, but it is not required for execution in this repo unless a Jest preset is explicitly configured with `injectGlobals: false`.

Applied to files:

  • installing/deps-resolver/test/resolvePeers.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.

Applied to files:

  • installing/deps-resolver/test/resolvePeers.ts
🔇 Additional comments (19)
pacquet/crates/config/src/lib.rs (1)

804-807: LGTM!

pacquet/crates/config/src/workspace_yaml.rs (1)

172-172: LGTM!

Also applies to: 607-607, 737-737

pacquet/crates/config/src/env_overlay.rs (1)

165-165: LGTM!

pacquet/crates/config/src/workspace_yaml/tests.rs (1)

35-45: LGTM!

pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)

69-69: LGTM!

pacquet/crates/package-manager/src/lib.rs (1)

8-8: LGTM!

pacquet/crates/package-manager/src/compat_package_extensions.rs (1)

1-19: LGTM!

pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)

1091-1091: LGTM!

pacquet/crates/package-manager/src/install/tests.rs (1)

6853-6870: LGTM!

Also applies to: 6872-6883, 6884-6940

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)

343-353: LGTM!

Also applies to: 359-370

pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)

390-421: LGTM!

pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)

421-455: LGTM!

Also applies to: 513-541, 655-805

pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs (1)

785-791: LGTM!

Also applies to: 842-959

pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs (1)

187-229: LGTM!

pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs (1)

1-1: LGTM!

Also applies to: 146-192, 193-249, 251-315, 368-417, 469-529, 634-1014, 1035-1035, 1042-1042

installing/deps-resolver/test/resolvePeers.ts (1)

129-217: LGTM!

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (2)

442-484: LGTM!


1313-1313: LGTM!

Also applies to: 1866-1866, 2109-2114

pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs (1)

57-60: LGTM!

Also applies to: 158-158, 294-294

Comment thread pacquet/crates/package-manager/src/compat_package_extensions.json Outdated
Comment thread pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
Comment thread pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
Comment thread pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
Comment thread pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs Outdated
@github-actions

github-actions Bot commented Jun 13, 2026

Copy link
Copy Markdown
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.087 ± 0.154 3.900 4.335 2.01 ± 0.15
pacquet@main 4.036 ± 0.171 3.878 4.334 1.98 ± 0.15
pnpr@HEAD 2.124 ± 0.106 1.987 2.294 1.04 ± 0.08
pnpr@main 2.037 ± 0.130 1.891 2.276 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.08673984776,
      "stddev": 0.1541843295828852,
      "median": 4.05769114646,
      "user": 3.86123616,
      "system": 3.4345120000000002,
      "min": 3.90018605696,
      "max": 4.334634905960001,
      "times": [
        4.334634905960001,
        4.11064238596,
        3.90018605696,
        4.207639755960001,
        4.306749708960001,
        3.94714456196,
        4.00697913596,
        4.108403156960001,
        4.00160777396,
        3.94341103496
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.03631521106,
      "stddev": 0.17116842071977054,
      "median": 3.94979049246,
      "user": 3.87303846,
      "system": 3.4119267,
      "min": 3.8777507669599998,
      "max": 4.33364248196,
      "times": [
        4.33364248196,
        3.92120121496,
        4.03031251096,
        3.92196624996,
        4.32087850996,
        3.8777507669599998,
        3.91132554096,
        3.95559742196,
        3.94398356296,
        4.146493849960001
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.12425670706,
      "stddev": 0.10577662611901181,
      "median": 2.1096502514599997,
      "user": 2.6428736600000002,
      "system": 2.8889918999999997,
      "min": 1.98710466896,
      "max": 2.29408655296,
      "times": [
        2.08861885396,
        2.20230156996,
        2.09768032696,
        1.98710466896,
        2.12507469396,
        2.00232819496,
        2.27865235196,
        2.29408655296,
        2.12162017596,
        2.04509968096
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.03722048566,
      "stddev": 0.13029910644167494,
      "median": 1.97977480396,
      "user": 2.59877106,
      "system": 2.9114753999999996,
      "min": 1.8910629059600002,
      "max": 2.27626312596,
      "times": [
        1.97451705896,
        2.27626312596,
        2.1850247499599997,
        1.97930397296,
        1.91895501396,
        1.8910629059600002,
        1.9258703859600002,
        1.9802456349600002,
        2.14936717396,
        2.09159483396
      ]
    }
  ]
}

Scenario: Isolated linker: fresh restore, hot cache + hot store

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 606.0 ± 7.5 598.1 619.0 1.00
pacquet@main 606.6 ± 13.0 589.6 628.6 1.00 ± 0.02
pnpr@HEAD 673.2 ± 45.3 647.6 796.6 1.11 ± 0.08
pnpr@main 660.1 ± 13.0 643.7 680.9 1.09 ± 0.03
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.60603915544,
      "stddev": 0.007482086171985058,
      "median": 0.60233727854,
      "user": 0.35050638000000006,
      "system": 1.2818652799999999,
      "min": 0.5981483680399999,
      "max": 0.61901345204,
      "times": [
        0.61901345204,
        0.60038379004,
        0.59925933704,
        0.60216636604,
        0.5981483680399999,
        0.60901978404,
        0.61192844504,
        0.61629405204,
        0.60166976904,
        0.60250819104
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6065616121399999,
      "stddev": 0.013014525877389594,
      "median": 0.61037103504,
      "user": 0.35141148,
      "system": 1.2891010799999998,
      "min": 0.58963199704,
      "max": 0.62860022204,
      "times": [
        0.61924514104,
        0.61206797504,
        0.61130571904,
        0.59090570404,
        0.6094363510399999,
        0.61271785004,
        0.62860022204,
        0.59287325804,
        0.58963199704,
        0.59883190404
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6732277919399999,
      "stddev": 0.045310657637190534,
      "median": 0.6575911355399999,
      "user": 0.35890758,
      "system": 1.3152478799999998,
      "min": 0.64756919404,
      "max": 0.79664478904,
      "times": [
        0.69062390504,
        0.66866066904,
        0.66558847104,
        0.65781796504,
        0.64885953804,
        0.64756919404,
        0.65736430604,
        0.64822129504,
        0.65092778704,
        0.79664478904
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.66006055184,
      "stddev": 0.01301442245544378,
      "median": 0.65678825704,
      "user": 0.37149878,
      "system": 1.30391898,
      "min": 0.6437080870399999,
      "max": 0.68089884204,
      "times": [
        0.64872759804,
        0.67881216404,
        0.65518901304,
        0.6496474880399999,
        0.67112577504,
        0.65125771404,
        0.66285133604,
        0.6437080870399999,
        0.68089884204,
        0.65838750104
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + cold store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.068 ± 0.036 4.005 4.111 1.92 ± 0.11
pacquet@main 4.018 ± 0.043 3.925 4.061 1.89 ± 0.11
pnpr@HEAD 2.169 ± 0.141 2.025 2.431 1.02 ± 0.09
pnpr@main 2.121 ± 0.124 2.007 2.368 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.06776541212,
      "stddev": 0.03564587380401678,
      "median": 4.065949012620001,
      "user": 3.7005358,
      "system": 3.2543571,
      "min": 4.00506813262,
      "max": 4.11092877362,
      "times": [
        4.06744289762,
        4.10753257462,
        4.0937250916200005,
        4.0644551276200005,
        4.041270062620001,
        4.11092877362,
        4.04389839362,
        4.00506813262,
        4.103862499620001,
        4.0394705676200005
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.018411327520001,
      "stddev": 0.04294298368758365,
      "median": 4.02803817762,
      "user": 3.5632481,
      "system": 3.2704850999999997,
      "min": 3.92453051162,
      "max": 4.061486958620001,
      "times": [
        4.035369809620001,
        4.02566470062,
        4.01655774662,
        3.92453051162,
        3.96401064162,
        4.020388690620001,
        4.0455916506200005,
        4.06010091062,
        4.061486958620001,
        4.03041165462
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.16916112512,
      "stddev": 0.1406568954072985,
      "median": 2.1489318521199996,
      "user": 2.496785,
      "system": 2.8464714,
      "min": 2.02462135962,
      "max": 2.43138015662,
      "times": [
        2.05805184562,
        2.13791038262,
        2.17156462262,
        2.0295908156199998,
        2.23254357562,
        2.1599533216199998,
        2.37416358562,
        2.02462135962,
        2.07183158562,
        2.43138015662
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.12106668842,
      "stddev": 0.12363619780918836,
      "median": 2.06455518162,
      "user": 2.4567580999999996,
      "system": 2.8356008,
      "min": 2.00704751262,
      "max": 2.36750080862,
      "times": [
        2.30796625762,
        2.02996031962,
        2.36750080862,
        2.04027800262,
        2.0449235086199997,
        2.16430387762,
        2.00704751262,
        2.11957623362,
        2.06828645862,
        2.06082390462
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, hot cache + hot store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.267 ± 0.015 1.238 1.291 2.03 ± 0.07
pacquet@main 1.189 ± 0.070 1.160 1.387 1.90 ± 0.13
pnpr@HEAD 0.625 ± 0.022 0.606 0.683 1.00
pnpr@main 0.632 ± 0.005 0.622 0.641 1.01 ± 0.04
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.2665641024600003,
      "stddev": 0.015259745627032433,
      "median": 1.2687752751599999,
      "user": 1.24332968,
      "system": 1.6604046800000003,
      "min": 1.23848721116,
      "max": 1.2907331341600001,
      "times": [
        1.27334936316,
        1.2907331341600001,
        1.28037859516,
        1.27529209116,
        1.26997564016,
        1.24951009616,
        1.26370256116,
        1.25663742216,
        1.23848721116,
        1.26757491016
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.1894348529599998,
      "stddev": 0.06964584968458079,
      "median": 1.1681610736599999,
      "user": 1.16016758,
      "system": 1.6843730800000003,
      "min": 1.15976308616,
      "max": 1.38718169416,
      "times": [
        1.17011332016,
        1.16621989316,
        1.16542160016,
        1.16426098816,
        1.38718169416,
        1.17382811216,
        1.17478085316,
        1.15976308616,
        1.17010225416,
        1.16267672816
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.62487271216,
      "stddev": 0.021672047668967756,
      "median": 0.6199057181600001,
      "user": 0.31924418,
      "system": 1.24213158,
      "min": 0.60643519816,
      "max": 0.68323493816,
      "times": [
        0.61576723716,
        0.61871517116,
        0.62232556516,
        0.62041002016,
        0.62524127016,
        0.60643519816,
        0.68323493816,
        0.62920399616,
        0.61940141616,
        0.60799230916
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.63169995986,
      "stddev": 0.005179178637344915,
      "median": 0.63105870166,
      "user": 0.31874808,
      "system": 1.2468761800000001,
      "min": 0.62215383916,
      "max": 0.6406224701600001,
      "times": [
        0.6406224701600001,
        0.63488725016,
        0.62766501716,
        0.63757635016,
        0.63241212016,
        0.63014278316,
        0.62942236516,
        0.62215383916,
        0.63167596716,
        0.63044143616
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + hot store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.886 ± 0.026 2.860 2.946 4.55 ± 0.10
pacquet@main 2.832 ± 0.070 2.772 3.004 4.46 ± 0.14
pnpr@HEAD 0.635 ± 0.012 0.613 0.648 1.00
pnpr@main 0.664 ± 0.070 0.630 0.864 1.05 ± 0.11
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.8863814967000003,
      "stddev": 0.025945309341112194,
      "median": 2.8775491359000003,
      "user": 1.6693994200000002,
      "system": 1.9046853,
      "min": 2.8598446549000003,
      "max": 2.9464343339,
      "times": [
        2.8819462239,
        2.9108672279000003,
        2.8789357689000004,
        2.8598446549000003,
        2.8988782859000004,
        2.8742928449000003,
        2.8663178279,
        2.9464343339,
        2.8761625029,
        2.8701352959
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.8320790987,
      "stddev": 0.06959770756296363,
      "median": 2.8059807179000003,
      "user": 1.61595092,
      "system": 1.9138223,
      "min": 2.7723099459,
      "max": 3.0038448789000003,
      "times": [
        2.7887368769000003,
        2.7775887579000003,
        2.7936834019,
        2.7723099459,
        2.8731289229000003,
        2.8498039929,
        2.8516406219,
        2.7917755539,
        3.0038448789000003,
        2.8182780339000004
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6348390969000001,
      "stddev": 0.01229722979197942,
      "median": 0.6376918504000001,
      "user": 0.3219912199999999,
      "system": 1.2454813999999998,
      "min": 0.6129020079,
      "max": 0.6481445239000001,
      "times": [
        0.6249841669,
        0.6129020079,
        0.6184668029000001,
        0.6438627129000001,
        0.6481445239000001,
        0.6333265679000001,
        0.6363980469000001,
        0.6389856539000001,
        0.6455535279000001,
        0.6457669579
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.6638824653000002,
      "stddev": 0.07037469431941187,
      "median": 0.6424302334,
      "user": 0.31711382,
      "system": 1.2610535999999999,
      "min": 0.6298032199000001,
      "max": 0.8637363279000001,
      "times": [
        0.6298032199000001,
        0.6424018199000001,
        0.6401907619000001,
        0.6458366419000001,
        0.6471336589000001,
        0.6424586469000001,
        0.6431293149000001,
        0.6419312499000001,
        0.6422030109000001,
        0.8637363279000001
      ]
    }
  ]
}

@github-actions

github-actions Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12372
Testbedpacquet
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
milliseconds (ms)
(Result Δ%)
Upper Boundary
milliseconds (ms)
(Limit %)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
🚷 view threshold
4,067.77 ms
(-20.74%)Baseline: 5,132.07 ms
6,158.49 ms
(66.05%)
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
🚷 view threshold
2,886.38 ms
(-10.16%)Baseline: 3,212.93 ms
3,855.52 ms
(74.86%)
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
🚷 view threshold
1,266.56 ms
(+5.92%)Baseline: 1,195.74 ms
1,434.88 ms
(88.27%)
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
🚷 view threshold
4,086.74 ms
(-27.03%)Baseline: 5,600.72 ms
6,720.86 ms
(60.81%)
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
🚷 view threshold
606.04 ms
(-7.61%)Baseline: 655.96 ms
787.15 ms
(76.99%)
🐰 View full continuous benchmarking report in Bencher

@zkochan zkochan force-pushed the codex/fix-12330-lockfile-parity branch from 05636c4 to 56c6a45 Compare June 13, 2026 11:54
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

Code Review by Qodo

Grey Divider

New Review Started

This review has been superseded by a new analysis

Grey Divider

Qodo Logo

@zkochan zkochan marked this pull request as draft June 13, 2026 11:55
@zkochan zkochan marked this pull request as ready for review June 13, 2026 11:56
@zkochan

zkochan commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

Updated the branch with the performance fix in 56c6a45e91.

The regression was not in the peer walker itself. The fresh-resolution path had a duplicate resolve-stage peer-provider traversal in pacquet; it walked the resolved tree again after resolve_peers() had already returned the same provider map. I removed that duplicate traversal and use peers_result.resolved_peer_providers_by_alias directly.

Local hot-cache/hot-store fixture results for resolve_workspace:

  • before fix: 4.9-6.5s locally, matching the CI shape
  • main binary on same fixture: 356ms
  • after fix: 335ms, 303ms, 307ms over three runs

Validation run:

  • cargo test -p pacquet-resolving-deps-resolver
  • cargo test -p pacquet-package-manager
  • cargo test -p pacquet-cli --test install transitive_pending_peer_uses_provider_final_suffix_in_lockfile
  • cargo fmt --check
  • git diff --check
  • targeted typos over changed files
  • cargo clippy --locked --workspace --all-targets -- -D warnings
  • pre-push hook completed successfully, including TS build/lint/spellcheck, Rust docs, dylint, and taplo

Note: a bare repository-wide typos scan still reports unrelated historical changelog/hash false positives, so I validated changed files for this PR.


Written by an agent (Codex, GPT-5).

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 56c6a45

@zkochan zkochan marked this pull request as draft June 13, 2026 11:56
Comment thread pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs Outdated
@zkochan zkochan marked this pull request as ready for review June 13, 2026 12:03
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 56c6a45

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
hooks/read-package-hook/test/createReadPackageHook.ts (1)

53-60: ⚡ Quick win

Extend test coverage for extension merge precedence.

The current test verifies that different dependency names from duplicate compat-DB selectors are combined, but does not cover:

  1. Colliding dependency names (same selector, same dep name, different versions in two compat-DB entries).
  2. User extensions overriding compat-DB extensions (when both define the same selector and dep name).

These scenarios would clarify the intended precedence and catch regressions if the merge order changes.

🤖 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 `@hooks/read-package-hook/test/createReadPackageHook.ts` around lines 53 - 60,
Update the existing test that calls getEffectivePackageExtensions to add two
explicit assertions: (1) verify merge behavior when two compat-DB entries use
the same selector and same dependency name but different versions—assert the
deterministic precedence (e.g., later compat-DB entry wins) by expecting the
chosen version for that dep; and (2) verify that a user-provided extension for
the same selector+dep overrides the compat-DB value by adding a user extension
and asserting its version is present in the merged result; locate the test
referencing getEffectivePackageExtensions and extend it with these two
assertions so collisions and user-overrides are covered.
🤖 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.

Nitpick comments:
In `@hooks/read-package-hook/test/createReadPackageHook.ts`:
- Around line 53-60: Update the existing test that calls
getEffectivePackageExtensions to add two explicit assertions: (1) verify merge
behavior when two compat-DB entries use the same selector and same dependency
name but different versions—assert the deterministic precedence (e.g., later
compat-DB entry wins) by expecting the chosen version for that dep; and (2)
verify that a user-provided extension for the same selector+dep overrides the
compat-DB value by adding a user extension and asserting its version is present
in the merged result; locate the test referencing getEffectivePackageExtensions
and extend it with these two assertions so collisions and user-overrides are
covered.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0f05731c-a9e3-46dd-9934-1328fe6f51f9

📥 Commits

Reviewing files that changed from the base of the PR and between 05636c4 and 56c6a45.

📒 Files selected for processing (37)
  • .typos.toml
  • hooks/read-package-hook/src/createReadPackageHook.ts
  • hooks/read-package-hook/src/index.ts
  • hooks/read-package-hook/test/createReadPackageHook.ts
  • installing/deps-installer/test/install/peerDependencies.ts
  • installing/deps-resolver/test/resolvePeers.ts
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.json
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/plans/TEST_PORTING.md
  • pnpm/test/install/peerDependencies.ts
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-a/1.0.0/package.json
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-b/1.0.0/package.json
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-c/1.0.0/package.json
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-x/1.0.0/package.json
✅ Files skipped from review due to trivial changes (7)
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-c/1.0.0/package.json
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-x/1.0.0/package.json
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-b/1.0.0/package.json
  • pacquet/plans/TEST_PORTING.md
  • pnpr/.fixtures/packages/@pnpm.e2e/final-peer-a/1.0.0/package.json
  • pacquet/crates/package-manager/src/lib.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.json
🚧 Files skipped from review as they are similar to previous changes (19)
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • .typos.toml
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/config/src/workspace_yaml/tests.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rs
  • pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rs
  • installing/deps-resolver/test/resolvePeers.ts
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.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 (macos-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx,js,jsx}: Use Standard Style with trailing commas, prefer functions over classes, declare functions after they are used (relying on hoisting), limit functions to no more than two or three arguments, and use a single options object for functions needing more parameters
Follow import order: standard libraries first, then external dependencies (sorted alphabetically), then relative imports
Do not write comments that restate what the code already says; rename variables, split helpers, or move checks to more obvious places instead
Do not repeat documentation at call sites that already lives on the callee; update the JSDoc once and let every call site benefit
Use JSDoc for the function's contract (preconditions, postconditions, edge cases, why the function exists), not for re-narrating the function body
Do not record past implementation shape, refactor history, or removed code in comments; use git log and git blame for that information instead
Write comments only when the reason for code is non-obvious, a hidden invariant exists, a workaround for a known bug is needed, or an exception to surrounding pattern is deliberate

Files:

  • pnpm/test/install/peerDependencies.ts
  • hooks/read-package-hook/src/index.ts
  • hooks/read-package-hook/test/createReadPackageHook.ts
  • hooks/read-package-hook/src/createReadPackageHook.ts
  • installing/deps-installer/test/install/peerDependencies.ts
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
pacquet/**/tests/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — use tempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq! assertions, dbg! complex structures, skip logging for simple scalar assert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer #[cfg_attr(target_os = "windows", ignore = "...")] (or matching #[cfg(unix)] gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests with insta and carefully review diffs when intentional changes alter snapshots; accept with cargo insta review only after careful review
Tests that need the mocked registry should start pnpr through pacquet-testing-utils; cargo test / cargo nextest run should not require a separate just registry-mock launch step

Files:

  • pacquet/crates/cli/tests/install.rs
🧠 Learnings (13)
📚 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:

  • pnpm/test/install/peerDependencies.ts
  • hooks/read-package-hook/src/index.ts
  • hooks/read-package-hook/test/createReadPackageHook.ts
  • hooks/read-package-hook/src/createReadPackageHook.ts
  • installing/deps-installer/test/install/peerDependencies.ts
📚 Learning: 2026-05-24T08:18:00.622Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11895
File: pnpm/test/deploy.ts:91-95
Timestamp: 2026-05-24T08:18:00.622Z
Learning: In pnpm/pnpm integration tests that intentionally hit the real npm registry (registry.npmjs.org)—for example when the test passes `--config.registry=https://registry.npmjs.org/` to `execPnpm` and simply increases the timeout—do not request adding a runtime environment-variable gate (e.g., `PNPM_RUN_PUBLIC_REGISTRY_TESTS`). This is the established pattern for those tests (see files like `pnpm/test/install/pacquet.ts` and `pnpm/test/deploy.ts`).

Applied to files:

  • pnpm/test/install/peerDependencies.ts
📚 Learning: 2026-06-05T13:47:05.929Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/test/install/frozenStore.ts:2-17
Timestamp: 2026-06-05T13:47:05.929Z
Learning: In the pnpm/pnpm repository, the shared Jest preset keeps `injectGlobals` at its default (`true`), so `test` and `expect` are available as Jest globals. Therefore, reviewers should not flag (or treat as TypeScript/compilation errors) missing `import { test, expect } from 'jest/globals'` when a test file uses `test`/`expect` without importing them. Importing from `jest/globals` may still be used for consistency with sibling files, but it is not required for execution in this repo unless a Jest preset is explicitly configured with `injectGlobals: false`.

Applied to files:

  • pnpm/test/install/peerDependencies.ts
  • hooks/read-package-hook/test/createReadPackageHook.ts
  • installing/deps-installer/test/install/peerDependencies.ts
📚 Learning: 2026-06-05T13:47:26.046Z
Learnt from: vsumner
Repo: pnpm/pnpm PR: 12190
File: installing/deps-installer/src/install/index.ts:2337-2343
Timestamp: 2026-06-05T13:47:26.046Z
Learning: In the pnpm/pnpm codebase, `PnpmError` automatically prefixes `err.code` with `ERR_PNPM_` when you pass a code that does not already start with `ERR_PNPM_` (it normalizes `this.code` via `code.startsWith('ERR_PNPM_') ? code : `ERR_PNPM_${code}``). Therefore, during code review you should NOT flag `new PnpmError(...)` call sites for passing a bare error code (e.g., `new PnpmError('FROZEN_STORE_INCOMPATIBLE_WITH_PNPR', ...)`); the resulting `err.code` will still be `ERR_PNPM_FROZEN_STORE_INCOMPATIBLE_WITH_PNPR`.

Applied to files:

  • pnpm/test/install/peerDependencies.ts
  • hooks/read-package-hook/src/index.ts
  • hooks/read-package-hook/test/createReadPackageHook.ts
  • hooks/read-package-hook/src/createReadPackageHook.ts
  • installing/deps-installer/test/install/peerDependencies.ts
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.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-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.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-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.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-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
📚 Learning: 2026-06-12T20:41:57.558Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12364
File: pacquet/crates/package-manager/src/install/tests.rs:5717-5717
Timestamp: 2026-06-12T20:41:57.558Z
Learning: In the pnpm/pnpm Rust workspace (toolchain Rust 1.95.0), keep using `std::time::Duration::from_mins(...)` and `std::time::Duration::from_hours(...)` as-is. Do not flag these as invalid or suggest replacing them with `Duration::from_secs(...)`, because Clippy’s `clippy::duration_suboptimal_units` lint is denied by `-D warnings` in CI, and changing to seconds may trigger CI failures.

Applied to files:

  • pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
📚 Learning: 2026-06-12T07:04:03.703Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12227
File: pacquet/crates/package-manager/src/install.rs:708-719
Timestamp: 2026-06-12T07:04:03.703Z
Learning: In this pnpm-based repo’s package-manager Rust code, `ThrottledClient` uses a two-class semaphore scheduling policy (“reserved half” for tarball/download work and a higher-priority “latency class” for lockfile-verification metadata fetches). Therefore, when reviewing, do NOT flag reuse of the same `Arc<ThrottledClient>` for both downloads and verification as a budget/contention issue—this reuse is intentional and handled by the internal two-class “reserved permits” design. Creating a separate client instance would break the EMFILE guard behavior that relies on shared throttling.

Applied to files:

  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/compat_package_extensions.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.rs
📚 Learning: 2026-06-12T14:51:20.984Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12361
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:1338-1341
Timestamp: 2026-06-12T14:51:20.984Z
Learning: When reviewing pacquet Rust dependency-tree resolution logic that performs `landed_on_prior_entry`-style comparisons, avoid comparing raw ids that may include a `patch_hash=(...)` suffix and/or peer suffixes. `PkgNameVerPeer`’s peer-suffix handling can already absorb the patch-hash token such that `prior_key.without_peer()` represents bare `nameversion` for registry packages. The id-mismatch risk is on the normalized-id side: `build_pkg_id_with_patch_hash` may add `file:`/`git:`/`tarball:` prefixes before `name@...`. For correctness, ensure both sides are normalized by stripping patch/peer suffixes—specifically compare `prior_key.without_peer()` with `pacquet_deps_path::remove_suffix(&id)` (both stripped), ideally via the `landed_on_prior_entry` helper so the normalization rules stay consistent.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.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.rs
🔇 Additional comments (19)
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs (1)

116-131: LGTM!

Also applies to: 133-137

pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)

50-50: LGTM!

Also applies to: 421-450, 508-537, 607-607

installing/deps-installer/test/install/peerDependencies.ts (1)

2114-2131: LGTM!

pnpm/test/install/peerDependencies.ts (1)

1-23: LGTM!

pacquet/crates/cli/tests/install.rs (1)

443-476: LGTM!

hooks/read-package-hook/src/createReadPackageHook.ts (6)

14-21: LGTM!


23-40: LGTM!


60-66: LGTM!


88-95: LGTM!


113-120: LGTM!


105-108: Fix precedence of packageExtensions vs compatibility DB (or document it).

mergePackageExtension merges fields as { ...next[field], ...merged[field] }, and getEffectivePackageExtensions applies compatPackageExtensions before user packageExtensions, so the compatibility DB overwrites user settings when both define the same selector/field. Current tests cover “compat DB enabled/disabled” and merging compat selectors, but not this user-vs-compat conflict; no docs clearly state whether “compat-wins” is intended. If user should win, swap the spread order to { ...merged[field], ...next[field] } (or equivalent) and add a test; otherwise document that compat DB has precedence.

hooks/read-package-hook/src/index.ts (1)

1-1: LGTM!

pacquet/crates/package-manager/src/compat_package_extensions.rs (5)

9-16: LGTM!


18-29: LGTM!


31-42: LGTM!


63-73: LGTM!


44-61: Compat DB merge precedence: duplicate selectors aren’t present in current JSON

  • pacquet/crates/package-manager/src/compat_package_extensions.json is a top-level array where the compat-DB selector is at .[]|.[0]; the duplicate-selector check over that field shows no duplicates (150 total entries), so the “previous overwrites next” behavior in merge_string_map isn’t exercised for duplicate selectors in the current dataset.
pacquet/crates/package-manager/src/install.rs (1)

1424-1425: LGTM!

pacquet/crates/package-manager/src/install/tests.rs (1)

4285-4352: LGTM!

Also applies to: 6922-7008

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 13, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 2f3c4a3

@zkochan

zkochan commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

Also addressed Qodo's summary-only performance recommendation in 2f3c4a3: the built-in compatibility extender cache now stores an Arc<PackageExtender> and callers clone the Arc, avoiding a deep clone of the embedded compatibility DB on each fresh install.


Written by an agent (Codex, GPT-5).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/compat_package_extensions.rs (1)

54-76: Clarify merge semantics for compatibility DB extension maps

merge_string_map/merge_peer_meta_map implement “first-seen wins” for duplicate keys (they start from next.clone() and then extend(previous) overwrites). However, compat_package_extensions.json contains 150 selector entries with 0 duplicate selectors, so these helpers won’t currently be exercised for the same selector. Optional: document the expectation of unique selectors (or add an assertion) since the merge-order behavior would matter only if duplicates are introduced later.

🤖 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/package-manager/src/compat_package_extensions.rs` around lines
54 - 76, The merge functions merge_string_map and merge_peer_meta_map currently
produce "first-seen wins" semantics by cloning next then extending with previous
(which would overwrite next keys), but the intent and uniqueness of selectors is
not explicit; add a runtime check that previous and next contain no overlapping
keys (e.g., check intersection of previous.keys() and next.keys()) and panic or
log an explicit error if overlaps are found, or alternatively document the
uniqueness expectation in the function comment; implement the check inside both
merge_string_map and merge_peer_meta_map before merging so duplicates are
detected early and behavior is explicit.
🤖 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.

Nitpick comments:
In `@pacquet/crates/package-manager/src/compat_package_extensions.rs`:
- Around line 54-76: The merge functions merge_string_map and
merge_peer_meta_map currently produce "first-seen wins" semantics by cloning
next then extending with previous (which would overwrite next keys), but the
intent and uniqueness of selectors is not explicit; add a runtime check that
previous and next contain no overlapping keys (e.g., check intersection of
previous.keys() and next.keys()) and panic or log an explicit error if overlaps
are found, or alternatively document the uniqueness expectation in the function
comment; implement the check inside both merge_string_map and
merge_peer_meta_map before merging so duplicates are detected early and behavior
is explicit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f7ff4ba3-6931-447f-a41e-c1a33c1b7d7d

📥 Commits

Reviewing files that changed from the base of the PR and between 56c6a45 and 2f3c4a3.

📒 Files selected for processing (4)
  • pacquet/crates/package-manager/src/compat_package_extensions.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_peers.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). (9)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Code Coverage
  • GitHub Check: Dylint
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Compile & Lint
🧰 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 fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/package-manager/src/compat_package_extensions.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/package-manager/src/compat_package_extensions.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/package-manager/src/compat_package_extensions.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/package-manager/src/compat_package_extensions.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/package-manager/src/compat_package_extensions.rs
📚 Learning: 2026-06-12T20:41:57.558Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12364
File: pacquet/crates/package-manager/src/install/tests.rs:5717-5717
Timestamp: 2026-06-12T20:41:57.558Z
Learning: In the pnpm/pnpm Rust workspace (toolchain Rust 1.95.0), keep using `std::time::Duration::from_mins(...)` and `std::time::Duration::from_hours(...)` as-is. Do not flag these as invalid or suggest replacing them with `Duration::from_secs(...)`, because Clippy’s `clippy::duration_suboptimal_units` lint is denied by `-D warnings` in CI, and changing to seconds may trigger CI failures.

Applied to files:

  • pacquet/crates/package-manager/src/compat_package_extensions.rs
📚 Learning: 2026-06-12T07:04:03.703Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12227
File: pacquet/crates/package-manager/src/install.rs:708-719
Timestamp: 2026-06-12T07:04:03.703Z
Learning: In this pnpm-based repo’s package-manager Rust code, `ThrottledClient` uses a two-class semaphore scheduling policy (“reserved half” for tarball/download work and a higher-priority “latency class” for lockfile-verification metadata fetches). Therefore, when reviewing, do NOT flag reuse of the same `Arc<ThrottledClient>` for both downloads and verification as a budget/contention issue—this reuse is intentional and handled by the internal two-class “reserved permits” design. Creating a separate client instance would break the EMFILE guard behavior that relies on shared throttling.

Applied to files:

  • pacquet/crates/package-manager/src/compat_package_extensions.rs
🔇 Additional comments (3)
pacquet/crates/package-manager/src/compat_package_extensions.rs (3)

4-7: LGTM!


10-19: LGTM!


21-45: LGTM!

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12372
Testbedpnpr

⚠️ 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-thresholds flag.

Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,169.16 ms
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
634.84 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
624.87 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,124.26 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
673.23 ms
🐰 View full continuous benchmarking report in Bencher

@zkochan zkochan merged commit 4819fb4 into main Jun 13, 2026
26 of 28 checks passed
@zkochan zkochan deleted the codex/fix-12330-lockfile-parity branch June 13, 2026 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pacquet: nondeterministic optional-peer suffix for webpack-dev-server on the babylon fixture

2 participants