feat(pacquet): support pnpmfile custom resolver hooks (adapter, IPC, chain integration, force-refresh)#12153
Conversation
📝 WalkthroughWalkthroughAdds 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. ChangesCustom Resolver Hooks Integration
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
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoImplement custom resolver hooks with adapter and IPC integration
WalkthroughsDescription• 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 Diagramflowchart 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
File Changes1. pacquet/crates/hooks/src/custom_resolver_adapter.rs
|
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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 winRemove the pending entry when sending the request fails.
If
write_allorflusherrors here, thePendinginserted 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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pacquet/crates/hooks/Cargo.tomlpacquet/crates/hooks/src/custom_resolver_adapter.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/src/worker.rspacquet/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 rootCargo.tomlbefore 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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/hooks/src/custom_resolver_adapter.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/worker.rspacquet/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.tomlpacquet/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.tomlpacquet/crates/hooks/src/lib.rspacquet/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.tomlpacquet/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.tomlpacquet/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.tomlpacquet/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.rspacquet/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.rspacquet/crates/hooks/src/lib.rspacquet/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.rspacquet/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.rspacquet/crates/hooks/src/lib.rspacquet/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.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/worker.rspacquet/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.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/worker.rspacquet/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.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/worker.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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-traitisn’t in root[workspace.dependencies]; add it there (or justify local pin)
async-traitis referenced only inpacquet/crates/hooks/Cargo.tomlasasync-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 viaasync-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
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.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
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
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
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
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
]
}
]
} |
|
| Branch | pr/12153 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 3,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%) |
3e6e3a3 to
8c83151
Compare
There was a problem hiding this comment.
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 liftPlease 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 forcingUpdateReuseScope::Nonewhen 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 liftKeep
dep_pathbranded on the public hook contract.If this is pacquet’s mirror of pnpm’s dependency-path surface, taking a plain
Stringhere 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
Stringor&strin 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 winWrap the combined resolver chain in
PrefetchingResolver.When
lockfile_onlyis false,resolveris 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. BuildDefaultResolver(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
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
pacquet/crates/hooks/Cargo.tomlpacquet/crates/hooks/src/custom_resolver_adapter.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/src/worker.rspacquet/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 rootCargo.tomlbefore 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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/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.tomlpacquet/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.tomlpacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/crates/hooks/src/lib.rs
There was a problem hiding this comment.
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 winCache key is too coarse for
canResolvesemantics.At Line 28, the cache key only uses
alias+bareSpecifier, butcan_resolveis 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
📒 Files selected for processing (3)
pacquet/crates/hooks/src/custom_resolver_adapter.rspacquet/crates/resolving-resolver-base/src/lib.rspacquet/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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/resolving-resolver-base/src/resolve.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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.rspacquet/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 winEnsure
CurrentPkg.peer_dependenciesserializes aspeerDependenciesThe payload consumed by pnpm/custom-resolver hooks expects the camelCase JSON key
peerDependencies;CurrentPkgcurrently haspeer_dependencieswith onlyskip_serializing_ifvisible. 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
There was a problem hiding this comment.
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 winCache key is too narrow for
can_resolvememoization.Line 30 only keys by
alias@bareSpecifier, butcan_resolveis called with the full wanted payload (optional,injected,prevSpecifiertoo). 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 liftAdd 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, missingresolution, invalidresolutionshape, and cache behavior with same alias/specifier but differentoptional/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
📒 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 firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/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
|
| Branch | pr/12153 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,206.27 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 693.52 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 695.29 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 2,190.93 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 731.93 ms |
|
cc: @zkochan |
…ver chain integration
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|
3014dc9 to
b7bedd8
Compare
Code Review by Qodo
1.
|
- 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.
|
Code review by qodo was updated up to the latest commit 5f8ce6f |
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.
|
Code review by qodo was updated up to the latest commit 8e80c36 |
Port pnpm's custom resolver hooks to the Rust pacquet engine: a pnpmfile can export a top-level
resolversarray 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
CustomResolvertrait (canResolve/resolve/shouldRefreshResolution) mirroringhooks/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'sif (!customResolver.canResolve || !customResolver.resolve) continueandcheckCustomResolverForceResolve's hook filter).resolvers(capabilities) andresolver(method invocation) requests. Methods are invoked withthisbound to the resolver object, like pnpm. Pending-request cleanup is cancellation-safe via an RAII guard.CustomResolverAdapterbridges the JSON hook contract to the typedResolvertrait. 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.canResolveresults are memoized keyedalias@bareSpecifier, exactly like pnpm'sgetCustomResolverCacheKey. A resolver-returnedmanifestpasses through (pnpm spreads the whole hook result). Payloads match upstream:prevSpecifier, and resolve opts carrylockfileDir/projectDir/preferredVersions/currentPkg.shouldRefreshResolutionsemantics — port ofcheckCustomResolverForceResolve: the hook receives the merged packages+snapshots entry (pnpm's in-memoryPackageSnapshot), checks run concurrently with first-true/first-error short-circuit, and a throwing hook aborts the install (PNPMFILE_FAIL). Atrueverdict defeats both up-to-date optimizations, as documented in the hook's contract:forceResolutionFromHook→needsFullResolutionblocksisFrozenInstallPossible) and routes to the fresh-resolve path with lockfile reuse disabled (UpdateReuseScope::None);patchesOrHooksAreModified: the workspace state records the loaded pnpmfile list, and an added/removed/edited pnpmfile invalidates the mtime check.CurrentPkg— added toResolveOptions, matching upstream'scurrentPkgshape{id, name?, version?, resolution, publishedAt?}(camelCase).Tests
id/resolution, invalid shapes,canResolvememoization, payload shapes, manifest passthrough.check_custom_resolver_force_resolveunit tests: port of upstream'scheckCustomResolverForceResolve.tssuite (capability filter, true/false/error propagation, merged snapshot payload).thisbinding, round trips, error propagation, cancellation cleanup.shouldRefreshResolutionre-resolving past an up-to-date lockfile, and a throwing hook failing the install.Known gaps / follow-ups
ResolveOptions.current_pkgis 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 receivecurrentPkg: null. The contract and serialization are in place for when that threading lands.LockfileResolutionenum (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.CustomFetcher) are not part of this PR.Description updated by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
New Features
Bug Fixes / Reliability