fix(pacquet): match pnpm lockfile resolution#12372
Conversation
📝 WalkthroughWalkthroughAdds 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 ChangesAll-in-one review checkpoint
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
8e34de7 to
05636c4
Compare
Code Review by Qodo
Context used 1. Peer provider merge mismatch
|
PR Summary by QodoMatch pacquet peer resolution and lockfile output to pnpm (compat DB + pending peers) WalkthroughsDescription• 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. Diagramgraph 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
High-Level AssessmentThe following are alternative approaches to this PR: 1. Port pnpm depPath/analyzeGraph logic verbatim
2. Store explicit 'final depPath' on nodes instead of recompute pass
3. Fetch compat DB at build/install time rather than vendoring JSON
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. File ChangesEnhancement (7)
Bug fix (6)
Refactor (1)
Tests (18)
Documentation (1)
Other (4)
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
.typos.toml (1)
12-12: ⚡ Quick winAvoid whitelisting
ectglobally if this is a one-off.
[default.extend-words]suppressesecteverywhere the typos job scans, which will also hide realetcmisspellings. If this came from a single identifier or fixture, prefer a narrower identifier- or file-scoped exception instead of teaching the linter thatectis 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 winCover the other new
<=normalization forms too.This only locks in the spaced
<=27path.normalize_partial_lte_comparators()also rewrites inline<=27and 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
📒 Files selected for processing (25)
.typos.tomlinstalling/deps-resolver/test/resolvePeers.tspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/package-manager/src/compat_package_extensions.jsonpacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/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.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/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.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/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.rspacquet/crates/config/src/env_overlay.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/config/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/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.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/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
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.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
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
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
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
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
]
}
]
} |
|
| Branch | pr/12372 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 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%) |
05636c4 to
56c6a45
Compare
|
Updated the branch with the performance fix in 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 Local hot-cache/hot-store fixture results for
Validation run:
Note: a bare repository-wide Written by an agent (Codex, GPT-5). |
|
Code review by qodo was updated up to the latest commit 56c6a45 |
|
Code review by qodo was updated up to the latest commit 56c6a45 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hooks/read-package-hook/test/createReadPackageHook.ts (1)
53-60: ⚡ Quick winExtend 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:
- Colliding dependency names (same selector, same dep name, different versions in two compat-DB entries).
- 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
📒 Files selected for processing (37)
.typos.tomlhooks/read-package-hook/src/createReadPackageHook.tshooks/read-package-hook/src/index.tshooks/read-package-hook/test/createReadPackageHook.tsinstalling/deps-installer/test/install/peerDependencies.tsinstalling/deps-resolver/test/resolvePeers.tspacquet/crates/cli/tests/install.rspacquet/crates/config/src/env_overlay.rspacquet/crates/config/src/lib.rspacquet/crates/config/src/workspace_yaml.rspacquet/crates/config/src/workspace_yaml/tests.rspacquet/crates/package-manager/src/compat_package_extensions.jsonpacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/install_package_from_registry/tests.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/lib.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents.rspacquet/crates/resolving-deps-resolver/src/dedupe_peer_dependents/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_peers/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rspacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/plans/TEST_PORTING.mdpnpm/test/install/peerDependencies.tspnpr/.fixtures/packages/@pnpm.e2e/final-peer-a/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/final-peer-b/1.0.0/package.jsonpnpr/.fixtures/packages/@pnpm.e2e/final-peer-c/1.0.0/package.jsonpnpr/.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.tshooks/read-package-hook/src/index.tshooks/read-package-hook/test/createReadPackageHook.tshooks/read-package-hook/src/createReadPackageHook.tsinstalling/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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/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 — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/cli/tests/install.rs
🧠 Learnings (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.tshooks/read-package-hook/src/index.tshooks/read-package-hook/test/createReadPackageHook.tshooks/read-package-hook/src/createReadPackageHook.tsinstalling/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.tshooks/read-package-hook/test/createReadPackageHook.tsinstalling/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.tshooks/read-package-hook/src/index.tshooks/read-package-hook/test/createReadPackageHook.tshooks/read-package-hook/src/createReadPackageHook.tsinstalling/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.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/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.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/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.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/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.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/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.rspacquet/crates/package-manager/src/install.rspacquet/crates/cli/tests/install.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/compat_package_extensions.rspacquet/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.rspacquet/crates/package-manager/src/install/tests.rspacquet/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 ofpackageExtensionsvs compatibility DB (or document it).
mergePackageExtensionmerges fields as{ ...next[field], ...merged[field] }, andgetEffectivePackageExtensionsappliescompatPackageExtensionsbefore userpackageExtensions, 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.jsonis 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 inmerge_string_mapisn’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
|
Code review by qodo was updated up to the latest commit 2f3c4a3 |
|
Also addressed Qodo's summary-only performance recommendation in 2f3c4a3: the built-in compatibility extender cache now stores an Written by an agent (Codex, GPT-5). |
There was a problem hiding this comment.
🧹 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_mapimplement “first-seen wins” for duplicate keys (they start fromnext.clone()and thenextend(previous)overwrites). However,compat_package_extensions.jsoncontains 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
📒 Files selected for processing (4)
pacquet/crates/package-manager/src/compat_package_extensions.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/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!
|
| Branch | pr/12372 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,169.16 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 634.84 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 624.87 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,124.26 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 673.23 ms |
Summary
ignoreCompatibilityDbsupport.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-resolvercargo test -p pacquet-config parses_ignore_compatibility_db_from_yaml_and_appliescargo test -p pacquet-package-manager compatibility_dbcargo test -p pacquet-package-manager duplicate_manifest_alias_uses_pnpm_dependency_field_precedencecargo test -p pacquet-resolving-npm-resolver version_range_lte_partial_allows_entire_majorcargo build --release -p pacquet-cli73db2a62f695f50a3816575c84b8bc82f9106051cb3d3698ccde9754aa890b26codex/fix-12330-lockfile-parityNote:
just readywas attempted separately, but its repo-widetyposstep currently fails on existing changelog/hash false positives outside this PR.Written by an agent (Codex, GPT-5).
Summary by CodeRabbit
New Features
Improvements
Tests