This repository was archived by the owner on May 14, 2026. It is now read-only.
feat(package-manager): linkHoistedModules linker (#438 slice 5)#505
Merged
Conversation
New module `crates/package-manager/src/link_hoisted_modules.rs`
produces the on-disk `node_modules/` tree from Slice 4's
`LockfileToDepGraphResult`. Ports
installing/deps-restorer/src/linkHoistedModules.ts.
## Three phases
1. **Orphan removal.** Every directory in `prev_graph` but not
in `graph` is silently `rimraf`'d before any insert.
`try_remove_dir` swallows all errors (NotFound +
PermissionDenied + EBUSY), matching upstream's `tryRemoveDir`
tolerance.
2. **Per-node import.** Hierarchy walked top-down, rayon-parallel
at each level. Every node goes through `import_indexed_dir`
with `force: true, keep_modules_dir: true` — the hoisted-linker
call shape Slice 1 was designed for.
3. **Per-`node_modules` bin link.** After a level's children are
all imported, `link_direct_dep_bins` populates
`<parent>/node_modules/.bin` from the just-materialized direct
children. Bin link runs only after every child's subtree is
done so package.json reads always see the fully-populated
package.
## API shape
```rust
pub fn link_hoisted_modules<R: Reporter>(
opts: &LinkHoistedModulesOpts<'_>,
) -> Result<(), LinkHoistedModulesError>;
```
`LinkHoistedModulesOpts` borrows `graph`, `prev_graph`,
`hierarchy`, and a `cas_paths_by_pkg_id: HashMap<String,
HashMap<String, PathBuf>>` keyed by `pkg_id_with_patch_hash`.
## Decoupling from the store
Upstream's linker is async and calls
`storeController.fetchPackage()` inline during the walk —
pacquet's is sync and takes pre-fetched CAS paths. Slice 6 (the
install pipeline) is responsible for fetching every package
through pacquet's existing tarball / store-dir / git-fetcher
machinery before invoking the linker. This keeps the linker
focused on file-system layout and reuses the proven fetch
chain that the isolated path already exercises.
## Optional-dep tolerance
A graph node whose `pkg_id_with_patch_hash` is missing from
`cas_paths_by_pkg_id`:
- If `node.optional`: silently skipped, no directory created.
Mirrors upstream's `if (depNode.optional) return` on fetch
failure.
- Otherwise: surfaces as
`LinkHoistedModulesError::MissingCasPaths { pkg_id, dir }`.
## Tests
7 real-tempdir tests in `link_hoisted_modules/tests.rs`:
- `import_pass_creates_package_directory` — single-package smoke.
- `orphan_directory_is_removed` — `prev_graph` diff produces
rimraf of stale directory; planted contents are gone after.
- `nested_hierarchy_materializes_inner_node_modules` —
version-conflict layout; loser ends up at
`<outer>/node_modules/<inner>`.
- `missing_cas_for_required_dep_errors` — required + missing
CAS → typed `MissingCasPaths` error.
- `missing_cas_for_optional_dep_skips_silently` — optional +
missing CAS → no error, no directory.
- `no_prev_graph_skips_orphan_pass` — fresh install (no prior
lockfile) path.
- `orphan_already_removed_is_tolerated` — phantom orphan in
`prev_graph` not present on disk doesn't error.
Each test plants synthetic CAS files in a tempdir and asserts
the on-disk tree after the linker runs.
📝 WalkthroughWalkthroughThis PR adds a synchronous hoisted-module linker that materializes node_modules/ hierarchies, removes orphaned directories from a previous graph, imports packages per-dependency level in parallel, and links package binaries; it exposes public types/errors and includes comprehensive tests. ChangesHoisted Module Linker Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/package-manager/src/link_hoisted_modules/tests.rs (1)
31-48: ⚡ Quick winAdd one explicit test for phase-3
.binlinking.Current cases validate import/orphan flows, but none verify that direct-dep bins are actually linked into
<parent>/node_modules/.bin. Since this is core behavior of this linker, adding a focused case (has_bin = true+ manifest/bin entry + assert linked shim exists) would close a meaningful coverage gap.Also applies to: 108-136
🤖 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 `@crates/package-manager/src/link_hoisted_modules/tests.rs` around lines 31 - 48, Add an explicit test that verifies phase-3 `.bin` linking by creating a DependenciesGraphNode with has_bin = true and a manifest that declares a bin (use or extend the helper make_node to set has_bin = true and add a manifest/bin entry), run the hoisted/linking flow used by the existing tests, and assert that the linked shim file exists under the parent node's node_modules/.bin; update or add assertions analogous to the import/orphan cases (see the other tests around lines 108-136) to check for the presence and correctness of the shim.crates/package-manager/src/link_hoisted_modules.rs (1)
9-10: ⚡ Quick winUse
pnpmblob/mainlinks in porting references, not pinned SHAs.These doc links point to a fixed commit SHA. For porting notes in this repo, link to
https://github.com/pnpm/pnpm/blob/main/...so references track upstream tip.As per coding guidelines: "If reading on GitHub, open files from
https://github.com/pnpm/pnpm/blob/main/...(which always resolves to the tip of main) rather than from a permalinked SHA."Also applies to: 75-76, 166-167, 180-180
🤖 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 `@crates/package-manager/src/link_hoisted_modules.rs` around lines 9 - 10, Update the pinned GitHub URLs in the module doc comments so they point to the upstream main branch instead of a specific SHA; replace occurrences like "https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/linkHoistedModules.ts" (and the other similar blob/<sha> links found in this file) with "https://github.com/pnpm/pnpm/blob/main/..." so the porting/reference links track the tip of upstream main (look for the doc comment at the top referencing installing/deps-restorer/src/linkHoistedModules.ts and the other occurrences noted in the file).
🤖 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/link_hoisted_modules.rs`:
- Around line 203-207: The code currently ignores hierarchy entries not present
in opts.graph (in the block around import_node::<R> and
link_all_pkgs_in_order::<R>), which can hide inconsistent state; change this to
fail fast: when iterating hierarchy entries, if opts.graph.get(dir) returns
None, return an Err (or propagate a suitable error) instead of continuing, so
the caller sees the missing node; update the call sites in
link_all_pkgs_in_order::<R> (and any surrounding closure) to propagate that
error and ensure import_node::<R> is only called when the node exists.
---
Nitpick comments:
In `@crates/package-manager/src/link_hoisted_modules.rs`:
- Around line 9-10: Update the pinned GitHub URLs in the module doc comments so
they point to the upstream main branch instead of a specific SHA; replace
occurrences like
"https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/linkHoistedModules.ts"
(and the other similar blob/<sha> links found in this file) with
"https://github.com/pnpm/pnpm/blob/main/..." so the porting/reference links
track the tip of upstream main (look for the doc comment at the top referencing
installing/deps-restorer/src/linkHoistedModules.ts and the other occurrences
noted in the file).
In `@crates/package-manager/src/link_hoisted_modules/tests.rs`:
- Around line 31-48: Add an explicit test that verifies phase-3 `.bin` linking
by creating a DependenciesGraphNode with has_bin = true and a manifest that
declares a bin (use or extend the helper make_node to set has_bin = true and add
a manifest/bin entry), run the hoisted/linking flow used by the existing tests,
and assert that the linked shim file exists under the parent node's
node_modules/.bin; update or add assertions analogous to the import/orphan cases
(see the other tests around lines 108-136) to check for the presence and
correctness of the shim.
🪄 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: 49d0ba8a-d172-4c79-bdf3-58759699132b
📒 Files selected for processing (3)
crates/package-manager/src/lib.rscrates/package-manager/src/link_hoisted_modules.rscrates/package-manager/src/link_hoisted_modules/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: copilot-pull-request-reviewer
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- 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)
🧰 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/lib.rscrates/package-manager/src/link_hoisted_modules/tests.rscrates/package-manager/src/link_hoisted_modules.rs
🧠 Learnings (4)
📚 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/lib.rscrates/package-manager/src/link_hoisted_modules/tests.rscrates/package-manager/src/link_hoisted_modules.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/lib.rscrates/package-manager/src/link_hoisted_modules/tests.rscrates/package-manager/src/link_hoisted_modules.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/lib.rscrates/package-manager/src/link_hoisted_modules/tests.rscrates/package-manager/src/link_hoisted_modules.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/link_hoisted_modules/tests.rs
🔇 Additional comments (1)
crates/package-manager/src/lib.rs (1)
22-22: LGTM!Also applies to: 51-51
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
==========================================
+ Coverage 88.64% 88.77% +0.13%
==========================================
Files 122 124 +2
Lines 13273 13481 +208
==========================================
+ Hits 11766 11968 +202
- Misses 1507 1513 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.53620429276,
"stddev": 0.14877825075382206,
"median": 2.48826210446,
"user": 2.5879179199999998,
"system": 3.5229927399999994,
"min": 2.4157728089600004,
"max": 2.9190474869600003,
"times": [
2.46346541196,
2.49042132096,
2.4574863169600003,
2.49827702296,
2.9190474869600003,
2.5903062929600003,
2.48610288796,
2.4157728089600004,
2.61330365496,
2.42785972296
]
},
{
"command": "pacquet@main",
"mean": 2.52625407056,
"stddev": 0.07666256647697456,
"median": 2.5226902679600003,
"user": 2.57613512,
"system": 3.4889083399999996,
"min": 2.43599593796,
"max": 2.65370025096,
"times": [
2.56186010796,
2.65370025096,
2.47358514496,
2.44473736296,
2.5699225779600003,
2.43599593796,
2.62660440596,
2.45075438096,
2.5150820359600004,
2.5302984999600002
]
},
{
"command": "pnpm",
"mean": 5.78752966726,
"stddev": 0.08170896348108272,
"median": 5.79738035046,
"user": 8.52204042,
"system": 4.250535739999999,
"min": 5.61555806196,
"max": 5.89507961896,
"times": [
5.85952344796,
5.61555806196,
5.84960905296,
5.89507961896,
5.81607234296,
5.75676881396,
5.82962287096,
5.77868835796,
5.76197953096,
5.712394573959999
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7347355185,
"stddev": 0.0323450196251902,
"median": 0.7288519614,
"user": 0.38175722,
"system": 1.6118197200000002,
"min": 0.7013344709,
"max": 0.8183022909,
"times": [
0.8183022909,
0.7168065459,
0.7453467889,
0.7389289629,
0.7270919559,
0.7306119669,
0.7013344709,
0.7104549589,
0.7208800909,
0.7375971529
]
},
{
"command": "pacquet@main",
"mean": 0.7986435606,
"stddev": 0.028447303754218094,
"median": 0.7981882034000001,
"user": 0.37981712,
"system": 1.6360847200000002,
"min": 0.7579989379,
"max": 0.8414859769,
"times": [
0.8414859769,
0.7986936019,
0.7786476409,
0.7691700369,
0.7579989379,
0.8131094729,
0.7760288619,
0.8163438029,
0.8372744689,
0.7976828049
]
},
{
"command": "pnpm",
"mean": 2.4107878164,
"stddev": 0.05783420265254261,
"median": 2.3887526394,
"user": 2.85625852,
"system": 2.17947262,
"min": 2.3340787319,
"max": 2.5287781469,
"times": [
2.5287781469,
2.3838982389,
2.3340787319,
2.4839500189000003,
2.3779281999,
2.3927630199,
2.4105214929,
2.3847422589000002,
2.4378942149,
2.3733238409
]
}
]
} |
…review)
The hierarchy walk silently skipped entries missing from
`graph`, which would produce a partial install layout instead
of surfacing the bug. Slice 4's walker keeps the two in sync
today, but a future bug there shouldn't yield a partial tree.
Add `LinkHoistedModulesError::MissingGraphNode { dir }` and
return it when a hierarchy directory has no graph entry.
Upstream effectively errors here too — its `graph[dir].fetching`
read would throw `Cannot read properties of undefined` — pacquet
just spells the failure out.
Regression test `hierarchy_entry_missing_from_graph_errors`
exercises the path with an empty graph and a hierarchy
referencing a phantom dir.
Caught by Coderabbit.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/package-manager/src/link_hoisted_modules/tests.rs (1)
152-152: ⚡ Quick winAdd failure context to bare
assert!checks.Line 152 and Line 343 use bare
assert!without diagnostic context, which makes failures harder to triage from CI logs.Suggested patch
- assert!(orphan_dir.exists()); + assert!(orphan_dir.exists(), "expected planted orphan dir to exist before linker run: {orphan_dir:?}"); ... - assert!(lockfile_dir.join("node_modules").join("a").join("package").join("index.js").exists()); + let installed = lockfile_dir.join("node_modules").join("a").join("package").join("index.js"); + assert!(installed.exists(), "expected installed file to exist: {installed:?}");Based on learnings: In Rust test code, add logging/diagnostic context for
assert!-style assertions so failures are debuggable without reruns.Also applies to: 343-343
🤖 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 `@crates/package-manager/src/link_hoisted_modules/tests.rs` at line 152, Replace the bare assertions that call assert!(orphan_dir.exists()) (at the occurrences referencing the orphan_dir variable around the tests) with assertions that include diagnostic context; e.g. change to assert!(orphan_dir.exists(), "expected orphan_dir to exist: {}", orphan_dir.display()) or use Debug: assert!(orphan_dir.exists(), "orphan_dir missing: {:?}", orphan_dir) so failures print the path and make CI failures debuggable. Ensure you update both occurrences (the one around the orphan_dir variable at the first failing check and the second at line ~343) to include similar messages.
🤖 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 `@crates/package-manager/src/link_hoisted_modules/tests.rs`:
- Line 152: Replace the bare assertions that call assert!(orphan_dir.exists())
(at the occurrences referencing the orphan_dir variable around the tests) with
assertions that include diagnostic context; e.g. change to
assert!(orphan_dir.exists(), "expected orphan_dir to exist: {}",
orphan_dir.display()) or use Debug: assert!(orphan_dir.exists(), "orphan_dir
missing: {:?}", orphan_dir) so failures print the path and make CI failures
debuggable. Ensure you update both occurrences (the one around the orphan_dir
variable at the first failing check and the second at line ~343) to include
similar messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: caf0d379-6746-4465-9409-e4ff0df3ab28
📒 Files selected for processing (2)
crates/package-manager/src/link_hoisted_modules.rscrates/package-manager/src/link_hoisted_modules/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/package-manager/src/link_hoisted_modules.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: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Dylint
- GitHub Check: Run benchmark on 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/link_hoisted_modules/tests.rs
🧠 Learnings (4)
📚 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/link_hoisted_modules/tests.rs
📚 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/link_hoisted_modules/tests.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/link_hoisted_modules/tests.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/link_hoisted_modules/tests.rs
🔇 Additional comments (1)
crates/package-manager/src/link_hoisted_modules/tests.rs (1)
1-151: LGTM!Also applies to: 153-342, 344-425
This was referenced May 13, 2026
zkochan
added a commit
that referenced
this pull request
May 13, 2026
…508) The Slice 5 linker (#505) was written against `String` for `pkg_id_with_patch_hash` and merged onto a main that had already switched the underlying `DependenciesGraphNode` field to the `PkgIdWithPatchHash` newtype (#504). The merge auto-resolved without flagging the type incompatibility — `cargo check` on main now errors with: error[E0277]: the trait bound `String: Borrow<PkgIdWithPatchHash>` is not satisfied error[E0308]: mismatched types expected `String`, found `PkgIdWithPatchHash` Switch the two slice-5-introduced surfaces to match the newtype: - `CasPathsByPkgId = HashMap<PkgIdWithPatchHash, HashMap<String, PathBuf>>` — the per-package CAS index now keys on the brand the graph node carries, matching the post-#504 invariant across the workspace. - `LinkHoistedModulesError::MissingCasPaths.pkg_id_with_patch_hash: PkgIdWithPatchHash` — same type the graph node has, so the error round-trips the value without `.to_string()`. Tests updated to construct `PkgIdWithPatchHash::from(...)` keys/fields. All 8 linker tests pass on the fixed branch. This unblocks main building again.
4 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 5 of #438. The hoisted linker — produces the on-disk
node_modules/tree from Slice 4'sLockfileToDepGraphResult. New modulecrates/package-manager/src/link_hoisted_modules.rs(~280 LoC + ~360 LoC tests). Portsinstalling/deps-restorer/src/linkHoistedModules.ts.Three phases
prev_graphbut not ingraphis silentlyrimraf'd before any insert.try_remove_dirswallows all errors (NotFound + PermissionDenied + EBUSY), matching upstream'stryRemoveDirtolerance at linkHoistedModules.ts:70-86.import_indexed_dirwithforce: true, keep_modules_dir: true— exactly the hoisted-linker call shape Slice 1 was designed for.node_modulesbin link. After a level's children are all imported,link_direct_dep_binspopulates<parent>/node_modules/.bin. Bin link runs only after every child's subtree is done so thepackage.jsonreads always see the fully-populated package.API shape
LinkHoistedModulesOptsborrowsgraph,prev_graph,hierarchy, plus acas_paths_by_pkg_id: HashMap<String, HashMap<String, PathBuf>>keyed bypkg_id_with_patch_hash.Decoupling from the store
Upstream's linker is async and calls
storeController.fetchPackage()inline during the walk. Pacquet's is sync and takes pre-fetched CAS paths. Slice 6 (the install pipeline) will be responsible for fetching every package through pacquet's existing tarball / store-dir / git-fetcher chain before invoking the linker.Rationale: pacquet's fetch infrastructure is already mature and exercised by the isolated path. Decoupling fetch from layout keeps Slice 5 focused on the on-disk topology, makes the linker easily testable with synthetic CAS files (no mock store needed), and avoids inventing a hoisted-only async fetch path.
The same CAS map is keyed per-package (not per-directory) because version-conflict layouts have one
pkg_id_with_patch_hashlanding at multiple directories — the CAS contents are the same regardless.Optional-dep tolerance
A graph node whose
pkg_id_with_patch_hashis missing fromcas_paths_by_pkg_id:node.optional: silently skipped, no directory created. Mirrors upstream'sif (depNode.optional) returnon fetch failure.LinkHoistedModulesError::MissingCasPaths { pkg_id, dir }— caller bug (incomplete pre-fetch), not a runtime failure.Tests
7 real-tempdir tests in
link_hoisted_modules/tests.rs. Each plants synthetic CAS files (<cas_root>/<pkg_id>/<rel_path>with arbitrary content), builds a graph + hierarchy by hand, runs the linker, then asserts the on-disk tree.import_pass_creates_package_directory— single-package smoke.orphan_directory_is_removed—prev_graphdiff produces rimraf; planted stale contents are gone after.nested_hierarchy_materializes_inner_node_modules— version-conflict layout; loser ends up at<outer>/node_modules/<inner>.missing_cas_for_required_dep_errors— required + missing CAS → typedMissingCasPathserror withpkg_idfor caller diagnostics.missing_cas_for_optional_dep_skips_silently— optional + missing CAS → no error, no directory.no_prev_graph_skips_orphan_pass— fresh install (no prior lockfile) path.orphan_already_removed_is_tolerated— phantom orphan inprev_graphnot present on disk doesn't error (matchestryRemoveDir's NotFound tolerance).Test plan
just ready(878 → 885 tests, all green).cargo doc --document-private-items,taplo format --check,just dylintclean.try_remove_dirto a no-op re-failsorphan_directory_is_removed(stale dir still there). Stubbingimport_nodeto early-return re-failsimport_pass_creates_package_directoryandnested_hierarchy_materializes_inner_node_modules.What's next
Slice 6 — wire the linker into the install pipeline. The pipeline:
lockfile_to_hoisted_dep_graph(lockfile, current_lockfile, opts)(Slice 4).result.graphvia existingpacquet-tarball/pacquet-store-dirinfrastructure, building acas_paths_by_pkg_idmap.link_hoisted_modules(opts)(this slice)..modules.yaml.hoisted_dependenciesand.modules.yaml.hoisted_locationsfromresult.Config.node_linker == Hoistedto take this path instead of the isolated path.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests