feat(pacquet): add pnpmfile hooks support#12044
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new pacquet-hooks crate that discovers and executes .pnpmfile.{cjs,mjs} hooks via a Node bridge, exposes the PnpmfileHooks async trait (readPackage, afterAllResolved, preResolution, filterLog), provides finder/loading helpers and tests, and threads a workspace-scoped pnpmfile_hook through install and resolution. ChangesPnpmfile hooks feature
Sequence DiagramsequenceDiagram
participant Installer as InstallWithFreshLockfile
participant Finder as finder::load_pnpmfile
participant Resolver as resolve_workspace / resolve_dependency_tree
participant NodeHook as NodeJsHooks
Installer->>Finder: load_pnpmfile(lockfile_dir)
Finder-->>Installer: Option<Arc<PnpmfileHooks>>
Installer->>Resolver: resolve_workspace(..., pnpmfile_hook)
Resolver->>NodeHook: pnpmfile_hook.read_package(pkg, HookContext)
NodeHook-->>Resolver: Option<JSON manifest>
Resolver->>Resolver: apply updated manifest if Some(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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)
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 QodoAdd pnpmfile hooks support for package manifest modification
WalkthroughsDescription• Implements pnpmfile hooks support for readPackage, afterAllResolved, preResolution, and filterLog • Creates new pacquet-hooks crate with trait-based hook interface and Node.js runtime execution • Integrates pnpmfile hooks into dependency resolution pipeline via readPackage hook • Adds comprehensive tests for pnpmfile discovery and hook execution Diagramflowchart LR
A["pnpmfile finder"] -->|discovers| B[".pnpmfile.cjs/mjs"]
B -->|loads| C["NodeJsHooks runtime"]
C -->|executes| D["readPackage hook"]
D -->|modifies| E["package manifest"]
E -->|feeds into| F["dependency resolution"]
File Changes1. pacquet/crates/hooks/src/lib.rs
|
969eb25 to
ed3cfd1
Compare
|
I am not sure about how pnpmfiles will be supported #11685 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pacquet/crates/hooks/Cargo.toml (1)
9-9: ⚡ Quick winMove
tokiotodev-dependencies(avoidfullin runtime dependencies)
tokioinpacquet/crates/hooks/Cargo.toml(line 9) is only used in tests via#[tokio::test]inpacquet/crates/hooks/tests/node_hooks.rs(no non-testtokio::...usage found). Move it to[dev-dependencies]and trim features to only what the test macro/runtime needs.🤖 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/Cargo.toml` at line 9, The tokio dependency is only used in tests (see #[tokio::test] in tests/node_hooks.rs), so remove the tokio entry from the regular [dependencies] block and add it under [dev-dependencies]; also drop the heavy "full" feature and use the minimal features needed for the test runtime (e.g., features = ["rt-multi-thread", "macros"] with workspace = true) so tokio is only a test-time dependency for the node_hooks tests.
🤖 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/finder.rs`:
- Around line 16-17: The check for pnpmfile candidates uses path.exists(), which
can match directories or symlinks and lead to non-file paths being returned;
update the condition in the finder function to use path.is_file()
(Path::is_file()) instead so only actual files are accepted before returning
Some(path), leaving the rest of the logic unchanged.
In `@pacquet/crates/hooks/src/node_runtime.rs`:
- Around line 14-23: The current construction of the `wrapper` string in
node_runtime.rs interpolates `self.file.to_string_lossy()` (and other dynamic
inputs) directly into the single-quoted `require('{}')` JS literal, which can
break for paths containing quotes/backslashes or enable injection; change the
`wrapper` to a fixed `node -e` script that reads the module path (and
func/payload) from `process.argv` or environment and calls `require(fileArg)` at
runtime instead of embedding `self.file` into the string, then pass `self.file`,
`func`, and `payload` as separate argv/env entries when spawning Node so the
`wrapper` variable no longer contains any unescaped dynamic file path.
- Around line 16-18: The generated Node wrapper unsafe interpolates
self.file.to_string_lossy() into require('...') and still uses require() for
.mjs, which risks JS injection and breaks async ESM; instead generate a wrapper
that passes the hook path via process.argv (or JSON-encodes it) rather than raw
string interpolation and uses JSON.stringify/escape when embedding any string;
for .mjs produce an async wrapper that uses dynamic import(path) inside an async
IIFE (await import(path)) and then calls (hooks.hooks &&
hooks.hooks['hookName'])?.(args) and console.log(JSON.stringify(res)); update
the code that emits "require('{}')" and the call to hooks.hooks['{}'] in
node_runtime.rs to use process.argv[2] (or a safely JSON-encoded literal) and to
branch to import() for files ending with .mjs so top-level await/async ESM works
correctly.
In `@pacquet/crates/hooks/tests/node_hooks.rs`:
- Around line 41-146: Add a new async test mirroring
test_node_js_hooks_read_package but using an ESM pnpmfile: create a TempDir and
write a ".pnpmfile.mjs" file that exports hooks with ESM syntax (e.g. `export
const hooks = { readPackage };` and `function readPackage(pkg) { if (pkg.name
=== 'foo') pkg.dependencies = { bar: '100.0.0' }; return pkg; }`), instantiate
pacquet_hooks::node_runtime::NodeJsHooks { file: pnpmfile_path }, call
hooks.read_package(manifest, HookContext { log: Arc::new(|_| {}) }).await, and
assert the returned manifest has dependencies.bar == "100.0.0"; this ensures
NodeJsHooks::read_package works for .mjs loader paths.
---
Nitpick comments:
In `@pacquet/crates/hooks/Cargo.toml`:
- Line 9: The tokio dependency is only used in tests (see #[tokio::test] in
tests/node_hooks.rs), so remove the tokio entry from the regular [dependencies]
block and add it under [dev-dependencies]; also drop the heavy "full" feature
and use the minimal features needed for the test runtime (e.g., features =
["rt-multi-thread", "macros"] with workspace = true) so tokio is only a
test-time dependency for the node_hooks tests.
🪄 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: c70a62d8-4b0d-4f4e-877f-af2d711d948b
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.changeset/add-pnpmfile-hooks.mdCargo.tomlpacquet/crates/hooks/Cargo.tomlpacquet/crates/hooks/src/finder.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/tests/__fixtures__/filterLogHook.cjspacquet/crates/hooks/tests/__fixtures__/readPackageHook.cjspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Warnings are errors (--deny warningsin lint). Do not silence them with#[allow(...)]unless there is a specific, justified reason.
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming: https://rust-lang.github.io/api-guidelines/naming.html
No star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;, and the same for any other glob whose target is a module you control. Two forms stay allowed: external-crate preludes such asuse rayon::prelude::*;and root-of-module re-exports such aspub use submodule::*;in alib.rs.
Doc comments (///,//!) document the contract: preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body.
Do not restate at call sites what the callee's doc comment already says. If///on the function says 'no-op when …', the caller should not repeat that. Update the doc once; let every call site benefit.
Tests are documentation. Do not duplicate them in prose. If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test, do not also narrate it in the doc comment on the implementation. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the why if it is not obvious.
Use// SAFETY:,// TODO:, and similar prefixes to signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
When editing existing code, do not break a method chain (includingpipe-trait.pipe(...)chains) into intermediateletbindings unless you can justify the rewrite. Valid justifications include a chain that fails to compile after your...
Files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/hooks/src/finder.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/hooks/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rs
pacquet/**/{tests,test}/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/{tests,test}/*.rs: Tests must not be tolerant of a missing build / runtime environment by silently returning early when a tool isn't found. If the test needs a tool, just call into it and let the existing.unwrap()/.expect(...)panic when the tool is absent.
Prefer platform-specific gates like#[cfg_attr(target_os = "windows", ignore = "...")]or#[cfg(unix)]over runtime probe-and-skip helpers for platform-locked tools, because the gate is visible tocargo testand shows up in the test report.
Follow the test-logging guidance in the style guide — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!.
Files:
pacquet/crates/hooks/tests/node_hooks.rs
🧠 Learnings (6)
📚 Learning: 2026-05-26T21:01:06.666Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11966
File: .changeset/require-tarball-integrity.md:6-6
Timestamp: 2026-05-26T21:01:06.666Z
Learning: In pnpm lockfile-related release notes/docs (especially changeset markdown), preserve URL hostnames exactly as they appear in pnpm-lock.yaml tarball resolution entries—keep hosts like `codeload.github.com`, `bitbucket.org`, and `gitlab.com` in lowercase. Do not “correct” them to title-case/preserve brand capitalization (e.g., LanguageTool rules like `GITHUB` capitalization) because these are literal URL fragments, not platform brand names.
Applied to files:
.changeset/add-pnpmfile-hooks.md
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/hooks/src/finder.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/hooks/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/hooks/src/finder.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/hooks/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/hooks/src/finder.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/hooks/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/tests.rs
🔇 Additional comments (12)
pacquet/crates/hooks/src/lib.rs (1)
1-50: LGTM!pacquet/crates/hooks/tests/__fixtures__/readPackageHook.cjs (1)
1-10: LGTM!pacquet/crates/hooks/tests/__fixtures__/filterLogHook.cjs (1)
1-7: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
13-13: LGTM!Also applies to: 52-53, 62-63, 180-181, 305-306, 321-322, 352-355, 462-467, 619-637
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
129-129: LGTM!Also applies to: 136-139, 159-162, 199-207, 240-247
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs (1)
66-70: LGTM!Also applies to: 109-115
pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs (1)
99-101: LGTM!pacquet/crates/package-manager/Cargo.toml (1)
34-34: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
18-18: LGTM!Also applies to: 597-597, 605-605, 653-653, 655-655
pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
18-18: LGTM!pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
120-120: LGTM!Also applies to: 189-189, 274-274, 315-315, 359-359, 438-438, 480-480, 523-523, 568-568, 609-609, 661-661, 711-711, 764-764, 888-888, 967-967, 1016-1016, 1113-1113, 1208-1208, 1315-1315, 1384-1384, 1494-1494, 1602-1602, 1712-1712, 1801-1801, 1849-1849, 1883-1883, 1933-1933, 2003-2003, 2058-2058, 2123-2123, 2176-2176
Cargo.toml (1)
49-49: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/node_runtime.rs`:
- Around line 25-31: NodeJsHooks::call_node currently uses blocking
std::process::Command::output() which can block Tokio worker threads; change
call_node into an async function that spawns an asynchronous process using
tokio::process::Command and await its output with a tokio::time::timeout
(configurable duration) so hung hooks error out instead of blocking; update all
callers mentioned (e.g., read_package and other #[async_trait] trait methods
used by resolve_dependency_tree::resolve_node) to call .await on the new async
call_node and propagate timeout/errors appropriately; consider exposing the
timeout value as a parameter or config and ensure stderr/stdout are converted to
String on timeout/error paths.
- Around line 47-71: The NodeJsHooks methods are swallowing call_node errors
(using .ok()/.unwrap_or) and ignoring HookContext; capture the Result from
self.call_node in read_package, after_all_resolved, pre_resolution, and
filter_log, and on Err(e) call ctx.log.error (or the appropriate HookContext
logging method) with a clear message including the hook name and the error, then
fall back to the previous return value; for read_package use ctx.log and return
None on error, for after_all_resolved/pre_resolution log and return None, and
for filter_log log and return true as before — if you want installs to fail like
pnpm consider changing these signatures to return Result<> and propagate the
error instead of falling back.
🪄 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: bea01777-76bd-46fa-a192-f1da4be37483
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.changeset/add-pnpmfile-hooks.mdCargo.tomlpacquet/crates/hooks/Cargo.tomlpacquet/crates/hooks/src/finder.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/tests/__fixtures__/filterLogHook.cjspacquet/crates/hooks/tests/__fixtures__/readPackageHook.cjspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/tests.rs
✅ Files skipped from review due to trivial changes (3)
- .changeset/add-pnpmfile-hooks.md
- Cargo.toml
- pacquet/crates/hooks/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (9)
- pacquet/crates/hooks/tests/fixtures/filterLogHook.cjs
- pacquet/crates/hooks/src/finder.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
- pacquet/crates/package-manager/Cargo.toml
- pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
- pacquet/crates/hooks/tests/node_hooks.rs
- pacquet/crates/hooks/src/lib.rs
- pacquet/crates/resolving-deps-resolver/src/tests.rs
- pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Warnings are errors (--deny warningsin lint). Do not silence them with#[allow(...)]unless there is a specific, justified reason.
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming: https://rust-lang.github.io/api-guidelines/naming.html
No star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;, and the same for any other glob whose target is a module you control. Two forms stay allowed: external-crate preludes such asuse rayon::prelude::*;and root-of-module re-exports such aspub use submodule::*;in alib.rs.
Doc comments (///,//!) document the contract: preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body.
Do not restate at call sites what the callee's doc comment already says. If///on the function says 'no-op when …', the caller should not repeat that. Update the doc once; let every call site benefit.
Tests are documentation. Do not duplicate them in prose. If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test, do not also narrate it in the doc comment on the implementation. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the why if it is not obvious.
Use// SAFETY:,// TODO:, and similar prefixes to signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
When editing existing code, do not break a method chain (includingpipe-trait.pipe(...)chains) into intermediateletbindings unless you can justify the rewrite. Valid justifications include a chain that fails to compile after your...
Files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/hooks/src/node_runtime.rs
🧠 Learnings (5)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/hooks/src/node_runtime.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/hooks/src/node_runtime.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/hooks/src/node_runtime.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
🔇 Additional comments (4)
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
136-138: LGTM!Also applies to: 159-161, 199-208, 242-247
pacquet/crates/resolving-deps-resolver/src/resolve_workspace.rs (1)
66-69: LGTM!Also applies to: 109-115
pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
18-18: LGTM!pacquet/crates/hooks/src/node_runtime.rs (1)
14-23: Unsafe JS interpolation ofself.fileandrequire()on.mjshooks.
self.file.to_string_lossy()is embedded into a single-quoted JS literal (require('{}')), so paths containing', backslashes, or other special characters produce broken JS or enable code injection. Pass the path viaprocess.argv/env (orJSON.stringifyit) instead of raw string interpolation. Separately,require()will fail for.pnpmfile.mjshooks that rely on async ESM (top-level await); prefer dynamicimport()for.mjs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12044 +/- ##
==========================================
+ Coverage 87.34% 87.40% +0.05%
==========================================
Files 255 260 +5
Lines 28703 29347 +644
==========================================
+ Hits 25072 25652 +580
- Misses 3631 3695 +64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.05215488258,
"stddev": 0.030803851839023223,
"median": 2.0593559935799997,
"user": 2.59145464,
"system": 3.3509695600000002,
"min": 1.9948375250800001,
"max": 2.10223386908,
"times": [
2.0678187870799998,
2.0364871660799997,
2.06040434508,
2.0690367780799996,
2.02262041508,
2.03279234108,
1.9948375250800001,
2.10223386908,
2.07700995708,
2.05830764208
]
},
{
"command": "pacquet@main",
"mean": 2.0391500709799995,
"stddev": 0.034510775269862806,
"median": 2.04285212108,
"user": 2.64039584,
"system": 3.3707286599999997,
"min": 1.97878239208,
"max": 2.08225380908,
"times": [
2.08225380908,
2.03236821908,
2.05124877608,
2.0100345850799997,
1.97878239208,
1.99958024008,
2.03445546608,
2.0753007700799997,
2.05770368908,
2.06977276308
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.65038852572,
"stddev": 0.015461715260718592,
"median": 0.64806880552,
"user": 0.35475871999999997,
"system": 1.3342111199999998,
"min": 0.62926649202,
"max": 0.68837923802,
"times": [
0.68837923802,
0.65727400702,
0.65419806502,
0.64969583402,
0.64963561102,
0.64650200002,
0.64516244402,
0.64435233902,
0.63941922702,
0.62926649202
]
},
{
"command": "pacquet@main",
"mean": 0.7103098370200001,
"stddev": 0.04745633529321599,
"median": 0.7043471945199999,
"user": 0.36994192000000004,
"system": 1.3285393199999997,
"min": 0.64866043702,
"max": 0.79916245802,
"times": [
0.79916245802,
0.67397988402,
0.75522206902,
0.71316708602,
0.65164302002,
0.73644706102,
0.73522736902,
0.69552730302,
0.64866043702,
0.69406168302
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.28971615116,
"stddev": 0.021233044189596498,
"median": 2.27971043726,
"user": 3.7397467200000003,
"system": 3.11472576,
"min": 2.26694589476,
"max": 2.32852518776,
"times": [
2.30935088576,
2.27173441276,
2.27390489476,
2.26694589476,
2.27812294676,
2.29928550176,
2.31324011776,
2.28129792776,
2.27475374176,
2.32852518776
]
},
{
"command": "pacquet@main",
"mean": 2.32339245786,
"stddev": 0.028903926919204296,
"median": 2.32538907726,
"user": 3.7587490199999998,
"system": 3.17947216,
"min": 2.27186920876,
"max": 2.37466470676,
"times": [
2.28316516576,
2.32349109476,
2.3446302597599997,
2.3215126757599998,
2.32990499776,
2.33390831476,
2.32448016376,
2.37466470676,
2.27186920876,
2.32629799076
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.46892133656,
"stddev": 0.0168255320521366,
"median": 1.46852695676,
"user": 1.6695813999999998,
"system": 1.8346263999999999,
"min": 1.44090346576,
"max": 1.49452149176,
"times": [
1.49452149176,
1.4671705407600002,
1.46916207976,
1.44090346576,
1.4891157217600002,
1.47655602976,
1.46789183376,
1.4521143007600001,
1.4529204177600001,
1.4788574837600001
]
},
{
"command": "pacquet@main",
"mean": 1.5001952348599998,
"stddev": 0.10340991654557849,
"median": 1.45906583426,
"user": 1.7059956,
"system": 1.8316168999999995,
"min": 1.44983353176,
"max": 1.78839553676,
"times": [
1.45588404376,
1.4584151487600001,
1.78839553676,
1.49950535276,
1.4597165197600002,
1.45010921376,
1.4511653237600002,
1.44983353176,
1.4818051787600002,
1.50712249876
]
}
]
} |
|
Bencher Report| Branch | pr/12044 |
| 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 | 2,289.72 ms(-2.83%)Baseline: 2,356.31 ms | 2,827.57 ms (80.98%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,468.92 ms(-3.52%)Baseline: 1,522.47 ms | 1,826.97 ms (80.40%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,052.15 ms(-1.69%)Baseline: 2,087.46 ms | 2,504.96 ms (81.92%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 650.39 ms(-2.68%)Baseline: 668.32 ms | 801.99 ms (81.10%) |
|
Using a Node.js subprocess to run .pnpmfile.mjs/.pnpmfile.cjs is only a temporary solution. Although the ideal goal for a Rust project is a WASM plugin system (pnpmfile.wasm), the current approach ensures immediate compatibility with the original pnpm ecosystem, exactly replicating its hooks and JavaScript interface. This technical bridge allows for support of both ESM and CommonJS right from the start, while keeping the internal architecture ready to transition to WASM in the future without disrupting the public APIs of the features. |
7baf1a3 to
483b238
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/node_runtime.rs`:
- Around line 58-61: The timeout currently drops the future returned by
Command::new("node").arg("-e").arg(&wrapper).output() which can leave a hung
node child running; update the Command invocation in the timeout call (the
expression that builds the child before calling output()) to call
.kill_on_drop(true) on the Command so the child process is terminated when the
future is dropped (i.e., modify the code around
Command::new("node").arg("-e").arg(&wrapper).output() inside the timeout in
node_runtime.rs).
- Around line 38-58: The current implementation interpolates the full JSON
payload into the wrapper string and passes it via node -e, risking E2BIG; change
NodeJsHooks::call_node so the wrapper no longer embeds {payload} but instead
reads the payload from stdin (or an env/temp file), and invoke
Command::new("node") with stdin piped and write the serialized payload into the
child process before awaiting output; keep existing logic that switches between
the async .mjs import wrapper and the require wrapper (the wrapper variable),
and preserve the HOOK_TIMEOUT usage around the Command::output() call while
replacing .arg("-e").arg(&wrapper).output() with a piped-stdin invocation that
feeds the payload to node.
- Around line 38-56: The wrapper sent by NodeJsHooks::call_node calls the hook
without awaiting it, causing Promise results to be JSON-serialized too early;
modify the wrapper generation so the .mjs branch awaits the hook invocation
(e.g., use "await (hooks.hooks && hooks.hooks['{func}'])?.({payload})" inside
the async IIFE) and change the CJS branch to also use an async IIFE (instead of
top-level require+call) so you can await the hook there as well before
console.log(JSON.stringify(res)); update the code that builds `wrapper` (the
file_path.ends_with(".mjs") conditional) accordingly so both branches await the
hook result.
🪄 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: 8c8cde1e-f794-4474-b27a-49ebd4261256
📒 Files selected for processing (5)
pacquet/crates/hooks/Cargo.tomlpacquet/crates/hooks/src/finder.rspacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/tests/node_hooks.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- pacquet/crates/hooks/src/finder.rs
- pacquet/crates/hooks/src/lib.rs
- pacquet/crates/hooks/tests/node_hooks.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Analyze (javascript)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Warnings are errors (--deny warningsin lint). Do not silence them with#[allow(...)]unless there is a specific, justified reason.
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming: https://rust-lang.github.io/api-guidelines/naming.html
No star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;, and the same for any other glob whose target is a module you control. Two forms stay allowed: external-crate preludes such asuse rayon::prelude::*;and root-of-module re-exports such aspub use submodule::*;in alib.rs.
Doc comments (///,//!) document the contract: preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body.
Do not restate at call sites what the callee's doc comment already says. If///on the function says 'no-op when …', the caller should not repeat that. Update the doc once; let every call site benefit.
Tests are documentation. Do not duplicate them in prose. If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test, do not also narrate it in the doc comment on the implementation. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the why if it is not obvious.
Use// SAFETY:,// TODO:, and similar prefixes to signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
When editing existing code, do not break a method chain (includingpipe-trait.pipe(...)chains) into intermediateletbindings unless you can justify the rewrite. Valid justifications include a chain that fails to compile after your...
Files:
pacquet/crates/hooks/src/node_runtime.rs
🧠 Learnings (3)
📚 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/node_runtime.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/node_runtime.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/node_runtime.rs
🔇 Additional comments (6)
pacquet/crates/hooks/src/node_runtime.rs (4)
83-96: Divergence from pnpm:readPackagefailure should abort, not continue.Logging via
ctx.logis a good improvement over silently swallowing. However, in pnpm a throw fromhooks.readPackageaborts the install, whereas here the error is logged and resolution continues with the original manifest. This is the same divergence raised previously. Based on learnings, follow the upstream pnpm compatibility rule: match pnpm's behavior exactly (a failingreadPackageshould fail the install rather than fall back).
16-28: LGTM!
63-78: LGTM!
98-116: 🏗️ Heavy liftMap
null/undefinedhook outputs toNoneforafter_all_resolvedandpre_resolution
NodeJsHooks::read_packagemapsOk(Value::Null)(from JSnull/undefined) toNone, butafter_all_resolvedandpre_resolutioncurrently wrap it asSome(Value::Null), which can causenullto be treated as a real value instead of “hook returned nothing”.🛠️ Proposed fix
match self.call_node("afterAllResolved", lockfile).await { + Ok(v) if v.is_null() => None, Ok(v) => Some(v), Err(e) => { (ctx.log)(format!("pnpmfile hook afterAllResolved failed: {}", e)); None } }match self.call_node("preResolution", Value::Null).await { + Ok(v) if v.is_null() => None, Ok(v) => Some(v), Err(e) => { (ctx.log)(format!("pnpmfile hook preResolution failed: {}", e)); None } }pacquet/crates/hooks/Cargo.toml (2)
9-9: LGTM!
10-10: ⚡ Quick winEnsure the workspace-level
derive_moreenables thedisplay/fullfeature
pacquet/crates/hooks/Cargo.tomldeclaresderive_more = { workspace = true }, and many other crates do the same—so the#[derive(Display)]usage depends entirely on howderive_moreis configured in the workspace dependency (e.g.,features = ["display"]or["full"]). The gathered results don’t include the workspace-levelderive_morefeatures, so that declaration needs inspection to guarantee this crate compiles.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
pacquet/crates/hooks/src/node_runtime.rs (3)
38-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winHook result is never
awaited — async pnpmfile hooks return aPromisethat gets serialized as{}.pnpm's
readPackage/afterAllResolved/etc. may beasync. In both branches the hook is invoked withoutawait, so an async hook'sPromisereachesJSON.stringify(res)→"{}". ForreadPackagethat means the manifest is silently replaced with an empty object (data corruption), not a no-op. The CJS branch also needs an async IIFE since top-levelawaitis invalid undernode -eCommonJS.🐛 Proposed fix (await in both branches; wrap CJS in async IIFE)
let wrapper = if file_path.ends_with(".mjs") { format!( r#"(async () => {{ const hooks = await import({file_path_escaped}); - const res = (hooks.hooks && hooks.hooks['{func}'])?.({payload}); + const res = await (hooks.hooks && hooks.hooks['{func}'])?.({payload}); console.log(JSON.stringify(res)); }})(); "#, file_path_escaped = file_path_escaped ) } else { format!( - r#"const hooks = require({file_path_escaped}); -const res = (hooks.hooks && hooks.hooks['{func}'])?.({payload}); -console.log(JSON.stringify(res)); + r#"(async () => {{ + const hooks = require({file_path_escaped}); + const res = await (hooks.hooks && hooks.hooks['{func}'])?.({payload}); + console.log(JSON.stringify(res)); +}})(); "#, file_path_escaped = file_path_escaped ) };Separately, the hook is invoked with only
{payload}; pnpm'sreadPackage(pkg, context)passes a secondcontextargument (with alogfunction). Hooks relying oncontextwould throw here. Please confirm the intended JS contract.What arguments does pnpm pass to the readPackage hook in .pnpmfile.cjs, and does the context include a log function?🤖 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/node_runtime.rs` around lines 38 - 56, The generated JS never awaits the hook result and the CJS branch runs top-level code, so async hooks serialize as {} and context is missing; update the template generation for variable wrapper so both branches invoke the hook inside an async IIFE and await the call (e.g. replace (hooks.hooks && hooks.hooks['{func}'])?.({payload}) with await (hooks.hooks && hooks.hooks['{func}'])?.({payload}, {context}) ), and for the CJS branch wrap require(...) usage in an async IIFE like the ESM branch so console.log(JSON.stringify(res)) receives the resolved value; keep the same identifiers (wrapper, file_path.ends_with(".mjs"), hooks.hooks['{func}'], console.log(JSON.stringify(res))) when making the changes.
58-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet
kill_on_drop(true)so a hungnodechild is terminated on timeout.When the timeout fires the
output()future is dropped, but Tokio does not kill the spawned process onChilddrop by default, leaving an orphanednodeprocess.🔪 Proposed fix
- let output = timeout(HOOK_TIMEOUT, Command::new("node").arg("-e").arg(&wrapper).output()) - .await - .map_err(|_| HookError::Timeout(func.to_string()))? - .map_err(|e| HookError::ExecutionFailed(e.to_string()))?; + let output = timeout( + HOOK_TIMEOUT, + Command::new("node").arg("-e").arg(&wrapper).kill_on_drop(true).output(), + ) + .await + .map_err(|_| HookError::Timeout(func.to_string()))? + .map_err(|e| HookError::ExecutionFailed(e.to_string()))?;🤖 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/node_runtime.rs` around lines 58 - 61, The spawned node process must be configured to be killed when its Child is dropped; update the Command chain used in the timeout call (the Command::new("node")... sequence used to produce .output()) to call .kill_on_drop(true) before .output() so that when the timeout future is dropped it will terminate the child instead of leaving an orphaned node process (keep the existing error mapping to HookError::Timeout and HookError::ExecutionFailed).
58-77:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe
node -eargv + raw-stdout IPC is fragile: large payloads can hitE2BIG, and any hook stdout output corrupts JSON parsing.Two problems with the current channel:
after_all_resolvedforwards the full lockfile, which gets JSON-serialized into the wrapper and passed as a singlenode -eargv string; large lockfiles can exceed the OS arg limit and fail withE2BIG.- The entire stdout is parsed as JSON (Line 68-77). If the hook (or anything it imports) writes to stdout (
console.log, banners, etc.), parsing fails and the hook is silently treated as a no-op.Prefer a fixed Node script that reads the payload from stdin and emits the result on a dedicated channel (e.g., a unique marker or fd 3), then extract only that on the Rust side.
🛠️ Sketch: marker-delimited result (still fixes stdout pollution; combine with stdin for E2BIG)
-console.log(JSON.stringify(res)); +process.stdout.write("\u0000PNPMFILE\u0000" + JSON.stringify(res) + "\u0000PNPMFILE\u0000");let stdout = String::from_utf8_lossy(&output.stdout); let json = stdout .split("\u{0}PNPMFILE\u{0}") .nth(1) .map(str::trim) .ok_or_else(|| HookError::HookFailed(func.to_string(), "missing hook result".into()))?;🤖 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/node_runtime.rs` around lines 58 - 77, The current invocation in node_runtime.rs that builds Command::new("node").arg("-e").arg(&wrapper).output() is fragile (E2BIG) and parses raw stdout as JSON; change it to feed the JSON payload into the child process via stdin (so nothing is passed as a large argv), run a fixed Node script (either a bundled small wrapper or -e that reads stdin) which emits the hook result wrapped in a unique marker (e.g. "\u{0}PNPMFILE\u{0}RESULT\u{0}...RESULT\u{0}"), and then in the Rust code (where output.stdout is inspected) extract only the marker-delimited JSON (fail with HookError::HookFailed(func.to_string(), "missing hook result") if missing) before calling serde_json::from_str; keep existing timeout handling (HOOK_TIMEOUT), status/stderr checks (HookError::HookFailed), and error mappings (HookError::Timeout, HookError::ExecutionFailed) while replacing the .arg(&wrapper) argv-payload usage and raw stdout parse with stdin-based payload and marker-delimited extraction.
🤖 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.
Duplicate comments:
In `@pacquet/crates/hooks/src/node_runtime.rs`:
- Around line 38-56: The generated JS never awaits the hook result and the CJS
branch runs top-level code, so async hooks serialize as {} and context is
missing; update the template generation for variable wrapper so both branches
invoke the hook inside an async IIFE and await the call (e.g. replace
(hooks.hooks && hooks.hooks['{func}'])?.({payload}) with await (hooks.hooks &&
hooks.hooks['{func}'])?.({payload}, {context}) ), and for the CJS branch wrap
require(...) usage in an async IIFE like the ESM branch so
console.log(JSON.stringify(res)) receives the resolved value; keep the same
identifiers (wrapper, file_path.ends_with(".mjs"), hooks.hooks['{func}'],
console.log(JSON.stringify(res))) when making the changes.
- Around line 58-61: The spawned node process must be configured to be killed
when its Child is dropped; update the Command chain used in the timeout call
(the Command::new("node")... sequence used to produce .output()) to call
.kill_on_drop(true) before .output() so that when the timeout future is dropped
it will terminate the child instead of leaving an orphaned node process (keep
the existing error mapping to HookError::Timeout and
HookError::ExecutionFailed).
- Around line 58-77: The current invocation in node_runtime.rs that builds
Command::new("node").arg("-e").arg(&wrapper).output() is fragile (E2BIG) and
parses raw stdout as JSON; change it to feed the JSON payload into the child
process via stdin (so nothing is passed as a large argv), run a fixed Node
script (either a bundled small wrapper or -e that reads stdin) which emits the
hook result wrapped in a unique marker (e.g.
"\u{0}PNPMFILE\u{0}RESULT\u{0}...RESULT\u{0}"), and then in the Rust code (where
output.stdout is inspected) extract only the marker-delimited JSON (fail with
HookError::HookFailed(func.to_string(), "missing hook result") if missing)
before calling serde_json::from_str; keep existing timeout handling
(HOOK_TIMEOUT), status/stderr checks (HookError::HookFailed), and error mappings
(HookError::Timeout, HookError::ExecutionFailed) while replacing the
.arg(&wrapper) argv-payload usage and raw stdout parse with stdin-based payload
and marker-delimited extraction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4397d9b-c471-4a28-8c27-615766c987af
📒 Files selected for processing (4)
pacquet/crates/hooks/Cargo.tomlpacquet/crates/hooks/src/finder.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/tests/node_hooks.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- pacquet/crates/hooks/tests/node_hooks.rs
- pacquet/crates/hooks/src/finder.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). (3)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Warnings are errors (--deny warningsin lint). Do not silence them with#[allow(...)]unless there is a specific, justified reason.
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming: https://rust-lang.github.io/api-guidelines/naming.html
No star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;, and the same for any other glob whose target is a module you control. Two forms stay allowed: external-crate preludes such asuse rayon::prelude::*;and root-of-module re-exports such aspub use submodule::*;in alib.rs.
Doc comments (///,//!) document the contract: preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body.
Do not restate at call sites what the callee's doc comment already says. If///on the function says 'no-op when …', the caller should not repeat that. Update the doc once; let every call site benefit.
Tests are documentation. Do not duplicate them in prose. If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test, do not also narrate it in the doc comment on the implementation. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the why if it is not obvious.
Use// SAFETY:,// TODO:, and similar prefixes to signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
When editing existing code, do not break a method chain (includingpipe-trait.pipe(...)chains) into intermediateletbindings unless you can justify the rewrite. Valid justifications include a chain that fails to compile after your...
Files:
pacquet/crates/hooks/src/node_runtime.rs
🧠 Learnings (3)
📚 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/node_runtime.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/node_runtime.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/node_runtime.rs
🔇 Additional comments (2)
pacquet/crates/hooks/Cargo.toml (1)
1-15: LGTM!pacquet/crates/hooks/src/node_runtime.rs (1)
81-127: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
pacquet/crates/hooks/src/node_runtime.rs (3)
58-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
kill_on_drop(true)to prevent orphaned Node processes on timeout.When the timeout fires, the
output()future is dropped but Tokio does not kill the spawned process by default. The Node child can continue running indefinitely.🔪 Proposed fix
- let output = timeout(HOOK_TIMEOUT, Command::new("node").arg("-e").arg(&wrapper).output()) + let output = timeout( + HOOK_TIMEOUT, + Command::new("node") + .arg("-e") + .arg(&wrapper) + .kill_on_drop(true) + .output(), + ) .await .map_err(|_| HookError::Timeout(func.to_string()))? .map_err(|e| HookError::ExecutionFailed(e.to_string()))?;🤖 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/node_runtime.rs` around lines 58 - 61, The Node child can become orphaned because the future returned by Command::output() is dropped on timeout; modify the Command used in the timeout call to set kill_on_drop(true) before calling output() so the child is killed when the future is dropped. Locate the expression using timeout(HOOK_TIMEOUT, Command::new("node").arg("-e").arg(&wrapper).output()) and call kill_on_drop(true) on the tokio::process::Command (the same chain that builds the Node invocation) so the Timeout branch that maps to HookError::Timeout(func.to_string()) won’t leave a running Node process.
38-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAsync pnpmfile hooks are not awaited before serialization.
Both
.mjsand.cjswrappers call the hook synchronously (const res = ...) withoutawait. If the hook is async (valid per pnpm docs),resis a Promise that serializes to{}or is parsed asnull, silently ignoring the hook's actual return value.🤖 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/node_runtime.rs` around lines 38 - 56, The wrapper currently captures hook results synchronously so async hooks get serialized as unresolved Promises; update both branches so the call to hooks.hooks['{func}'] is awaited (use await (hooks.hooks && hooks.hooks['{func}'])?.({payload})), ensure the surrounding code is inside an async context (the .mjs branch already uses an async IIFE but the .cjs branch should be converted to an async IIFE like the .mjs one) and keep JSON.stringify on the awaited result; adjust the code that builds wrapper (the file_path.ends_with(".mjs") conditional and the wrapper variable) accordingly.
129-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
pre_resolutionalways usesrequire(), breaking.mjspnpmfiles.The wrapper hardcodes
require({file_path_escaped})but thecall_nodemethod (lines 38-56) branches on.mjsto useimport(). If the user's pnpmfile ispnpmfile.mjs, this hook will fail with an ES module error.🐛 Proposed fix: branch on `.mjs` like `call_node` does
+ let wrapper = if file_path.ends_with(".mjs") { + format!( + r#"(async () => {{ + const hooks = await import({file_path_escaped}); + const ctx = {ctx_payload}; + const logger = {{ + info: (m) => {{ console.log(JSON.stringify({{"level":"info","message":m}})); }}, + warn: (m) => {{ console.log(JSON.stringify({{"level":"warn","message":m}})); }} + }}; + await hooks.hooks?.['preResolution']?.(ctx, logger); +}})(); +"#, + file_path_escaped = file_path_escaped, + ctx_payload = ctx_payload + ) + } else { - let wrapper = format!( + format!( r#"const hooks = require({file_path_escaped}); const ctx = {ctx_payload}; const logger = {{ info: (m) => {{ console.log(JSON.stringify({{"level":"info","message":m}})); }}, warn: (m) => {{ console.log(JSON.stringify({{"level":"warn","message":m}})); }} }}; hooks.hooks && hooks.hooks['preResolution']?.(ctx, logger); "#, - file_path_escaped = file_path_escaped, - ctx_payload = ctx_payload - ); + file_path_escaped = file_path_escaped, + ctx_payload = ctx_payload + ) + };🤖 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/node_runtime.rs` around lines 129 - 140, The wrapper for invoking preResolution always uses require(...) which breaks ES module pnpmfiles; update the wrapper generation in node_runtime.rs to branch on the file extension like call_node does (check for ".mjs") and use dynamic import(...) for ES modules and require(...) for CommonJS, then call hooks.hooks['preResolution']?.(ctx, logger) (or the equivalent invocation) with the same ctx_payload and logger wiring so both .mjs and .js pnpmfiles are supported.
🤖 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/node_runtime.rs`:
- Around line 142-148: The timeout call returns Result<Result<Output,
io::Error>, Elapsed>, so update the handling around
HOOK_TIMEOUT/Command::new("node")/wrapper to unwrap both layers: match the outer
timeout result and then the inner Result<Output, io::Error> (e.g.,
Ok(Ok(output)) case) before accessing output.status or output.stderr; handle
Ok(Err(e)) to log the spawn/IO error and Err(_) to handle timeout, and in the
Ok(Ok(output)) branch keep the existing non-panic logging of
String::from_utf8_lossy(&output.stderr) when !output.status.success().
In `@pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs`:
- Around line 598-630: The code block that constructs and calls the
pnpmfile_hook (pnpmfile_hook, PreResolutionHookContext, hook.pre_resolution) is
misformatted and failing cargo fmt; run rustfmt (cargo fmt) and reformat this
block so it matches project style (fix indentation, spacing, and line breaks
around the wanted_lockfile/current_lockfile/registries/context construction and
the hook.pre_resolution call), then re-run cargo fmt to ensure no remaining
differences before committing.
- Around line 610-621: The field exists_current_lockfile is using
wanted_lockfile.is_some() which is wrong; change it to reflect the actual
current lockfile presence (use the variable that holds the loaded virtual-store
lockfile, e.g. current_lockfile or current_lockfile_json.is_some() as
appropriate) so that
pacquet_hooks::PreResolutionHookContext.exists_current_lockfile correctly
reports the current lockfile rather than the wanted_lockfile; update the
assignment where the context is constructed (the exists_current_lockfile field)
to check the correct variable.
---
Duplicate comments:
In `@pacquet/crates/hooks/src/node_runtime.rs`:
- Around line 58-61: The Node child can become orphaned because the future
returned by Command::output() is dropped on timeout; modify the Command used in
the timeout call to set kill_on_drop(true) before calling output() so the child
is killed when the future is dropped. Locate the expression using
timeout(HOOK_TIMEOUT, Command::new("node").arg("-e").arg(&wrapper).output()) and
call kill_on_drop(true) on the tokio::process::Command (the same chain that
builds the Node invocation) so the Timeout branch that maps to
HookError::Timeout(func.to_string()) won’t leave a running Node process.
- Around line 38-56: The wrapper currently captures hook results synchronously
so async hooks get serialized as unresolved Promises; update both branches so
the call to hooks.hooks['{func}'] is awaited (use await (hooks.hooks &&
hooks.hooks['{func}'])?.({payload})), ensure the surrounding code is inside an
async context (the .mjs branch already uses an async IIFE but the .cjs branch
should be converted to an async IIFE like the .mjs one) and keep JSON.stringify
on the awaited result; adjust the code that builds wrapper (the
file_path.ends_with(".mjs") conditional and the wrapper variable) accordingly.
- Around line 129-140: The wrapper for invoking preResolution always uses
require(...) which breaks ES module pnpmfiles; update the wrapper generation in
node_runtime.rs to branch on the file extension like call_node does (check for
".mjs") and use dynamic import(...) for ES modules and require(...) for
CommonJS, then call hooks.hooks['preResolution']?.(ctx, logger) (or the
equivalent invocation) with the same ctx_payload and logger wiring so both .mjs
and .js pnpmfiles are supported.
🪄 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: 093719fb-ce06-4298-9aad-3ac98b788134
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
pacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/hooks/tests/node_hooks.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- pacquet/crates/hooks/tests/node_hooks.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). (1)
- GitHub Check: windows-latest / Node.js 22.13.0 / Test
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Warnings are errors (--deny warningsin lint). Do not silence them with#[allow(...)]unless there is a specific, justified reason.
Choose owned vs. borrowed parameters to minimize copies; widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming: https://rust-lang.github.io/api-guidelines/naming.html
No star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;, and the same for any other glob whose target is a module you control. Two forms stay allowed: external-crate preludes such asuse rayon::prelude::*;and root-of-module re-exports such aspub use submodule::*;in alib.rs.
Doc comments (///,//!) document the contract: preconditions, postconditions, panics, the reason the function exists. They are not a re-narration of the body.
Do not restate at call sites what the callee's doc comment already says. If///on the function says 'no-op when …', the caller should not repeat that. Update the doc once; let every call site benefit.
Tests are documentation. Do not duplicate them in prose. If a behavioral scenario, edge case, failure mode, or worked example is already captured by a test, do not also narrate it in the doc comment on the implementation. The same applies in reverse: a test's own doc comment should not re-explain what the asserts already say, only the why if it is not obvious.
Use// SAFETY:,// TODO:, and similar prefixes to signal hidden invariants or known follow-ups that a reader cannot recover from the code alone.
When editing existing code, do not break a method chain (includingpipe-trait.pipe(...)chains) into intermediateletbindings unless you can justify the rewrite. Valid justifications include a chain that fails to compile after your...
Files:
pacquet/crates/hooks/src/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🧠 Learnings (3)
📚 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/lib.rspacquet/crates/hooks/src/node_runtime.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/lib.rspacquet/crates/hooks/src/node_runtime.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/lib.rspacquet/crates/hooks/src/node_runtime.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
🪛 GitHub Actions: Pacquet CI / 0_Format.txt
pacquet/crates/hooks/src/node_runtime.rs
[error] 105-105: cargo fmt --all -- --check failed: formatting differences detected (async fn pre_resolution parameter formatting).
[error] 115-115: cargo fmt --all -- --check failed: formatting differences detected (whitespace/blank line change).
[error] 139-139: cargo fmt --all -- -- --check failed: formatting differences detected (timeout(...) and Command::new(...) call formatting/line breaks).
[error] 158-158: cargo fmt --all -- --check failed: formatting differences detected (trailing whitespace/blank line).
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
[error] 665-665: cargo fmt --all -- --check failed: formatting differences detected (whitespace/blank line change).
[error] 672-672: cargo fmt --all -- --check failed: formatting differences detected (current_lockfile_json expression line breaks/indentation).
[error] 698-698: cargo fmt --all -- --check failed: formatting differences detected (whitespace/blank line change).
🪛 GitHub Actions: Pacquet CI / 1_Lint and Test (ubuntu-latest).txt
pacquet/crates/hooks/src/lib.rs
[error] 1-1: Command failed: cargo clippy --locked --workspace --all-targets -- -D warnings (exit code 101). could not compile pacquet-hooks (lib) due to 2 previous errors.
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: clippy (via cargo) failed to compile: error[E0609]: no field status on type std::result::Result<std::process::Output, std::io::Error>. Suggestion: use output.unwrap().status.success().
[error] 144-144: clippy (via cargo) failed to compile: error[E0609]: no field stderr on type std::result::Result<std::process::Output, std::io::Error>. Suggestion: use output.unwrap().stderr.
[error] 1-1: Command failed: cargo clippy --locked --workspace --all-targets -- -D warnings (exit code 101). could not compile pacquet-hooks (lib) due to 2 previous errors.
🪛 GitHub Actions: Pacquet CI / 2_Lint and Test (macos-latest).txt
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: cargo clippy (with -D warnings) failed to compile: error[E0609] no field status on type std::result::Result<std::process::Output, std::io::Error>.
[error] 144-144: cargo clippy (with -D warnings) failed to compile: error[E0609] no field stderr on type std::result::Result<std::process::Output, std::io::Error>.
[error] 143-144: Build failed for crate pacquet-hooks due to 2 previous compilation errors.
🪛 GitHub Actions: Pacquet CI / 7_Doc.txt
pacquet/crates/hooks/src/lib.rs
[error] 1-1: cargo doc --no-deps --workspace --document-private-items: could not compile pacquet-hooks due to 2 previous errors (exit code 101).
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: cargo doc (RUSTDOCFLAGS: -D warnings) failed to compile pacquet-hooks: error[E0609]: no field status on type Result<Output, std::io::Error>. Suggested fix: use output.unwrap().status.success().
[error] 144-144: cargo doc failed to compile pacquet-hooks: error[E0609]: no field stderr on type Result<Output, std::io::Error>. Suggested fix: use output.unwrap().stderr.
[error] 1-1: cargo doc --no-deps --workspace --document-private-items: could not compile pacquet-hooks due to 2 previous errors (exit code 101).
🪛 GitHub Actions: Pacquet CI / Doc
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: Rust compile error E0609: no field status on type Result<Output, std::io::Error>. Suggested fix: use output.unwrap().status.success().
[error] 144-144: Rust compile error E0609: no field stderr on type Result<Output, std::io::Error>. Suggested fix: use output.unwrap().stderr.
🪛 GitHub Actions: Pacquet CI / Dylint
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: Rust compilation error E0609: no field status on type std::result::Result<std::process::Output, std::io::Error>. Help: use output.unwrap().status.success().
[error] 144-144: Rust compilation error E0609: no field stderr on type std::result::Result<std::process::Output, std::io::Error>. Help: use output.unwrap().stderr.
🪛 GitHub Actions: Pacquet CI / Format
pacquet/crates/hooks/src/node_runtime.rs
[error] 105-115: rustfmt check failed: async fn pre_resolution signature formatting (line breaks/indentation) differs.
[error] 115-139: rustfmt check failed: whitespace/newline differences detected around ctx_json / file_path handling.
[error] 139-158: rustfmt check failed: timeout(Command::new(...).output()).await formatting differs (line breaks).
pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
[error] 665-672: rustfmt check failed: whitespace/newline differences detected before calling pre-resolution hook.
[error] 672-698: rustfmt check failed: current_lockfile_json assignment formatting differs (line breaks/indentation).
[error] 698-699: rustfmt check failed: whitespace/newline differences detected after hook call / before workspace opts.
🪛 GitHub Actions: Pacquet CI / Lint and Test (macos-latest)
pacquet/crates/hooks/src/lib.rs
[error] 1-1: Command failed: cargo clippy --locked --workspace --all-targets -- -D warnings. Compilation failed for crate pacquet-hooks due to 2 previous errors (exit code 101).
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: clippy (rustc) failed: E0609 no field status on type std::result::Result<std::process::Output, std::io::Error>. Use output.unwrap().status.success() (or handle the Result).
[error] 144-144: clippy (rustc) failed: E0609 no field stderr on type std::result::Result<std::process::Output, std::io::Error>. Use output.unwrap().stderr (or handle the Result).
[error] 1-1: Command failed: cargo clippy --locked --workspace --all-targets -- -D warnings. Compilation failed for crate pacquet-hooks due to 2 previous errors (exit code 101).
🪛 GitHub Actions: Pacquet CI / Lint and Test (ubuntu-latest)
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: Rust compile error (E0609): no field status on type std::result::Result<std::process::Output, std::io::Error>. Command/step: cargo clippy --locked --workspace --all-targets -- -D warnings.
[error] 144-144: Rust compile error (E0609): no field stderr on type std::result::Result<std::process::Output, std::io::Error>. Command/step: cargo clippy --locked --workspace --all-targets -- -D warnings.
🪛 GitHub Actions: Pacquet Code Coverage / Code Coverage
pacquet/crates/hooks/src/lib.rs
[error] 1-1: Build failed: could not compile pacquet-hooks due to 2 previous errors.
pacquet/crates/hooks/src/node_runtime.rs
[error] 143-143: Rust compilation failed: E0609 no field status on type Result<Output, std::io::Error>. Suggestion: use output.unwrap().status.success().
[error] 144-144: Rust compilation failed: E0609 no field stderr on type Result<Output, std::io::Error>. Suggestion: use output.unwrap().stderr.
[error] 1-1: Build failed: could not compile pacquet-hooks due to 2 previous errors.
🔇 Additional comments (1)
pacquet/crates/hooks/src/lib.rs (1)
16-31: LGTM!Also applies to: 42-43, 60-62
c2c6ae4 to
ede2317
Compare
ede2317 to
a46b8f9
Compare
3b70f48 to
e3414c0
Compare
pnpm's requireHooks recognizes only .pnpmfile.mjs and .pnpmfile.cjs (preferring .mjs); the non-dotted pnpmfile.cjs/pnpmfile.mjs candidates were behavior pnpm does not have.
`await hooks.hooks && hooks.hooks[func]?.(...)` parses as `(await hooks.hooks) && (...)`, so an async preResolution hook was never awaited. Group the operands so the hook call itself is awaited, matching the call_node path.
Every other pacquet crate is 0.0.1; pacquet-hooks was the lone 0.1.0.
The rebase carried over cargo update artifacts (chacha20, system-config, etc.); regenerate from main so the lockfile only adds pacquet-hooks.
pnpm/setup already installs the standalone pnpm, which can provision Node itself; drop the redundant actions/setup-node step in favor of `pnpm runtime set node 22 --global`.
…pnpm
Mirror pnpm's requirePnpmfile contract for the readPackage hook: default the
four dependency fields to {} before the hook runs, validate that the hook
returns a manifest object whose dependency fields are objects (not arrays),
and propagate a HookError (PNPMFILE_FAIL) so a throwing/syntactically-invalid
pnpmfile or a hook that returns nothing aborts the install instead of being
silently swallowed.
Ports the pnpm readPackage tests: validation/normalization unit tests in the
hooks crate, and install-level tests (pin a transitive dep, fail when the hook
returns nothing, fail on a pnpmfile syntax error).
Run the afterAllResolved pnpmfile hook on the resolved lockfile before writing pnpm-lock.yaml, matching pnpm. The lockfile is round-tripped through serde_json::Value so hook-added keys the typed Lockfile cannot represent survive to disk; serde_json's preserve_order keeps the output byte-identical to the typed write when no hook is present (the round-trip only runs when an afterAllResolved hook exists). A throwing hook aborts the install. Ports the pnpm afterAllResolved tests (hook mutates the written lockfile; a throwing hook fails the install).
…eneric name) Add the trailing comma the multi-line readPackage format! invocation needs and rename the single-letter generic on save_value_to_path.
Replace the fresh node -e process per hook call with one persistent Node worker per pnpmfile, addressed by a newline-delimited JSON protocol multiplexed by request id so concurrent readPackage calls (resolution runs in parallel) share one process. This removes the per-package node startup cost on the resolution hot path and forwards each context.log(...) back to the call's HookContext log callback, the basis for the pnpm:hook reporter channel. readPackage/afterAllResolved/filterLog now go through the worker; preResolution keeps its one-shot invocation (it runs once per install and needs an info/warn logger). Adds concurrency and log-forwarding tests.
e9fd840 to
a589b47
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/pacquet-ci.yml (2)
85-85: ⚡ Quick winConsider parameterizing the Node.js version.
The Node.js version "22" is hardcoded. Consider using a variable or matrix parameter to make version updates easier in the future. For example:
env: NODE_VERSION: 22Then reference it as:
run: pnpm runtime set node ${{ env.NODE_VERSION }} --globalThis makes the version easier to update and more visible at the workflow level.
🤖 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 @.github/workflows/pacquet-ci.yml at line 85, The workflow hardcodes the Node.js version in the step that runs "pnpm runtime set node 22 --global"; change this to use a parameter by adding an env variable (e.g., NODE_VERSION) at the workflow or job level and reference that variable in the run command (replace the literal "22" with the env variable like ${{ env.NODE_VERSION }}), so updates to Node version are made in one place and optionally expose it for use in a matrix.
84-86: pnpm runtime setup step is using correct syntax across the CI OS matrix
pnpm runtime set node 22 --globalmatches pnpm’s documented syntax for installing/activating a Node runtime globally (--global==-g), and pnpm runtime is intended to work on Windows/macOS/Linux.- Optional: pin the
pnpm/setuppnpm CLI version (to ensure pnpm v11+ runtime support) and/or parameterize the Node version instead of hardcoding22.🤖 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 @.github/workflows/pacquet-ci.yml around lines 84 - 86, Update the CI workflow step that currently runs "pnpm runtime set node 22 --global" to ensure cross-OS compatibility by using the documented pnpm runtime syntax (keep the --global/-g flag) and make the Node version configurable (replace hardcoded "22" with a parameter/variable like NODE_VERSION); also consider pinning the pnpm/setup action to a minimum version that supports runtime (e.g., v11+) so the runtime command is supported across the matrix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/pacquet-ci.yml:
- Line 85: The workflow hardcodes the Node.js version in the step that runs
"pnpm runtime set node 22 --global"; change this to use a parameter by adding an
env variable (e.g., NODE_VERSION) at the workflow or job level and reference
that variable in the run command (replace the literal "22" with the env variable
like ${{ env.NODE_VERSION }}), so updates to Node version are made in one place
and optionally expose it for use in a matrix.
- Around line 84-86: Update the CI workflow step that currently runs "pnpm
runtime set node 22 --global" to ensure cross-OS compatibility by using the
documented pnpm runtime syntax (keep the --global/-g flag) and make the Node
version configurable (replace hardcoded "22" with a parameter/variable like
NODE_VERSION); also consider pinning the pnpm/setup action to a minimum version
that supports runtime (e.g., v11+) so the runtime command is supported across
the matrix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9631bddc-a4cd-40cc-a9e7-cd97579955c9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.changeset/add-pnpmfile-hooks.md.github/workflows/pacquet-ci.ymlCargo.toml
✅ Files skipped from review due to trivial changes (1)
- .changeset/add-pnpmfile-hooks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
📜 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). (2)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
🔇 Additional comments (2)
.github/workflows/pacquet-ci.yml (2)
4-7: LGTM!Also applies to: 19-19, 38-38
151-151: LGTM!
Implements Tier 4 pnpmfile hooks for pacquet (#11633, point 4.1): pacquet now discovers and runs a project
.pnpmfileduring dependency management, matching pnpm.What it does
.pnpmfile.mjsthen.pnpmfile.cjs(dotted names only,.mjspreferred), matching pnpm'srequireHooks. Only actual files are accepted (is_file()).readPackage— wired into resolution. Mirrors pnpm'srequirePnpmfilecontract: the four dependency fields are defaulted to{}before the hook runs, and the returned manifest is validated (must be a non-null object whose dependency fields, when present, are objects rather than arrays). A throwing/syntactically-invalid pnpmfile, a missingrequire, or a hook that returns nothing aborts the install (PNPMFILE_FAIL) instead of being silently ignored.afterAllResolved— wired into the lockfile write. The resolved lockfile is passed to the hook and its return value is what gets written topnpm-lock.yaml. The round-trip goes throughserde_json::Value(the workspace already enablespreserve_order) so hook-added keys the typedLockfilecannot represent survive to disk; the round-trip only runs when a hook is present, so unmodified installs write byte-identical lockfiles. A throwing hook aborts the install.preResolution— wired. Receives the resolution context (wanted/current lockfile,existsCurrentLockfile,existsNonEmptyWantedLockfile, lockfile dir, store dir, registries) over stdin.filterLog— implemented in the bridge but not yet routed through the reporter (pacquet's reporter is a stateless synchronous emitter); deferred, see follow-ups.How hooks run
Hooks are served by a long-lived Node.js worker, spawned lazily once per pnpmfile. Requests and responses are newline-delimited JSON over the worker's stdin/stdout, multiplexed by a monotonic request id so the concurrent
readPackagecalls the resolver makes (it resolves dependencies in parallel) share one process. This removes the per-packagenodestartup cost on the resolution hot path and avoids interpolating payloads into anode -eargument (noE2BIGrisk for large lockfiles). Eachcontext.log(...)a hook emits is forwarded back to the call's log callback.preResolutionkeeps a one-shotnodeinvocation since it runs once per install and needs aninfo/warnlogger.Tests
context.logforwarding.readPackagehook pins a transitive dependency version; a hook that returns nothing aborts the install; a pnpmfile syntax error aborts the install; anafterAllResolvedhook's mutation is written topnpm-lock.yaml; a throwingafterAllResolvedaborts the install.Scope
The remaining pnpmfile-hook surface pnpm has but pacquet does not yet implement — wiring
filterLogand thepnpm:hooklog channel into the reporter, the--pnpmfile/--global-pnpmfile/--ignore-pnpmfileflags, pnpmfile checksum invalidation,updateConfig, and finders/resolvers/fetchers — is tracked in #12118.Changeset:
pacquetminor.