This repository was archived by the owner on May 14, 2026. It is now read-only.
feat(package-manager): wire nodeLinker=hoisted into install pipeline (#438 slice 6)#518
Merged
Conversation
…#438 slice 6) Branches `Install::run` / `InstallFrozenLockfile::run` on `config.node_linker == Hoisted`. Threads the slice 4 `lockfile_to_hoisted_dep_graph` walker output and the per-package CAS index produced by `CreateVirtualStore` (slot writes skipped under hoisted) into the slice 5 `link_hoisted_modules` linker. Persists the walker's `hoisted_locations` into `.modules.yaml` for rebuild and follow-up installs to consume. Pipeline changes under hoisted: - `CreateVirtualStore` skips `CreateVirtualDirBySnapshot` for both warm and cold batches, collects each snapshot's CAS file index keyed by `PkgIdWithPatchHash` into a new `cas_paths_by_pkg_id` output field. - `InstallPackageBySnapshot::run` returns the per-package CAS map unconditionally and skips the virtual-store-slot write when its new `node_linker` field is `Hoisted`. - `InstallFrozenLockfile::run` skips `SymlinkDirectDependencies`, `LinkVirtualStoreBins`, the isolated hoist pass, and `BuildModules` under hoisted; runs `lockfile_to_hoisted_dep_graph` + `link_hoisted_modules` in their place. Folds the walker's augmented skip set back into the install-time `SkippedSnapshots` so `.modules.yaml.skipped` reflects the union. - `Install::run`'s `build_modules_manifest` now takes the walker's `hoisted_locations` and writes it through `Modules.hoisted_locations` (only when non-empty so the isolated path doesn't produce a hoisted-only key). The build phase under hoisted (rebuild over `hoistedLocations` with ancestor-`.bin` lookup, `MISSING_HOISTED_LOCATIONS`) is slice 7's scope and is intentionally left as a no-op here. Workspace and `hoistingLimits` / `externalDependencies` knobs are slices 9-10. Mirrors upstream's hoisted branch in `headlessInstall` at https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L369-L425. --- Written by an agent (Claude Code, claude-opus-4-7).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
📝 WalkthroughWalkthroughThe PR threads ChangesHoisted node-linker install flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #518 +/- ##
==========================================
- Coverage 88.63% 88.54% -0.10%
==========================================
Files 125 125
Lines 13688 13821 +133
==========================================
+ Hits 12133 12238 +105
- Misses 1555 1583 +28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/package-manager/src/install_frozen_lockfile.rs`:
- Around line 659-665: The current merge builds walker_skipped from the input
skipped set and then later treats walker_result.skipped as authoritative, which
can cause transient reasons (e.g., "optional_excluded", "fetch_failed") from
walker_result.skipped to be persisted into .modules.yaml.skipped; fix by
filtering out transient skip reasons before merging/constructing the BTreeSet
used for LockfileToHoistedDepGraphOptions (the variable walker_skipped) or
before writing persisted skips so only permanent skip reasons remain;
specifically, when using walker_result.skipped and when creating
LockfileToHoistedDepGraphOptions::skipped, exclude entries whose reason is
optional_excluded or fetch_failed so they are not converted into installability
skips.
🪄 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: a5b2869e-ea22-449a-8c92-caaf71fe8f91
📒 Files selected for processing (5)
crates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_by_snapshot.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). (6)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install_frozen_lockfile.rs
🧠 Learnings (5)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install_frozen_lockfile.rs
📚 Learning: 2026-05-13T22:52:32.579Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 506
File: crates/package-manager/src/link_bins.rs:238-241
Timestamp: 2026-05-13T22:52:32.579Z
Learning: In Rust code using `matches!` (or `match`) on a borrowed value (e.g., `meta: &PackageMetadata` and a field access like `meta.resolution`), it is safe to use wildcard/OR patterns such as `LockfileResolution::Binary(_) | LockfileResolution::Variations(_)` when the pattern does not bind the inner payload by value. `_` wildcard patterns (and similar discriminant-only patterns) should not move non-`Copy` data; they only test the variant and avoid ownership transfer. Do not flag these patterns as move-out-of-borrow/ownership compile errors. Only patterns that bind the inner value by value (e.g., `LockfileResolution::Binary(b)`) would require ownership and may trigger move errors when matching on data behind a borrow.
Applied to files:
crates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install_frozen_lockfile.rs
📚 Learning: 2026-05-13T19:22:48.951Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 478
File: crates/package-manager/src/hoisted_dep_graph.rs:51-55
Timestamp: 2026-05-13T19:22:48.951Z
Learning: When reviewing this Rust codebase, avoid introducing/using a newtype like `PkgIdWithPatchHash` in only one module (e.g., `hoisted_dep_graph.rs`) if other related pacquet modules still represent the upstream pnpm “pkg id with patch hash” as a plain `String` (e.g., `virtual_store_layout`). For type consistency, either keep `pkg_id_with_patch_hash` as `String` here, or require a workspace-wide sweep that defines/extracts the `PkgIdWithPatchHash` newtype once and updates all call sites together (otherwise defer the refactor to a coordinated follow-up PR).
Applied to files:
crates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install_frozen_lockfile.rs
📚 Learning: 2026-05-13T20:09:22.171Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 486
File: crates/package-manager/src/hoisted_dep_graph.rs:472-476
Timestamp: 2026-05-13T20:09:22.171Z
Learning: In pnpm/pacquet, when generating serialized `hoisted_locations` strings (used for `.modules.yaml`), normalize path separators to forward slashes (e.g., replace `\\` with `/`). This preserves cross-platform portability: the output must not use OS-native separators because upstream pnpm’s `path.relative` can return platform-specific separators, which would break consistency with pnpm/pacquet’s other serialized formats on Windows.
Applied to files:
crates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/create_virtual_store.rscrates/package-manager/src/install_frozen_lockfile.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/install/tests.rs
🔇 Additional comments (4)
crates/package-manager/src/install_package_by_snapshot.rs (1)
7-7: LGTM!Also applies to: 71-83, 172-189, 417-442
crates/package-manager/src/install/tests.rs (1)
1619-1620: LGTM!Also applies to: 1655-1662, 2236-2395
crates/package-manager/src/install.rs (1)
272-286: LGTM!Also applies to: 416-441, 467-468, 537-583
crates/package-manager/src/create_virtual_store.rs (1)
2-2: LGTM!Also applies to: 8-12, 83-96, 142-153, 195-209, 595-610, 613-675, 680-704, 728-738, 780-828
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.6757123685199997,
"stddev": 0.10439504425899129,
"median": 2.6660590469199996,
"user": 2.51531108,
"system": 3.68613202,
"min": 2.5440634159199997,
"max": 2.82026797292,
"times": [
2.82026797292,
2.61920904592,
2.6210863279199996,
2.55359198692,
2.7110317659199996,
2.5800648949199996,
2.8067744279199998,
2.5440634159199997,
2.7516428029199997,
2.7493910439199998
]
},
{
"command": "pacquet@main",
"mean": 2.58594273882,
"stddev": 0.05800137490785645,
"median": 2.6068402754199997,
"user": 2.5034626799999997,
"system": 3.6997058200000006,
"min": 2.4928675719199997,
"max": 2.65535602692,
"times": [
2.55336780792,
2.60399407292,
2.6113303549199998,
2.4928675719199997,
2.60968647792,
2.5069560429199997,
2.6277988779199997,
2.5448718149199996,
2.65535602692,
2.65319833992
]
},
{
"command": "pnpm",
"mean": 6.10738941072,
"stddev": 0.06790402252270009,
"median": 6.11768931942,
"user": 8.968919679999997,
"system": 4.50621452,
"min": 6.0180670869199995,
"max": 6.19736492792,
"times": [
6.1358553879199995,
6.03067586192,
6.05841268692,
6.18757311492,
6.09952325092,
6.15811457592,
6.0180670869199995,
6.03534962292,
6.15295759092,
6.19736492792
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.73829334236,
"stddev": 0.09203601723973015,
"median": 0.7182277557600001,
"user": 0.36683547999999994,
"system": 1.5799856,
"min": 0.67921881176,
"max": 0.99660775576,
"times": [
0.99660775576,
0.7220001837600001,
0.70625644176,
0.7235919617600001,
0.72502995776,
0.7164458477600001,
0.6899956017600001,
0.67921881176,
0.70377719776,
0.7200096637600001
]
},
{
"command": "pacquet@main",
"mean": 0.7784179336600001,
"stddev": 0.0831737183982484,
"median": 0.7580507487600001,
"user": 0.37433668,
"system": 1.6249027999999999,
"min": 0.6987573917600001,
"max": 0.9667860907600001,
"times": [
0.8562771987600001,
0.74642342776,
0.7092579317600001,
0.81140169376,
0.71760870676,
0.71857880576,
0.6987573917600001,
0.9667860907600001,
0.78941001976,
0.7696780697600001
]
},
{
"command": "pnpm",
"mean": 2.63113542766,
"stddev": 0.08401649749469109,
"median": 2.64256537876,
"user": 3.1752179799999993,
"system": 2.240551,
"min": 2.51419482976,
"max": 2.75972548476,
"times": [
2.51419482976,
2.66505559576,
2.66106119676,
2.75972548476,
2.55877919976,
2.57929591376,
2.70831184076,
2.5279256557600003,
2.62406956076,
2.71293499876
]
}
]
} |
… fix (#438 slice 6) Docs CI was failing because `[`crate::link_hoisted_modules`]` is ambiguous between the function and the module of the same name. Disambiguate by switching to the function form `[`crate::link_hoisted_modules()`]` everywhere. Slice 6's hoisted-walker skip-set merge previously folded every entry in `walker_result.skipped` into `SkippedSnapshots::insert_installability`, which would promote pre-existing transient skips (`fetch_failed` / `optional_excluded`) into the persisted-on-disk `.modules.yaml.skipped` set. Diff against the input `walker_skipped` so only walker-discovered (genuinely-new) installability skips flow into the persisted subset.
This was referenced May 14, 2026
6 tasks
5 tasks
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Slice 6 of #438. Plugs the existing slice 3-5 building blocks
(
real-hoist,lockfile_to_hoisted_dep_graph,link_hoisted_modules)into
Install::run/InstallFrozenLockfile::runbehind a branch onconfig.node_linker == Hoisted. With this PR,pacquet install --frozen-lockfile --node-linker hoistedproduces a real-directory`node_modules/` tree rooted at the project (no symlinks, no virtual
store), and
.modules.yaml.hoistedLocationsis persisted so afollow-up install or rebuild can locate every package.
Pipeline changes under hoisted
CreateVirtualStoreskipsCreateVirtualDirBySnapshotfor both warmand cold batches, collects each snapshot's CAS file index keyed by
PkgIdWithPatchHashinto a newcas_paths_by_pkg_idoutput field.InstallPackageBySnapshot::runnow returns the per-package CAS mapunconditionally and skips the virtual-store-slot write when its new
node_linkerfield isHoisted.InstallFrozenLockfile::runskipsSymlinkDirectDependencies,LinkVirtualStoreBins, the isolated hoist pass, andBuildModulesunder hoisted; runs
lockfile_to_hoisted_dep_graph+link_hoisted_modulesin their place. Folds the walker's augmentedskip set back into the install-time
SkippedSnapshotsso.modules.yaml.skippedreflects the union of both passes.Install::run'sbuild_modules_manifestnow takes the walker'shoisted_locationsand writes it throughModules.hoisted_locations(only when non-empty so the isolatedpath doesn't produce a hoisted-only key).
Mirrors upstream's hoisted branch in
headlessInstallathttps://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L369-L425.
Out of scope (deferred to later slices)
`hoistedLocations` directories with `extraBinPaths` gathered from
every ancestor `node_modules/.bin`. `MISSING_HOISTED_LOCATIONS`
also lives there. Today's PR skips `BuildModules` under hoisted so
the install completes without trying to walk a virtual store that
was never written.
the walker rejects multi-importer lockfiles with
`HoistError::UnsupportedWorkspace`.
inputs default to empty.
field). Left empty under hoisted because upstream's
`linkHoistedModules` uses `hoistedLocations` directly; the
isolated-mode adapter shape is unused under `nodeLinker: hoisted`
and pacquet matches that.
Test plan
`Install::run` → `InstallFrozenLockfile::run` → walker → linker
composition against an empty lockfile, asserts
`.modules.yaml.nodeLinker == hoisted` and
`hoisted_locations is None` (the field drops to `None` when empty).
that `/node_modules/.pacquet` is never created under
hoisted.
`@pnpm.e2e/foo@100.0.0` style fixture with hand-written lockfile.
Tracked as a follow-up; the existing slice 4 / slice 5 unit tests
already cover the underlying components and the slice 7 build-phase
retargeting will benefit from the same fixture.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Refactor
Tests