Skip to content

feat(pacquet): support pnpmfile custom resolver hooks (adapter, IPC, chain integration, force-refresh)#12153

Merged
zkochan merged 6 commits into
pnpm:mainfrom
kairosci:feat/custom-resolvers
Jun 12, 2026
Merged

feat(pacquet): support pnpmfile custom resolver hooks (adapter, IPC, chain integration, force-refresh)#12153
zkochan merged 6 commits into
pnpm:mainfrom
kairosci:feat/custom-resolvers

Conversation

@kairosci

@kairosci kairosci commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Port pnpm's custom resolver hooks to the Rust pacquet engine: a pnpmfile can export a top-level resolvers array whose entries override built-in dependency resolution and force re-resolution when needed. See #10389 for the TypeScript-side feature request that motivated this port.

What's included

  • Hook contractCustomResolver trait (canResolve / resolve / shouldRefreshResolution) mirroring hooks/types/src/index.ts. All three methods are optional upstream, so the Node worker reports per-resolver capability flags in one IPC round trip and pacquet skips calls a resolver doesn't implement (mirrors pnpm's if (!customResolver.canResolve || !customResolver.resolve) continue and checkCustomResolverForceResolve's hook filter).
  • Node IPC — the long-lived pnpmfile worker gained resolvers (capabilities) and resolver (method invocation) requests. Methods are invoked with this bound to the resolver object, like pnpm. Pending-request cleanup is cancellation-safe via an RAII guard.
  • Adapter & chain integrationCustomResolverAdapter bridges the JSON hook contract to the typed Resolver trait. Custom resolvers are built into the inner resolver chain ahead of the built-ins (upstream chain priority), inside the prefetching/observing wrappers so their tarball results get resolve-time prefetch and pnpr streaming. canResolve results are memoized keyed alias@bareSpecifier, exactly like pnpm's getCustomResolverCacheKey. A resolver-returned manifest passes through (pnpm spreads the whole hook result). Payloads match upstream: prevSpecifier, and resolve opts carry lockfileDir / projectDir / preferredVersions / currentPkg.
  • shouldRefreshResolution semantics — port of checkCustomResolverForceResolve: the hook receives the merged packages+snapshots entry (pnpm's in-memory PackageSnapshot), checks run concurrently with first-true/first-error short-circuit, and a throwing hook aborts the install (PNPMFILE_FAIL). A true verdict defeats both up-to-date optimizations, as documented in the hook's contract:
    • the prefer-frozen dispatch consults the hook (pnpm: forceResolutionFromHookneedsFullResolution blocks isFrozenInstallPossible) and routes to the fresh-resolve path with lockfile reuse disabled (UpdateReuseScope::None);
    • the optimistic repeat-install fast path now ports the pnpmfile branch of patchesOrHooksAreModified: the workspace state records the loaded pnpmfile list, and an added/removed/edited pnpmfile invalidates the mtime check.
  • CurrentPkg — added to ResolveOptions, matching upstream's currentPkg shape {id, name?, version?, resolution, publishedAt?} (camelCase).

Tests

  • Adapter unit tests: missing id/resolution, invalid shapes, canResolve memoization, payload shapes, manifest passthrough.
  • check_custom_resolver_force_resolve unit tests: port of upstream's checkCustomResolverForceResolve.ts suite (capability filter, true/false/error propagation, merged snapshot payload).
  • Node IPC integration tests against a real pnpmfile: capabilities, this binding, round trips, error propagation, cancellation cleanup.
  • CLI e2e tests against the mock registry: custom resolver precedence over the npm resolver, shouldRefreshResolution re-resolving past an up-to-date lockfile, and a throwing hook failing the install.

Known gaps / follow-ups

  • ResolveOptions.current_pkg is never populated yet — pacquet's deps-resolver doesn't thread lockfile-reuse info into per-dependency resolve options the way pnpm's package-requester does, so resolvers currently receive currentPkg: null. The contract and serialization are in place for when that threading lands.
  • Custom resolution shapes outside pacquet's typed LockfileResolution enum (e.g. {type: 'custom:cdn', ...}) are rejected with a clear error, whereas pnpm's structural typing passes them through. Supporting arbitrary resolution shapes in the typed lockfile is a separate effort.
  • Custom fetcher hooks (CustomFetcher) are not part of this PR.

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

Summary by CodeRabbit

  • New Features

    • pnpmfiles can provide custom resolvers that run before built-in resolvers and are prepended to the resolver chain.
    • Install now queries custom resolvers to decide when package resolutions must be refreshed.
    • Node-backed hooks can enumerate and invoke custom resolvers at runtime.
    • Resolver options can include information about prior resolutions to avoid unnecessary updates.
  • Bug Fixes / Reliability

    • Errors from custom resolvers are surfaced during install, preventing silent failures.

Review Change Stack

@kairosci kairosci requested a review from zkochan as a code owner June 2, 2026 22:16
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

Adds pnpmfile custom resolver support: new CustomResolver trait and Node-backed implementations, a JSON-to-typed Resolver adapter, Node worker RPC for resolver calls, and installer wiring that prepends custom resolvers and enforces refresh when needed.

Changes

Custom Resolver Hooks Integration

Layer / File(s) Summary
Hooks trait and CustomResolver contract
pacquet/crates/hooks/src/lib.rs
Export custom_resolver_adapter and add PnpmfileHooks::get_custom_resolvers. Define CustomResolver trait with async can_resolve, resolve, and should_refresh_resolution. Update NoopHooks to return Ok(vec![]).
CustomResolverAdapter bridge to typed resolution
pacquet/crates/hooks/src/custom_resolver_adapter.rs
Add CustomResolverAdapter implementing the typed Resolver trait: cache can_resolve results, serialize inputs to JSON, call the JSON-based resolver, validate returned { id, resolution }, deserialize resolution, and map errors. Implements resolve_latest returning Ok(None).
Node worker resolver protocol support
pacquet/crates/hooks/src/worker.rs
Refactor NodeWorker::call to use send_line; add call_resolver and get_resolver_count; extend the Node runner to handle req.target === 'resolverCount' and req.target === 'resolver', invoking resolver methods and returning { ok: ... }.
NodeJsHooks custom resolver implementation
pacquet/crates/hooks/src/node_runtime.rs
Implement NodeJsHooks::get_custom_resolvers that reads resolver count and returns NodeJsCustomResolver instances. NodeJsCustomResolver delegates can_resolve, resolve, and should_refresh_resolution to NodeWorker::call_resolver.
Install flow custom resolver wiring and refresh logic
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
Load custom resolvers from pnpmfile, wrap them with CustomResolverAdapter, and prepend to the resolver chain. Precompute update_reuse_scope and set to None if any resolver’s should_refresh_resolution returns true. Add error variants for hook/load failures and force-resolve triggers.
Cargo.toml dependency updates
pacquet/crates/hooks/Cargo.toml
Expand dependencies to include workspace-managed serde_json, tokio, derive_more, async-trait, pacquet-resolving-resolver-base, and pacquet-lockfile.
CurrentPkg & ResolveOptions
pacquet/crates/resolving-resolver-base/src/resolve.rs
Add CurrentPkg struct and extend ResolveOptions with current_pkg: Option<CurrentPkg> to carry previously-resolved lockfile entry data.

Sequence Diagram(s)

sequenceDiagram
  participant InstallWithFreshLockfile
  participant PnpmfileHooks
  participant NodeWorker
  participant CustomResolver
  participant CustomResolverAdapter
  participant DefaultResolver
  InstallWithFreshLockfile->>PnpmfileHooks: get_custom_resolvers()
  PnpmfileHooks->>NodeWorker: get_resolver_count()
  InstallWithFreshLockfile->>NodeWorker: call_resolver (canResolve/resolve/shouldRefreshResolution)
  NodeWorker->>CustomResolver: invoke method with JSON payload
  CustomResolver-->>NodeWorker: JSON result { ok: ... }
  NodeWorker-->>PnpmfileHooks: resolver results
  InstallWithFreshLockfile->>CustomResolverAdapter: wrap and prepend custom resolvers
  CustomResolverAdapter->>DefaultResolver: delegate when not resolvable / return typed ResolveResult
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#12102: Modifies similar update_seed_policy/snapshots logic in the install flow.
  • pnpm/pnpm#11867: Related adjustments to InstallWithFreshLockfile resolver wiring and install flow.
  • pnpm/pnpm#12046: Related changes to InstallWithFreshLockfile::run and fresh-resolution gating.

Suggested reviewers

  • zkochan

"🐰
I hopped through hooks both old and new,
Bridged JSON whispers into Rusty view.
Resolvers march in chain, fresh checks in tow,
A tiny rabbit cheers — let installations flow! 🥕"

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly summarizes the main change: adding support for pnpmfile custom resolver hooks with specific technical components (adapter, IPC, chain integration, force-refresh).

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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

Review Summary by Qodo

Implement custom resolver hooks with adapter and IPC integration

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add custom resolver trait and adapter for pnpmfile hooks
• Implement Node.js IPC communication for resolver methods
• Integrate custom resolvers into dependency resolution chain
• Support force re-resolution via shouldRefreshResolution callback
Diagram
flowchart LR
  PnpmfileHooks["PnpmfileHooks trait"]
  CustomResolver["CustomResolver trait"]
  NodeJsCustomResolver["NodeJsCustomResolver impl"]
  CustomResolverAdapter["CustomResolverAdapter impl"]
  NodeWorker["NodeWorker IPC"]
  InstallWithFreshLockfile["InstallWithFreshLockfile"]
  
  PnpmfileHooks -- "get_custom_resolvers" --> CustomResolver
  CustomResolver -- "implemented by" --> NodeJsCustomResolver
  NodeJsCustomResolver -- "calls via" --> NodeWorker
  CustomResolver -- "adapted to" --> CustomResolverAdapter
  CustomResolverAdapter -- "inserted into" --> InstallWithFreshLockfile
  InstallWithFreshLockfile -- "checks" --> CustomResolver

Loading

Grey Divider

File Changes

1. pacquet/crates/hooks/src/custom_resolver_adapter.rs ✨ Enhancement +140/-0

Add custom resolver adapter for resolver chain

• New file implementing CustomResolverAdapter struct that bridges CustomResolver trait to typed
 Resolver trait
• Converts JSON-based custom resolver interface to structured resolver chain format
• Implements caching for can_resolve checks to optimize performance
• Handles conversion between WantedDependency/ResolveOptions and JSON values

pacquet/crates/hooks/src/custom_resolver_adapter.rs


2. pacquet/crates/hooks/src/lib.rs ✨ Enhancement +24/-0

Define CustomResolver trait and hook integration

• Add CustomResolver trait with three async methods: can_resolve, resolve, should_refresh_resolution
• Add get_custom_resolvers method to PnpmfileHooks trait
• Implement get_custom_resolvers for NoopHooks returning empty vector
• Export custom_resolver_adapter module

pacquet/crates/hooks/src/lib.rs


3. pacquet/crates/hooks/src/node_runtime.rs ✨ Enhancement +60/-0

Implement Node.js custom resolver with IPC

• Implement get_custom_resolvers for NodeJsHooks to fetch resolver count and create
 NodeJsCustomResolver instances
• Add NodeJsCustomResolver struct wrapping worker and resolver index
• Implement CustomResolver trait for NodeJsCustomResolver with IPC calls to Node.js worker
• Support can_resolve, resolve, and should_refresh_resolution methods via worker.call_resolver

pacquet/crates/hooks/src/node_runtime.rs


View more (3)
4. pacquet/crates/hooks/src/worker.rs ✨ Enhancement +67/-0

Add IPC support for custom resolver communication

• Add call_resolver method to NodeWorker for invoking custom resolver methods via IPC
• Add get_resolver_count method to query number of custom resolvers from pnpmfile
• Extract common send_line logic to reduce duplication
• Update Node.js handler to support resolverCount and resolver method targets

pacquet/crates/hooks/src/worker.rs


5. pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs ✨ Enhancement +93/-11

Integrate custom resolvers into install resolution flow

• Load custom resolvers from pnpmfile and wrap them in adapters
• Prepend custom resolvers to resolver chain before built-in resolvers
• Add should_refresh_resolution check to force re-resolution when custom resolvers request it
• Move update_reuse_scope calculation before workspace resolution to account for custom resolver
 overrides
• Add helper function should_refresh_resolution to check all custom resolvers against lockfile
 snapshots

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


6. pacquet/crates/hooks/Cargo.toml Dependencies +6/-4

Add resolver and lockfile dependencies

• Add pacquet-resolving-resolver-base dependency for Resolver trait
• Add pacquet-lockfile dependency for snapshot types
• Reformat dependencies section for consistency

pacquet/crates/hooks/Cargo.toml


Grey Divider

Qodo Logo

@codecov-commenter

codecov-commenter commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.85542% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.02%. Comparing base (a751c7f) to head (8e80c36).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...acquet/crates/hooks/src/custom_resolver_adapter.rs 83.47% 19 Missing ⚠️
pacquet/crates/hooks/src/lib.rs 0.00% 11 Missing ⚠️
pacquet/crates/hooks/src/node_runtime.rs 87.50% 3 Missing ⚠️
...s/package-manager/src/optimistic_repeat_install.rs 87.50% 3 Missing ⚠️
pacquet/crates/hooks/src/worker.rs 97.43% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12153      +/-   ##
==========================================
+ Coverage   87.97%   88.02%   +0.05%     
==========================================
  Files         292      294       +2     
  Lines       36752    37137     +385     
==========================================
+ Hits        32332    32690     +358     
- Misses       4420     4447      +27     

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

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pacquet/crates/hooks/src/worker.rs (1)

171-175: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove the pending entry when sending the request fails.

If write_all or flush errors here, the Pending inserted by the caller is never removed because this path returns before the timeout cleanup runs. Repeated calls against a broken worker will keep leaking entries until the reader task eventually tears the process down.

Proposed fix
     async fn send_line(
         &self,
         line: String,
         id: u64,
         hook: &str,
         rx: oneshot::Receiver<Result<Value, String>>,
     ) -> Result<Value, HookError> {
-        {
-            let mut stdin = self.stdin.lock().await;
-            stdin.write_all(line.as_bytes()).await.map_err(|err| self.exec_err(err.to_string()))?;
-            stdin.flush().await.map_err(|err| self.exec_err(err.to_string()))?;
+        let write_result = {
+            let mut stdin = self.stdin.lock().await;
+            if let Err(err) = stdin.write_all(line.as_bytes()).await {
+                Err(err)
+            } else {
+                stdin.flush().await
+            }
+        };
+        if let Err(err) = write_result {
+            self.pending.lock().await.remove(&id);
+            return Err(self.exec_err(err.to_string()));
         }
 
         match timeout(HOOK_TIMEOUT, rx).await {
🤖 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/hooks/src/worker.rs` around lines 171 - 175, The write path
that uses self.stdin.lock().await and calls write_all/flush can fail and
currently returns early, leaving the caller-inserted Pending entry in the
pending map; modify the send logic so that on any error from write_all or flush
you remove the corresponding Pending entry (e.g. call
pending_map.remove(&request_id) or self.pending.remove(&id)) before returning
the error (instead of returning directly from the map_err closure); locate the
block around self.stdin, write_all, flush and exec_err and change the error
handling to explicitly remove the Pending entry and then return
self.exec_err(err.to_string()).
🤖 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/hooks/src/custom_resolver_adapter.rs`:
- Around line 35-42: The JSON emitted by wanted_to_value currently uses the
wrong key "pref" for WantedDependency.prev_specifier; update the serializer in
wanted_to_value to emit the key "prevSpecifier" (matching the upstream
WantedDependency shape) instead of "pref" so custom resolvers receive the
original specifier field name.
- Around line 45-50: opts_to_value is currently serializing a stubbed payload
(empty preferredVersions and missing currentPkg) which alters resolver behavior;
change it to serialize the real ResolveOptions fields and forward them to
customResolver.resolve. Specifically, have opts_to_value read and serialize
opts.preferred_versions and opts.current_pkg (alongside lockfile_dir and
project_dir using to_string_lossy) into the JSON Value so the payload matches
upstream pnpm (same keys: lockfileDir, projectDir, preferredVersions,
currentPkg) before calling customResolver.resolve.

In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 942-947: The current logic downgrades errors from
should_refresh_resolution to warnings and proceeds to reuse stale lockfile
subtrees; instead, make should_refresh_resolution failures propagate and abort
the install like pnpm's checkCustomResolverForceResolve does. Concretely, modify
the branch around should_refresh_resolution(...) so that any Err returned is
propagated (return Err or map_err?) to the caller rather than just logging, and
only set pacquet_resolving_deps_resolver::UpdateReuseScope::None when
should_refresh_resolution succeeds and returns true; apply the same change to
the other occurrence referenced (the block around the function near the
1767-1778 region) so both uses of should_refresh_resolution enforce hook
failures consistently.
- Around line 854-867: The code currently swallows errors from
pnpmfile_hook.get_custom_resolvers() and returns an empty vec
(custom_resolvers_raw), which silently disables resolver overrides; instead
detect Err(e) and fail the install by propagating or returning that error (do
not fallback to vec![]). Update the block around pnpmfile_hook /
get_custom_resolvers() so that on Err you log the error and return an Err from
the surrounding async function (or use ? to propagate), ensuring the install
aborts when custom resolvers cannot be loaded; keep references to pnpmfile_hook,
get_custom_resolvers, and custom_resolvers_raw when making the change so the
correct code path is modified.

---

Outside diff comments:
In `@pacquet/crates/hooks/src/worker.rs`:
- Around line 171-175: The write path that uses self.stdin.lock().await and
calls write_all/flush can fail and currently returns early, leaving the
caller-inserted Pending entry in the pending map; modify the send logic so that
on any error from write_all or flush you remove the corresponding Pending entry
(e.g. call pending_map.remove(&request_id) or self.pending.remove(&id)) before
returning the error (instead of returning directly from the map_err closure);
locate the block around self.stdin, write_all, flush and exec_err and change the
error handling to explicitly remove the Pending entry and then return
self.exec_err(err.to_string()).
🪄 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: 730be9d1-38aa-4d19-a67a-6beef122acd9

📥 Commits

Reviewing files that changed from the base of the PR and between 1c73e83 and 3e6e3a3.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/worker.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Compile & Lint
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/Cargo.toml

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/Cargo.toml: Check whether the workspace already depends on something suitable in [workspace.dependencies] in the root Cargo.toml before adding a new dependency
Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it

Files:

  • pacquet/crates/hooks/Cargo.toml
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/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/hooks/src/worker.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🧠 Learnings (32)
📓 Common learnings
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/tests/**/*.rs : Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.
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:30.632Z
Learning: In the pacquet Rust port of pnpm, the `is_http_url` helper in `pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs` intentionally uses `bare.starts_with("http:") || bare.starts_with("https:")` (not `"http://"` / `"https://"`) to match upstream pnpm's `startsWith('http:')` / `startsWith('https:')` check byte-for-byte. Pacquet's cardinal rule (pacquet/AGENTS.md) requires matching pnpm even on quirks; malformed non-URL inputs are rejected downstream by `reqwest::Url::parse` as a `ResolveError`.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs:70-88
Timestamp: 2026-05-20T23:07:43.668Z
Learning: In `pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`, `resolve_impl` intentionally passes `ResolveOptions::default()` (not the caller-provided `opts`) when delegating to the npm resolver. This mirrors the upstream TypeScript code at `engine/runtime/bun-resolver/src/index.ts#L46` which passes an empty `{}` literal; forwarding outer opts would diverge from pnpm's behavior.
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/Cargo.toml : Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/**/Cargo.toml : Declare new shared dependencies in the root [workspace.dependencies] and use { workspace = true } in pnpr crate's Cargo.toml

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/Cargo.toml : Check whether the workspace already depends on something suitable in `[workspace.dependencies]` in the root `Cargo.toml` before adding a new dependency

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Do not add a dependency not already declared in `[workspace.dependencies]` without explicit human request; ask for approval and request addition to the workspace root `Cargo.toml`

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Prefer existing pacquet-* crates over writing new code; check pacquet-tarball, pacquet-crypto-hash, pacquet-crypto-shasums-file, pacquet-package-manifest, pacquet-network, pacquet-registry, pacquet-fs, and pacquet-diagnostics before implementing non-trivial functionality

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/crates/**/Cargo.toml : New registry-only crates must be placed under pnpr/crates/<short-name>/ and named pnpr-<short-name> in Cargo.toml, never using the pacquet- prefix

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-25T12:36:42.202Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: The `pre-push` hook runs `pacquet/scripts/pre-push-rust.sh` which checks `rustfmt`, `taplo`, `cargo doc` with `-D warnings`, and `cargo dylint`

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/crates/**/Cargo.toml : Cargo.toml files for new registry-only crates must use the pnpr- prefix for the package name

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/**/*.rs : Follow the pacquet code-style guide (../pacquet/CODE_STYLE_GUIDE.md) for Rust-level conventions including imports, naming, ownership, and error handling

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-19T19:23:00.981Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11752
File: pacquet/crates/config/src/lib.rs:1062-1073
Timestamp: 2026-05-19T19:23:00.981Z
Learning: In `pacquet/crates/config/src/lib.rs`, `modules_dir` does not need a `!virtual_store_dir_explicit` guard on its workspace re-anchor because `modules_dir` is in pnpm's `excludedPnpmKeys` (filtered out by `WorkspaceSettings::clear_workspace_only_fields`) and therefore can only be set by workspace yaml (applied immediately after) or env vars (applied later in the cascade) — not by global `config.yaml`. `virtual_store_dir`, by contrast, IS settable from global config and requires the `if !virtual_store_dir_explicit` guard to survive the workspace-root re-anchor.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-21T00:33:05.035Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11789
File: pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs:145-146
Timestamp: 2026-05-21T00:33:05.035Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs`, the guard `bare.starts_with("workspace:.")` is intentionally broad — matching pnpm upstream's identical `startsWith('workspace:.')` check at `resolving/npm-resolver/src/index.ts`. All dot-prefixed workspace forms including `workspace:.foo` are intentionally passed through to the local-resolver, which handles them as `link:`-style directory specs via its prefix-stripping regex. Do not suggest narrowing this to `workspace:./` or `workspace:../` only.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-06-02T14:39:19.809Z
Learnt from: KSXGitHub
Repo: pnpm/pnpm PR: 11938
File: pacquet/crates/config/src/lib.rs:959-966
Timestamp: 2026-06-02T14:39:19.809Z
Learning: In the pnpm/pnpm pacquet Rust port (`pacquet/crates/config/src/lib.rs`), `Config.extra_bin_paths` is intentionally left empty (`Vec::new()`) until workspace fan-out support for `run`/`exec` is implemented. It mirrors pnpm's `Config.extraBinPaths` (the workspace-root `node_modules/.bin`), which is also empty outside a workspace. Populating it before the workspace-root is resolved would put a non-existent path on `PATH`, so it should only be derived once workspace support lands. Do not flag this as a bug or missing derivation in reviews.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-21T00:33:05.035Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11789
File: pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs:145-146
Timestamp: 2026-05-21T00:33:05.035Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs`, the guard `bare.starts_with("workspace:.")` is intentionally broad — matching pnpm upstream's identical `startsWith('workspace:.')` check. All dot-prefixed workspace forms including `workspace:.foo` are intentionally handed off to the local-resolver, which handles them as `link:`-style directory specs via its prefix-stripping regex. Do not suggest narrowing this to `workspace:./` or `workspace:../` only.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/*.rs : 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

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-20T10:06:55.749Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs:15-18
Timestamp: 2026-05-20T10:06:55.749Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`, the `DirectDep.id`, `ResolvedPackage.id`, and `ResolvedTree.packages` HashMap keys are intentionally plain `String` for now. The natural branded type would be `pacquet_lockfile::PkgNameVer`, but it cannot be used as a HashMap key because `node_semver::Version` does not derive `Hash`. The upstream parity type is `PkgResolutionId` (which carries an optional peer-dep suffix), and the branded type should be introduced alongside peer-dep resolution and lockfile generation work to avoid locking the seam too early.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/lib.rs
📚 Learning: 2026-05-20T23:07:43.668Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs:70-88
Timestamp: 2026-05-20T23:07:43.668Z
Learning: In `pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`, `resolve_impl` intentionally passes `ResolveOptions::default()` (not the caller-provided `opts`) when delegating to the npm resolver. This mirrors the upstream TypeScript code at `engine/runtime/bun-resolver/src/index.ts#L46` which passes an empty `{}` literal; forwarding outer opts would diverge from pnpm's behavior.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:07:40.480Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs:79-80
Timestamp: 2026-05-20T23:07:40.480Z
Learning: In `pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs` (and the analogous Bun resolver), passing `ResolveOptions::default()` when delegating version selection to the inner npm resolver is intentional. It mirrors the upstream TypeScript code in `engine/runtime/deno-resolver/src/index.ts#L56` which explicitly passes `{}` (an empty/default options literal). Forwarding the outer `ResolveOptions` would diverge from pnpm upstream behavior and must NOT be flagged as a bug.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 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/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/hooks/src/worker.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.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/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/hooks/src/worker.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.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/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/hooks/src/worker.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:07:40.413Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs:167-191
Timestamp: 2026-05-20T23:07:40.413Z
Learning: In `pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs`, the offline guard in `resolve_impl` (NodeResolver) is intentionally absent from `resolve_latest_impl`. This mirrors upstream pnpm's behavior: `resolveNodeRuntime` checks offline, but `resolveLatestNodeRuntime` does not carry `offline` in its context and never checks it. The asymmetry is required so `pnpm outdated` / `pnpm update --latest` can still query for newer runtime versions even in offline mode.

Applied to files:

  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.

Applied to files:

  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T21:18:56.391Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11778
File: pacquet/crates/resolving-local-resolver/tests/resolve.rs:365-372
Timestamp: 2026-05-20T21:18:56.391Z
Learning: In `pacquet/crates/resolving-local-resolver/tests/resolve.rs`, the test `fail_when_resolving_from_not_existing_directory_an_injected_dependency` intentionally uses `injected: false`. The test is a verbatim port of the upstream pnpm TypeScript test (resolving/local-resolver/test/index.ts at ef87f3ccff). The `injected` flag only affects the file/link protocol choice for plain directory paths; when the `file:` scheme is explicit in the bare specifier, the flag has no effect on the resolution code path. The misleading test name is inherited from upstream.

Applied to files:

  • pacquet/crates/hooks/src/lib.rs
📚 Learning: 2026-05-20T10:51:16.296Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-npm-resolver/src/named_registry.rs:109-127
Timestamp: 2026-05-20T10:51:16.296Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/named_registry.rs`, the `pick_registry_for_package` function's logic `scope_from_bare_specifier(bare_specifier).or_else(|| scope_of(pkg_name))` is intentionally correct and matches upstream pnpm's `getScope` in `config/pick-registry-for-package/src/index.ts`. When bare_specifier is an npm: alias for an unscoped package (e.g., `npm:lodash@^1`), `scope_from_bare_specifier` returns `None`, and the fallthrough to `scope_of(pkg_name)` is correct behavior — allowing a scoped local name like `private/foo` to still route through the `private` registry. Do NOT suggest changing this to an early-return on `npm:` prefix, as that would break pnpm compatibility.

Applied to files:

  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-23T09:14:43.635Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not run lifecycle scripts (`BuildModules` is not invoked, and `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput` is discarded). This is pre-existing behavior documented by an in-code comment mirroring upstream `link.ts:167-170`. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Wiring lifecycle scripts into the fresh-lockfile path is tracked as future work, separate from this file's concern.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-23T09:14:43.635Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not invoke `BuildModules` and discards `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput`. This is pre-existing, documented behavior (mirroring upstream `link.ts:167-170`): `importing_done` fires once extraction and symlink linking are complete, and the fresh-lockfile path does not run lifecycle scripts. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Do not flag this omission as a bug; wiring lifecycle scripts into the fresh-lockfile path is tracked as future work separate from perf refactors.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:08:06.093Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs`, the `else` branch that calls `max_satisfying_any` is intentional and matches upstream pnpm's `hoistPeers.ts` (lines 45-59). When `is_exact_version && !auto_install_peers`, pnpm still runs `semver.maxSatisfying(versions, '*', { includePrerelease: true })` and picks the closest available preferred version; any version mismatch is reported downstream as a `bad` peer issue rather than dropping the edge. Do NOT suggest changing this branch to skip `max_satisfying_any` for exact ranges — it would diverge from pnpm.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🔇 Additional comments (2)
pacquet/crates/hooks/Cargo.toml (1)

15-15: ⚡ Quick win

async-trait isn’t in root [workspace.dependencies]; add it there (or justify local pin)

  • async-trait is referenced only in pacquet/crates/hooks/Cargo.toml as async-trait = "0.1" and has no corresponding entry in the root [workspace.dependencies].
  • If this PR is introducing async-trait, it should be added to the root [workspace.dependencies] and consumed here via async-trait = { workspace = true }; otherwise document the explicit approval for keeping it local.
async-trait                     = "0.1"
pacquet/crates/hooks/src/lib.rs (1)

6-6: LGTM!

Also applies to: 91-109, 133-135

Comment thread pacquet/crates/hooks/src/custom_resolver_adapter.rs
Comment thread pacquet/crates/hooks/src/custom_resolver_adapter.rs
Comment thread pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs Outdated
Comment thread pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs Outdated
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD.

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.119 ± 0.218 3.897 4.535 1.90 ± 0.13
pacquet@main 4.132 ± 0.153 3.971 4.337 1.91 ± 0.11
pnpr@HEAD 2.191 ± 0.151 1.961 2.367 1.01 ± 0.08
pnpr@main 2.166 ± 0.095 2.007 2.319 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 4.118893337779999,
      "stddev": 0.21795646998853166,
      "median": 4.043077178779999,
      "user": 3.9258338999999998,
      "system": 3.4763250599999997,
      "min": 3.89721809778,
      "max": 4.53494550478,
      "times": [
        4.348516946779999,
        4.17491753478,
        4.01147781278,
        4.05735361778,
        4.53494550478,
        4.31568852678,
        3.91126534478,
        4.028800739779999,
        3.90874925178,
        3.89721809778
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 4.131784361279999,
      "stddev": 0.1525190998198899,
      "median": 4.113974282779999,
      "user": 3.8968915,
      "system": 3.50090986,
      "min": 3.97149489178,
      "max": 4.33709685678,
      "times": [
        4.327467331779999,
        3.98147112878,
        4.24724103278,
        4.227732695779999,
        4.00904495878,
        4.33709685678,
        4.21890360678,
        3.97149489178,
        3.99639946478,
        4.000991644779999
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.19093419288,
      "stddev": 0.15122253649807044,
      "median": 2.2237689117799997,
      "user": 2.6886217999999995,
      "system": 3.0206895599999997,
      "min": 1.9610654527800002,
      "max": 2.36717151778,
      "times": [
        2.17244052678,
        2.07810596178,
        2.33928256478,
        2.27509729678,
        1.98826568978,
        2.36717151778,
        2.32502462378,
        2.30262836178,
        1.9610654527800002,
        2.10025993278
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.1656499170800005,
      "stddev": 0.09517980250986526,
      "median": 2.16484279378,
      "user": 2.6517647999999996,
      "system": 3.0107908599999993,
      "min": 2.00693344778,
      "max": 2.31948768778,
      "times": [
        2.11130178778,
        2.10556993178,
        2.22962690778,
        2.27521021178,
        2.31948768778,
        2.00693344778,
        2.15536802678,
        2.07433849778,
        2.20434511078,
        2.17431756078
      ]
    }
  ]
}

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

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 673.2 ± 12.9 656.1 704.2 1.00
pacquet@main 693.8 ± 12.6 682.6 720.5 1.03 ± 0.03
pnpr@HEAD 731.9 ± 26.6 707.9 780.1 1.09 ± 0.04
pnpr@main 720.3 ± 14.6 700.3 741.5 1.07 ± 0.03
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6731743934799999,
      "stddev": 0.012933625404518981,
      "median": 0.67048255568,
      "user": 0.39160112,
      "system": 1.3425305399999998,
      "min": 0.6560605741800001,
      "max": 0.70421433018,
      "times": [
        0.67854826518,
        0.66917786918,
        0.6644942541800001,
        0.66801188018,
        0.68079033318,
        0.70421433018,
        0.6717872421800001,
        0.6560605741800001,
        0.67186027918,
        0.66679890718
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.69383672398,
      "stddev": 0.012553492666263323,
      "median": 0.68989421368,
      "user": 0.39796302000000006,
      "system": 1.35679224,
      "min": 0.68257128718,
      "max": 0.72053646118,
      "times": [
        0.7072217631800001,
        0.68329093218,
        0.68726506418,
        0.7023734031800001,
        0.6841400091800001,
        0.6925233631800001,
        0.68257128718,
        0.68511411818,
        0.6933308381800001,
        0.72053646118
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.73193074098,
      "stddev": 0.026570033072647083,
      "median": 0.72465837768,
      "user": 0.40072092000000004,
      "system": 1.36254364,
      "min": 0.70789107918,
      "max": 0.78006266518,
      "times": [
        0.73897776818,
        0.72435976918,
        0.7274120591800001,
        0.70991652618,
        0.70789107918,
        0.71553252318,
        0.71206307918,
        0.78006266518,
        0.77813495418,
        0.72495698618
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.7202755536800001,
      "stddev": 0.014627904847007274,
      "median": 0.72452052868,
      "user": 0.42057131999999997,
      "system": 1.3548192399999999,
      "min": 0.70026823918,
      "max": 0.7414748801800001,
      "times": [
        0.70397937718,
        0.72435771818,
        0.73616603618,
        0.70026823918,
        0.72594793118,
        0.73190366318,
        0.72468333918,
        0.7414748801800001,
        0.7078118371800001,
        0.7061625151800001
      ]
    }
  ]
}

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 3.984 ± 0.063 3.852 4.068 1.88 ± 0.10
pacquet@main 3.883 ± 0.050 3.818 3.964 1.84 ± 0.09
pnpr@HEAD 2.206 ± 0.075 2.087 2.364 1.04 ± 0.06
pnpr@main 2.115 ± 0.106 1.986 2.294 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 3.98438979824,
      "stddev": 0.06313474374622727,
      "median": 3.99224921284,
      "user": 3.7064038999999993,
      "system": 3.3824939599999992,
      "min": 3.85229536184,
      "max": 4.067808573840001,
      "times": [
        4.030450875840001,
        3.96733029884,
        4.050139150840001,
        3.9636481198399998,
        3.99465419084,
        3.98984423484,
        3.92310375884,
        4.00462341684,
        4.067808573840001,
        3.85229536184
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 3.8825918677399995,
      "stddev": 0.049681555428636004,
      "median": 3.87952360884,
      "user": 3.6348516999999996,
      "system": 3.33956206,
      "min": 3.81756949484,
      "max": 3.9637598118399997,
      "times": [
        3.89788352384,
        3.90169830284,
        3.86116369384,
        3.81756949484,
        3.8532736068399998,
        3.9637598118399997,
        3.84183863084,
        3.92622129684,
        3.93562713484,
        3.82688318084
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 2.2062669019400003,
      "stddev": 0.07519156461888829,
      "median": 2.21234452884,
      "user": 2.5034411,
      "system": 2.90447476,
      "min": 2.0869015098399997,
      "max": 2.3637275068399997,
      "times": [
        2.2341500918399997,
        2.0869015098399997,
        2.24765729084,
        2.23266965484,
        2.18729598384,
        2.14412166984,
        2.3637275068399997,
        2.2034551858399998,
        2.22123387184,
        2.14145625384
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 2.1146463260400004,
      "stddev": 0.10569553936838769,
      "median": 2.09026470434,
      "user": 2.4927221999999998,
      "system": 2.8794742600000003,
      "min": 1.98641580284,
      "max": 2.29351304884,
      "times": [
        2.08535578284,
        2.0951736258399998,
        2.03990417784,
        2.1496337358399997,
        2.17074934884,
        2.0253403618399997,
        1.98641580284,
        2.29351304884,
        2.27349784984,
        2.02687952584
      ]
    }
  ]
}

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.148 ± 0.012 1.137 1.177 1.69 ± 0.03
pacquet@main 1.172 ± 0.040 1.150 1.285 1.73 ± 0.06
pnpr@HEAD 0.695 ± 0.031 0.666 0.773 1.03 ± 0.05
pnpr@main 0.677 ± 0.008 0.666 0.694 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.1480477567999998,
      "stddev": 0.01205642305469167,
      "median": 1.1449537309999998,
      "user": 1.14302538,
      "system": 1.67500088,
      "min": 1.137458208,
      "max": 1.1774334469999999,
      "times": [
        1.1413689059999998,
        1.1381011989999998,
        1.1469312619999998,
        1.1774334469999999,
        1.147425165,
        1.1429761999999999,
        1.137458208,
        1.1521297289999999,
        1.156836856,
        1.139816596
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.1722296752999999,
      "stddev": 0.04029064699684883,
      "median": 1.160434307,
      "user": 1.18002728,
      "system": 1.67792238,
      "min": 1.1496182529999999,
      "max": 1.284892736,
      "times": [
        1.164951468,
        1.1533961209999999,
        1.1496182529999999,
        1.284892736,
        1.155256549,
        1.165741746,
        1.166626947,
        1.1529945769999999,
        1.155917146,
        1.17290121
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6952916362000001,
      "stddev": 0.030996858333474493,
      "median": 0.6899760970000001,
      "user": 0.34099577999999997,
      "system": 1.29099118,
      "min": 0.6659010790000001,
      "max": 0.7733442460000001,
      "times": [
        0.6659010790000001,
        0.687634311,
        0.6923178830000001,
        0.6745883450000001,
        0.7733442460000001,
        0.6829817940000001,
        0.6941297590000001,
        0.6681834090000001,
        0.7026133430000001,
        0.7112221930000001
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.6774844474000001,
      "stddev": 0.008348602478041508,
      "median": 0.6764003795000001,
      "user": 0.34554887999999995,
      "system": 1.29071398,
      "min": 0.666215956,
      "max": 0.694274432,
      "times": [
        0.675918808,
        0.6759949890000001,
        0.6801580730000001,
        0.6667881780000001,
        0.6768057700000001,
        0.6829379710000001,
        0.6723219960000001,
        0.694274432,
        0.6834283010000001,
        0.666215956
      ]
    }
  ]
}

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.636 ± 0.027 2.590 2.687 3.87 ± 0.11
pacquet@main 2.639 ± 0.052 2.585 2.774 3.88 ± 0.13
pnpr@HEAD 0.694 ± 0.015 0.674 0.723 1.02 ± 0.03
pnpr@main 0.681 ± 0.018 0.665 0.730 1.00
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.6362493223800003,
      "stddev": 0.026996613514120558,
      "median": 2.6348679625800004,
      "user": 1.63088646,
      "system": 1.9227550999999998,
      "min": 2.58990823408,
      "max": 2.6874688310800003,
      "times": [
        2.6104123440800002,
        2.6182575210800003,
        2.58990823408,
        2.63624156308,
        2.6316701680800003,
        2.64732757408,
        2.6874688310800003,
        2.65745262708,
        2.65025999908,
        2.6334943620800004
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.6393030250799994,
      "stddev": 0.05153819403047136,
      "median": 2.62823816208,
      "user": 1.64525006,
      "system": 1.9273947,
      "min": 2.58471590008,
      "max": 2.77397145708,
      "times": [
        2.64218938508,
        2.62184571408,
        2.6560279590800002,
        2.63807734508,
        2.58471590008,
        2.6201616440800004,
        2.62991301108,
        2.59956452208,
        2.77397145708,
        2.62656331308
      ]
    },
    {
      "command": "pnpr@HEAD",
      "mean": 0.6935196884800001,
      "stddev": 0.01475666613158196,
      "median": 0.6890447915800001,
      "user": 0.34712226,
      "system": 1.30595,
      "min": 0.67430582508,
      "max": 0.72281879908,
      "times": [
        0.68756609508,
        0.69908899908,
        0.6896384670800001,
        0.72281879908,
        0.6836552930800001,
        0.68416237108,
        0.7143644250800001,
        0.6884511160800001,
        0.67430582508,
        0.69114549408
      ]
    },
    {
      "command": "pnpr@main",
      "mean": 0.6809219503799999,
      "stddev": 0.018056945381745355,
      "median": 0.6771977595800001,
      "user": 0.34838786,
      "system": 1.2790506999999998,
      "min": 0.66520069008,
      "max": 0.72988313808,
      "times": [
        0.6814599600800001,
        0.68110522908,
        0.6732278540800001,
        0.67568281308,
        0.66520069008,
        0.6817656000800001,
        0.67871270608,
        0.6696320600800001,
        0.67254945308,
        0.72988313808
      ]
    }
  ]
}

@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12153
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,984.39 ms
(-45.04%)Baseline: 7,249.04 ms
8,698.85 ms
(45.80%)
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
🚷 view threshold
2,636.25 ms
(-36.07%)Baseline: 4,123.69 ms
4,948.43 ms
(53.27%)
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
🚷 view threshold
1,148.05 ms
(-11.65%)Baseline: 1,299.38 ms
1,559.25 ms
(73.63%)
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
🚷 view threshold
4,118.89 ms
(-48.43%)Baseline: 7,987.70 ms
9,585.24 ms
(42.97%)
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
🚷 view threshold
673.17 ms
(+0.66%)Baseline: 668.74 ms
802.49 ms
(83.89%)
🐰 View full continuous benchmarking report in Bencher

@kairosci kairosci force-pushed the feat/custom-resolvers branch from 3e6e3a3 to 8c83151 Compare June 3, 2026 08:58

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)

863-946: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Please port the upstream custom-resolver cases to Rust tests.

This adds resolver precedence plus shouldRefreshResolution-driven reuse invalidation, but these paths are still lightly covered. We need parity tests here for hook-load failure propagation, resolver ordering, and forcing UpdateReuseScope::None when any resolver requests refresh.

Based on learnings: match pnpm's behavior and existing tests, and port relevant pnpm tests to Rust tests whenever they translate.

Also applies to: 1756-1770

🤖 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/install_with_fresh_lockfile.rs` around
lines 863 - 946, Add Rust tests that mirror the upstream pnpm cases: write
unit/integration tests invoking the install_with_fresh_lockfile flow (the code
that builds custom_resolvers_raw, wraps them with CustomResolverAdapter,
constructs DefaultResolver::new, calls hook.pre_resolution, and then runs
should_refresh_resolution) to cover (1) hook-load failure propagation — simulate
pnpmfile_hook.get_custom_resolvers() returning an error and assert it maps to
InstallWithFreshLockfileError::CustomResolverHook, (2) resolver
ordering/precedence — register multiple mock custom resolvers (via
pacquet_hooks::CustomResolver implementations or fakes) with deterministic
behaviors and assert resolution uses the first resolver in custom_resolvers_raw
before falling back to the original resolver, and (3)
shouldRefreshResolution-driven reuse invalidation — create a resolver whose
shouldRefreshResolution returns true and assert update_reuse_scope becomes
pacquet_resolving_deps_resolver::UpdateReuseScope::None (and that errors from
shouldRefreshResolution map to
InstallWithFreshLockfileError::CustomResolverForceResolve). Locate test hooks
around the functions/types: should_refresh_resolution,
CustomResolverAdapter::new, DefaultResolver::new, UpdateSeedPolicy, and
InstallWithFreshLockfileError to implement mocks and assertions.
🧹 Nitpick comments (2)
pacquet/crates/hooks/src/lib.rs (1)

104-109: 🏗️ Heavy lift

Keep dep_path branded on the public hook contract.

If this is pacquet’s mirror of pnpm’s dependency-path surface, taking a plain String here bakes the brand away at the API boundary and forces downstream callers to stringify typed keys before every hook call. Prefer a dedicated newtype and do the string conversion only at the IPC/serde boundary.

As per coding guidelines, "Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust."

🤖 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/hooks/src/lib.rs` around lines 104 - 109, The public hook
signature should preserve the branded dep_path type instead of using a plain
String: create a newtype (e.g. DepPath or DependencyPath) wrapping String
(implementing Clone/Debug/Serialize/Deserialize/From<String>/Into<String> as
needed) and change the method signature of should_refresh_resolution(&self,
dep_path: DepPath, pkg_snapshot: Value) -> Result<bool, HookError> so downstream
callers keep the brand; perform any String <-> DepPath conversions only at the
IPC/serde boundary (where you serialize/deserialize hooks) rather than inside
business logic.
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)

