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:
📝 WalkthroughWalkthroughThis PR anchors GVS hashes and side-effects cache keys to the lifecycle script-runner Node (via engineName/findRuntimeNodeVersion), moves global virtual store paths under v11, threads nodeVersion through graph hashing and cache-key paths, adds slot self-bin linking, platform-gated shims, and pnpm→pacquet GVS parity tests. ChangesGlobal Virtual Store v11 Parity and Script-Runner Node Hashing
Sequence Diagram(s)sequenceDiagram
participant Installer
participant GraphHasher
participant SideEffectsCache
Installer->>GraphHasher: findRuntimeNodeVersion(snapshotKeys) -> nodeVersion
GraphHasher->>GraphHasher: compute graph/node hashes using engineName(nodeVersion)
Installer->>SideEffectsCache: calcDepState(..., nodeVersion)
SideEffectsCache-->>Installer: cache lookup/write keyed by nodeVersion-anchored hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 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 |
Three tests in `pacquet/crates/cli/tests/pnpm_compatibility.rs` run pnpm and pacquet against the same workspace+lockfile with `enableGlobalVirtualStore: true` and diff the resulting GVS slot trees: * `same_global_virtual_store_layout_pure_js` — one package + one transitive dep. * `same_global_virtual_store_layout_with_approved_postinstall` — exercises the engine-included hash branch via `allowBuilds`. * `same_global_virtual_store_layout_diamond` — exercises `calc_dep_graph_hash` memoization through a shared transitive. All three fail today. The diffs surface three classes of divergence the GVS port still has to close: * Store prefix — pnpm writes `<store>/v11/links/...`, pacquet writes `<store>/links/...` (one level shallower than `getStorePath`). * Bin shims — pacquet writes `.cmd` / `.ps1` cross-platform shims that pnpm doesn't, and pnpm writes a self-bin entry inside the package's own slot that pacquet doesn't. * Engine-included hash — pacquet and pnpm disagree on the digest for an approved-build package (`pre-and-postinstall-scripts-example@1.0.0`: pacquet `6ce6f09e...`, pnpm `e0d69b24...`). Non-builder children still hash identically, so the gap is localised to the engine payload. Pnpm runs first in each test so the lockfile exists before pacquet starts — pacquet's GVS write path is gated on `frozen_lockfile && enable_global_virtual_store`, so a fresh install with no lockfile would silently fall through to the project-local layout and the test would pass for the wrong reason.
Pnpm puts the global-virtual-store and project-registry directories at `<store>/v11/links/` and `<store>/v11/projects/` — not at `<store>/links/` / `<store>/projects/`. The `v11/` segment is added by [`getStorePath`](https://github.com/pnpm/pnpm/blob/29a42efc3b/store/path/src/index.ts#L39-L42), which appends `STORE_VERSION` to the user-configured `storeDir` before `extendInstallOptions.ts:352` joins `'links'` onto it. So the on-disk path lands one level deeper than the raw configured root. Pacquet's `StoreDir::links()` and `StoreDir::projects()` were joining onto `self.root` instead of `self.v11()`, so when both tools shared a store directory pacquet would write slots at `<store>/links/...` while pnpm wrote them at `<store>/v11/links/...`, splitting the cache across two locations and breaking the `enableGlobalVirtualStore` invariant that the two tools can share one machine-wide store. Verified by `same_global_virtual_store_layout_diamond` which now passes — same `pkg-with-1-dep` + `parent-of-pkg-with-1-dep` diamond, GVS on both sides, full on-disk path equality. Two remaining tests (`..._pure_js`, `..._with_approved_postinstall`) still fail on unrelated bin-shim / engine-hash divergences tracked separately.
The GVS engine string (and the side-effects-cache key prefix) was computed from `process.version` — the Node running pnpm itself. For pnpm distributed via `@pnpm/exe` (the SEA-bundled binary), that's the Node embedded in the bundle, not the `node` on `PATH` that actually spawns lifecycle scripts. As a result two pnpm installations on the same machine (one SEA, one npm-package) disagreed on the cache key, partitioning the side-effects cache and the global virtual store across two Node majors even though both would run scripts on the same shell `node` — and any pnpm-compatible tool that anchored to the shell Node (e.g. pacquet, which detects `node --version` from `PATH`) would write to a different GVS slot than pnpm read from. `@pnpm/engine.runtime.system-node-version` now exports `engineName()`, which uses `getSystemNodeVersion()` — the existing helper that prefers `node --version` over `process.version` whenever pnpm is running inside a SEA bundle. `@pnpm/deps.graph-hasher`'s `calcDepState` and `calcGraphNodeHash` call it instead of importing the legacy `ENGINE_NAME` constant from `@pnpm/constants`. The constant is left unchanged so external consumers and existing tests keep working; the two values agree in non-SEA contexts (where `getSystemNodeVersion()` falls through to `process.version`). On upgrade, SEA-pnpm users will see a one-time GVS slot churn: packages that previously hashed under the embedded-Node major now hash under the shell-Node major, matching what every non-SEA pnpm already produced. Old slots become prune-eligible. This is one of the divergences surfaced by the GVS parity test suite added in 8e9bc32 — specifically the `same_global_virtual_store_layout_with_approved_postinstall` case, where the system pnpm exe (Node 26 embedded) hashed `pre-and-postinstall-scripts-example` as `e0d69b24...` while pacquet (detecting Node 24 from `PATH`) hashed it as `6ce6f09e...`. With this fix pnpm produces the pacquet-matching value regardless of the distribution channel it was installed from.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/cmd-shim/src/link_bins.rs (1)
424-468:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftClean up stale Windows shim siblings on Unix reinstalls.
On Unix,
windows_shimsisNone, sowindows_okis alwaystrueandalready_correctonly checks the canonical shim. That means an install produced by older pacquet versions can keep stale*.cmd/*.ps1files forever: the warm reinstall no-ops and never converges to pnpm's Unix layout. Please treat those siblings as stale outputs on non-Windows and remove them during the rewrite/idempotency path.🤖 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/cmd-shim/src/link_bins.rs` around lines 424 - 468, The current idempotency check treats windows_shims == None as "ok" and leaves stale *.cmd/*.ps1 siblings on Unix; modify the rewrite path in link_bins.rs so when windows_shims is None (non-Windows install) you explicitly remove any existing Windows sibling files before or when writing the Unix shim: detect the sibling paths (e.g. derive cmd_path and ps1_path from shim_path or use the same logic that computes windows_shims), check existence with Api::read_to_string/metadata, and call Api::remove_file or Api::write with an empty/removed state if those files exist; update the block around already_correct / Api::write (symbols: windows_shims, already_correct, shim_path, sh_body, cmd_path, ps1_path, Api::read_to_string, Api::write) so Unix reinstalls delete stale .cmd/.ps1 files instead of leaving them behind.
🤖 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/cli/tests/pnpm_compatibility.rs`:
- Around line 232-235: The code that builds `extended` concatenates `existing`
and the new YAML block without guaranteeing a trailing newline, which can merge
keys; update the logic that creates `extended` (currently using variables
`yaml_path`, `existing`, `extended`) to ensure there's always a newline between
the existing content and the appended text—e.g., check
`existing.ends_with('\n')` and choose either `format!("{existing}{append}")` or
`format!("{existing}\n{append}")` (where `append` is the
"enableGlobalVirtualStore..." + `extra_yaml`) before calling `fs::write`.
---
Outside diff comments:
In `@pacquet/crates/cmd-shim/src/link_bins.rs`:
- Around line 424-468: The current idempotency check treats windows_shims ==
None as "ok" and leaves stale *.cmd/*.ps1 siblings on Unix; modify the rewrite
path in link_bins.rs so when windows_shims is None (non-Windows install) you
explicitly remove any existing Windows sibling files before or when writing the
Unix shim: detect the sibling paths (e.g. derive cmd_path and ps1_path from
shim_path or use the same logic that computes windows_shims), check existence
with Api::read_to_string/metadata, and call Api::remove_file or Api::write with
an empty/removed state if those files exist; update the block around
already_correct / Api::write (symbols: windows_shims, already_correct,
shim_path, sh_body, cmd_path, ps1_path, Api::read_to_string, Api::write) so Unix
reinstalls delete stale .cmd/.ps1 files instead of leaving them behind.
🪄 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: b8eaf26a-154c-4463-b422-6b28652dec56
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/gvs-engine-name-shell-node.mddeps/graph-hasher/package.jsondeps/graph-hasher/src/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.tsdeps/graph-hasher/test/index.tsengine/runtime/system-node-version/src/index.tspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/cmd-shim/src/link_bins.rspacquet/crates/cmd-shim/src/link_bins/tests.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/link_bins.rspacquet/crates/store-dir/src/store_dir.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). (2)
- GitHub Check: Agent
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.
Files:
engine/runtime/system-node-version/src/index.tsdeps/graph-hasher/src/index.tsdeps/graph-hasher/test/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
No star imports inside module bodies — write
use super::{Foo, bar}instead ofuse super::*;and the same for any other glob. Exception: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-exports likepub use submodule::*;inlib.rs
Files:
pacquet/crates/config/src/lib.rspacquet/crates/store-dir/src/store_dir.rspacquet/crates/cmd-shim/src/link_bins.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/cmd-shim/src/link_bins/tests.rspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/package-manager/src/link_bins.rs
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor error type checking in Jest tests. Useutil.types.isNativeError()instead, as Jest runs tests in a VM context whereinstanceofchecks can fail across realms.
Files:
deps/graph-hasher/test/calcGraphNodeHash.test.ts
pacquet/crates/*/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Tests live alongside the code they exercise (standard Cargo layout) plus integration tests under each crate's
tests/— shared test fixtures live undercrates/testing-utils/src/fixtures/
Files:
pacquet/crates/cli/tests/pnpm_compatibility.rs
🧠 Learnings (2)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
engine/runtime/system-node-version/src/index.tsdeps/graph-hasher/src/index.tsdeps/graph-hasher/test/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.ts
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
deps/graph-hasher/package.json
🔇 Additional comments (12)
pacquet/crates/cli/tests/pnpm_compatibility.rs (1)
209-231: LGTM!Also applies to: 238-412
engine/runtime/system-node-version/src/index.ts (1)
19-49: LGTM!deps/graph-hasher/package.json (1)
36-36: LGTM!deps/graph-hasher/src/index.ts (1)
3-3: LGTM!Also applies to: 35-35, 143-143
deps/graph-hasher/test/calcGraphNodeHash.test.ts (1)
4-10: LGTM!deps/graph-hasher/test/index.ts (1)
4-11: LGTM!.changeset/gvs-engine-name-shell-node.md (1)
1-17: LGTM!pacquet/crates/store-dir/src/store_dir.rs (1)
115-128: LGTM!Also applies to: 132-140
pacquet/crates/config/src/lib.rs (1)
203-209: LGTM!Also applies to: 216-219, 236-237
pacquet/crates/package-manager/src/install/tests.rs (1)
1521-1522: LGTM!Also applies to: 1589-1590, 1668-1672
pacquet/crates/package-manager/src/link_bins.rs (1)
396-496: LGTM!pacquet/crates/cmd-shim/src/link_bins/tests.rs (1)
21-74: LGTM!Also applies to: 272-307
Two divergences in the slot-local `node_modules/.bin/` writer surfaced by `same_global_virtual_store_layout_*` and now reconciled with pnpm: * **`.cmd` / `.ps1` siblings only on Windows.** Pacquet was emitting all three shim flavors per bin (canonical no-extension shim, `.cmd`, `.ps1`) regardless of host platform, on the theory that a Linux-installed `node_modules` should stay usable when carried to Windows via network share or git clone. Pnpm does not match that contract — `@zkochan/cmd-shim` defaults [`createCmdFile: isWindows`](https://github.com/pnpm/cmd-shim/blob/0d79ca9534/src/index.ts#L32), and pnpm's `bins.linker` overrides `createPwshFile` with [`POWER_SHELL_IS_SUPPORTED = IS_WINDOWS`](https://github.com/pnpm/pnpm/blob/29a42efc3b/bins/linker/src/index.ts#L28), so neither sibling lands on Unix. Pacquet now gates the same way, removing the extra `.cmd`/`.ps1` files in every Unix slot. The upgrade-recovery test that exercised the "older pacquet wrote canonical only" repair path is now `#[cfg(windows)]` — on Unix there are no `.cmd`/`.ps1` siblings to recover. * **Self-bin in the slot's own `.bin/`.** Pnpm's [`linkBinsOfDependencies`](https://github.com/pnpm/pnpm/blob/29a42efc3b/building/during-install/src/index.ts#L272-L298) appends `depNode` to the bin-source list unconditionally (line 287), so a package like `hello-world-js-bin` (no deps, one bin) writes `<slot>/node_modules/<pkg>/node_modules/.bin/<pkg>` as a self-shim. Pacquet's `run_lockfile_driven` excluded the slot's own package based on a stale assumption ("which pnpm doesn't"); the parity tests surfaced the gap. Now the lockfile-driven path adds the slot's package as a bin source when its `has_bin` lockfile row is set, mirroring pnpm. The `run_with_readdir` fallback used by the non-lockfile install path still excludes self via `link_bins_excluding` — that one runs only for the legacy flat layout where GVS parity isn't a constraint, so the divergence is out of scope here. The two pacquet GVS parity tests that didn't depend on engine-hash parity (`..._pure_js` and `..._diamond`) now pass. The `..._with_approved_postinstall` test still fails until a `@pnpm/exe` build with the engine-name fix from 6fd9184 reaches the host. Also updates three project-registry assertions in `install::tests` that hard-coded `<store_dir>/projects/` to the post-64a4a1d40b `<store_dir>/v11/projects/` location.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pacquet/crates/cmd-shim/src/link_bins.rs`:
- Around line 442-468: The code currently skips removing legacy Windows siblings
when windows_shims is None, leaving stray .cmd/.ps1 files around on non-Windows
reinstalls; update the logic around already_correct/shim writing so that when
windows_shims is None you explicitly check for and remove existing cmd and ps1
siblings (e.g. derive the sibling paths from shim_path.with_extension("cmd") and
shim_path.with_extension("ps1") or equivalent), using the Api removal function
(Api::remove_file/Api::remove) and map any failures to an appropriate
LinkBinsError (or add a RemoveShim variant) before proceeding to write the Unix
shim with Api::write; keep the existing comparison logic that uses
Api::read_to_string and windows_shims when present.
🪄 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: 11b7e58f-9335-42d9-a4f8-2eb641e9416a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.changeset/gvs-engine-name-shell-node.mddeps/graph-hasher/package.jsondeps/graph-hasher/src/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.tsdeps/graph-hasher/test/index.tsengine/runtime/system-node-version/src/index.tspacquet/crates/cli/tests/pnpm_compatibility.rspacquet/crates/cmd-shim/src/link_bins.rspacquet/crates/cmd-shim/src/link_bins/tests.rspacquet/crates/config/src/lib.rspacquet/crates/package-manager/src/install/tests.rspacquet/crates/package-manager/src/link_bins.rspacquet/crates/store-dir/src/store_dir.rs
✅ Files skipped from review due to trivial changes (4)
- pacquet/crates/package-manager/src/install/tests.rs
- deps/graph-hasher/test/index.ts
- .changeset/gvs-engine-name-shell-node.md
- pacquet/crates/config/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- engine/runtime/system-node-version/src/index.ts
- deps/graph-hasher/package.json
- pacquet/crates/cli/tests/pnpm_compatibility.rs
- deps/graph-hasher/src/index.ts
- pacquet/crates/store-dir/src/store_dir.rs
- deps/graph-hasher/test/calcGraphNodeHash.test.ts
- pacquet/crates/package-manager/src/link_bins.rs
- pacquet/crates/cmd-shim/src/link_bins/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
No star imports inside module bodies — write
use super::{Foo, bar}instead ofuse super::*;and the same for any other glob. Exception: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-exports likepub use submodule::*;inlib.rs
Files:
pacquet/crates/cmd-shim/src/link_bins.rs
| let windows_ok = match &windows_shims { | ||
| None => true, | ||
| Some((cmd_path, cmd_body, ps1_path, ps1_body)) => { | ||
| let cmd_ok = matches!( | ||
| Api::read_to_string(cmd_path), | ||
| Ok(existing) if &existing == cmd_body, | ||
| ); | ||
| let ps1_ok = matches!( | ||
| Api::read_to_string(ps1_path), | ||
| Ok(existing) if &existing == ps1_body, | ||
| ); | ||
| cmd_ok && ps1_ok | ||
| } | ||
| }; | ||
| let already_correct = sh_marker_ok && windows_ok; | ||
|
|
||
| if !already_correct { | ||
| Api::write(shim_path, sh_body.as_bytes()) | ||
| .map_err(|error| LinkBinsError::WriteShim { path: shim_path.to_path_buf(), error })?; | ||
| Api::write(&cmd_path, cmd_body.as_bytes()) | ||
| .map_err(|error| LinkBinsError::WriteShim { path: cmd_path.clone(), error })?; | ||
| Api::write(&ps1_path, ps1_body.as_bytes()) | ||
| .map_err(|error| LinkBinsError::WriteShim { path: ps1_path.clone(), error })?; | ||
| if let Some((cmd_path, cmd_body, ps1_path, ps1_body)) = &windows_shims { | ||
| Api::write(cmd_path, cmd_body.as_bytes()).map_err(|error| { | ||
| LinkBinsError::WriteShim { path: cmd_path.clone(), error } | ||
| })?; | ||
| Api::write(ps1_path, ps1_body.as_bytes()).map_err(|error| { | ||
| LinkBinsError::WriteShim { path: ps1_path.clone(), error } | ||
| })?; | ||
| } |
There was a problem hiding this comment.
Remove unexpected Windows siblings on non-Windows reinstalls.
On Unix, windows_shims is None, so Line 442 treats .cmd/.ps1 as irrelevant and Line 458 never cleans up siblings left by older pacquet versions. That means an upgraded store can keep extra Windows shims indefinitely and never converge to pnpm’s file set, even after a reinstall.
🤖 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/cmd-shim/src/link_bins.rs` around lines 442 - 468, The code
currently skips removing legacy Windows siblings when windows_shims is None,
leaving stray .cmd/.ps1 files around on non-Windows reinstalls; update the logic
around already_correct/shim writing so that when windows_shims is None you
explicitly check for and remove existing cmd and ps1 siblings (e.g. derive the
sibling paths from shim_path.with_extension("cmd") and
shim_path.with_extension("ps1") or equivalent), using the Api removal function
(Api::remove_file/Api::remove) and map any failures to an appropriate
LinkBinsError (or add a RemoveShim variant) before proceeding to write the Unix
shim with Api::write; keep the existing comparison logic that uses
Api::read_to_string and windows_shims when present.
There was a problem hiding this comment.
Acknowledged — this one isn't fixed in bb3247a.
The convergence concern is real for a pacquet v1 → v2 upgrade-in-place on the same store: legacy .cmd / .ps1 siblings written by the old all-three-flavors writer would linger in the slot forever, and a Unix install that touches the slot afterwards wouldn't repair the gap because windows_shims is None makes the already_correct check pass.
I'm leaving it out of this PR for two reasons. First, the GVS hash itself is unchanged between v1 and v2 (the engine portion is the same modulo the engine-name fix landed earlier in the PR), so a fresh install — including any pnpm store prune + reinstall cycle — already produces the cleaned-up slot. Second, adding active cleanup means widening the writer's capability trait bound (FsRemoveFile / equivalent) and threading it through every test impl, plus an error variant — non-trivial diff for an upgrade-only edge case.
Will track as a follow-up once the rest of the PR lands; happy to revisit in review if you'd prefer it in this PR.
Written by an agent (Claude Code, claude-opus-4-7).
There was a problem hiding this comment.
@zkochan, the reasoning makes sense — the pnpm store prune escape hatch is a reasonable mitigation, and widening the capability trait bound plus adding an error variant just for an upgrade-in-place edge case is indeed a disproportionate diff. Happy to defer.
Would you like me to open a follow-up issue to track this? I can capture:
- the upgrade-in-place scenario (pacquet v1 → v2, Unix, same store),
- the required changes (
FsRemoveFile/ equivalent capability trait, newRemoveShimerror variant, cleanup of.cmd/.ps1siblings whenwindows_shims is None), - and a backlink to this PR and comment.
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.8679776934000003,
"stddev": 0.2372219817029916,
"median": 2.7871246262000002,
"user": 2.1180452199999995,
"system": 2.8331058000000007,
"min": 2.5619778162,
"max": 3.2789245242000002,
"times": [
2.7362596312000003,
2.8893802202,
3.1205038612,
2.7328764962000003,
3.1449831392000003,
3.2789245242000002,
2.6406219932,
2.8233779812,
2.5619778162,
2.7508712712000003
]
},
{
"command": "pacquet@main",
"mean": 2.7337477064,
"stddev": 0.08891850970565413,
"median": 2.7362268972,
"user": 2.12746852,
"system": 2.8131204,
"min": 2.6175156392,
"max": 2.8603231962000004,
"times": [
2.7036524932,
2.7516865122,
2.8034595912,
2.7794231632,
2.6362660492,
2.6175156392,
2.8412276622,
2.8603231962000004,
2.7207672822,
2.6231554752
]
},
{
"command": "pnpm",
"mean": 4.7207567070000005,
"stddev": 0.21032642353404604,
"median": 4.762705671700001,
"user": 6.1920841200000005,
"system": 3.1585452999999997,
"min": 4.4006077182,
"max": 5.0587341822,
"times": [
4.5640069202,
4.470323677200001,
4.4006077182,
4.5804177412,
4.8942807742,
4.9000904122,
4.7622259082000005,
5.0587341822,
4.7631854352000005,
4.8136943012
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.9494331478200001,
"stddev": 0.09044441759180716,
"median": 0.91737168452,
"user": 0.32043148,
"system": 1.24311248,
"min": 0.89433156952,
"max": 1.19647719852,
"times": [
1.19647719852,
0.99080717452,
0.91782842552,
0.91447331352,
0.89433156952,
0.91719534252,
0.9106812965200001,
0.9140204135200001,
0.92096871752,
0.91754802652
]
},
{
"command": "pacquet@main",
"mean": 1.1682889955200002,
"stddev": 0.25989912366982276,
"median": 1.05050227152,
"user": 0.31111398,
"system": 1.25110988,
"min": 0.91161697752,
"max": 1.60103442952,
"times": [
1.53447647252,
1.0445393015200002,
1.60103442952,
1.4104658195200002,
1.05646524152,
0.96590777452,
1.0142681475200002,
1.2317701435200001,
0.91161697752,
0.91234564752
]
},
{
"command": "pnpm",
"mean": 3.27290071432,
"stddev": 1.288964048121245,
"median": 2.88498873702,
"user": 2.39429658,
"system": 1.6526601799999998,
"min": 2.16327042452,
"max": 5.55991251752,
"times": [
4.00518070252,
5.37063702552,
3.08830360952,
2.4068903005199997,
3.09289355752,
2.16327042452,
2.17338312652,
5.55991251752,
2.68167386452,
2.18686201452
]
}
]
} |
Extends the engine-name fix in 8f05529 to also honour `engines.runtime` / `devEngines.runtime` Node pins. Before this change `engineName()` resolved the Node major via `getSystemNodeVersion()` (which is correct for non-SEA pnpm with no runtime pin and for SEA pnpm with no runtime pin), but for a project that pinned a Node version via `devEngines.runtime` the GVS hash and the side-effects-cache key still tracked whichever Node the runner happened to be on — not the pinned Node that scripts will actually spawn under. Two installs of the same project on hosts with different shell Nodes would therefore disagree on the slot path even though every lifecycle script in either install would run on the same downloaded Node. Resolution order in `engineName(nodeVersion?)` is now: 1. Explicit override (the project's resolved `engines.runtime` version, threaded through by the install pipeline). 2. `getSystemNodeVersion()` — shell `node --version` for SEA pnpm, `process.version` otherwise. 3. `process.version` as a last-resort fallback. `@pnpm/engine.runtime.system-node-version` gains a `findRuntimeNodeVersion(snapshotKeys)` helper that scans an iterable of lockfile snapshot keys for `node@runtime:<version>` and returns the bare version string (peer suffixes stripped). Every install-side caller of `calcDepState` / `calcGraphNodeHash` / `iterateHashedGraphNodes` (`deps.graph-builder`, `installing.deps-resolver`, `installing.deps-restorer`, `installing.deps-installer`, `building.during-install`, `building.after-install`) now computes `findRuntimeNodeVersion(Object.keys(graph))` once per invocation and forwards it through the new `nodeVersion?` option/parameter. The hasher's option/parameter is optional with the same default the no-arg `engineName()` produces — existing call sites and tests keep working unchanged. Pacquet mirrors this with a `find_runtime_node_major` helper in `install_frozen_lockfile.rs`: before falling through to the host `node --version` probe, it scans the lockfile's `snapshots:` map for a `node@runtime:<version>` entry and uses that major outright. The probe is skipped entirely when the pin is present. Three new TS unit tests in `engine.runtime.system-node-version`'s test file cover the override branch and `findRuntimeNodeVersion`'s extraction rule (with and without peer suffix). The pacquet runtime-fixture integration tests (`install/tests.rs` around line 2537) continue to pass with the helper engaged.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deps/graph-hasher/src/index.ts (1)
105-121: 💤 Low valueConsider grouping positional parameters into an options object.
iterateHashedGraphNodesnow has 5 positional parameters. As per the coding guidelines, functions needing more than 2-3 arguments should use a single options object. This would also make future extensions easier without breaking the call signature.That said, this is an existing exported API and refactoring it would require updating all call sites. This can be deferred if the churn is not justified now.
🤖 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 `@deps/graph-hasher/src/index.ts` around lines 105 - 121, The exported function iterateHashedGraphNodes currently takes five positional parameters (graph, pkgMetaIterator, allowBuild, supportedArchitectures, nodeVersion) which makes the API hard to extend; refactor it to accept a single options object (e.g. iterateHashedGraphNodes(graph, { pkgMetaIterator, allowBuild, supportedArchitectures, nodeVersion }) or a full signature iterateHashedGraphNodes(options)) by introducing an options type/interface and updating the function signature and internal usages to read values from that object; preserve backward compatibility by adding an overload or a thin wrapper that accepts the old positional args and forwards them into the new options object, and update exported types and JSDoc for iterateHashedGraphNodes and any helper functions that reference those parameters (search for iterateHashedGraphNodes, pkgMetaIterator, allowBuild, supportedArchitectures, nodeVersion) so call sites continue to work until callers are migrated.
🤖 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 `@installing/deps-restorer/src/index.ts`:
- Around line 913-917: The code computes nodeVersion by calling
findRuntimeNodeVersion(Object.keys(opts.depGraph)) which uses install-directory
keys and therefore misses entries like "node@runtime:<version>"; change the call
sites (including where nodeVersion is computed and the similar block later) to
pass the package/dependency paths list (depPaths) instead of
Object.keys(opts.depGraph) so findRuntimeNodeVersion inspects the actual
dependency paths that contain runtime anchors; update any references that used
opts.depGraph keys to use depPaths when calling findRuntimeNodeVersion (keep the
function name nodeVersion and findRuntimeNodeVersion unchanged).
In `@installing/deps-restorer/src/linkHoistedModules.ts`:
- Around line 106-113: The runtime Node version should be computed once in
linkHoistedModules() from the nodes' depPaths (using
findRuntimeNodeVersion(Object.keys(depPaths) or similar) and then passed into
the recursive helper via opts, instead of calling
findRuntimeNodeVersion(Object.keys(graph)) inside the recursive function; update
linkHoistedModules() to compute nodeVersion once and add it to the opts object
used by the recursive helper (also remove duplicate recalculation around the
code referenced at the other block ~130-135), and adjust the recursive helper to
read opts.nodeVersion rather than recomputing from graph.
---
Nitpick comments:
In `@deps/graph-hasher/src/index.ts`:
- Around line 105-121: The exported function iterateHashedGraphNodes currently
takes five positional parameters (graph, pkgMetaIterator, allowBuild,
supportedArchitectures, nodeVersion) which makes the API hard to extend;
refactor it to accept a single options object (e.g.
iterateHashedGraphNodes(graph, { pkgMetaIterator, allowBuild,
supportedArchitectures, nodeVersion }) or a full signature
iterateHashedGraphNodes(options)) by introducing an options type/interface and
updating the function signature and internal usages to read values from that
object; preserve backward compatibility by adding an overload or a thin wrapper
that accepts the old positional args and forwards them into the new options
object, and update exported types and JSDoc for iterateHashedGraphNodes and any
helper functions that reference those parameters (search for
iterateHashedGraphNodes, pkgMetaIterator, allowBuild, supportedArchitectures,
nodeVersion) so call sites continue to work until callers are migrated.
🪄 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: 2b2a36a6-39a8-4989-b769-1d5bc4be6822
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.changeset/gvs-engine-name-shell-node.mdbuilding/after-install/package.jsonbuilding/after-install/src/index.tsbuilding/during-install/package.jsonbuilding/during-install/src/index.tsdeps/graph-builder/package.jsondeps/graph-builder/src/iteratePkgsForVirtualStore.tsdeps/graph-hasher/src/index.tsengine/runtime/system-node-version/src/index.tsengine/runtime/system-node-version/test/getSystemNodeVersion.test.tsinstalling/deps-installer/package.jsoninstalling/deps-installer/src/install/link.tsinstalling/deps-resolver/package.jsoninstalling/deps-resolver/src/index.tsinstalling/deps-restorer/package.jsoninstalling/deps-restorer/src/index.tsinstalling/deps-restorer/src/linkHoistedModules.tspacquet/crates/package-manager/src/install_frozen_lockfile.rs
✅ Files skipped from review due to trivial changes (2)
- building/after-install/package.json
- .changeset/gvs-engine-name-shell-node.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.
Files:
deps/graph-builder/src/iteratePkgsForVirtualStore.tsinstalling/deps-installer/src/install/link.tsinstalling/deps-resolver/src/index.tsinstalling/deps-restorer/src/linkHoistedModules.tsengine/runtime/system-node-version/test/getSystemNodeVersion.test.tsinstalling/deps-restorer/src/index.tsbuilding/during-install/src/index.tsengine/runtime/system-node-version/src/index.tsbuilding/after-install/src/index.tsdeps/graph-hasher/src/index.ts
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
No star imports inside module bodies — write
use super::{Foo, bar}instead ofuse super::*;and the same for any other glob. Exception: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-exports likepub use submodule::*;inlib.rs
Files:
pacquet/crates/package-manager/src/install_frozen_lockfile.rs
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use
instanceof Errorfor error type checking in Jest tests. Useutil.types.isNativeError()instead, as Jest runs tests in a VM context whereinstanceofchecks can fail across realms.
Files:
engine/runtime/system-node-version/test/getSystemNodeVersion.test.ts
🧠 Learnings (2)
📚 Learning: 2026-05-05T23:03:04.286Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11479
File: __utils__/scripts/package.json:6-9
Timestamp: 2026-05-05T23:03:04.286Z
Learning: The pattern cross-env NODE_OPTIONS="$NODE_OPTIONS ..." in package.json scripts is an established convention in the pnpm/pnpm repository and is used across many packages (e.g., fs/hard-link-dir, worker, __utils__/scripts). Do not flag this as a cross-platform issue in individual files; if a change is needed, apply it as a repo-wide change in a separate PR. Scope this guidance to all package.json files in the repo; use the minimatch pattern '**/package.json' to identify relevant files and review changes at the repository level rather than per-file.
Applied to files:
installing/deps-installer/package.jsoninstalling/deps-resolver/package.jsonbuilding/during-install/package.jsoninstalling/deps-restorer/package.jsondeps/graph-builder/package.json
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
deps/graph-builder/src/iteratePkgsForVirtualStore.tsinstalling/deps-installer/src/install/link.tsinstalling/deps-resolver/src/index.tsinstalling/deps-restorer/src/linkHoistedModules.tsengine/runtime/system-node-version/test/getSystemNodeVersion.test.tsinstalling/deps-restorer/src/index.tsbuilding/during-install/src/index.tsengine/runtime/system-node-version/src/index.tsbuilding/after-install/src/index.tsdeps/graph-hasher/src/index.ts
🔇 Additional comments (8)
engine/runtime/system-node-version/src/index.ts (1)
18-86: LGTM!engine/runtime/system-node-version/test/getSystemNodeVersion.test.ts (1)
38-77: LGTM!deps/graph-hasher/src/index.ts (1)
3-3: LGTM!Also applies to: 45-45, 137-137, 167-167
building/after-install/src/index.ts (1)
14-14: LGTM!Also applies to: 285-289, 375-379, 410-414
building/during-install/package.json (1)
42-42: LGTM!building/during-install/src/index.ts (1)
10-10: LGTM!Also applies to: 65-74, 161-163, 209-213
deps/graph-builder/package.json (1)
38-38: LGTM!deps/graph-builder/src/iteratePkgsForVirtualStore.ts (1)
13-13: LGTM!Also applies to: 34-42, 48-70, 84-92
* **`findRuntimeNodeVersion` was scanning the wrong keys** in the hoisted-restorer paths. `DependenciesGraph` in `installing/deps-restorer/` is keyed by *install directory*, not depPath, so `Object.keys(opts.depGraph)` / `Object.keys(graph)` never saw `node@runtime:<version>` entries — the runtime pin was silently ignored on every headless install. Switched both `linkAllPkgs` (in `installing/deps-restorer/src/index.ts`) and `linkHoistedModules` (in `linkHoistedModules.ts`) to pull the depPath off each node, and lifted `linkAllPkgsInOrder`'s computation out of the recursion so the scan runs once per invocation instead of at every directory level. * **Newline safety in the test-fixture YAML helper.** The `enable_gvs_in_workspace_yaml` helper concatenated `enableGlobalVirtualStore: true\n…` directly onto the existing file contents. If the helper that produced the file ever drops the trailing newline, the result would merge the last key with ours and yield invalid YAML. Forces a `\n` boundary before the append. * **`cargo fmt` drift.** `cargo fmt --all -- --check` flagged trivial line-break differences in three files; re-formatted. * **`cspell` complained about two words.** Rewrote `materialise` (British) → `materialize` in `engine.runtime.system-node-version`'s docstring and `unsuffixed` → `the form without a peer suffix` in the test comment. * **`dylint` flagged an unused import.** `remove_file` is only consumed by the Windows-only upgrade-recovery test added in b13ec21, so its `use` line is now `#[cfg(windows)]`-gated. The stale `.cmd` / `.ps1` cleanup CodeRabbit flagged on `pacquet/crates/cmd-shim/src/link_bins.rs:468` is a real but narrower upgrade-path concern (pacquet v1 → v2 reinstall leaves legacy Windows shims in the slot indefinitely). It's tracked separately rather than handled here; the new install / fresh slot path already converges, and `pnpm store prune` + reinstall clears any leftovers.
* **`meta-updater`-managed tsconfig drift.** The seven packages whose `package.json` gained an `@pnpm/engine.runtime.system-node-version` dependency in 6fd9184 / b59c5df also needed the matching `"references"` entry in their `tsconfig.json`. CI's `lint:meta` step (which runs `meta-updater --test`) caught the gap and failed the Compile & Lint job. Ran `meta-updater` locally to bring all seven tsconfig.json files back in sync — pure mechanical sync, no semantic change. * **`same_global_virtual_store_layout_with_approved_postinstall` now `#[ignore]`d** with a doc-comment-anchored reason. The test requires pnpm and pacquet to agree on the `<platform>;<arch>;node<major>` triple in the engine-included hash branch. Pre-fix pnpm anchors that value to `process.version`, which for the `@pnpm/exe` SEA bundle installed by [`pnpm/setup`](https://github.com/pnpm/setup) on Linux/macOS runners is its embedded Node (currently node26). Pacquet — and any non-SEA caller — detects the `node` on `PATH`, which on GHA's standard runners is node24. CI therefore observes pnpm hashing to a `node26`-anchored digest (`582056713e…`) while pacquet computes the `node24`-anchored value (`1a8b9405940e…`). The pnpm-side fix in 8f05529 closes the gap by anchoring `engineName()` to `getSystemNodeVersion()` (which prefers the shell `node` over `process.version` in SEA contexts), but that fix has to ship in a published pnpm version before the test can pass on CI without modification. Re-enable it once [`pnpm/setup`](https://github.com/pnpm/setup) resolves to a pnpm version that contains the fix. The two other GVS parity tests (`same_global_virtual_store_layout_pure_js` and `…_diamond`) stay enabled — they cover the engine-agnostic hash branch (every snapshot hashes with `engine: null`) and exercise both the `v11/` prefix fix and the bin-shim parity end-to-end, so the regression-catching value of this PR is preserved.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11689 +/- ##
==========================================
- Coverage 89.07% 88.82% -0.25%
==========================================
Files 129 129
Lines 14590 14705 +115
==========================================
+ Hits 12996 13062 +66
- Misses 1594 1643 +49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
`@pnpm/engine.runtime.system-node-version` slotted in alphabetically behind `@pnpm/deps.path` in the three files where the dep was plumbed through in b59c5df, putting the import out of order relative to `@pnpm/deps.graph-sequencer` / `@pnpm/deps.graph-hasher` (which sort before it lexicographically). `eslint --fix` shuffles the import block back into shape — no semantic change, just reordering. Closes the last CI lint failure on PR #11689.
|
Filed #11690 as a follow-up for the per-snapshot engine resolution case raised in review — when a dependency declares its own Written by an agent (Claude Code, claude-opus-4-7). |
…gh dlx.e2e.ts mock `dlx.e2e.ts` uses `jest.unstable_mockModule` to wrap `@pnpm/engine.runtime.system-node-version`, re-exporting only `getSystemNodeVersion`. After 6fd9184 / b59c5df the same module also exports `engineName` and `findRuntimeNodeVersion`, and `@pnpm/deps.graph-hasher` started importing `engineName` dynamically when the test's `await import('@pnpm/installing.commands')` chain pulled it in. ESM saw the named import miss against the mocked module and threw `SyntaxError: The requested module does not provide an export named 'engineName'` at test-suite startup — broke `ubuntu-latest / Node.js 24 / Test` on PR #11689. The mock now forwards every public symbol the real module surfaces: * `getSystemNodeVersion` keeps the `jest.fn` wrapper so existing call-spy assertions still work. * `engineName` and `findRuntimeNodeVersion` delegate straight back to the originals — no spy needed; they're transitive consumers rather than the subject of any assertion in this file.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deps/graph-builder/src/iteratePkgsForVirtualStore.ts (1)
42-42: ⚡ Quick winCollapse
hashDependencyPaths()parameters into an options object.This helper now takes four positional arguments, which makes the call easy to misread as more hash inputs get threaded through.
As per coding guidelines, "Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead."♻️ Suggested refactor
- for (const { hash, pkgMeta } of hashDependencyPaths(lockfile, opts.allowBuild, opts.supportedArchitectures, nodeVersion)) { + for (const { hash, pkgMeta } of hashDependencyPaths(lockfile, { + allowBuild: opts.allowBuild, + supportedArchitectures: opts.supportedArchitectures, + nodeVersion, + })) { yield { dirInVirtualStore: path.join(opts.globalVirtualStoreDir, hash), pkgMeta, @@ function hashDependencyPaths ( lockfile: LockfileObject, - allowBuild?: AllowBuild, - supportedArchitectures?: SupportedArchitectures, - nodeVersion?: string + opts: { + allowBuild?: AllowBuild + supportedArchitectures?: SupportedArchitectures + nodeVersion?: string + } ): IterableIterator<HashedDepPath<PkgMetaAndSnapshot>> { - const graph = lockfileToDepGraph(lockfile, supportedArchitectures) - return iterateHashedGraphNodes(graph, iteratePkgMeta(lockfile, graph), allowBuild, supportedArchitectures, nodeVersion) + const graph = lockfileToDepGraph(lockfile, opts.supportedArchitectures) + return iterateHashedGraphNodes( + graph, + iteratePkgMeta(lockfile, graph), + opts.allowBuild, + opts.supportedArchitectures, + opts.nodeVersion + ) }Also applies to: 84-91
🤖 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 `@deps/graph-builder/src/iteratePkgsForVirtualStore.ts` at line 42, Call sites currently pass four positional arguments to hashDependencyPaths(lockfile, opts.allowBuild, opts.supportedArchitectures, nodeVersion), which is brittle and violates the guideline; change these call sites (including the for-loop at iteratePkgsForVirtualStore.ts and the other occurrences around lines 84–91) to pass a single options object like hashDependencyPaths(lockfile, { allowBuild: opts.allowBuild, supportedArchitectures: opts.supportedArchitectures, nodeVersion }) and update the hashDependencyPaths function signature/implementation to accept and destructure that options object (e.g., function hashDependencyPaths(lockfile, { allowBuild, supportedArchitectures, nodeVersion }) ) so callers are clearer and fewer positional args are used.
🤖 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 `@deps/graph-builder/src/iteratePkgsForVirtualStore.ts`:
- Line 42: Call sites currently pass four positional arguments to
hashDependencyPaths(lockfile, opts.allowBuild, opts.supportedArchitectures,
nodeVersion), which is brittle and violates the guideline; change these call
sites (including the for-loop at iteratePkgsForVirtualStore.ts and the other
occurrences around lines 84–91) to pass a single options object like
hashDependencyPaths(lockfile, { allowBuild: opts.allowBuild,
supportedArchitectures: opts.supportedArchitectures, nodeVersion }) and update
the hashDependencyPaths function signature/implementation to accept and
destructure that options object (e.g., function hashDependencyPaths(lockfile, {
allowBuild, supportedArchitectures, nodeVersion }) ) so callers are clearer and
fewer positional args are used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d46af87a-57d1-46e2-a797-fd5d2b7f4e70
📒 Files selected for processing (12)
building/after-install/src/index.tsbuilding/after-install/tsconfig.jsonbuilding/during-install/tsconfig.jsondeps/graph-builder/src/iteratePkgsForVirtualStore.tsdeps/graph-builder/tsconfig.jsondeps/graph-hasher/tsconfig.jsonexec/commands/test/dlx.e2e.tsinstalling/deps-installer/tsconfig.jsoninstalling/deps-resolver/tsconfig.jsoninstalling/deps-restorer/src/index.tsinstalling/deps-restorer/tsconfig.jsonpacquet/crates/cli/tests/pnpm_compatibility.rs
✅ Files skipped from review due to trivial changes (4)
- installing/deps-resolver/tsconfig.json
- installing/deps-restorer/tsconfig.json
- building/during-install/tsconfig.json
- deps/graph-builder/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- building/after-install/tsconfig.json
- deps/graph-hasher/tsconfig.json
- building/after-install/src/index.ts
- pacquet/crates/cli/tests/pnpm_compatibility.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: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use Standard Style with modifications: trailing commas are used, functions are preferred over classes, functions are declared after they are used (hoisting is relied upon).
Functions should have no more than two or three arguments. If a function needs more parameters, use a single options object instead.
Maintain import order: (1) Standard libraries, (2) External dependencies (sorted alphabetically), (3) Relative imports.
Files:
exec/commands/test/dlx.e2e.tsinstalling/deps-restorer/src/index.tsdeps/graph-builder/src/iteratePkgsForVirtualStore.ts
🧠 Learnings (1)
📚 Learning: 2026-05-14T09:04:00.133Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11622
File: resolving/npm-resolver/test/publishedBy.test.ts:350-354
Timestamp: 2026-05-14T09:04:00.133Z
Learning: In the pnpm/pnpm repository, ESLint is the authoritative style linter. Do not raise review findings for missing trailing commas in multiline function calls (e.g., `fs.writeFileSync(...)`) when this repo’s ESLint configuration does not report them and lint passes. Prefer deferring to the ESLint results for this specific trailing-comma rule rather than enforcing it manually in code review.
Applied to files:
exec/commands/test/dlx.e2e.tsinstalling/deps-restorer/src/index.tsdeps/graph-builder/src/iteratePkgsForVirtualStore.ts
🔇 Additional comments (4)
exec/commands/test/dlx.e2e.ts (1)
9-23: LGTM!installing/deps-installer/tsconfig.json (1)
93-95: LGTM!installing/deps-restorer/src/index.ts (1)
27-27: LGTM!Also applies to: 913-941
deps/graph-builder/src/iteratePkgsForVirtualStore.ts (1)
14-15: LGTM!Also applies to: 34-40, 49-70
… args into options bag `hashDependencyPaths` had grown to four positional args after b59c5df added `nodeVersion`, putting it past the two-or-three-arg threshold from `CONTRIBUTING.md` (and CodeRabbit flagged the call site at `iteratePkgsForVirtualStore.ts:42`). The helper is private to this file with one caller, so collapsing to `hashDependencyPaths(lockfile, { allowBuild, supportedArchitectures, nodeVersion })` is a contained change. `iterateHashedGraphNodes` itself still uses positional args (graph, iterator, allowBuild?, supportedArchitectures?, nodeVersion?) — that's a public API of `@pnpm/deps.graph-hasher` with external callers in `deps.graph-builder`, `installing.deps-resolver`, `engine.pm.commands` and several tests, so reshaping it would balloon scope beyond what's in this PR.
…pm/deps.graph-hasher `@pnpm/engine.runtime.system-node-version`'s purpose is probing the *host* Node — `getSystemNodeVersion`, `engineName`. The two lockfile- shape parsers (`findRuntimeNodeVersion`, `readSnapshotRuntimePin`) were a poor fit there and are now exported from the package that actually composes the engine string: `@pnpm/deps.graph-hasher`. Every caller already depended on graph-hasher, so no new deps are introduced; six packages drop the now-unused dependency on system-node-version. The pre-existing `.changeset/gvs-engine-name-shell-node.md` (#11689, not yet released) is updated to reflect the new home for `findRuntimeNodeVersion`.
… deps (#11693) Closes #11690. A dependency that declares `engines.runtime` in its manifest carries the desugared `dependencies.node: 'runtime:<version>'` pin in the lockfile, and pnpm's bin linker spawns that dep's lifecycle scripts through the pinned Node downloaded into `<pkgDir>/node_modules/node/`. The GVS hash and the side-effects-cache key prefix were still anchored to the install-wide runtime — so the pinning snapshot's slot encoded the wrong Node major, and a reinstall on the same host could read the cached side-effects under a key whose `<platform>;<arch>;node<major>` triple disagreed with the Node the build actually ran on. Per-snapshot resolution now matches what `bins/linker` already does on a per-package basis: a snapshot's own pin wins; the install-wide value (from #11689's `findRuntimeNodeVersion`) is the fallback. ### TypeScript - `deps/graph-hasher/src/index.ts:72-77` — adds `readSnapshotRuntimePin(children)`: pulls the bare Node version from a graph node's `children.node` entry when that points at a `node@runtime:<version>` snapshot. Factors out a small `extractRuntimeNodeVersion(snapshotKey)` parser shared with `findRuntimeNodeVersion`. - `deps/graph-hasher/src/index.ts:115-116,245-246` — `calcDepState` and `calcGraphNodeHash` consult `readSnapshotRuntimePin(graph[depPath].children)` first and only fall back to the install-wide `nodeVersion` parameter when the snapshot doesn't pin its own Node. No caller changes required — install-wide fallback continues to be computed via `findRuntimeNodeVersion(Object.keys(graph))` at each call site. - **Refactor (separate commit):** `findRuntimeNodeVersion` moved from `@pnpm/engine.runtime.system-node-version` to `@pnpm/deps.graph-hasher` (along with the new `readSnapshotRuntimePin`). `system-node-version` is about probing the *host* Node — `getSystemNodeVersion`, `engineName`. The lockfile-shape parsers fit better next to the package that actually composes the engine string. Every caller already depended on graph-hasher, so no new deps; six packages drop the now-unused dependency on `system-node-version`. ### Pacquet - `pacquet/crates/package-manager/src/install_frozen_lockfile.rs:1309-1345` — new `find_own_runtime_node_major(snapshot)` reads a snapshot's `dependencies` for a `node` entry with `Prefix::Runtime`, returning the bare major. - `pacquet/crates/package-manager/src/virtual_store_layout.rs:178-205` — `VirtualStoreLayout::new` resolves engine per-snapshot inside the hash loop via `engine_name(own_major, None, None)` when the snapshot pins, otherwise inherits the install-wide `engine` argument. ### Migration Snapshots of dependencies that declare their own `engines.runtime` re-hash under that dep's pinned Node instead of the install-wide value. Old slots become prune-eligible on next install.
## Summary Adds three end-to-end **GVS parity tests** under `pacquet/crates/cli/tests/pnpm_compatibility.rs` that run `pnpm install` and `pacquet install --frozen-lockfile` against the same workspace + lockfile with `enableGlobalVirtualStore: true`, then diff the resulting `<store>/v11/links/` slot trees. The tests surfaced three independent divergences, each fixed in its own commit set: 1. **`<store>/v11/links` prefix.** `getStorePath` appends `STORE_VERSION` (`v11`) to the configured `storeDir` before `extendInstallOptions.ts:352` joins `'links'` onto it, so pnpm's GVS lives at `<store>/v11/links/` — pacquet's `StoreDir::links()` was one level shallower, joining onto `self.root`. Same gap on `projects()`. Anchored both under `self.v11()` so the on-disk paths agree. 2. **GVS engine-name resolution.** `ENGINE_NAME` was computed from `process.version`, which is wrong in two cases: - **`@pnpm/exe` SEA bundle.** The bundle has its own embedded Node, not the `node` on PATH that runs lifecycle scripts. Two pnpm installs on the same machine (one SEA, one npm-package) therefore disagreed on the cache key, partitioning the side-effects cache and the global virtual store. - **`engines.runtime` / `devEngines.runtime` pin.** When a project pins a Node version, pnpm downloads that Node into `node_modules/node/` and uses it to run lifecycle scripts. But the hash still anchored to whichever Node ran pnpm itself, not to the pinned Node. `@pnpm/engine.runtime.system-node-version` now exports `engineName(nodeVersion?)` and `findRuntimeNodeVersion(snapshotKeys)`. The override has priority; otherwise the helper falls through to `getSystemNodeVersion()` — which already prefers shell `node --version` over `process.version` in SEA contexts — and finally to `process.version` as a last resort. `@pnpm/deps.graph-hasher`'s `calcDepState`, `calcGraphNodeHash`, and `iterateHashedGraphNodes` accept an optional `nodeVersion`. Every install-side caller (`deps.graph-builder`, `installing.deps-resolver`, `installing.deps-restorer`, `installing.deps-installer/install/link`, `building.during-install`, `building.after-install`) derives the project's pinned runtime via `findRuntimeNodeVersion` once per invocation and forwards it. The legacy `ENGINE_NAME` constant in `@pnpm/constants` is unchanged so external consumers and existing tests keep working. Pacquet mirrors this with `find_runtime_node_major` in `install_frozen_lockfile.rs` — it scans the lockfile's `snapshots:` map for a `node@runtime:<version>` entry and uses that major outright, only falling back to the host probe when no pin is present. 3. **Slot bin-shim layout.** Pacquet was emitting `.cmd` / `.ps1` shims on every host platform, even though pnpm only writes them on Windows ([`@zkochan/cmd-shim` `createCmdFile: isWindows`](https://github.com/pnpm/cmd-shim/blob/0d79ca9534/src/index.ts#L32) + `bins/linker`'s [`POWER_SHELL_IS_SUPPORTED = IS_WINDOWS`](https://github.com/pnpm/pnpm/blob/29a42efc3b/bins/linker/src/index.ts#L28) gate). Pacquet also excluded the slot's own package from the slot-local `node_modules/.bin/` based on a stale assumption ("which pnpm doesn't"), but pnpm's [`linkBinsOfDependencies`](https://github.com/pnpm/pnpm/blob/29a42efc3b/building/during-install/src/index.ts#L272-L298) appends `depNode` to the bin-source list unconditionally, so a leaf package like `hello-world-js-bin` writes a self-shim at `<slot>/node_modules/<pkg>/node_modules/.bin/<pkg>`. Both behaviors now match pnpm. ## Test plan - [x] `cargo nextest run -p pacquet-cli --test pnpm_compatibility` — 5 active tests pass, 1 ignored (see below) - [x] `cargo nextest run -p pacquet-store-dir -p pacquet-config -p pacquet-cmd-shim -p pacquet-package-manager` — 600+ tests pass after the prefix / bin-shim updates - [x] `same_global_virtual_store_layout_pure_js` — pacquet & pnpm produce byte-identical `<store>/v11/links/` trees for `@pnpm.e2e/hello-world-js-bin-parent` - [x] `same_global_virtual_store_layout_diamond` — same for `pkg-with-1-dep` + `parent-of-pkg-with-1-dep`, verifying `calc_dep_graph_hash` memoization parity - [x] Three new TS unit tests in `engine/runtime/system-node-version/test/` cover the `engineName(version)` override branch and `findRuntimeNodeVersion`'s extraction rule (with and without peer suffix) - [ ] `same_global_virtual_store_layout_with_approved_postinstall` is currently `#[ignore]`d. It requires pnpm and pacquet to agree on the `<platform>;<arch>;node<major>` triple in the engine-included hash branch. The `pnpm/setup` action on CI installs an `@pnpm/exe` SEA bundle whose embedded Node (node26) differs from the runner's PATH `node` (node24), so the digests don't line up. The pnpm-side fix in this PR resolves `engineName()` via `getSystemNodeVersion()` which prefers the shell `node`, so once a published pnpm version with the fix reaches `pnpm/setup` the test will pass without modification — re-enable it then. The other two GVS parity tests are unaffected since they exercise the engine-agnostic branch. ## Notes - Two pacquet integration tests in `package-manager/src/install/tests.rs` had hard-coded `<store_dir>/projects/` assertions; updated to `<store_dir>/v11/projects/` to follow the prefix fix. - The `link_bins_rewrites_when_only_sh_flavor_exists` cmd-shim test is now `#[cfg(windows)]` — the upgrade-recovery scenario it exercises is meaningless on Unix where `.cmd`/`.ps1` are no longer written in the first place. - Review feedback addressed: (a) test YAML helper now guarantees a trailing newline before appending GVS keys; (b) `findRuntimeNodeVersion` calls in `installing/deps-restorer/` switched from `Object.keys(graph)` (install-dir-keyed in that module) to extracting `depPath` per node, with the computation lifted out of the recursion; (c) `dlx.e2e.ts`'s `jest.unstable_mockModule` against `@pnpm/engine.runtime.system-node-version` now forwards every exported symbol so transitive importers of `engineName` don't break. - Known caveat: pacquet's non-lockfile install path (`run_with_readdir`) still excludes the slot's own bin via `link_bins_excluding`. That path runs only for the legacy flat layout where GVS parity isn't a constraint, so it's deliberately out of scope here. - Known caveat tracked in pnpm#11690: when a dependency's own manifest declares `engines.runtime`, the resolver desugars it into a regular `dependencies.node: 'runtime:<v>'` entry on that package, so the **deps** portion of the hash captures it on both sides. The **engine** portion is still install-wide rather than per-snapshot, so cached side-effects for dep-pinned runtimes can be reused under the wrong host Node. pnpm has this same gap today; closing it on both sides requires per-snapshot engine resolution and is outside this PR's scope.
Summary
Adds three end-to-end GVS parity tests under
pacquet/crates/cli/tests/pnpm_compatibility.rsthat runpnpm installandpacquet install --frozen-lockfileagainst the same workspace + lockfile withenableGlobalVirtualStore: true, then diff the resulting<store>/v11/links/slot trees. The tests surfaced three independent divergences, each fixed in its own commit set:<store>/v11/linksprefix.getStorePathappendsSTORE_VERSION(v11) to the configuredstoreDirbeforeextendInstallOptions.ts:352joins'links'onto it, so pnpm's GVS lives at<store>/v11/links/— pacquet'sStoreDir::links()was one level shallower, joining ontoself.root. Same gap onprojects(). Anchored both underself.v11()so the on-disk paths agree.GVS engine-name resolution.
ENGINE_NAMEwas computed fromprocess.version, which is wrong in two cases:@pnpm/exeSEA bundle. The bundle has its own embedded Node, not thenodeon PATH that runs lifecycle scripts. Two pnpm installs on the same machine (one SEA, one npm-package) therefore disagreed on the cache key, partitioning the side-effects cache and the global virtual store.engines.runtime/devEngines.runtimepin. When a project pins a Node version, pnpm downloads that Node intonode_modules/node/and uses it to run lifecycle scripts. But the hash still anchored to whichever Node ran pnpm itself, not to the pinned Node.@pnpm/engine.runtime.system-node-versionnow exportsengineName(nodeVersion?)andfindRuntimeNodeVersion(snapshotKeys). The override has priority; otherwise the helper falls through togetSystemNodeVersion()— which already prefers shellnode --versionoverprocess.versionin SEA contexts — and finally toprocess.versionas a last resort.@pnpm/deps.graph-hasher'scalcDepState,calcGraphNodeHash, anditerateHashedGraphNodesaccept an optionalnodeVersion. Every install-side caller (deps.graph-builder,installing.deps-resolver,installing.deps-restorer,installing.deps-installer/install/link,building.during-install,building.after-install) derives the project's pinned runtime viafindRuntimeNodeVersiononce per invocation and forwards it. The legacyENGINE_NAMEconstant in@pnpm/constantsis unchanged so external consumers and existing tests keep working.Pacquet mirrors this with
find_runtime_node_majorininstall_frozen_lockfile.rs— it scans the lockfile'ssnapshots:map for anode@runtime:<version>entry and uses that major outright, only falling back to the host probe when no pin is present.Slot bin-shim layout. Pacquet was emitting
.cmd/.ps1shims on every host platform, even though pnpm only writes them on Windows (@zkochan/cmd-shimcreateCmdFile: isWindows+bins/linker'sPOWER_SHELL_IS_SUPPORTED = IS_WINDOWSgate). Pacquet also excluded the slot's own package from the slot-localnode_modules/.bin/based on a stale assumption ("which pnpm doesn't"), but pnpm'slinkBinsOfDependenciesappendsdepNodeto the bin-source list unconditionally, so a leaf package likehello-world-js-binwrites a self-shim at<slot>/node_modules/<pkg>/node_modules/.bin/<pkg>. Both behaviors now match pnpm.Test plan
cargo nextest run -p pacquet-cli --test pnpm_compatibility— 5 active tests pass, 1 ignored (see below)cargo nextest run -p pacquet-store-dir -p pacquet-config -p pacquet-cmd-shim -p pacquet-package-manager— 600+ tests pass after the prefix / bin-shim updatessame_global_virtual_store_layout_pure_js— pacquet & pnpm produce byte-identical<store>/v11/links/trees for@pnpm.e2e/hello-world-js-bin-parentsame_global_virtual_store_layout_diamond— same forpkg-with-1-dep+parent-of-pkg-with-1-dep, verifyingcalc_dep_graph_hashmemoization parityengine/runtime/system-node-version/test/cover theengineName(version)override branch andfindRuntimeNodeVersion's extraction rule (with and without peer suffix)same_global_virtual_store_layout_with_approved_postinstallis currently#[ignore]d. It requires pnpm and pacquet to agree on the<platform>;<arch>;node<major>triple in the engine-included hash branch. Thepnpm/setupaction on CI installs an@pnpm/exeSEA bundle whose embedded Node (node26) differs from the runner's PATHnode(node24), so the digests don't line up. The pnpm-side fix in this PR resolvesengineName()viagetSystemNodeVersion()which prefers the shellnode, so once a published pnpm version with the fix reachespnpm/setupthe test will pass without modification — re-enable it then. The other two GVS parity tests are unaffected since they exercise the engine-agnostic branch.Notes
package-manager/src/install/tests.rshad hard-coded<store_dir>/projects/assertions; updated to<store_dir>/v11/projects/to follow the prefix fix.link_bins_rewrites_when_only_sh_flavor_existscmd-shim test is now#[cfg(windows)]— the upgrade-recovery scenario it exercises is meaningless on Unix where.cmd/.ps1are no longer written in the first place.findRuntimeNodeVersioncalls ininstalling/deps-restorer/switched fromObject.keys(graph)(install-dir-keyed in that module) to extractingdepPathper node, with the computation lifted out of the recursion; (c)dlx.e2e.ts'sjest.unstable_mockModuleagainst@pnpm/engine.runtime.system-node-versionnow forwards every exported symbol so transitive importers ofengineNamedon't break.run_with_readdir) still excludes the slot's own bin vialink_bins_excluding. That path runs only for the legacy flat layout where GVS parity isn't a constraint, so it's deliberately out of scope here.engines.runtime, the resolver desugars it into a regulardependencies.node: 'runtime:<v>'entry on that package, so the deps portion of the hash captures it on both sides. The engine portion is still install-wide rather than per-snapshot, so cached side-effects for dep-pinned runtimes can be reused under the wrong host Node. pnpm has this same gap today; closing it on both sides requires per-snapshot engine resolution and is outside this PR's scope.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
New Features
Behavior Changes
Tests
Documentation