Skip to content

feat(pacquet): hand custom resolvers the currentPkg of a re-resolving edge#12361

Merged
zkochan merged 2 commits into
mainfrom
feat/custom-resolver-current-pkg
Jun 12, 2026
Merged

feat(pacquet): hand custom resolvers the currentPkg of a re-resolving edge#12361
zkochan merged 2 commits into
mainfrom
feat/custom-resolver-current-pkg

Conversation

@zkochan

@zkochan zkochan commented Jun 12, 2026

Copy link
Copy Markdown
Member

Follow-up to #12153, which left ResolveOptions.current_pkg declared but never populated — custom resolvers always received currentPkg: null. This ports pnpm's hand-off so a re-resolving edge carries its prior lockfile entry to the resolver, matching currentPkg: extendedWantedDep.infoFromLockfile in resolveDependencies.ts and the resolver-level shape built by packageRequester.

What's included

  • Prior-key derivation — the tree walker derives each edge's recorded snapshot key: importer refs are satisfies-gated like pnpm's referenceSatisfiesWantedSpec; transitive keys come from the parent's snapshot. The same key now feeds both subtree reuse (unchanged semantics, try_reuse_node just takes it precomputed) and the new currentPkg payload.
  • currentPkg payload — built like pnpm's getInfoFromLockfile + pkgSnapshotToResolution: {id, name, version, resolution} where a Registry ({integrity}-only) entry regains its tarball URL derived from the (scope-routed) registry; recorded tarball/git/directory shapes pass through. To route scoped packages, pick_registry_for_package moved from pacquet-resolving-npm-resolver to pacquet-lockfile (next to npm_tarball_url); a re-export keeps existing callers source-compatible.
  • Children of fresh parents — pnpm keeps a re-resolved parent's prior child refs alive when the parent landed back on its previously recorded version (the non-updated arm of resolvedDependencies = parentPkg.updated ? undefined : …), so the children's own re-resolution still receives currentPkg. The new ReuseSource::PriorOnly arm carries that key without enabling reuse — subtree-reuse behavior is untouched.
  • Memo-key correctness — the per-wanted resolve memo now includes the prior key, so two edges sharing a specifier but recording different versions never share a currentPkg-dependent result.

Tests

  • Unit tests for the payload builder (Registry→tarball URL derivation, scope routing, tarball passthrough, missing-entry and missing-registry-map withholding) and the child-ref satisfies gate.
  • E2E port of upstream's "custom resolver receives currentPkg on subsequent installs": the first install records the npm resolver's pick (an {integrity}-only lockfile entry); a shouldRefreshResolution-forced second install receives that entry as currentPkg with the re-derived tarball URL, and echoing it back keeps the pinned version.

Remaining follow-ups from #12153

  • Custom resolution shapes outside the typed LockfileResolution enum (e.g. {type: 'custom:cdn'}) are still rejected; supporting them is only useful together with custom fetcher hooks (CustomFetcher), which need a bidirectional IPC design (the JS hook receives live cafs + delegation fetchers) and are a separate effort.

Written by an agent (Claude Code, claude-fable-5).

Summary by CodeRabbit

  • New Features

    • Custom resolvers now receive prior-install package context on subsequent installs.
    • Registry routing improved to select scope-specific registries and respect resolved registry mappings.
  • Bug Fixes

    • Lockfile reuse preserves prior-package resolution context during re-resolve, avoiding incorrect re-resolutions and preserving integrity data.
    • Prior-key context prevents cache reuse across differing prior snapshots.
  • Tests

    • Added integration and unit tests validating resolver behavior, registry routing, and lockfile-reuse gates.

… edge

Custom resolvers received currentPkg: null on every call because nothing
populated ResolveOptions.current_pkg. Mirror pnpm's hand-off
(currentPkg: extendedWantedDep.infoFromLockfile in resolveDependencies):

- the tree walker now derives each edge's prior lockfile snapshot key
  (importer refs satisfies-gated like pnpm's referenceSatisfiesWantedSpec;
  transitive keys from the parent's snapshot) and, when the edge
  re-resolves anyway, threads the recorded entry into the per-call
  ResolveOptions as currentPkg
- a Registry ({integrity}-only) entry regains its derived tarball URL,
  like pnpm's pkgSnapshotToResolution; pick_registry_for_package moved
  from pacquet-resolving-npm-resolver to pacquet-lockfile (next to
  npm_tarball_url) so the deps-resolver can route scoped packages, with
  a re-export keeping existing callers unchanged
- children of a freshly resolved parent that landed back on its
  recorded version keep their prior refs for currentPkg purposes
  (pnpm's non-updated parentPkg arm) via the new ReuseSource::PriorOnly;
  subtree reuse semantics are unchanged
- the per-wanted memo key includes the prior key so two edges sharing a
  specifier but recording different versions never share a
  currentPkg-dependent result

The e2e test ports upstream's 'custom resolver receives currentPkg on
subsequent installs': a forced re-resolve receives the prior entry with
the re-derived tarball URL, and echoing it back keeps the pinned
version.
@qodo-free-for-open-source-projects

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

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

Grey Divider


Action required

1. Wrong parent id compare ✓ Resolved 🐞 Bug ≡ Correctness
Description
resolve_node() gates prior_children_snapshot by comparing prior_key.without_peer() to
result.id, but result.id is not necessarily the canonical depPath key (it can be normalized
later by build_pkg_id_with_patch_hash, e.g. file:...name@file:...). This makes the gate
incorrectly fail and prevents PriorOnly from carrying prior child keys, so child edges won’t
receive currentPkg on re-resolve even when the parent landed back on its prior lockfile entry.
Code

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[R1338-1341]

+        let prior_children_snapshot = prior_key
+            .as_ref()
+            .filter(|key| key.without_peer().to_string() == result.id.to_string())
+            .and_then(|key| ctx.workspace.wanted_lockfile.as_ref()?.snapshots.as_ref()?.get(key));
Evidence
The prior_children_snapshot gate compares to result.id, but build_pkg_id_with_patch_hash()
explicitly normalizes ids by adding a name@ prefix for non-registry resolvers (e.g.
file:project-1project-1@file:project-1). Since lockfile snapshot keys are based on the
canonical depPath/id, comparing against the unnormalized result.id can never match for those
cases, disabling the intended prior-child-ref propagation.

pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1331-1341]
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1833-1869]

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

## Issue description
`prior_children_snapshot` is only retained when `prior_key.without_peer().to_string() == result.id.to_string()`. However, `result.id` is not guaranteed to match the canonical depPath string used in lockfile snapshot keys because `build_pkg_id_with_patch_hash()` may normalize it (notably by prefixing `name@` for resolvers that return ids like `file:...`, tarball URLs, git URLs, etc.). This causes the filter to incorrectly reject valid prior refs, so children of freshly-resolved parents will not get `ReuseSource::PriorOnly` keys and therefore won’t receive `currentPkg` on their own re-resolves.
### Issue Context
You already compute the canonical resolved package id as `id = build_pkg_id_with_patch_hash(ctx, &result).await?;` earlier in `resolve_node`. That `id` (optionally with suffix stripping for `(patch_hash=...)`) is the value that should be used to decide whether the freshly resolved parent “landed back” on its recorded lockfile entry.
### Fix Focus Areas
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1338-1341]
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs[1833-1869]
### Suggested fix
Change the comparison to use the canonicalized `id` (and strip suffix consistently):
- Compute `let resolved_pkg_id = pacquet_deps_path::remove_suffix(&id);` (or another canonical form that matches `prior_key.without_peer().to_string()` semantics).
- Use `resolved_pkg_id` instead of `result.id.to_string()` in the `filter(...)`.
This ensures `PriorOnly` works for `file:`/tarball/git/directory resolutions where `build_pkg_id_with_patch_hash` prefixes the id.

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


Grey Divider

Qodo Logo

@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a3f61d65-d986-4253-a683-f07ce8bebf1b

📥 Commits

Reviewing files that changed from the base of the PR and between b104311 and b199a10.

📒 Files selected for processing (2)
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Doc
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Code Coverage
  • 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/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
🧠 Learnings (7)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.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_dependency_tree/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
🔇 Additional comments (1)
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs (1)

1-38: LGTM!


📝 Walkthrough

Walkthrough

This PR threads prior-lockfile state through the resolution pipeline to enable custom resolvers to receive opts.currentPkg on subsequent installs after forced refresh. Changes include consolidating registry routing, propagating registries through options, implementing prior-package derivation helpers, and extending the resolution reuse model to track and materialize prior snapshot context.

Changes

Prior-lockfile context threading for custom resolver currentPkg

Layer / File(s) Summary
Registry routing consolidation
pacquet/crates/lockfile/src/resolution.rs, pacquet/crates/resolving-npm-resolver/src/named_registry.rs, pacquet/crates/resolving-npm-resolver/src/lib.rs
pick_registry_for_package is extracted from named_registry.rs into lockfile/resolution.rs as a public helper, enabling registry routing logic to be shared across resolution and lockfile-reuse contexts; re-exports are adjusted accordingly.
Registry propagation through options and context
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs, pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs, pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs, pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
registries: HashMap field is added to WorkspaceResolveOptions and WorkspaceTreeCtx with a .with_registries(...) setter; registry map is threaded from the install pipeline through workspace resolution, enabling prior-lockfile data materialization.
Prior package derivation from lockfile
pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs, pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
current_pkg_from_lockfile reconstructs CurrentPkg from prior lockfile entries, materializing registry-based resolutions into tarball URLs with preserved integrity; prior_child_key gates child-edge reuse by semver satisfaction; comprehensive unit tests cover scoped registries, tarball pass-through, and edge cases.
Resolution reuse model with prior-key tracking
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs, pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree/tests.rs
ReuseSource gains PriorOnly { key } variant and helper methods; WantedKey cache key is expanded to include optional prior snapshot key for cache segregation; resolve_node computes prior keys, gates subtree reuse, derives current_pkg, and propagates prior context to children; try_reuse_node and resolve_reused_node signatures updated; tests for landed_on_prior_entry added.
Integration test validating currentPkg on refresh
pacquet/crates/cli/tests/custom_resolvers.rs
New test custom_resolver_receives_current_pkg_on_subsequent_installs verifies custom resolvers receive correct opts.currentPkg with id/name/version/resolution fields from prior lockfile after forced refresh; validates tarball URL derivation and integrity preservation.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#12153: Related work integrating custom resolver adapters and IPC with the resolution pipeline; both touch how resolvers receive context like currentPkg.
  • pnpm/pnpm#12128: Related changes extending WantedKey memoization to include importer/project-specific context; both PRs add dimensions to the resolve memoization key.
  • pnpm/pnpm#12113: Prior PR modifying lockfile-reuse mechanics; this PR extends that mechanism to materialize prior currentPkg for resolvers.

Poem

🐰 A rabbit's resolve hops with prior delight,
Registry routes now consolidated just right,
CurrentPkg flows through each lockfile frame,
Custom resolvers see the prior state—same game,
Thread it, refresh it, the cache knows its claim!

🚥 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 clearly and concisely describes the main feature: enabling custom resolvers to receive currentPkg during edge re-resolution, which is the core objective of this PR.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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 feat/custom-resolver-current-pkg

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.

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

Copy link
Copy Markdown

PR Summary by Qodo

feat(pacquet): populate resolver currentPkg from prior lockfile entries
🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

Grey Divider

Walkthroughs

Description
• Populate ResolveOptions.current_pkg from prior lockfile entries during re-resolution.
• Materialize Registry lockfile resolutions into tarball URLs using scoped registry routing.
• Add unit + e2e coverage ensuring custom resolvers receive currentPkg on subsequent installs.
Diagram
graph TD
  pm["package-manager install"] --> ws["resolve_workspace"] --> tree["resolve_dependency_tree"] --> opts["ResolveOptions (currentPkg)"] --> cr{{"Custom resolver"}}
  tree --> reuse["lockfile_reuse"] --> lf[("Lockfile packages/snapshots")]
  reuse --> reg["registry routing + tarball URL"]

  subgraph Legend
    direction LR
    _mod["Module"] ~~~ _data[("Lockfile data")] ~~~ _ext{{"External hook"}}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Always store full tarball URLs in lockfile (avoid re-derivation)
  • ➕ Eliminates need to materialize RegistryTarball for currentPkg
  • ➕ Simplifies resolver payload building and reduces registry-routing dependency
  • ➖ Diverges from pnpm’s compact {integrity}-only Registry entries
  • ➖ Lockfile bloat and potential behavior drift vs upstream expectations
2. Build `currentPkg` inside the JS custom-resolver adapter
  • ➕ Keeps lockfile interpretation and URL derivation out of core tree resolver
  • ➕ Potentially reduces Rust-side cross-crate wiring
  • ➖ Requires exposing lockfile + registry config to hook runtime
  • ➖ Duplicates correctness-sensitive logic (scope routing, tarball URL format) in JS
3. Send integrity-only `currentPkg` when registries are unavailable
  • ➕ Custom resolvers always receive something, even in minimal entry points
  • ➖ Does not match pnpm’s resolver-level shape (missing tarball URL)
  • ➖ May cause resolvers to mis-handle Registry entries that expect URLs

Recommendation: Keep the current approach: thread a prior snapshot key through the walker, build currentPkg in Rust to mirror pnpm’s getInfoFromLockfile + pkgSnapshotToResolution, and withhold Registry-shaped payloads when a registry map isn’t available. This best matches upstream semantics and avoids pushing lockfile/registry correctness into the JS hook layer.

Grey Divider

File Changes

Enhancement (3)
install_with_fresh_lockfile.rs Thread resolved registries into workspace dependency resolution +2/-1

Thread resolved registries into workspace dependency resolution

• Clones the resolved registry map when constructing 'NpmResolver' and passes the registry map into 'WorkspaceResolveOptions' so the deps resolver can materialize prior 'Registry' entries into tarball URLs for 'currentPkg'.

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


lockfile_reuse.rs Build 'currentPkg' payload and derive prior child snapshot keys +74/-2

Build 'currentPkg' payload and derive prior child snapshot keys

