Skip to content

fix(pacquet): align GVS slot layout with pnpm#11689

Merged
zkochan merged 10 commits into
mainfrom
gvs-match
May 16, 2026
Merged

fix(pacquet): align GVS slot layout with pnpm#11689
zkochan merged 10 commits into
mainfrom
gvs-match

Conversation

@zkochan

@zkochan zkochan commented May 16, 2026

Copy link
Copy Markdown
Member

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 + bins/linker's POWER_SHELL_IS_SUPPORTED = IS_WINDOWS 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 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

  • 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 updates
  • same_global_virtual_store_layout_pure_js — pacquet & pnpm produce byte-identical <store>/v11/links/ trees for @pnpm.e2e/hello-world-js-bin-parent
  • 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
  • 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 GVS engine field should resolve per-snapshot for deps with engines.runtime pins #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.

Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

  • Bug Fixes

    • Side-effects cache keys and global-virtual-store hashing now follow project/runtime Node version (may cause one-time slot churn on upgrade).
    • Global virtual store paths use the v11 layout.
  • New Features

    • Bin linking now includes a package’s own binaries.
  • Behavior Changes

    • Shim generation now emits Windows (.cmd/.ps1) only on Windows and canonical shell shims on Unix.
  • Tests

    • Added integration and unit tests validating GVS parity and runtime-sensitive hashing.
  • Documentation

    • Config docs updated to reference v11-style store paths.

Copilot AI review requested due to automatic review settings May 16, 2026 15:55
@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

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

Changes

Global Virtual Store v11 Parity and Script-Runner Node Hashing

Layer / File(s) Summary
Engine runtime helpers and tests
engine/runtime/system-node-version/src/index.ts, engine/runtime/system-node-version/test/*, .changeset/gvs-engine-name-shell-node.md
Adds engineName() and findRuntimeNodeVersion(); tests validate behavior and changeset documents one-time churn scenarios.
deps/graph-hasher API updates
deps/graph-hasher/src/index.ts, deps/graph-hasher/package.json, deps/graph-hasher/test/*
Removes static ENGINE_NAME, imports engineName(), and extends calcDepState / iterateHashedGraphNodes / calcGraphNodeHash to accept and forward an optional nodeVersion.
Graph-builder: virtual-store hashing
deps/graph-builder/src/iteratePkgsForVirtualStore.ts
Resolves nodeVersion once per invocation and passes it into hashDependencyPaths/iterateHashedGraphNodes so GVS hashes include pinned runtime when enableGlobalVirtualStore is used.
Build/install plumbing (thread nodeVersion into cache keys)
building/after-install/src/index.ts, building/during-install/src/index.ts, installing/*/src/*
Install/build/restorer/installer modules compute the runtime Node version once per invocation and thread nodeVersion into calcDepState and graph hashing, anchoring side-effects cache keys and GVS hashes to the script-runner runtime when available.
GVS v11 layout and frozen-lockfile
pacquet/crates/store-dir/src/store_dir.rs, pacquet/crates/config/src/lib.rs, pacquet/crates/package-manager/src/install/tests.rs, pacquet/crates/package-manager/src/install_frozen_lockfile.rs
StoreDir::links() / ::projects() return <store_root>/v11/links and <store_root>/v11/projects; docs/tests updated; frozen-lockfile install prefers lockfile-pinned runtime major for engine_name used in GVS and cache anchoring.
Slot self-bin linking
pacquet/crates/package-manager/src/link_bins.rs
Per-slot bin linker computes self_has_bin and conditionally includes the slot’s own package as a bin source (from manifests or by reading package.json) so self-bins are linked alongside child-package bins.
Platform-specific shim generation & tests
pacquet/crates/cmd-shim/src/link_bins.rs, pacquet/crates/cmd-shim/src/link_bins/tests.rs
write_shim emits .cmd/.ps1 only on Windows and checks their contents for idempotency; tests updated to assert host-matching shim outputs and Windows recovery paths.
pnpm→pacquet GVS parity integration tests
pacquet/crates/cli/tests/pnpm_compatibility.rs
Adds orchestration tests that run pnpm to populate v11/links, wipe store/node_modules (keep lockfile), run pacquet --frozen-lockfile, and assert identical v11/links slot paths across scenarios (pure-JS, approved-postinstall, diamond).
Package deps & tsconfig refs
*/package.json, */tsconfig.json
Adds @pnpm/engine.runtime.system-node-version workspace dependency to many packages and updates TypeScript project references to include the engine/runtime project.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Poem

🐰 I hopped through locks and hashes bright,
Found the runner Node and set it right,
v11 paths tucked neatly in a row,
Self-bins linked where binaries grow,
Pnpm and pacquet hop in sync — let's go!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: aligning GVS (global virtual store) slot layout in pacquet with pnpm's structure.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gvs-match

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