878-892: ⚡ Quick win

Wrap the combined resolver chain in PrefetchingResolver.

When lockfile_only is false, resolver is already the prefetch-wrapped built-in chain. Prepending custom adapters here puts them outside that wrapper, so any tarball result returned by a custom resolver skips the resolve-time warm-cache path entirely. Build DefaultResolver(custom + built-ins) first, then wrap that whole chain once.

🤖 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/install_with_fresh_lockfile.rs` around
lines 878 - 892, The custom resolver adapters are being prepended outside the
prefetch wrapper; instead build the combined chain first (push custom adapters
from custom_resolvers_raw, then push the existing resolver), create the
DefaultResolver with DefaultResolver::new(chain), and then wrap that
DefaultResolver in PrefetchingResolver (e.g.
Box::new(PrefetchingResolver::new(Box::new(DefaultResolver::new(chain))))) so
the entire custom+built-in chain goes through the prefetching logic; keep the
resulting value assigned to the resolver variable of type Box<dyn Resolver>.
🤖 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.

Outside diff comments:
In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 863-946: Add Rust tests that mirror the upstream pnpm cases: write
unit/integration tests invoking the install_with_fresh_lockfile flow (the code
that builds custom_resolvers_raw, wraps them with CustomResolverAdapter,
constructs DefaultResolver::new, calls hook.pre_resolution, and then runs
should_refresh_resolution) to cover (1) hook-load failure propagation — simulate
pnpmfile_hook.get_custom_resolvers() returning an error and assert it maps to
InstallWithFreshLockfileError::CustomResolverHook, (2) resolver
ordering/precedence — register multiple mock custom resolvers (via
pacquet_hooks::CustomResolver implementations or fakes) with deterministic
behaviors and assert resolution uses the first resolver in custom_resolvers_raw
before falling back to the original resolver, and (3)
shouldRefreshResolution-driven reuse invalidation — create a resolver whose
shouldRefreshResolution returns true and assert update_reuse_scope becomes
pacquet_resolving_deps_resolver::UpdateReuseScope::None (and that errors from
shouldRefreshResolution map to
InstallWithFreshLockfileError::CustomResolverForceResolve). Locate test hooks
around the functions/types: should_refresh_resolution,
CustomResolverAdapter::new, DefaultResolver::new, UpdateSeedPolicy, and
InstallWithFreshLockfileError to implement mocks and assertions.

---

Nitpick comments:
In `@pacquet/crates/hooks/src/lib.rs`:
- Around line 104-109: The public hook signature should preserve the branded
dep_path type instead of using a plain String: create a newtype (e.g. DepPath or
DependencyPath) wrapping String (implementing
Clone/Debug/Serialize/Deserialize/From<String>/Into<String> as needed) and
change the method signature of should_refresh_resolution(&self, dep_path:
DepPath, pkg_snapshot: Value) -> Result<bool, HookError> so downstream callers
keep the brand; perform any String <-> DepPath conversions only at the IPC/serde
boundary (where you serialize/deserialize hooks) rather than inside business
logic.

In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 878-892: The custom resolver adapters are being prepended outside
the prefetch wrapper; instead build the combined chain first (push custom
adapters from custom_resolvers_raw, then push the existing resolver), create the
DefaultResolver with DefaultResolver::new(chain), and then wrap that
DefaultResolver in PrefetchingResolver (e.g.
Box::new(PrefetchingResolver::new(Box::new(DefaultResolver::new(chain))))) so
the entire custom+built-in chain goes through the prefetching logic; keep the
resulting value assigned to the resolver variable of type Box<dyn Resolver>.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 55a66359-0c1b-48a0-995b-6c6134f91693

📥 Commits

Reviewing files that changed from the base of the PR and between 3e6e3a3 and 8c83151.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/lib.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/worker.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/hooks/src/node_runtime.rs
  • pacquet/crates/hooks/src/worker.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: Lint and Test (windows-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Dylint
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Compile & Lint
  • GitHub Check: Code Coverage
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/Cargo.toml

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/Cargo.toml: Check whether the workspace already depends on something suitable in [workspace.dependencies] in the root Cargo.toml before adding a new dependency
Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it

Files:

  • pacquet/crates/hooks/Cargo.toml
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using #[serde(try_from = "String")] for deserialization and #[serde(into = "String")] for serialization
Derive simple conversions for branded strings using #[derive(derive_more::From)] and #[derive(derive_more::Into)] instead of handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like `${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/hooks/src/lib.rs
🧠 Learnings (33)
📓 Common learnings
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/tests/**/*.rs : Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.
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:30.632Z
Learning: In the pacquet Rust port of pnpm, the `is_http_url` helper in `pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs` intentionally uses `bare.starts_with("http:") || bare.starts_with("https:")` (not `"http://"` / `"https://"`) to match upstream pnpm's `startsWith('http:')` / `startsWith('https:')` check byte-for-byte. Pacquet's cardinal rule (pacquet/AGENTS.md) requires matching pnpm even on quirks; malformed non-URL inputs are rejected downstream by `reqwest::Url::parse` as a `ResolveError`.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/Cargo.toml : Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/**/Cargo.toml : Declare new shared dependencies in the root [workspace.dependencies] and use { workspace = true } in pnpr crate's Cargo.toml

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/Cargo.toml : Check whether the workspace already depends on something suitable in `[workspace.dependencies]` in the root `Cargo.toml` before adding a new dependency

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Do not add a dependency not already declared in `[workspace.dependencies]` without explicit human request; ask for approval and request addition to the workspace root `Cargo.toml`

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Prefer existing pacquet-* crates over writing new code; check pacquet-tarball, pacquet-crypto-hash, pacquet-crypto-shasums-file, pacquet-package-manifest, pacquet-network, pacquet-registry, pacquet-fs, and pacquet-diagnostics before implementing non-trivial functionality

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/crates/**/Cargo.toml : New registry-only crates must be placed under pnpr/crates/<short-name>/ and named pnpr-<short-name> in Cargo.toml, never using the pacquet- prefix

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-25T12:36:42.202Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: The `pre-push` hook runs `pacquet/scripts/pre-push-rust.sh` which checks `rustfmt`, `taplo`, `cargo doc` with `-D warnings`, and `cargo dylint`

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/crates/**/Cargo.toml : Cargo.toml files for new registry-only crates must use the pnpr- prefix for the package name

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Applies to pnpr/**/pnpr/**/*.rs : Follow the pacquet code-style guide (../pacquet/CODE_STYLE_GUIDE.md) for Rust-level conventions including imports, naming, ownership, and error handling

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-19T19:23:00.981Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11752
File: pacquet/crates/config/src/lib.rs:1062-1073
Timestamp: 2026-05-19T19:23:00.981Z
Learning: In `pacquet/crates/config/src/lib.rs`, `modules_dir` does not need a `!virtual_store_dir_explicit` guard on its workspace re-anchor because `modules_dir` is in pnpm's `excludedPnpmKeys` (filtered out by `WorkspaceSettings::clear_workspace_only_fields`) and therefore can only be set by workspace yaml (applied immediately after) or env vars (applied later in the cascade) — not by global `config.yaml`. `virtual_store_dir`, by contrast, IS settable from global config and requires the `if !virtual_store_dir_explicit` guard to survive the workspace-root re-anchor.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-21T00:33:05.035Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11789
File: pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs:145-146
Timestamp: 2026-05-21T00:33:05.035Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs`, the guard `bare.starts_with("workspace:.")` is intentionally broad — matching pnpm upstream's identical `startsWith('workspace:.')` check at `resolving/npm-resolver/src/index.ts`. All dot-prefixed workspace forms including `workspace:.foo` are intentionally passed through to the local-resolver, which handles them as `link:`-style directory specs via its prefix-stripping regex. Do not suggest narrowing this to `workspace:./` or `workspace:../` only.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-06-02T14:39:19.809Z
Learnt from: KSXGitHub
Repo: pnpm/pnpm PR: 11938
File: pacquet/crates/config/src/lib.rs:959-966
Timestamp: 2026-06-02T14:39:19.809Z
Learning: In the pnpm/pnpm pacquet Rust port (`pacquet/crates/config/src/lib.rs`), `Config.extra_bin_paths` is intentionally left empty (`Vec::new()`) until workspace fan-out support for `run`/`exec` is implemented. It mirrors pnpm's `Config.extraBinPaths` (the workspace-root `node_modules/.bin`), which is also empty outside a workspace. Populating it before the workspace-root is resolved would put a non-existent path on `PATH`, so it should only be derived once workspace support lands. Do not flag this as a bug or missing derivation in reviews.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-21T00:33:05.035Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11789
File: pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs:145-146
Timestamp: 2026-05-21T00:33:05.035Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/resolve_from_workspace.rs`, the guard `bare.starts_with("workspace:.")` is intentionally broad — matching pnpm upstream's identical `startsWith('workspace:.')` check. All dot-prefixed workspace forms including `workspace:.foo` are intentionally handed off to the local-resolver, which handles them as `link:`-style directory specs via its prefix-stripping regex. Do not suggest narrowing this to `workspace:./` or `workspace:../` only.

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/*.rs : 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

Applied to files:

  • pacquet/crates/hooks/Cargo.toml
📚 Learning: 2026-05-23T09:14:43.635Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not run lifecycle scripts (`BuildModules` is not invoked, and `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput` is discarded). This is pre-existing behavior documented by an in-code comment mirroring upstream `link.ts:167-170`. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Wiring lifecycle scripts into the fresh-lockfile path is tracked as future work, separate from this file's concern.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-23T09:14:43.635Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11867
File: pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs:726-730
Timestamp: 2026-05-23T09:14:43.635Z
Learning: In `pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`, the fresh-lockfile path intentionally does not invoke `BuildModules` and discards `side_effects_maps_by_snapshot` from `CreateVirtualStoreOutput`. This is pre-existing, documented behavior (mirroring upstream `link.ts:167-170`): `importing_done` fires once extraction and symlink linking are complete, and the fresh-lockfile path does not run lifecycle scripts. The frozen-lockfile path wires `BuildModules` end-to-end as normal. Do not flag this omission as a bug; wiring lifecycle scripts into the fresh-lockfile path is tracked as future work separate from perf refactors.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T20:41:50.322Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11773
File: pacquet/crates/package-manager/src/install_package_from_registry.rs:111-114
Timestamp: 2026-05-20T20:41:50.322Z
Learning: In pacquet (pnpm/pnpm repo, Rust codebase), `install_package_from_registry` is the npm-only install path. The npm resolver always stamps `ResolveResult.id` (a `PkgResolutionId`) as `nameversion`. Parsing it back through `PkgNameVer` with `.expect()` is intentional — a parse failure means a mis-dispatch bug, not malformed external input. Per pacquet's CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." Do not suggest replacing such `.expect()` calls with graceful error handling.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:08:06.093Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs`, the `else` branch that calls `max_satisfying_any` is intentional and matches upstream pnpm's `hoistPeers.ts` (lines 45-59). When `is_exact_version && !auto_install_peers`, pnpm still runs `semver.maxSatisfying(versions, '*', { includePrerelease: true })` and picks the closest available preferred version; any version mismatch is reported downstream as a `bad` peer issue rather than dropping the edge. Do NOT suggest changing this branch to skip `max_satisfying_any` for exact ranges — it would diverge from pnpm.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T10:06:55.749Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs:15-18
Timestamp: 2026-05-20T10:06:55.749Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`, the `DirectDep.id`, `ResolvedPackage.id`, and `ResolvedTree.packages` HashMap keys are intentionally plain `String` for now. The natural branded type would be `pacquet_lockfile::PkgNameVer`, but it cannot be used as a HashMap key because `node_semver::Version` does not derive `Hash`. The upstream parity type is `PkgResolutionId` (which carries an optional peer-dep suffix), and the branded type should be introduced alongside peer-dep resolution and lockfile generation work to avoid locking the seam too early.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:07:40.413Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs:167-191
Timestamp: 2026-05-20T23:07:40.413Z
Learning: In `pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs`, the offline guard in `resolve_impl` (NodeResolver) is intentionally absent from `resolve_latest_impl`. This mirrors upstream pnpm's behavior: `resolveNodeRuntime` checks offline, but `resolveLatestNodeRuntime` does not carry `offline` in its context and never checks it. The asymmetry is required so `pnpm outdated` / `pnpm update --latest` can still query for newer runtime versions even in offline mode.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.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/install_with_fresh_lockfile.rs
  • pacquet/crates/hooks/src/lib.rs
📚 Learning: 2026-05-20T01:42:44.958Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:370-380
Timestamp: 2026-05-20T01:42:44.958Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package.rs`, the fetch-error fallback that returns `Ok(PickPackageResult { meta: disk, picked_package: picked })` — even when `picked` is `None` — is intentional upstream parity with pnpm's `pickPackage.ts` catch block (https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431). When a fetch fails and a disk mirror exists, the stale-mirror pick (including null/None) is returned verbatim; the transport error is logged via `tracing::debug!`. Do not flag this as a bug.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-24T16:07:54.784Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11904
File: pacquet/crates/package-manager/src/install.rs:556-560
Timestamp: 2026-05-24T16:07:54.784Z
Learning: In pacquet's `is_modules_yaml_consistent` (pacquet/crates/package-manager/src/install.rs), `enableGlobalVirtualStore` is intentionally NOT checked as a separate field. Upstream pnpm's `validateModules.ts` does not persist or check `enableGlobalVirtualStore` in `.modules.yaml` either. Drift on this setting is caught indirectly: toggling `enableGlobalVirtualStore` changes `config.effective_virtual_store_dir()` (GVS-on → `<store>/v11/links`, GVS-off → `<project>/node_modules/.pnpm`), so the existing `modules.virtual_store_dir == config.effective_virtual_store_dir()` comparison in `is_modules_yaml_consistent` already detects the mismatch and prevents the short-circuit. Do not flag the absence of an explicit `enableGlobalVirtualStore` field as a bug.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/*.rs : User-facing errors go through `miette` via the `pacquet-diagnostics` crate; match pnpm's error codes and messages where pnpm defines them since error codes are part of the public contract

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
📚 Learning: 2026-05-20T23:07:43.668Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs:70-88
Timestamp: 2026-05-20T23:07:43.668Z
Learning: In `pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`, `resolve_impl` intentionally passes `ResolveOptions::default()` (not the caller-provided `opts`) when delegating to the npm resolver. This mirrors the upstream TypeScript code at `engine/runtime/bun-resolver/src/index.ts#L46` which passes an empty `{}` literal; forwarding outer opts would diverge from pnpm's behavior.

Applied to files:

  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/hooks/src/lib.rs
📚 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/install_with_fresh_lockfile.rs
  • pacquet/crates/hooks/src/lib.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/install_with_fresh_lockfile.rs
  • pacquet/crates/hooks/src/lib.rs

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pacquet/crates/hooks/src/custom_resolver_adapter.rs (1)

28-33: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cache key is too coarse for canResolve semantics.

At Line 28, the cache key only uses alias + bareSpecifier, but can_resolve is called with additional fields (injected, optional, prevSpecifier). Reusing cached results across different values of those fields can incorrectly skip or force the custom resolver.

Suggested fix
-    fn cache_key(wanted: &WantedDependency) -> String {
-        format!(
-            "{}@{}",
-            wanted.alias.as_deref().unwrap_or(""),
-            wanted.bare_specifier.as_deref().unwrap_or(""),
-        )
-    }
+    fn cache_key(wanted: &WantedDependency) -> String {
+        serde_json::to_string(&Self::wanted_to_value(wanted))
+            .unwrap_or_else(|_| String::new())
+    }

Based on learnings: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests.

Also applies to: 86-103

🤖 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/hooks/src/custom_resolver_adapter.rs` around lines 28 - 33,
The cache_key function currently only uses wanted.alias and
wanted.bare_specifier which is too coarse for can_resolve semantics; update
cache_key (and any other cache key uses around the can_resolve logic at the same
spot referenced by lines ~86-103) to incorporate the additional parameters that
affect resolution—specifically include wanted.injected, wanted.optional and the
prev_specifier (prevSpecifier) passed into can_resolve—ensuring a deterministic
string representation (e.g., boolean and Option values serialized consistently)
so cached results aren’t incorrectly reused across different
injected/optional/prevSpecifier states; update the cache key generator used by
the can_resolve path (function cache_key and its callers) accordingly.
🤖 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.

Outside diff comments:
In `@pacquet/crates/hooks/src/custom_resolver_adapter.rs`:
- Around line 28-33: The cache_key function currently only uses wanted.alias and
wanted.bare_specifier which is too coarse for can_resolve semantics; update
cache_key (and any other cache key uses around the can_resolve logic at the same
spot referenced by lines ~86-103) to incorporate the additional parameters that
affect resolution—specifically include wanted.injected, wanted.optional and the
prev_specifier (prevSpecifier) passed into can_resolve—ensuring a deterministic
string representation (e.g., boolean and Option values serialized consistently)
so cached results aren’t incorrectly reused across different
injected/optional/prevSpecifier states; update the cache key generator used by
the can_resolve path (function cache_key and its callers) accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 590b53e4-0860-41d1-878d-b019ea4fc6cc

📥 Commits

Reviewing files that changed from the base of the PR and between 8c83151 and 2f9fcfd.

📒 Files selected for processing (3)
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
  • pacquet/crates/resolving-resolver-base/src/lib.rs
  • pacquet/crates/resolving-resolver-base/src/resolve.rs
✅ Files skipped from review due to trivial changes (1)
  • pacquet/crates/resolving-resolver-base/src/lib.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that 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-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
🧠 Learnings (23)
📓 Common learnings
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/tests/**/*.rs : Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
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:30.632Z
Learning: In the pacquet Rust port of pnpm, the `is_http_url` helper in `pacquet/crates/resolving-tarball-resolver/src/tarball_resolver.rs` intentionally uses `bare.starts_with("http:") || bare.starts_with("https:")` (not `"http://"` / `"https://"`) to match upstream pnpm's `startsWith('http:')` / `startsWith('https:')` check byte-for-byte. Pacquet's cardinal rule (pacquet/AGENTS.md) requires matching pnpm even on quirks; malformed non-URL inputs are rejected downstream by `reqwest::Url::parse` as a `ResolveError`.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Reference the upstream pnpm commit/PR when porting code from pnpm in commit messages
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs`, the `else` branch that calls `max_satisfying_any` is intentional and matches upstream pnpm's `hoistPeers.ts` (lines 45-59). When `is_exact_version && !auto_install_peers`, pnpm still runs `semver.maxSatisfying(versions, '*', { includePrerelease: true })` and picks the closest available preferred version; any version mismatch is reported downstream as a `bad` peer issue rather than dropping the edge. Do NOT suggest changing this branch to skip `max_satisfying_any` for exact ranges — it would diverge from pnpm.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
📚 Learning: 2026-05-20T10:06:55.749Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs:15-18
Timestamp: 2026-05-20T10:06:55.749Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`, the `DirectDep.id`, `ResolvedPackage.id`, and `ResolvedTree.packages` HashMap keys are intentionally plain `String` for now. The natural branded type would be `pacquet_lockfile::PkgNameVer`, but it cannot be used as a HashMap key because `node_semver::Version` does not derive `Hash`. The upstream parity type is `PkgResolutionId` (which carries an optional peer-dep suffix), and the branded type should be introduced alongside peer-dep resolution and lockfile generation work to avoid locking the seam too early.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/Cargo.toml : Keep dependencies at the right level — add a new dependency to the specific crate that needs it, not to the workspace root or shared crate unless multiple crates depend on it

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
📚 Learning: 2026-06-02T13:18:26.437Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12134
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:311-325
Timestamp: 2026-06-02T13:18:26.437Z
Learning: In pacquet's lockfile resolution verifier (`pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`), URL-keyed tarball dependencies do NOT need a separate `non_semver_version` field in `VerifyCtx`. Unlike the TypeScript side (which derives `version` from `snapshot.version` and threads `nonSemverVersion` separately), pacquet's `collect_candidates` takes `version` from the lockfile key suffix. For a URL-keyed dep the key is `name@<url>`, so `ctx.version` is the URL string, which fails `node_semver::Version::parse(ctx.version)` and the existing guard `if node_semver::Version::parse(ctx.version).is_err() { return ResolutionVerification::Ok; }` already skips the registry lookup correctly. Adding a `non_semver_version` field to `VerifyCtx` for this purpose would be inert.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-23T17:30:06.849Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T23:07:40.413Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs:167-191
Timestamp: 2026-05-20T23:07:40.413Z
Learning: In `pacquet/crates/engine-runtime-node-resolver/src/node_resolver.rs`, the offline guard in `resolve_impl` (NodeResolver) is intentionally absent from `resolve_latest_impl`. This mirrors upstream pnpm's behavior: `resolveNodeRuntime` checks offline, but `resolveLatestNodeRuntime` does not carry `offline` in its context and never checks it. The asymmetry is required so `pnpm outdated` / `pnpm update --latest` can still query for newer runtime versions even in offline mode.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
📚 Learning: 2026-05-29T18:03:24.760Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pnpr/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:24.760Z
Learning: Prefer existing pacquet-* crates over writing new code; check pacquet-tarball, pacquet-crypto-hash, pacquet-crypto-shasums-file, pacquet-package-manifest, pacquet-network, pacquet-registry, pacquet-fs, and pacquet-diagnostics before implementing non-trivial functionality

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
📚 Learning: 2026-05-20T23:07:43.668Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs:70-88
Timestamp: 2026-05-20T23:07:43.668Z
Learning: In `pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`, `resolve_impl` intentionally passes `ResolveOptions::default()` (not the caller-provided `opts`) when delegating to the npm resolver. This mirrors the upstream TypeScript code at `engine/runtime/bun-resolver/src/index.ts#L46` which passes an empty `{}` literal; forwarding outer opts would diverge from pnpm's behavior.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T23:07:40.480Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs:79-80
Timestamp: 2026-05-20T23:07:40.480Z
Learning: In `pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs` (and the analogous Bun resolver), passing `ResolveOptions::default()` when delegating version selection to the inner npm resolver is intentional. It mirrors the upstream TypeScript code in `engine/runtime/deno-resolver/src/index.ts#L56` which explicitly passes `{}` (an empty/default options literal). Forwarding the outer `ResolveOptions` would diverge from pnpm upstream behavior and must NOT be flagged as a bug.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T20:41:50.322Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11773
File: pacquet/crates/package-manager/src/install_package_from_registry.rs:111-114
Timestamp: 2026-05-20T20:41:50.322Z
Learning: In pacquet (pnpm/pnpm repo, Rust codebase), `install_package_from_registry` is the npm-only install path. The npm resolver always stamps `ResolveResult.id` (a `PkgResolutionId`) as `nameversion`. Parsing it back through `PkgNameVer` with `.expect()` is intentional — a parse failure means a mis-dispatch bug, not malformed external input. Per pacquet's CLAUDE.md: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." Do not suggest replacing such `.expect()` calls with graceful error handling.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
📚 Learning: 2026-05-20T01:42:44.958Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:370-380
Timestamp: 2026-05-20T01:42:44.958Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package.rs`, the fetch-error fallback that returns `Ok(PickPackageResult { meta: disk, picked_package: picked })` — even when `picked` is `None` — is intentional upstream parity with pnpm's `pickPackage.ts` catch block (https://github.com/pnpm/pnpm/blob/f657b5cb44/resolving/npm-resolver/src/pickPackage.ts#L420-L431). When a fetch fails and a disk mirror exists, the stale-mirror pick (including null/None) is returned verbatim; the transport error is logged via `tracing::debug!`. Do not flag this as a bug.

Applied to files:

  • pacquet/crates/resolving-resolver-base/src/resolve.rs
📚 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-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.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-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.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-resolver-base/src/resolve.rs
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T10:51:16.296Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-npm-resolver/src/named_registry.rs:109-127
Timestamp: 2026-05-20T10:51:16.296Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/named_registry.rs`, the `pick_registry_for_package` function's logic `scope_from_bare_specifier(bare_specifier).or_else(|| scope_of(pkg_name))` is intentionally correct and matches upstream pnpm's `getScope` in `config/pick-registry-for-package/src/index.ts`. When bare_specifier is an npm: alias for an unscoped package (e.g., `npm:lodash@^1`), `scope_from_bare_specifier` returns `None`, and the fallthrough to `scope_of(pkg_name)` is correct behavior — allowing a scoped local name like `private/foo` to still route through the `private` registry. Do NOT suggest changing this to an early-return on `npm:` prefix, as that would break pnpm compatibility.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T21:18:55.266Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11778
File: pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs:253-278
Timestamp: 2026-05-20T21:18:55.266Z
Learning: In `pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs`, the `resolve_path` function intentionally short-circuits absolute specifiers verbatim (returns them unchanged without normalizing `..` components), mirroring the upstream TypeScript `resolvePath` in `resolving/local-resolver/src/parseBareSpecifier.ts` at ef87f3ccff. The OS resolves `..` at `fs.read` time. Do not suggest normalizing the absolute branch — it would invent behavior pnpm doesn't have, violating the pacquet AGENTS.md cardinal rule of fidelity to upstream.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-25T12:36:42.202Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T23:08:06.093Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs`, the `else` branch that calls `max_satisfying_any` is intentional and matches upstream pnpm's `hoistPeers.ts` (lines 45-59). When `is_exact_version && !auto_install_peers`, pnpm still runs `semver.maxSatisfying(versions, '*', { includePrerelease: true })` and picks the closest available preferred version; any version mismatch is reported downstream as a `bad` peer issue rather than dropping the edge. Do NOT suggest changing this branch to skip `max_satisfying_any` for exact ranges — it would diverge from pnpm.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T01:52:55.764Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs:193-200
Timestamp: 2026-05-20T01:52:55.764Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs`, the package-level `modified` shortcut for the maturity/minimumReleaseAge filter uses inclusive `<=` (not strict `<`) when comparing `modified_date <= cutoff`. This mirrors the corrected behavior in pnpm (fixed in ab4c96ead5). The reasoning: `modified` is "last modification time," which is an upper bound on every version's `time[v]`. The per-version maturity filter uses `<=` (a version published exactly at the cutoff is mature). Since `modified == cutoff` means every version satisfies the per-version filter, the abbreviated-metadata fast path should accept this case rather than forcing a full-metadata re-fetch or raising `MissingTime`. The same fix was applied to pnpm TS: `pickPackage.ts` (×2) and `pickPackageFromMeta.ts`.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
🔇 Additional comments (2)
pacquet/crates/resolving-resolver-base/src/resolve.rs (1)

199-200: ⚡ Quick win

Ensure CurrentPkg.peer_dependencies serializes as peerDependencies

The payload consumed by pnpm/custom-resolver hooks expects the camelCase JSON key peerDependencies; CurrentPkg currently has peer_dependencies with only skip_serializing_if visible. Confirm the struct/field serde settings (e.g. rename_all/rename) or add #[serde(rename = "peerDependencies")] so resolver hooks receive peer deps under the expected key.

pacquet/crates/hooks/src/custom_resolver_adapter.rs (1)

36-44: LGTM!

Also applies to: 67-73, 117-152

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pacquet/crates/hooks/src/custom_resolver_adapter.rs (1)

30-35: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cache key is too narrow for can_resolve memoization.

Line 30 only keys by alias@bareSpecifier, but can_resolve is called with the full wanted payload (optional, injected, prevSpecifier too). Different inputs can collide and reuse the wrong decision.

Suggested fix
-    fn cache_key(wanted: &WantedDependency) -> String {
-        format!(
-            "{}@{}",
-            wanted.alias.as_deref().unwrap_or(""),
-            wanted.bare_specifier.as_deref().unwrap_or(""),
-        )
-    }
+    fn cache_key(wanted: &WantedDependency) -> String {
+        Self::wanted_to_value(wanted).to_string()
+    }

Based on learnings: User-visible changes (including hook semantics) in pnpm must be mirrored to pacquet in the same PR.

🤖 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/hooks/src/custom_resolver_adapter.rs` around lines 30 - 35,
The cache_key used for can_resolve memoization is too narrow (only
alias@bare_specifier) and can collide for different WantedDependency flags;
update the cache_key function to include the other distinguishing fields (e.g.,
wanted.optional, wanted.injected, and wanted.prev_specifier) when building the
key so memoization considers those inputs too; locate the
cache_key(&WantedDependency) function and extend its formatted string to
incorporate these fields (using as_deref() or equivalent for Option<String>
values) so can_resolve decisions aren’t erroneously reused across different
wanted payloads.
🧹 Nitpick comments (1)
pacquet/crates/hooks/src/custom_resolver_adapter.rs (1)

79-155: 🏗️ Heavy lift

Add parity-focused tests for adapter contract and memoization behavior.

This file is contract-heavy (JSON shape + validation + cache behavior) and currently has 0% patch coverage. Please add Rust tests for at least: missing id, missing resolution, invalid resolution shape, and cache behavior with same alias/specifier but different optional/injected/prevSpecifier.

Based on learnings: Applies to pacquet/**/tests/**/*.rs — port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm.

🤖 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/hooks/src/custom_resolver_adapter.rs` around lines 79 - 155,
Add unit tests for CustomResolverAdapter::resolve covering: 1) resolver returns
a map missing "id" — assert it errors with InvalidData; 2) resolver returns a
map missing "resolution" — assert InvalidData; 3) resolver returns a
"resolution" with invalid JSON shape — assert the parse error is propagated; and
4) memoization: verify can_resolve_cache behavior by calling resolve twice with
the same alias/specifier but differing WantedDependency flags
(optional/injected/prev_specifier) and ensure can_resolve is recalculated when
those distinguishing fields differ and reused when identical. Implement tests by
mocking the inner resolver that CustomResolverAdapter.resolver awaits (returning
serde_json::Value maps), constructing WantedDependency instances via the same
fields used by Self::wanted_to_value and ResolveOptions via Self::opts_to_value,
invoking resolve, and asserting results/errors and can_resolve_cache contents
(or call counts) in pacquet/**/tests/**/*.rs.
🤖 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.

Outside diff comments:
In `@pacquet/crates/hooks/src/custom_resolver_adapter.rs`:
- Around line 30-35: The cache_key used for can_resolve memoization is too
narrow (only alias@bare_specifier) and can collide for different
WantedDependency flags; update the cache_key function to include the other
distinguishing fields (e.g., wanted.optional, wanted.injected, and
wanted.prev_specifier) when building the key so memoization considers those
inputs too; locate the cache_key(&WantedDependency) function and extend its
formatted string to incorporate these fields (using as_deref() or equivalent for
Option<String> values) so can_resolve decisions aren’t erroneously reused across
different wanted payloads.

---

Nitpick comments:
In `@pacquet/crates/hooks/src/custom_resolver_adapter.rs`:
- Around line 79-155: Add unit tests for CustomResolverAdapter::resolve
covering: 1) resolver returns a map missing "id" — assert it errors with
InvalidData; 2) resolver returns a map missing "resolution" — assert
InvalidData; 3) resolver returns a "resolution" with invalid JSON shape — assert
the parse error is propagated; and 4) memoization: verify can_resolve_cache
behavior by calling resolve twice with the same alias/specifier but differing
WantedDependency flags (optional/injected/prev_specifier) and ensure can_resolve
is recalculated when those distinguishing fields differ and reused when
identical. Implement tests by mocking the inner resolver that
CustomResolverAdapter.resolver awaits (returning serde_json::Value maps),
constructing WantedDependency instances via the same fields used by
Self::wanted_to_value and ResolveOptions via Self::opts_to_value, invoking
resolve, and asserting results/errors and can_resolve_cache contents (or call
counts) in pacquet/**/tests/**/*.rs.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 07711283-8cf7-4c4a-a3af-fdb727a52d79

📥 Commits

Reviewing files that changed from the base of the PR and between 2f9fcfd and 3014dc9.

📒 Files selected for processing (1)
  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that 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/hooks/src/custom_resolver_adapter.rs
🧠 Learnings (18)
📓 Common learnings
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: Pacquet (pnpm's Rust port) has a cardinal rule: "match pnpm exactly — do not fix pnpm quirks unless the same fix has landed in pnpm first." Review comments should not suggest behavioral deviations from upstream pnpm, even when the upstream behavior appears buggy. If a real bug is identified, it must be fixed upstream first.
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Applies to pacquet/**/tests/**/*.rs : Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).
📚 Learning: 2026-05-20T10:06:55.749Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs:15-18
Timestamp: 2026-05-20T10:06:55.749Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs`, the `DirectDep.id`, `ResolvedPackage.id`, and `ResolvedTree.packages` HashMap keys are intentionally plain `String` for now. The natural branded type would be `pacquet_lockfile::PkgNameVer`, but it cannot be used as a HashMap key because `node_semver::Version` does not derive `Hash`. The upstream parity type is `PkgResolutionId` (which carries an optional peer-dep suffix), and the branded type should be introduced alongside peer-dep resolution and lockfile generation work to avoid locking the seam too early.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-25T14:58:11.105Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11931
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:560-589
Timestamp: 2026-05-25T14:58:11.105Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`, all per-`(registry, name[, version])` caches in `NpmResolutionVerifier` (`published_at`, `full_meta`, `full_meta_for_trust`, `abbreviated_meta`, `local_meta`) intentionally use the same pattern: lock → miss-check → release lock → await fetch/load → re-acquire lock → insert. This uniform pattern is deliberate; do not flag individual caches for using it. The known follow-up improvement (replacing the pattern with `tokio::sync::OnceCell` per key inside a `Mutex<HashMap<…>>`) is tracked as a future structural change to cover all five caches simultaneously.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T10:51:16.296Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11760
File: pacquet/crates/resolving-npm-resolver/src/named_registry.rs:109-127
Timestamp: 2026-05-20T10:51:16.296Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/named_registry.rs`, the `pick_registry_for_package` function's logic `scope_from_bare_specifier(bare_specifier).or_else(|| scope_of(pkg_name))` is intentionally correct and matches upstream pnpm's `getScope` in `config/pick-registry-for-package/src/index.ts`. When bare_specifier is an npm: alias for an unscoped package (e.g., `npm:lodash@^1`), `scope_from_bare_specifier` returns `None`, and the fallthrough to `scope_of(pkg_name)` is correct behavior — allowing a scoped local name like `private/foo` to still route through the `private` registry. Do NOT suggest changing this to an early-return on `npm:` prefix, as that would break pnpm compatibility.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-23T17:30:06.849Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11878
File: resolving/npm-resolver/src/createNpmResolutionVerifier.ts:381-418
Timestamp: 2026-05-23T17:30:06.849Z
Learning: In `resolving/npm-resolver/src/pickPackage.ts` (pnpm/pnpm), the resolver's `PackageMetaCache` keys by `name` (abbreviated) and `name:full` (full metadata) only — no registry component is included. This is a pre-existing limitation meaning that if two different registries serve packages of the same name in one install, the cache will only hold the first fetched entry. The `createNpmResolutionVerifier.ts` shares this same cache and inherits the limitation; a `validateSharedMeta` name-check guards against cross-package contamination but cannot distinguish same-named packages from different registries. Tightening to a registry-qualified key would require a coordinated change to the resolver's cache key shape. The Pacquet/Rust side is already registry-qualified (`{registry}\x00{name}:full`).

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-29T18:03:15.354Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: pacquet/AGENTS.md:0-0
Timestamp: 2026-05-29T18:03:15.354Z
Learning: Match how the same feature is implemented in the TypeScript pnpm CLI — any change in pacquet must match pnpm's behavior, logic, edge cases, config resolution, error messages, file/lockfile formats, and existing tests

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-06-02T13:18:26.437Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12134
File: pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs:311-325
Timestamp: 2026-06-02T13:18:26.437Z
Learning: In pacquet's lockfile resolution verifier (`pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs`), URL-keyed tarball dependencies do NOT need a separate `non_semver_version` field in `VerifyCtx`. Unlike the TypeScript side (which derives `version` from `snapshot.version` and threads `nonSemverVersion` separately), pacquet's `collect_candidates` takes `version` from the lockfile key suffix. For a URL-keyed dep the key is `name@<url>`, so `ctx.version` is the URL string, which fails `node_semver::Version::parse(ctx.version)` and the existing guard `if node_semver::Version::parse(ctx.version).is_err() { return ResolutionVerification::Ok; }` already skips the registry lookup correctly. Adding a `non_semver_version` field to `VerifyCtx` for this purpose would be inert.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In the pacquet Rust port (pnpm/pnpm repo), the `ResolvedPackage.optional` AND-folding on revisit intentionally mirrors pnpm's `resolveDependencies.ts:1627-1648` behavior: only the directly-revisited package's `optional` flag is updated; transitive descendants are not re-walked. pnpm CLI corrects stale optional flags downstream via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`, which tracks a `nonOptional` set and re-stamps any package reachable by an all-non-optional path. Pacquet does not yet have this pruner equivalent, so the stale flags flow directly through `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up to port `copyDependencySubGraph` is planned; until then, do not flag the resolver-layer optional propagation gap as a bug in pacquet PRs — it is intentional parity with pnpm's resolver layer.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T21:18:55.266Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11778
File: pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs:253-278
Timestamp: 2026-05-20T21:18:55.266Z
Learning: In `pacquet/crates/resolving-local-resolver/src/parse_bare_specifier.rs`, the `resolve_path` function intentionally short-circuits absolute specifiers verbatim (returns them unchanged without normalizing `..` components), mirroring the upstream TypeScript `resolvePath` in `resolving/local-resolver/src/parseBareSpecifier.ts` at ef87f3ccff. The OS resolves `..` at `fs.read` time. Do not suggest normalizing the absolute branch — it would invent behavior pnpm doesn't have, violating the pacquet AGENTS.md cardinal rule of fidelity to upstream.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-25T12:36:42.202Z
Learnt from: CR
Repo: pnpm/pnpm PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-05-25T12:36:42.202Z
Learning: User-visible changes (CLI flags, defaults, environment variables, lockfile/manifest/state-file formats, error codes/messages, log emissions, store layout, hook semantics) in pnpm must be mirrored to pacquet in the same PR

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T23:08:06.093Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:08:06.093Z
Learning: In `pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs`, the `else` branch that calls `max_satisfying_any` is intentional and matches upstream pnpm's `hoistPeers.ts` (lines 45-59). When `is_exact_version && !auto_install_peers`, pnpm still runs `semver.maxSatisfying(versions, '*', { includePrerelease: true })` and picks the closest available preferred version; any version mismatch is reported downstream as a `bad` peer issue rather than dropping the edge. Do NOT suggest changing this branch to skip `max_satisfying_any` for exact ranges — it would diverge from pnpm.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.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/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T01:52:55.764Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11755
File: pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs:193-200
Timestamp: 2026-05-20T01:52:55.764Z
Learning: In `pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta.rs`, the package-level `modified` shortcut for the maturity/minimumReleaseAge filter uses inclusive `<=` (not strict `<`) when comparing `modified_date <= cutoff`. This mirrors the corrected behavior in pnpm (fixed in ab4c96ead5). The reasoning: `modified` is "last modification time," which is an upper bound on every version's `time[v]`. The per-version maturity filter uses `<=` (a version published exactly at the cutoff is mature). Since `modified == cutoff` means every version satisfies the per-version filter, the abbreviated-metadata fast path should accept this case rather than forcing a full-metadata re-fetch or raising `MissingTime`. The same fix was applied to pnpm TS: `pickPackage.ts` (×2) and `pickPackageFromMeta.ts`.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T23:07:40.480Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs:79-80
Timestamp: 2026-05-20T23:07:40.480Z
Learning: In `pacquet/crates/engine-runtime-deno-resolver/src/deno_resolver.rs` (and the analogous Bun resolver), passing `ResolveOptions::default()` when delegating version selection to the inner npm resolver is intentional. It mirrors the upstream TypeScript code in `engine/runtime/deno-resolver/src/index.ts#L56` which explicitly passes `{}` (an empty/default options literal). Forwarding the outer `ResolveOptions` would diverge from pnpm upstream behavior and must NOT be flagged as a bug.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-20T23:07:43.668Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11783
File: pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs:70-88
Timestamp: 2026-05-20T23:07:43.668Z
Learning: In `pacquet/crates/engine-runtime-bun-resolver/src/bun_resolver.rs`, `resolve_impl` intentionally passes `ResolveOptions::default()` (not the caller-provided `opts`) when delegating to the npm resolver. This mirrors the upstream TypeScript code at `engine/runtime/bun-resolver/src/index.ts#L46` which passes an empty `{}` literal; forwarding outer opts would diverge from pnpm's behavior.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 Learning: 2026-05-24T21:11:04.272Z
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:11:04.272Z
Learning: In pacquet (pnpm/pnpm repo), `ResolvedPackage.optional` AND-folding intentionally mirrors pnpm's resolveDependencies.ts:1627-1648 revisit behavior: only the directly-visited package's `optional` flag is updated on revisit, not transitive descendants. pnpm CLI corrects stale optional flags via `copyDependencySubGraph` BFS in `lockfile/pruner/src/index.ts:160-205`. Pacquet does not yet have this pruner equivalent, so raw `node.optional` flows directly into snapshot/virtual-store via `dependencies_graph_to_lockfile.rs:409` → `create_virtual_store.rs:762` → `installability.rs:394`. A follow-up issue to port `copyDependencySubGraph` is planned.

Applied to files:

  • pacquet/crates/hooks/src/custom_resolver_adapter.rs
📚 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/hooks/src/custom_resolver_adapter.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/hooks/src/custom_resolver_adapter.rs

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12153
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,206.27 ms
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
693.52 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
695.29 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
2,190.93 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
731.93 ms
🐰 View full continuous benchmarking report in Bencher

@kairosci

kairosci commented Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

cc: @zkochan

kairosci added 3 commits June 12, 2026 13:09
Add CurrentPkg struct to resolver-base (mirrors upstream's
{ name, version, peerDependencies }) and thread it through
ResolveOptions so opts_to_value properly serializes the
currentPkg field for custom resolver hooks.
…re_param rules

- Collapse std imports into one use tree
- Rename single-letter |e| closure params to |err|
Copilot AI review requested due to automatic review settings June 12, 2026 11:11
@zkochan zkochan force-pushed the feat/custom-resolvers branch from 3014dc9 to b7bedd8 Compare June 12, 2026 11:11

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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 (3) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0)

Grey Divider


Action required

1. Canceled IPC leaks pending ✓ Resolved 🐞 Bug ☼ Reliability
Description
NodeWorker::request inserts a Pending entry but only removes it on send failure or when the
timeout branch executes; if the request future is dropped, no cleanup runs.
check_custom_resolver_force_resolve returns early on the first true and drops the remaining
futures, so any hook call that never responds can leave stale pending entries until worker exit.
Code

pacquet/crates/hooks/src/worker.rs[R181-199]

     self.pending.lock().await.insert(id, Pending { log, done });
     body["id"] = serde_json::json!(id);
-        let mut line =
-            serde_json::to_string(&body).map_err(|err| self.exec_err(err.to_string()))?;
-        line.push('\n');
-        {
+        let send_result = async {
+            let mut line = serde_json::to_string(&body).map_err(|err| err.to_string())?;
+            line.push('\n');
         let mut stdin = self.stdin.lock().await;
-            stdin.write_all(line.as_bytes()).await.map_err(|err| self.exec_err(err.to_string()))?;
-            stdin.flush().await.map_err(|err| self.exec_err(err.to_string()))?;
+            stdin.write_all(line.as_bytes()).await.map_err(|err| err.to_string())?;
+            stdin.flush().await.map_err(|err| err.to_string())
+        }
+        .await;
+        if let Err(message) = send_result {
+            // The reader task can never answer a request that was not
+            // sent, so drop the entry here or it leaks until shutdown.
+            self.pending.lock().await.remove(&id);
+            return Err(self.exec_err(message));
     }
     match timeout(HOOK_TIMEOUT, rx).await {
Evidence
The worker request path inserts into pending and only removes on send error or timeout, with no
cancellation cleanup; the force-resolve check intentionally drops outstanding futures on first
true, which can drop in-flight call_resolver requests and bypass the timeout-based removal.

pacquet/crates/hooks/src/worker.rs[178-207]
pacquet/crates/package-manager/src/check_custom_resolver_force_resolve.rs[48-67]
pacquet/crates/hooks/src/node_runtime.rs[248-263]

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

## Issue description
`NodeWorker::request()` registers a pending request before writing to stdin, but cleanup only happens on explicit error paths (send failure / timeout). If callers drop the future (cancellation), the pending entry can remain indefinitely when the Node worker never replies.
This becomes reachable because `check_custom_resolver_force_resolve()` returns early on the first `true` and drops the remaining in-flight `shouldRefreshResolution` futures.
### Issue Context
- Cancellation can happen during `check_custom_resolver_force_resolve()` early-return.
- Without cancellation cleanup, the 30s timeout guard is bypassed (because it is implemented inside the dropped future).
### Fix Focus Areas
- pacquet/crates/hooks/src/worker.rs[178-207]
- pacquet/crates/package-manager/src/check_custom_resolver_force_resolve.rs[48-66]
### Suggested fix approach
1. Add cancellation-safe cleanup for pending requests. Options:
- Implement a small RAII guard that schedules `pending.remove(id)` on drop via `tokio::spawn(async move { pending.lock().await.remove(&id); })` (since `tokio::sync::Mutex` cannot be locked in a synchronous `Drop`).
- Or refactor `request()` to use `tokio::select!` with a cancellation signal and ensure `pending.remove(id)` happens in a `finally`-style block.
2. Ensure cleanup is idempotent (it’s OK if `dispatch_line()` already removed the entry).

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


2. CurrentPkg payload incompatible ✓ Resolved 🐞 Bug ≡ Correctness
Description
ResolveOptions.current_pkg is serialized into custom resolver opts as currentPkg, but pacquet’s
CurrentPkg struct omits required pnpm fields (id, resolution) and uses non-pnpm field names
(e.g., peer_dependencies), breaking pnpmfile resolvers that expect pnpm’s
ResolveOptions.currentPkg shape.
Code

pacquet/crates/resolving-resolver-base/src/resolve.rs[R193-203]

+/// Previously-resolved entry from the lockfile, threaded so resolvers
+/// can short-circuit when the install is not requesting an update. Mirrors
+/// upstream's
+/// [`CurrentPkg`](https://github.com/pnpm/pnpm/blob/3687b0e180/resolving/resolver-base/src/index.ts#L274-L275).
+#[derive(Debug, Default, Clone, Serialize)]
+pub struct CurrentPkg {
+    pub name: String,
+    pub version: String,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub peer_dependencies: Option<BTreeMap<String, String>>,
+}
Evidence
Pacquet defines CurrentPkg as { name, version, peer_dependencies } and then serializes it under
currentPkg, while pnpm’s resolver-base contract requires id and resolution inside currentPkg
(camelCase).

pacquet/crates/resolving-resolver-base/src/resolve.rs[193-213]
pacquet/crates/hooks/src/custom_resolver_adapter.rs[69-76]
resolving/resolver-base/src/index.ts[296-310]

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

## Issue description
`ResolveOptions.current_pkg` is exposed to custom resolvers via `CustomResolverAdapter::opts_to_value` as `currentPkg`, but the Rust `CurrentPkg` struct does not match pnpm’s resolver-base contract (missing `id` and `resolution`, wrong naming/serialization).
### Issue Context
In pnpm resolver-base, `currentPkg` includes at least `{ id, resolution, name?, version?, publishedAt? }` (camelCase). Custom resolvers will likely read these fields.
### Fix Focus Areas
- pacquet/crates/resolving-resolver-base/src/resolve.rs[193-226]
- pacquet/crates/hooks/src/custom_resolver_adapter.rs[69-76]
### What to change
- Redefine `CurrentPkg` to mirror pnpm’s `ResolveOptions.currentPkg`:
- include `id: PkgResolutionId`
- include `resolution: pacquet_lockfile::LockfileResolution`
- optional `name`, `version`, optional `published_at` (serialized as `publishedAt`)
- add `#[serde(rename_all = "camelCase")]` and `skip_serializing_if` on optional fields.
- Remove/avoid non-pnpm fields like `peer_dependencies` in this payload (unless you also extend the Node/TS side to expect it).

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


3. Null resolve aborts install ✓ Resolved 🐞 Bug ≡ Correctness
Description
If a pnpmfile resolver object lacks a resolve method, the Node worker returns ok: null, but
CustomResolverAdapter treats the result as an object and errors on missing id/resolution,
aborting the install; pnpm skips resolvers that don’t implement both canResolve and resolve.
Code

pacquet/crates/hooks/src/custom_resolver_adapter.rs[R113-133]

+            let result =
+                self.resolver.resolve(wanted_val, opts_val).await.map_err(|err| {
+                    Box::new(std::io::Error::other(err.to_string())) as ResolveError
+                })?;
+
+            let id = result.get("id").and_then(Value::as_str).ok_or_else(|| {
+                let err: ResolveError = Box::new(std::io::Error::new(
+                    std::io::ErrorKind::InvalidData,
+                    "Custom resolver did not return an 'id' field",
+                ));
+                err
+            })?;
+
+            let resolution_val = result.get("resolution").ok_or_else(|| {
+                let err: ResolveError = Box::new(std::io::Error::new(
+                    std::io::ErrorKind::InvalidData,
+                    "Custom resolver did not return a 'resolution' field",
+                ));
+                err
+            })?;
+
Evidence
The worker explicitly returns ok: null for missing resolver methods;
NodeJsCustomResolver::resolve forwards the raw value; and the adapter then errors when it can’t
find required fields. pnpm’s implementation avoids this by skipping resolvers without
canResolve/resolve.

pacquet/crates/hooks/src/worker.rs[245-262]
pacquet/crates/hooks/src/node_runtime.rs[222-231]
pacquet/crates/hooks/src/custom_resolver_adapter.rs[113-140]
resolving/default-resolver/src/index.ts[75-90]

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

## Issue description
The Node worker returns `ok: null` when a resolver method is missing. `NodeJsCustomResolver::resolve` forwards this `null` to Rust, and `CustomResolverAdapter` then errors because it requires `id` and `resolution`. This aborts installs for pnpmfiles that contain partially-implemented resolver objects.
### Issue Context
pnpm’s default resolver explicitly skips custom resolvers that don’t have both `canResolve` and `resolve`.
### Fix Focus Areas
- pacquet/crates/hooks/src/worker.rs[245-262]
- pacquet/crates/hooks/src/node_runtime.rs[190-231]
- pacquet/crates/hooks/src/custom_resolver_adapter.rs[113-140]
### What to change
Implement pnpm-compatible filtering so incomplete resolver objects are ignored instead of causing an install failure:
- Option A (preferred): extend the IPC protocol with a `resolverMeta`/`resolverHasMethod` query that returns method availability for a given index. In `get_custom_resolvers()`, only construct `NodeJsCustomResolver` instances for entries with both `canResolve` and `resolve`.
- Option B: change the worker to distinguish "method missing" from a legitimate `null` return (e.g., `{ ok: { __missingMethod: true } }`) and treat missing-method as "not handled".
- Option C (fallback): in `CustomResolverAdapter::resolve`, if the returned value is `null`, treat it as `Ok(None)` (skip) rather than an InvalidData error; keep strict validation for non-null objects missing required fields.

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



Remediation recommended

4. Null resolver crashes worker 🐞 Bug ☼ Reliability
Description
The Node worker’s resolvers capability scan dereferences resolver.canResolve/resolver.resolve
without guarding against null, so a pnpmfile exporting resolvers: [null] will throw and abort
get_custom_resolvers. This is inconsistent with the per-resolver call path which already handles
missing/invalid resolver entries defensively.
Code

pacquet/crates/hooks/src/worker.rs[R275-281]

+    if (req.target === 'resolvers') {{
+      const resolvers = mod && mod.resolvers;
+      send({{ ok: (Array.isArray(resolvers) ? resolvers.map((resolver) => ({{
+        canResolve: typeof resolver.canResolve === 'function',
+        resolve: typeof resolver.resolve === 'function',
+        shouldRefreshResolution: typeof resolver.shouldRefreshResolution === 'function',
+      }})) : []) }});
Evidence
The worker’s capability query dereferences properties on each resolver element without a
null/undefined guard, which will throw for null entries; this error will bubble up through
get_custom_resolvers() because it relies on get_resolver_capabilities(). The per-resolver call
path already includes an explicit !resolver guard, showing the inconsistency.

pacquet/crates/hooks/src/worker.rs[275-289]
pacquet/crates/hooks/src/node_runtime.rs[190-200]

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

## Issue description
The embedded Node worker script crashes when `mod.resolvers` contains `null` (or other non-object values where property access can throw). This happens during resolver capability discovery (`target: 'resolvers'`) because the code uses `typeof resolver.canResolve` without checking `resolver` first.
### Issue Context
`NodeJsHooks::get_custom_resolvers()` calls `NodeWorker::get_resolver_capabilities()`. If the worker throws in the capability scan, the install fails even though the per-resolver invocation path already treats missing/invalid resolvers as returning `null`.
### Fix Focus Areas
- pacquet/crates/hooks/src/worker.rs[275-281]
### Suggested change
In the `resolvers.map(...)` capability scan, guard each entry:
- Use optional chaining: `typeof resolver?.canResolve === 'function'` (Node >=18), or
- Use a truthy check: `resolver && typeof resolver.canResolve === 'function'`
Apply the same guard for `resolve` and `shouldRefreshResolution`.

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


5. Unbounded refresh-check fanout 🐞 Bug ➹ Performance
Description
check_custom_resolver_force_resolve eagerly creates one future per (snapshot, resolver) pair and
pushes them all into FuturesUnordered, which can create a very large number of in-flight IPC calls
on big lockfiles. This can cause significant CPU/memory pressure (and higher timeout probability)
because each IPC request is tracked in the worker pending map until completion.
Code

pacquet/crates/package-manager/src/check_custom_resolver_force_resolve.rs[R48-61]

+    // Fire every (package, hook) check up front so the Node worker can
+    // interleave the async hooks, mirroring upstream's `anyTrue` over
+    // eagerly created promises; the first `true` wins and the rest are
+    // dropped.
+    let mut checks: FuturesUnordered<_> = snapshots
+        .iter()
+        .flat_map(|(dep_path, entry)| {
+            let snapshot_json = merged_package_snapshot_json(lockfile, dep_path, entry);
+            hooks.iter().map(move |hook| {
+                let snapshot_json = snapshot_json.clone();
+                async move { hook.should_refresh_resolution(dep_path, snapshot_json).await }
+            })
+        })
+        .collect();
Evidence
The force-resolve check constructs all (snapshot, hook) futures at once, and each Node-backed
check becomes an IPC request tracked in NodeWorker.pending until a response is received, so the
in-flight set grows with lockfile size.

pacquet/crates/package-manager/src/check_custom_resolver_force_resolve.rs[48-67]
pacquet/crates/hooks/src/worker.rs[51-56]
pacquet/crates/hooks/src/worker.rs[178-207]

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

## Issue description
`check_custom_resolver_force_resolve()` fans out requests for every `(dep_path, hook)` pair at once. On large `snapshots` maps this can create tens/hundreds of thousands of futures and IPC requests concurrently.
### Issue Context
While this mirrors pnpm’s eager promise creation, in Rust the combination of:
- allocating all futures up front,
- sending all IPC requests quickly,
- and tracking each request in `NodeWorker.pending`
can create substantial memory/CPU pressure.
### Fix Focus Areas
- pacquet/crates/package-manager/src/check_custom_resolver_force_resolve.rs[48-67]
- pacquet/crates/hooks/src/worker.rs[51-56]
- pacquet/crates/hooks/src/worker.rs[178-207]
### Suggested fix approach
1. Replace `flat_map(...).collect::<FuturesUnordered<_>>()` with a bounded-concurrency stream, e.g.:
- `stream::iter(items).map(...).buffer_unordered(MAX_IN_FLIGHT)`
2. Keep the early-exit behavior (stop once any returns `true`), but ensure cancellation cleanup is safe (see the other finding).
3. Consider choosing a MAX_IN_FLIGHT that still allows good overlap (e.g. 64/128) without unbounded growth.

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


6. Refresh checks run sequentially ✓ Resolved 🐞 Bug ➹ Performance
Description
should_refresh_resolution awaits every should_refresh_resolution(depPath, snapshot) call
sequentially across all snapshots/resolvers, which can be extremely slow on large lockfiles and
diverges from pnpm’s implementation that runs async checks concurrently and only invokes hooks that
exist.
Code

pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs[R2001-2015]

+/// A throwing hook propagates and aborts.
+async fn should_refresh_resolution(
+    custom_resolvers: &[Arc<dyn pacquet_hooks::CustomResolver>],
+    snapshots: &HashMap<pacquet_lockfile::PackageKey, pacquet_lockfile::SnapshotEntry>,
+) -> Result<bool, pacquet_hooks::HookError> {
+    for resolver in custom_resolvers {
+        for (dep_path, snapshot) in snapshots.iter() {
+            let snapshot_val = serde_json::to_value(snapshot).unwrap_or_default();
+            if resolver.should_refresh_resolution(dep_path.to_string(), snapshot_val).await? {
+                return Ok(true);
+            }
+        }
+    }
+    Ok(false)
+}
Evidence
Pacquet’s function is a strictly sequential nested await loop, while pnpm’s reference
implementation collects async checks and resolves them concurrently, and also skips resolvers
without the hook.

pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs[2001-2015]
installing/deps-installer/src/install/checkCustomResolverForceResolve.ts[17-33]

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

## Issue description
The current implementation performs nested loops and `await`s each `should_refresh_resolution` call in sequence. For lockfiles with many snapshots, this scales poorly (O(resolvers × snapshots) round-trips) and can make installs much slower than pnpm.
### Issue Context
pnpm’s `checkCustomResolverForceResolve` collects async checks and resolves them concurrently (`anyTrue(...)`), and it pre-filters to only resolvers that implement the hook.
### Fix Focus Areas
- pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs[2001-2015]
- installing/deps-installer/src/install/checkCustomResolverForceResolve.ts[17-33]
### What to change
- Use concurrency for async hook calls (e.g., `FuturesUnordered`) with early-exit on first `true`.
- Consider bounding concurrency (e.g., per-resolver batch size) to avoid spawning thousands of in-flight IPC calls at once.
- If possible, pre-filter resolvers that don’t implement `shouldRefreshResolution` (requires a capability query as noted in the other finding) to avoid unnecessary IPC calls.

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



Informational

7. Blocking mutex in async 🐞 Bug ➹ Performance
Description
CustomResolverAdapter uses std::sync::Mutex for can_resolve_cache inside an async resolver
path; under contention this can block tokio worker threads and it panics on poisoned locks via
unwrap().
Code

pacquet/crates/hooks/src/custom_resolver_adapter.rs[R20-23]

+pub struct CustomResolverAdapter {
+    resolver: Arc<dyn CustomResolver>,
+    can_resolve_cache: Mutex<HashMap<String, bool>>,
+}
Evidence
The adapter declares a std::sync::Mutex> and locks it inside resolve() using lock().unwrap().

pacquet/crates/hooks/src/custom_resolver_adapter.rs[20-23]
pacquet/crates/hooks/src/custom_resolver_adapter.rs[88-103]

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

## Issue description
The cache lock is a blocking `std::sync::Mutex` acquired in an async `resolve()` method, and uses `unwrap()` which will panic if the mutex is poisoned.
### Issue Context
This lock is not held across `.await`, so it’s not an async deadlock bug, but it can still block executor threads under load and turn unrelated panics into repeated panics.
### Fix Focus Areas
- pacquet/crates/hooks/src/custom_resolver_adapter.rs[20-23]
- pacquet/crates/hooks/src/custom_resolver_adapter.rs[88-103]
### What to change
- Prefer a poison-free lock (e.g., `parking_lot::Mutex`) or recover from poisoning (`unwrap_or_else(PoisonError::into_inner)`).
- If you want to avoid blocking tokio threads entirely, use `tokio::sync::Mutex` (though it has its own overhead) or a concurrent map (e.g., `dashmap`) for this cache.

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


Grey Divider

Qodo Logo

Comment thread pacquet/crates/resolving-resolver-base/src/resolve.rs
Comment thread pacquet/crates/hooks/src/custom_resolver_adapter.rs
zkochan added 2 commits June 12, 2026 14:25
- worker: return per-resolver method capabilities from the pnpmfile
  (target 'resolvers'), invoke resolver methods with 'this' bound to the
  resolver object like pnpm does, and drop the pending-map entry when a
  request fails to write so it cannot leak
- chain: build custom resolver adapters into the inner resolver chain so
  the prefetching and observing wrappers cover them; skip resolvers that
  do not implement both canResolve and resolve, mirroring upstream
- adapter: pass a resolver-returned manifest through to the typed
  ResolveResult (pnpm spreads the whole hook result)
- CurrentPkg: match the upstream resolver-base shape
  {id, name?, version?, resolution, publishedAt?} with camelCase serde
- port checkCustomResolverForceResolve: only consult resolvers that
  implement shouldRefreshResolution, hand the hook the merged
  packages+snapshots entry (pnpm's in-memory PackageSnapshot), and run
  the checks concurrently with first-true/first-error short-circuit
- consume async-trait from the workspace dependency instead of a local
  pin

Unit tests cover the adapter contract (missing id/resolution, invalid
shapes, canResolve memoization keyed alias@bareSpecifier like upstream,
payload shapes, manifest passthrough), the force-resolve check (port of
upstream's checkCustomResolverForceResolve tests), and the Node IPC
round trip against a real pnpmfile.
…paths

The hook's contract is to force re-resolution on subsequent installs,
but both repeat-install optimizations skipped it entirely:

- the prefer-frozen dispatch now consults the custom resolvers when the
  freshness gate passes, mirroring pnpm where forceResolutionFromHook
  feeds needsFullResolution and blocks isFrozenInstallPossible; a
  throwing hook aborts the install (PNPMFILE_FAIL)
- the optimistic repeat-install check now ports the pnpmfile branch of
  patchesOrHooksAreModified: the workspace state records the loaded
  pnpmfile list, and an added, removed, or edited pnpmfile invalidates
  the mtime fast path

End-to-end tests drive the pacquet binary against the mock registry:
custom resolver precedence over the npm resolver, a shouldRefreshResolution
verdict re-resolving past an up-to-date lockfile, and a throwing hook
failing the install.
@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 5f8ce6f

Comment thread pacquet/crates/hooks/src/worker.rs Outdated
A caller dropping a NodeWorker request future (e.g.
check_custom_resolver_force_resolve returning early on the first true
verdict) left the request's pending-map entry behind whenever the
worker never replied, because cleanup only ran on the send-failure and
timeout paths. Hold the entries in a std mutex (never locked across an
await) and remove them from an RAII guard, so every exit — reply, send
failure, timeout, cancellation — cleans up.

Also Box::pin the CLI command future: clippy::large_futures trips its
stack-size threshold on Windows after the install dispatch grew.
Copilot AI review requested due to automatic review settings June 12, 2026 13:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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 8e80c36

@zkochan zkochan changed the title feat(pacquet): implement custom resolver hooks (adapter, IPC, chain integration) feat(pacquet): support pnpmfile custom resolver hooks (adapter, IPC, chain integration, force-refresh) Jun 12, 2026
@zkochan zkochan merged commit a9d2ec8 into pnpm:main Jun 12, 2026
23 checks passed
@kairosci kairosci deleted the feat/custom-resolvers branch June 12, 2026 14:14
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.

4 participants