• Adds 'current_pkg_from_lockfile' to convert prior lockfile metadata into the resolver-facing 'CurrentPkg' shape, including 'Registry' → tarball URL materialization and integrity carry-over. Adds 'prior_child_key' to reuse recorded child references only when the recorded version still satisfies the requested range.

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


resolve_workspace.rs Add registries to workspace resolver options and context +9/-2

Add registries to workspace resolver options and context

• Extends 'WorkspaceResolveOptions' with a resolved registry map and threads it into 'WorkspaceTreeCtx' via 'with_registries', enabling downstream 'currentPkg' tarball materialization.

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


Bug fix (1)
resolve_dependency_tree.rs Thread prior lockfile entries into resolver calls as 'currentPkg' +110/-26

Thread prior lockfile entries into resolver calls as 'currentPkg'

• Computes each edge’s prior snapshot key once and reuses it for both subtree reuse and 'currentPkg' payloads during fresh resolves. Adds 'ReuseSource::PriorOnly' to preserve prior child refs for 'currentPkg' without enabling subtree reuse, and updates memoization keys to include the prior key to avoid cross-edge contamination.

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


Refactor (3)
resolution.rs Move 'pick_registry_for_package' into lockfile resolution utilities +39/-1

Move 'pick_registry_for_package' into lockfile resolution utilities

• Introduces 'pick_registry_for_package' (and scope parsing helper) alongside 'npm_tarball_url' so non-npm-resolver code can route scoped packages consistently. Updates imports to include 'HashMap' for registry maps.

pacquet/crates/lockfile/src/resolution.rs


lib.rs Re-export 'pick_registry_for_package' from pacquet-lockfile +2/-1

Re-export 'pick_registry_for_package' from pacquet-lockfile

• Keeps existing call sites source-compatible by re-exporting the moved registry selection helper from the resolver crate’s public API.

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


named_registry.rs Remove local 'pick_registry_for_package' implementation +1/-38

Remove local 'pick_registry_for_package' implementation

• Deletes the duplicate implementation and re-exports the shared helper from 'pacquet_lockfile', keeping 'pick_registry_for_version' behavior intact while centralizing routing logic.

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


Tests (3)
custom_resolvers.rs Add e2e test asserting custom resolvers receive 'currentPkg' +76/-0

Add e2e test asserting custom resolvers receive 'currentPkg'

• Ports pnpm’s upstream scenario verifying that the second install (forced re-resolve) passes the prior lockfile entry as 'opts.currentPkg'. Asserts the payload includes id/name/version plus a tarball URL re-derived from registry routing, and that echoing it back preserves the pinned version.

pacquet/crates/cli/tests/custom_resolvers.rs


tests.rs Unit tests for 'currentPkg' materialization and satisfies gate +94/-0

Unit tests for 'currentPkg' materialization and satisfies gate

• Adds coverage for 'Registry'→tarball URL derivation, scoped registry routing, tarball passthrough, withholding when metadata/registries are missing, and the semver-satisfies guard for prior child references.

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


tests.rs Update workspace resolve tests for new registries option +1/-0

Update workspace resolve tests for new registries option

• Initializes 'WorkspaceResolveOptions.registries' in test helpers to satisfy the expanded options struct.

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


Grey Divider

Qodo Logo

@github-actions

github-actions Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01      7.5±0.20ms   581.3 KB/sec    1.00      7.4±0.06ms   589.3 KB/sec

