Conversation
… deps A dependency that declares `engines.runtime` carries the desugared `dependencies.node: 'runtime:<version>'` pin, and the bin linker spawns that dep's lifecycle scripts through the pinned Node — but the GVS hash and side-effects-cache key prefix still anchored to the install-wide runtime, mis-keying the cache for cross-pinning installs. Per-snapshot resolution now reads each snapshot's own pin from its graph children, falling back to the install-wide value only when absent. Closes #11690.
|
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:
📝 WalkthroughWalkthroughPer-snapshot engine pinning: snapshots that contain a ChangesPer-snapshot engine pinning for GVS hash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 docstrings
🧪 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 |
Contributor
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.changeset/gvs-engine-per-snapshot-runtime-pin.md (2)
9-9: 💤 Low valueConsider rephrasing for clarity.
The phrase "triple disagreed with the Node" is slightly awkward. Consider:
- "triple that disagreed with the Node the build actually ran on"
- "triple mismatched the Node the build actually ran on"
🤖 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 @.changeset/gvs-engine-per-snapshot-runtime-pin.md at line 9, Reword the awkward phrase "triple disagreed with the Node" to a clearer alternative; replace it with one of the suggested phrasings such as "triple that disagreed with the Node the build actually ran on" or "triple mismatched the Node the build actually ran on" in the changelog text so the sentence reads smoothly and unambiguously.
11-11: 💤 Low valueClarify the path reference.
The backticked
`bins/linker`suggests a code reference but isn't a complete package name. Consider either:
- Using the full package name if referring to a specific package (e.g.,
@pnpm/bins/linker)- Removing backticks if describing functionality generically: "the bin linker"
🤖 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 @.changeset/gvs-engine-per-snapshot-runtime-pin.md at line 11, The reference to `bins/linker` is ambiguous; update the text to either use the full package name (for example, `@pnpm/bins/linker`) if you mean that specific module, or remove the backticks and rephrase to a descriptive term like "the bin linker" if you mean the functionality generically; ensure the chosen form replaces `bins/linker` wherever it appears so the reference is clear.
🤖 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 @.changeset/gvs-engine-per-snapshot-runtime-pin.md:
- Line 9: Reword the awkward phrase "triple disagreed with the Node" to a
clearer alternative; replace it with one of the suggested phrasings such as
"triple that disagreed with the Node the build actually ran on" or "triple
mismatched the Node the build actually ran on" in the changelog text so the
sentence reads smoothly and unambiguously.
- Line 11: The reference to `bins/linker` is ambiguous; update the text to
either use the full package name (for example, `@pnpm/bins/linker`) if you mean
that specific module, or remove the backticks and rephrase to a descriptive term
like "the bin linker" if you mean the functionality generically; ensure the
chosen form replaces `bins/linker` wherever it appears so the reference is
clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f103bf90-b5e8-4c48-968f-cd7b313a07f1
📒 Files selected for processing (8)
.changeset/gvs-engine-per-snapshot-runtime-pin.mddeps/graph-hasher/src/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.tsdeps/graph-hasher/test/index.tsengine/runtime/system-node-version/src/index.tsengine/runtime/system-node-version/test/getSystemNodeVersion.test.tspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/virtual_store_layout.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). (8)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-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:
engine/runtime/system-node-version/test/getSystemNodeVersion.test.tsdeps/graph-hasher/test/index.tsengine/runtime/system-node-version/src/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.tsdeps/graph-hasher/src/index.ts
**/*.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.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/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/virtual_store_layout.rs
🧠 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:
engine/runtime/system-node-version/test/getSystemNodeVersion.test.tsdeps/graph-hasher/test/index.tsengine/runtime/system-node-version/src/index.tsdeps/graph-hasher/test/calcGraphNodeHash.test.tsdeps/graph-hasher/src/index.ts
🪛 GitHub Check: Spell Check
pacquet/crates/package-manager/src/install_frozen_lockfile.rs
[warning] 1325-1325:
"mis" should be "miss" or "mist".
pacquet/crates/package-manager/src/virtual_store_layout.rs
[warning] 606-606:
"mis" should be "miss" or "mist".
🔇 Additional comments (8)
.changeset/gvs-engine-per-snapshot-runtime-pin.md (1)
1-18: LGTM!engine/runtime/system-node-version/src/index.ts (1)
56-71: LGTM!Also applies to: 94-95, 100-122
engine/runtime/system-node-version/test/getSystemNodeVersion.test.ts (1)
14-14: LGTM!Also applies to: 79-93
deps/graph-hasher/src/index.ts (1)
3-3: LGTM!Also applies to: 34-41, 46-47, 113-122, 171-177
deps/graph-hasher/test/calcGraphNodeHash.test.ts (1)
359-476: LGTM!deps/graph-hasher/test/index.ts (1)
59-81: LGTM!pacquet/crates/package-manager/src/install_frozen_lockfile.rs (1)
1309-1345: LGTM!Also applies to: 1347-1408
pacquet/crates/package-manager/src/virtual_store_layout.rs (1)
23-27: LGTM!Also applies to: 100-115, 191-210, 343-347, 597-694
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5177451582000003,
"stddev": 0.0758590309601731,
"median": 2.5517359694,
"user": 2.6484907399999993,
"system": 3.8197028599999996,
"min": 2.3996839374,
"max": 2.5967945164,
"times": [
2.5853663294,
2.5578381394,
2.4507052474,
2.3996839374,
2.5456337994,
2.5948562204,
2.5967945164,
2.4551837234,
2.4274328324,
2.5639568364
]
},
{
"command": "pacquet@main",
"mean": 2.4461827885999994,
"stddev": 0.06359939201817398,
"median": 2.4427604969,
"user": 2.70078094,
"system": 3.7900567599999997,
"min": 2.3711333324,
"max": 2.6023979624,
"times": [
2.4524917964,
2.3711333324,
2.4571728644,
2.4360800964,
2.3898370073999997,
2.6023979624,
2.4746132134,
2.4494408973999997,
2.4207641274,
2.4078965884
]
},
{
"command": "pnpm",
"mean": 4.9564477562,
"stddev": 0.06032096692470123,
"median": 4.9619811869,
"user": 8.34159854,
"system": 4.33348946,
"min": 4.8563210113999995,
"max": 5.0647666984,
"times": [
5.0075585384,
4.9552289114,
4.9718653094,
5.0008929233999995,
4.9284650984,
4.9046698833999995,
4.8563210113999995,
4.9687334624,
4.9059757254,
5.0647666984
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6917548530200001,
"stddev": 0.042461631515393186,
"median": 0.6787576726200001,
"user": 0.38742965999999995,
"system": 1.5733559399999997,
"min": 0.67338222962,
"max": 0.8121499536200001,
"times": [
0.8121499536200001,
0.67338222962,
0.6860581926200001,
0.6813038386200001,
0.6795592586200001,
0.6769356986200001,
0.6779560866200001,
0.6739249996200001,
0.6797635126200001,
0.6765147596200001
]
},
{
"command": "pacquet@main",
"mean": 0.7013323051200001,
"stddev": 0.01896591474959654,
"median": 0.6964362541200001,
"user": 0.38630285999999997,
"system": 1.5892413399999996,
"min": 0.6742326606200001,
"max": 0.7281796756200001,
"times": [
0.7272794616200001,
0.6854834106200001,
0.6984137506200001,
0.7251570196200001,
0.6988009256200001,
0.6944587576200001,
0.6904368416200001,
0.7281796756200001,
0.6908805476200001,
0.6742326606200001
]
},
{
"command": "pnpm",
"mean": 2.7418156945199996,
"stddev": 0.06596700206934673,
"median": 2.72181855962,
"user": 3.30502186,
"system": 2.3022015399999995,
"min": 2.68777647862,
"max": 2.89992904662,
"times": [
2.69457079062,
2.75428717862,
2.68777647862,
2.89992904662,
2.74132961162,
2.7964874806199997,
2.69595215962,
2.69558157062,
2.70230750762,
2.74993512062
]
}
]
} |
- Widen `readSnapshotRuntimePin`'s parameter to `Record<string, string>`
so callers can pass arbitrary graph children, not just `{node?}`.
- Replace "mis-key" wording (flagged by typos) with explicit phrasing.
- Demote the doc link to `find_own_runtime_node_major` to plain code
since the helper is `pub(crate)` (private-intra-doc-links lint).
- Apply `cargo fmt` to the new tests in install_frozen_lockfile.rs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11693 +/- ##
==========================================
+ Coverage 88.83% 88.94% +0.10%
==========================================
Files 129 129
Lines 14705 14824 +119
==========================================
+ Hits 13063 13185 +122
+ Misses 1642 1639 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add `desugars` / `desugared` to the cspell dictionary — standard programming jargon, used in the new doc comments and changeset. - Drop the trailing comma inside a single-line `assert_ne!` macro invocation (perfectionist::macro-trailing-comma lint).
`exec/commands/test/dlx.e2e.ts` mocks `@pnpm/engine.runtime.system-node-version` and re-exports every public symbol so transitive consumers keep working under the mock. The new `readSnapshotRuntimePin` export needs the same forward — without it `@pnpm/deps.graph-hasher`'s dynamic import sees an empty binding and the test suite fails to load.
…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`.
Contributor
Benchmark Results
Run 25988659855 · 10 runs per scenario · triggered by @zkochan |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #11690.
A dependency that declares
engines.runtimein its manifest carries the desugareddependencies.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/linkeralready does on a per-package basis: a snapshot's own pin wins; the install-wide value (from #11689'sfindRuntimeNodeVersion) is the fallback.TypeScript
deps/graph-hasher/src/index.ts:72-77— addsreadSnapshotRuntimePin(children): pulls the bare Node version from a graph node'schildren.nodeentry when that points at anode@runtime:<version>snapshot. Factors out a smallextractRuntimeNodeVersion(snapshotKey)parser shared withfindRuntimeNodeVersion.deps/graph-hasher/src/index.ts:115-116,245-246—calcDepStateandcalcGraphNodeHashconsultreadSnapshotRuntimePin(graph[depPath].children)first and only fall back to the install-widenodeVersionparameter when the snapshot doesn't pin its own Node. No caller changes required — install-wide fallback continues to be computed viafindRuntimeNodeVersion(Object.keys(graph))at each call site.findRuntimeNodeVersionmoved from@pnpm/engine.runtime.system-node-versionto@pnpm/deps.graph-hasher(along with the newreadSnapshotRuntimePin).system-node-versionis 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 onsystem-node-version.Pacquet
pacquet/crates/package-manager/src/install_frozen_lockfile.rs:1309-1345— newfind_own_runtime_node_major(snapshot)reads a snapshot'sdependenciesfor anodeentry withPrefix::Runtime, returning the bare major.pacquet/crates/package-manager/src/virtual_store_layout.rs:178-205—VirtualStoreLayout::newresolves engine per-snapshot inside the hash loop viaengine_name(own_major, None, None)when the snapshot pins, otherwise inherits the install-wideengineargument.Migration
Snapshots of dependencies that declare their own
engines.runtimere-hash under that dep's pinned Node instead of the install-wide value. Old slots become prune-eligible on next install.Test plan
pnpm --filter @pnpm/deps.graph-hasher test— 33 tests pass (6 new:findRuntimeNodeVersionextraction,readSnapshotRuntimePinextraction, own-pin precedence incalcGraphNodeHash, fallback path, cross-pinning siblings produce distinct slots, own-pin precedence incalcDepState)pnpm --filter @pnpm/engine.runtime.system-node-version test— 5 tests pass (unchanged;engineName/getSystemNodeVersiononly)pnpm --filter @pnpm/installing.deps-resolver test— 60 tests passpnpm --filter @pnpm/building.during-install test— 3 tests passcargo nextest run -p pacquet-package-manager— 273 tests pass (5 new: 4 coveringfind_own_runtime_node_major's extraction rule +cross_pinning_siblings_get_distinct_slotsat the layout level)cargo nextest run -p pacquet-graph-hasher— 32 tests passcargo clippy -p pacquet-package-manager --tests -- --deny warnings— no new lints in the touched files (pre-existing clippy errors inhoist/tests.rsandlink_hoisted_modules/tests.rsare unrelated)engines.runtimepins — would need the mocked registry to host packages withengines.runtimemanifest fields; tracked as a follow-up to extendpacquet/crates/cli/tests/pnpm_compatibility.rsWritten by an agent (Claude Code, claude-opus-4-7).