zkochan added 3 commits May 16, 2026 17:56
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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 lift

Clean up stale Windows shim siblings on Unix reinstalls.

On Unix, windows_shims is None, so windows_ok is always true and already_correct only checks the canonical shim. That means an install produced by older pacquet versions can keep stale *.cmd / *.ps1 files 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

📥 Commits

Reviewing files that changed from the base of the PR and between c178d13 and b13ec21.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (13)
  • .changeset/gvs-engine-name-shell-node.md
  • deps/graph-hasher/package.json
  • deps/graph-hasher/src/index.ts
  • deps/graph-hasher/test/calcGraphNodeHash.test.ts
  • deps/graph-hasher/test/index.ts
  • engine/runtime/system-node-version/src/index.ts
  • pacquet/crates/cli/tests/pnpm_compatibility.rs
  • pacquet/crates/cmd-shim/src/link_bins.rs
  • pacquet/crates/cmd-shim/src/link_bins/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/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.ts
  • deps/graph-hasher/src/index.ts
  • deps/graph-hasher/test/index.ts
  • deps/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 of use super::*; and the same for any other glob. Exception: external-crate preludes like use rayon::prelude::*; and root-of-module re-exports like pub use submodule::*; in lib.rs

Files:

  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/store-dir/src/store_dir.rs
  • pacquet/crates/cmd-shim/src/link_bins.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/cmd-shim/src/link_bins/tests.rs
  • pacquet/crates/cli/tests/pnpm_compatibility.rs
  • pacquet/crates/package-manager/src/link_bins.rs