@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: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs`:
- Around line 1338-1341: The equality check that sets prior_children_snapshot is
comparing prior_key against result.id (the unpatched resolver id); change it to
compare prior_key.without_peer().to_string() against the patched dependency path
used for snapshots (i.e. the resolved/patched id string produced when the
package was recorded), not result.id. In other words, replace the right-hand
side of the comparison with the patched/resolved id string (the same id used
when writing into ctx.workspace.wanted_lockfile.snapshots) so
prior_children_snapshot can be found when parent ids acquire a (patch_hash=...)
suffix; keep references to prior_key, prior_children_snapshot, result.id and
ctx.workspace.wanted_lockfile.snapshots to locate the change.
🪄 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: eec0e9f6-4db0-4d89-8edb-ab935455e4f1

📥 Commits

Reviewing files that changed from the base of the PR and between 52148e6 and b104311.

📒 Files selected for processing (10)
  • pacquet/crates/cli/tests/custom_resolvers.rs
  • pacquet/crates/lockfile/src/resolution.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/lib.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.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: zizmor latest via PyPI
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Dylint
  • GitHub Check: Code Coverage
  • GitHub Check: Compile & Lint
🧰 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/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/cli/tests/custom_resolvers.rs
  • pacquet/crates/lockfile/src/resolution.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.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/custom_resolvers.rs
🧠 Learnings (8)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/cli/tests/custom_resolvers.rs
  • pacquet/crates/lockfile/src/resolution.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/cli/tests/custom_resolvers.rs
  • pacquet/crates/lockfile/src/resolution.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/cli/tests/custom_resolvers.rs
  • pacquet/crates/lockfile/src/resolution.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.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_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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_workspace/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.

Applied to files:

  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs
  • pacquet/crates/cli/tests/custom_resolvers.rs
  • pacquet/crates/lockfile/src/resolution.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse/tests.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/lockfile_reuse.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-npm-resolver/src/named_registry.rs
📚 Learning: 2026-05-20T23:23:40.606Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11788
File: pacquet/crates/lockfile/src/snapshot_dep_ref.rs:52-56
Timestamp: 2026-05-20T23:23:40.606Z
Learning: In the pacquet lockfile crate, the link target is intentionally represented as a raw `Link(String)` (not a `LinkTarget` newtype) for both `SnapshotDepRef::Link` and `ImporterDepVersion::Link`. pnpm upstream preserves `link:` targets verbatim with only a prefix check (e.g., `refToRelative`), so do not suggest introducing a `LinkTarget` newtype in a bug-fix/targeted PR. If type branding is ever desired, it should be done together for both variants in a separate dedicated refactor PR rather than as part of a small fix.

Applied to files:

  • pacquet/crates/lockfile/src/resolution.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_with_fresh_lockfile.rs
🔇 Additional comments (12)
pacquet/crates/lockfile/src/resolution.rs (3)

5-5: LGTM!


338-367: LGTM!


369-374: LGTM!

pacquet/crates/resolving-npm-resolver/src/named_registry.rs (1)

20-20: LGTM!

pacquet/crates/resolving-npm-resolver/src/lib.rs (1)

51-57: LGTM!

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

150-224: LGTM!

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

37-37: LGTM!

Also applies to: 116-120, 168-178

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

36-38: LGTM!

Also applies to: 80-115, 462-463, 550-555, 590-590, 696-701, 1048-1052, 1062-1064, 1093-1110, 1113-1115, 1126-1126, 1430-1455, 1577-1577

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

146-146: LGTM!

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

596-596: LGTM!

Also applies to: 1099-1099

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

13-16: LGTM!

Also applies to: 21-68, 70-89

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

164-256: LGTM!

@codecov-commenter

codecov-commenter commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.19355% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.05%. Comparing base (a9d2ec8) to head (b199a10).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...lving-deps-resolver/src/resolve_dependency_tree.rs 97.95% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12361      +/-   ##
==========================================
+ Coverage   88.02%   88.05%   +0.02%     
==========================================
  Files         294      294              
  Lines       37137    37227      +90     
==========================================
+ Hits        32690    32779      +89     
- Misses       4447     4448       +1     

☔ 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.

The 'parent landed back on its recorded entry' check compared the
recorded snapshot key against the raw resolver id, which is not the
canonical dep-path form: build_pkg_id_with_patch_hash may append a
(patch_hash=...) suffix and name@-prefix file:/git/tarball ids. Extract
the comparison into landed_on_prior_entry, which strips suffixes from
both sides (the recorded key's peers/patch hash via without_peer, the
resolved id via remove_suffix) so the gate keys on which package
version the parent is, like pnpm's parentPkg.updated flag.
@qodo-free-for-open-source-projects

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

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit b199a10

@github-actions

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.090 ± 0.172 3.932 4.460 1.94 ± 0.14
pacquet@main 4.117 ± 0.182 3.909 4.459 1.95 ± 0.14
pnpr@HEAD 2.191 ± 0.127 2.022 2.334 1.04 ± 0.08
pnpr@main 2.113 ± 0.118 1.993 2.332 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.09020226162,
      "stddev": 0.17242723626253081,
      "median": 4.0479766256200005,
      "user": 3.8121335199999997,
      "system": 3.4323209399999994,
      "min": 3.9316902351200005,
      "max": 4.46015906412,
      "times": [
        4.46015906412,
        4.05233389412,
        4.12528290912,
        3.96933656312,
        4.13047813912,
        4.04361935712,
        4.29692773212,
        3.9316902351200005,
        3.95734697612,
        3.93484774612
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.11742104042,
      "stddev": 0.18191775544271102,
      "median": 4.04433121912,
      "user": 3.8957096199999994,
      "system": 3.4174468399999993,
      "min": 3.9087241781200004,
      "max": 4.45896697812,
      "times": [
        4.29919330612,
        4.00349256512,
        3.94806748512,
        4.45896697812,
        4.31457995812,
        3.9087241781200004,
        4.07053516812,
        4.01061196612,
        4.141911529120001,
        4.01812727012
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.19143302822,
      "stddev": 0.1274735561090547,
      "median": 2.2279076791200003,
      "user": 2.5720213199999997,
      "system": 2.90803944,
      "min": 2.0224909291200004,
      "max": 2.3337744121200004,
      "times": [
        2.2638974171200004,
        2.21166731412,
        2.12421959712,
        2.24414804412,
        2.3337744121200004,
        2.3255562761200004,
        2.0449740271200003,
        2.02301281712,
        2.0224909291200004,
        2.3205894481200002
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.1131987370200003,
      "stddev": 0.11768767929844175,
      "median": 2.0841286956200005,
      "user": 2.6353175199999996,
      "system": 2.9287774399999997,
      "min": 1.99320048312,
      "max": 2.33162960812,
      "times": [
        2.2868120751200003,
        1.99320048312,
        2.33162960812,
        2.0142830701200003,
        2.0613429131200003,
        2.02194107512,
        2.1325721591200004,
        2.02003880212,
        2.10691447812,
        2.16325270612
      ]
    }
  ]
}

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

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 651.8 ± 12.2 630.8 676.4 1.00
pacquet@main 652.2 ± 25.8 620.3 711.4 1.00 ± 0.04
pnpr@HEAD 706.4 ± 12.6 692.0 736.7 1.08 ± 0.03
pnpr@main 721.9 ± 103.3 662.9 1009.3 1.11 ± 0.16
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.65184456514,
      "stddev": 0.012240590343310013,
      "median": 0.65346456844,
      "user": 0.38731418,
      "system": 1.29316482,
      "min": 0.63078193344,
      "max": 0.67639385744,
      "times": [
        0.64692257044,
        0.67639385744,
        0.63078193344,
        0.63942058944,
        0.65926283844,
        0.65368461644,
        0.65324452044,
        0.64659703644,
        0.65603020544,
        0.65610748344
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.65221468364,
      "stddev": 0.02582731305672129,
      "median": 0.65045393694,
      "user": 0.36500048,
      "system": 1.30138172,
      "min": 0.62025982544,
      "max": 0.71137859844,
      "times": [
        0.65223735644,
        0.65815162744,
        0.65425579144,
        0.6412588004399999,
        0.62025982544,
        0.67295100044,
        0.63473822644,
        0.64867051744,
        0.62824509244,
        0.71137859844
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.7063793977399999,
      "stddev": 0.012623439923911824,
      "median": 0.70484039644,
      "user": 0.40535708000000004,
      "system": 1.3183451200000003,
      "min": 0.69200243244,
      "max": 0.73674518944,
      "times": [
        0.71507922944,
        0.69701317544,
        0.70434780944,
        0.70968505544,
        0.70533298344,
        0.69831644044,
        0.69858737944,
        0.69200243244,
        0.73674518944,
        0.70668428244
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.72187335014,
      "stddev": 0.10333065605635806,
      "median": 0.69112477594,
      "user": 0.38274068,
      "system": 1.3253424200000001,
      "min": 0.66293456844,
      "max": 1.00929212644,
      "times": [
        0.68043447444,
        0.70166638544,
        0.66293456844,
        0.67423655344,
        0.68616946344,
        0.69716842944,
        0.66986385144,
        0.69608008844,
        1.00929212644,
        0.74088756044
      ]
    }
  ]
}

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 3.820 ± 0.037 3.778 3.879 1.78 ± 0.11
pacquet@main 3.834 ± 0.049 3.783 3.941 1.79 ± 0.11
pnpr@HEAD 2.232 ± 0.118 2.051 2.389 1.04 ± 0.08
pnpr@main 2.148 ± 0.128 1.998 2.362 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 3.8200737708000005,
      "stddev": 0.03663607156034752,
      "median": 3.8029287436,
      "user": 3.5179330599999994,
      "system": 3.24959684,
      "min": 3.7778489316,
      "max": 3.8791235876,
      "times": [
        3.8505597796,
        3.8524414146,
        3.7980668826,
        3.7878874916000003,
        3.8609455816000002,
        3.7778489316,
        3.7892097526,
        3.7968636816,
        3.8077906046,
        3.8791235876
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 3.833507848000001,
      "stddev": 0.049217690577663335,
      "median": 3.8321455266,
      "user": 3.5818527599999994,
      "system": 3.2568360400000005,
      "min": 3.7833696046,
      "max": 3.9414025576,
      "times": [
        3.8570939156,
        3.7930144416,
        3.8528828596,
        3.7917291626000003,
        3.7833696046,
        3.8289913746,
        3.8352996786,
        3.7857584186,
        3.8655364666,
        3.9414025576
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.2322603667000003,
      "stddev": 0.11845874295708138,
      "median": 2.2338557411,
      "user": 2.42719156,
      "system": 2.8372575400000004,
      "min": 2.0511546786,
      "max": 2.3894959986,
      "times": [
        2.3179953136,
        2.1635576716,
        2.3184361116,
        2.3617622656,
        2.1640445746,
        2.1145910166,
        2.3894959986,
        2.3036669076,
        2.1378991286,
        2.0511546786
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.1476198107,
      "stddev": 0.1277607287606455,
      "median": 2.0961123586,
      "user": 2.48792806,
      "system": 2.8546227400000004,
      "min": 1.9980411336000001,
      "max": 2.3615442716,
      "times": [
        2.3585466236,
        2.0716819346,
        2.3615442716,
        2.1639734946,
        2.1205427826,
        1.9980411336000001,
        2.0605000676,
        2.0713632146,
        2.2180298996,
        2.0519746845999998
      ]
    }
  ]
}

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.144 ± 0.007 1.133 1.157 1.70 ± 0.13
pacquet@main 1.145 ± 0.031 1.119 1.228 1.71 ± 0.14
pnpr@HEAD 0.681 ± 0.022 0.659 0.715 1.01 ± 0.09
pnpr@main 0.671 ± 0.053 0.639 0.818 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.14411350738,
      "stddev": 0.006748260250413865,
      "median": 1.14257443148,
      "user": 1.1779695399999999,
      "system": 1.63383082,
      "min": 1.13258509798,
      "max": 1.15710194198,
      "times": [
        1.15710194198,
        1.14156869098,
        1.13816861698,
        1.14180098398,
        1.14665077598,
        1.1490627739800001,
        1.13258509798,
        1.14184339198,
        1.14330547098,
        1.14904732898
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.14469504858,
      "stddev": 0.03077701572712875,
      "median": 1.1371095794800001,
      "user": 1.13695114,
      "system": 1.62820442,
      "min": 1.11856705998,
      "max": 1.2282051169800001,
      "times": [
        1.11856705998,
        1.12841669198,
        1.14613363898,
        1.2282051169800001,
        1.14461039498,
        1.14714211798,
        1.12753815698,
        1.13338444898,
        1.13211814898,
        1.14083470998
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6811746279800002,
      "stddev": 0.021803693998898057,
      "median": 0.6758030664800001,
      "user": 0.34696234,
      "system": 1.27157242,
      "min": 0.6592081309800001,
      "max": 0.71525049598,
      "times": [
        0.71353675798,
        0.6603009079800001,
        0.66091485398,
        0.6592081309800001,
        0.6765060889800001,
        0.7043694099800001,
        0.67544264298,
        0.6761634899800001,
        0.67005350098,
        0.71525049598
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.67123757718,
      "stddev": 0.05276914686267396,
      "median": 0.65372535548,
      "user": 0.34231784,
      "system": 1.2477608199999999,
      "min": 0.63938245898,
      "max": 0.8180166399800001,
      "times": [
        0.63938245898,
        0.6494270589800001,
        0.65107226998,
        0.66655793698,
        0.67904565598,
        0.6474426829800001,
        0.6563784409800001,
        0.64752697498,
        0.8180166399800001,
        0.65752565198
      ]
    }
  ]
}

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.598 ± 0.029 2.560 2.648 3.95 ± 0.06
pacquet@main 2.592 ± 0.068 2.546 2.774 3.94 ± 0.11
pnpr@HEAD 0.670 ± 0.011 0.660 0.686 1.02 ± 0.02
pnpr@main 0.658 ± 0.007 0.647 0.671 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.5981651234400003,
      "stddev": 0.02851815687663889,
      "median": 2.60133143914,
      "user": 1.5888849399999998,
      "system": 1.8718608799999998,
      "min": 2.55969730214,
      "max": 2.6483540871400004,
      "times": [
        2.60899869114,
        2.5949831641400003,
        2.61362530414,
        2.5654752911400003,
        2.6281405391400003,
        2.60767971414,
        2.55969730214,
        2.58122271514,
        2.57347442614,
        2.6483540871400004
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.59173670344,
      "stddev": 0.06801738386874491,
      "median": 2.56490625114,
      "user": 1.57099814,
      "system": 1.87693708,
      "min": 2.54620587114,
      "max": 2.7735940301400004,
      "times": [
        2.6098737131400003,
        2.60299609814,
        2.5487626691400003,
        2.5587671541400003,
        2.5539544971400003,
        2.57104534814,
        2.5955389071400004,
        2.54620587114,
        2.5566287461400004,
        2.7735940301400004
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6697152744399999,
      "stddev": 0.010573003017328145,
      "median": 0.66548598414,
      "user": 0.34091594000000003,
      "system": 1.28674948,
      "min": 0.65988727514,
      "max": 0.68641098314,
      "times": [
        0.67406676514,
        0.68641098314,
        0.66831962514,
        0.66121625914,
        0.66265234314,
        0.66035847914,
        0.67932975614,
        0.66035706014,
        0.65988727514,
        0.68455419814
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.6581024478399999,
      "stddev": 0.007439091012104766,
      "median": 0.65802814014,
      "user": 0.33799954000000004,
      "system": 1.24612358,
      "min": 0.64650490614,
      "max": 0.67147762514,
      "times": [
        0.65541841214,
        0.66236821714,
        0.64650490614,
        0.65084384014,
        0.65235956114,
        0.6664287451400001,
        0.65905539414,
        0.65700088614,
        0.67147762514,
        0.65956689114
      ]
    }
  ]
}

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12361
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
3,820.07 ms
(-44.56%)Baseline: 6,890.70 ms
8,268.84 ms
(46.20%)
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
🚷 view threshold
2,598.17 ms
(-34.39%)Baseline: 3,960.01 ms
4,752.01 ms
(54.68%)
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
🚷 view threshold
1,144.11 ms
(-10.89%)Baseline: 1,283.92 ms
1,540.70 ms
(74.26%)
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
🚷 view threshold
4,090.20 ms
(-46.23%)Baseline: 7,607.44 ms
9,128.92 ms
(44.80%)
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
🚷 view threshold
651.84 ms
(-2.63%)Baseline: 669.48 ms
803.38 ms
(81.14%)
🐰 View full continuous benchmarking report in Bencher

@github-actions

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12361
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,232.26 ms
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
669.72 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
681.17 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,191.43 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
706.38 ms
🐰 View full continuous benchmarking report in Bencher

@zkochan zkochan merged commit 2aa2eaa into main Jun 12, 2026
28 checks passed
@zkochan zkochan deleted the feat/custom-resolver-current-pkg branch June 12, 2026 15:26
zkochan added a commit that referenced this pull request Jun 12, 2026
…rounds (#12357)

## Summary

Two parity changes for pacquet's resolver, continuing #12266. Measured on the monorepo (fresh state, `install --lockfile-only`, back-to-back vs **pnpm 11.6.0**), the real-lockfile document diff drops from **128 to 5 changed lines** (re-measured after rebasing over #12361/#12362: **132 → 11**, where 8 of the 11 are a divergence the pacquet side of #12362 itself introduced — see the analysis on #12266 — and 3 are the known cycle-closing-edge gap).

### 1. Per-level preferred-version fold

pnpm extends the preferred-versions map per resolution level: after a package's direct dependencies settle, their `(name, version)` pairs join the map the *children's* subtree resolutions pick against ([resolveDependencies.ts#L717-L746](https://github.com/pnpm/pnpm/blob/ce9c096e8e/installing/deps-resolver/src/resolveDependencies.ts#L717-L746)). So `signed-varint`'s `varint@~5.0.0` dedupes to the `varint@5.0.0` its parent pinned as a sibling instead of drifting to `5.0.2`. pacquet picked against a static seed only; besides `varint`/`es-abstract`, this turned out to drive the remaining `jest`/`@types/node` duplicate variants too.

- The walk resolves a whole sibling level before any child subtree starts (upstream's postponed-resolution barrier): `resolve_node` splits into `resolve_node_seed` + `walk_node_children`.
- Each level layers its versions onto a new `PreferredVersionsOverlay` (O(1) `Arc`-chained layers in `resolver-base`); the npm picker folds the per-name view in as plain `version` selectors at both registry seams.
- The overlay's per-name view joins the per-wanted dedup cache key; lockfile-reuse subtrees keep the no-overlay path (exact pins).

### 2. Hoist rounds across all importers (deterministic barrier, same logic as pnpm)

pnpm resolves **every importer's initial wave before any peer hoist**, then repeats global hoist rounds (per round: each importer's required-peer loop to a fixpoint, then one optional-peer hoist) until no importer hoists ([resolveDependencies.ts#L335-L445](https://github.com/pnpm/pnpm/blob/ce9c096e8e/installing/deps-resolver/src/resolveDependencies.ts#L335-L445)). pacquet ran each importer's whole hoist loop before the next importer's initial wave, so an early importer's optional-peer pick couldn't see versions a later importer resolves — `@cyclonedx`'s `spdx-expression-parse` hoisted `3.0.1` where pnpm's barrier-visible map picks `4.0.0`. `resolve_importer_with_workspace` is now an `ImporterHoistState` (`init` / `run_required_round` / `hoist_optional_round`) driven by `resolve_workspace` in upstream's exact phase order. Both implementations are deterministic here; the rule is identical.

## Verification

- New regression test `child_resolution_prefers_parent_level_sibling_versions` (fails with the fold disabled) + full `resolving-*`, `package-manager`, `cli` suites: 1,242 tests pass; clippy `--deny warnings`, rustfmt, typos clean.
- Whole-monorepo diff vs fresh pnpm 11.6.0: 128 → 5 changed lines; consecutive pacquet runs byte-identical.
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.

2 participants