**/*.test.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not use instanceof Error for error type checking in Jest tests. Use util.types.isNativeError() instead, as Jest runs tests in a VM context where instanceof checks 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 under crates/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.ts
  • deps/graph-hasher/src/index.ts
  • deps/graph-hasher/test/index.ts
  • deps/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

Comment thread pacquet/crates/cli/tests/pnpm_compatibility.rs Outdated
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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

Inline comments:
In `@pacquet/crates/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

📥 Commits

Reviewing files that changed from the base of the PR and between b13ec21 and 0c432e1.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (13)
  • .changeset/gvs-engine-name-shell-node.md
  • deps/graph-hasher/package.json
  • deps/graph-hasher/src/index.ts
  • deps/graph-hasher/test/calcGraphNodeHash.test.ts
  • deps/graph-hasher/test/index.ts
  • engine/runtime/system-node-version/src/index.ts
  • pacquet/crates/cli/tests/pnpm_compatibility.rs
  • pacquet/crates/cmd-shim/src/link_bins.rs
  • pacquet/crates/cmd-shim/src/link_bins/tests.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/link_bins.rs
  • pacquet/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 of use super::*; and the same for any other glob. Exception: external-crate preludes like use rayon::prelude::*; and root-of-module re-exports like pub use submodule::*; in lib.rs

Files:

  • pacquet/crates/cmd-shim/src/link_bins.rs

Comment on lines +442 to +468
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 }
})?;
}

@coderabbitai coderabbitai Bot May 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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, new RemoveShim error variant, cleanup of .cmd/.ps1 siblings when windows_shims is None),
  • and a backlink to this PR and comment.

@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      7.6±0.18ms   569.9 KB/sec    1.00      7.6±0.25ms   568.7 KB/sec

@github-actions

github-actions Bot commented May 16, 2026

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.868 ± 0.237 2.562 3.279 1.05 ± 0.09
pacquet@main 2.734 ± 0.089 2.618 2.860 1.00
pnpm 4.721 ± 0.210 4.401 5.059 1.73 ± 0.10
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 949.4 ± 90.4 894.3 1196.5 1.00
pacquet@main 1168.3 ± 259.9 911.6 1601.0 1.23 ± 0.30
pnpm 3272.9 ± 1289.0 2163.3 5559.9 3.45 ± 1.40
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.
Copilot AI review requested due to automatic review settings May 16, 2026 17:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

coderabbitai[bot]
coderabbitai Bot previously requested changes May 16, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
deps/graph-hasher/src/index.ts (1)

105-121: 💤 Low value

Consider grouping positional parameters into an options object.

iterateHashedGraphNodes now 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0c432e1 and b59c5df.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (18)
  • .changeset/gvs-engine-name-shell-node.md
  • building/after-install/package.json
  • building/after-install/src/index.ts
  • building/during-install/package.json
  • building/during-install/src/index.ts
  • deps/graph-builder/package.json
  • deps/graph-builder/src/iteratePkgsForVirtualStore.ts
  • deps/graph-hasher/src/index.ts
  • engine/runtime/system-node-version/src/index.ts
  • engine/runtime/system-node-version/test/getSystemNodeVersion.test.ts
  • installing/deps-installer/package.json
  • installing/deps-installer/src/install/link.ts
  • installing/deps-resolver/package.json
  • installing/deps-resolver/src/index.ts
  • installing/deps-restorer/package.json
  • installing/deps-restorer/src/index.ts
  • installing/deps-restorer/src/linkHoistedModules.ts
  • pacquet/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.ts
  • installing/deps-installer/src/install/link.ts
  • installing/deps-resolver/src/index.ts
  • installing/deps-restorer/src/linkHoistedModules.ts
  • engine/runtime/system-node-version/test/getSystemNodeVersion.test.ts
  • installing/deps-restorer/src/index.ts
  • building/during-install/src/index.ts
  • engine/runtime/system-node-version/src/index.ts
  • building/after-install/src/index.ts
  • deps/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 of use super::*; and the same for any other glob. Exception: external-crate preludes like use rayon::prelude::*; and root-of-module re-exports like pub use submodule::*; in lib.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 Error for error type checking in Jest tests. Use util.types.isNativeError() instead, as Jest runs tests in a VM context where instanceof checks 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.json
  • installing/deps-resolver/package.json
  • building/during-install/package.json
  • installing/deps-restorer/package.json
  • deps/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.ts
  • installing/deps-installer/src/install/link.ts
  • installing/deps-resolver/src/index.ts
  • installing/deps-restorer/src/linkHoistedModules.ts
  • engine/runtime/system-node-version/test/getSystemNodeVersion.test.ts
  • installing/deps-restorer/src/index.ts
  • building/during-install/src/index.ts
  • engine/runtime/system-node-version/src/index.ts
  • building/after-install/src/index.ts
  • deps/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

Comment thread installing/deps-restorer/src/index.ts Outdated
Comment thread installing/deps-restorer/src/linkHoistedModules.ts Outdated
zkochan added 2 commits May 16, 2026 20:48
* **`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.
Copilot AI review requested due to automatic review settings May 16, 2026 18:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@codecov-commenter

codecov-commenter commented May 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.82051% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.82%. Comparing base (29a42ef) to head (331fe4d).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pacquet/crates/cmd-shim/src/link_bins.rs 29.16% 17 Missing ⚠️
...tes/package-manager/src/install_frozen_lockfile.rs 83.33% 6 Missing ⚠️
pacquet/crates/package-manager/src/link_bins.rs 62.50% 6 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

zkochan commented May 16, 2026

Copy link
Copy Markdown
Member Author

Filed #11690 as a follow-up for the per-snapshot engine resolution case raised in review — when a dependency declares its own engines.runtime pin, the resolver desugars it into a regular dep at installing/deps-resolver/src/resolveDependencies.ts:1477-1479 and the deps half of the GVS hash captures it, but the engine field still uses the install-wide value. This PR intentionally leaves that asymmetry untouched (pnpm has it today, so pacquet matches pnpm); #11690 outlines the per-snapshot refactor needed to close it on both sides.


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.
Copilot AI review requested due to automatic review settings May 16, 2026 19:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@zkochan

zkochan commented May 16, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 16, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
deps/graph-builder/src/iteratePkgsForVirtualStore.ts (1)

42-42: ⚡ Quick win

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

♻️ 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
+  )
 }
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."

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

📥 Commits

Reviewing files that changed from the base of the PR and between bb3247a and a6b192a.

📒 Files selected for processing (12)
  • building/after-install/src/index.ts
  • building/after-install/tsconfig.json
  • building/during-install/tsconfig.json
  • deps/graph-builder/src/iteratePkgsForVirtualStore.ts
  • deps/graph-builder/tsconfig.json
  • deps/graph-hasher/tsconfig.json
  • exec/commands/test/dlx.e2e.ts
  • installing/deps-installer/tsconfig.json
  • installing/deps-resolver/tsconfig.json
  • installing/deps-restorer/src/index.ts
  • installing/deps-restorer/tsconfig.json
  • pacquet/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.ts
  • installing/deps-restorer/src/index.ts
  • deps/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.ts
  • installing/deps-restorer/src/index.ts
  • deps/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.
@zkochan zkochan dismissed coderabbitai[bot]’s stale review May 16, 2026 21:41

all comments resolved

@zkochan zkochan merged commit 3ddde2b into main May 16, 2026
28 checks passed
@zkochan zkochan deleted the gvs-match branch May 16, 2026 21:58
zkochan added a commit that referenced this pull request May 17, 2026
…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`.
zkochan added a commit that referenced this pull request May 17, 2026
… 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.
github-actions Bot pushed a commit to Eyalm321/pnpm that referenced this pull request May 18, 2026
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants