feat(package-manager): hoisted dep-graph walker (#438 slice 4b)#486
Conversation
Implements `lockfile_to_hoisted_dep_graph(lockfile, opts) -> LockfileToDepGraphResult` — the walker that takes a lockfile, runs `pacquet_real_hoist::hoist`, then assembles the directory-keyed `DependenciesGraph`, `DepHierarchy`, `hoisted_locations`, `direct_dependencies_by_importer_id`, and `injection_targets_by_dep_path` outputs introduced by 4a (#478). Single-importer only (multi-importer lockfiles surface upstream's `HoistError::UnsupportedWorkspace`). Defers to follow-ups: - Store fetch wiring (`fetching` / `files_index_file` left defaulted on the graph node). - The installability check that adds packages to `skipped` — today the walker only honors *pre-skipped* inputs, it doesn't add new ones. - The `currentLockfile`-driven `prev_graph` diff for the linker's orphan-removal pass. ## Two-pass design (children resolved after the walk) Upstream's `fetchDeps` walks asynchronously via `await Promise.all(deps.map(async (dep) => { ... }))`. Each sibling's async function runs its sync prologue — insert into graph, push to `pkgLocationsByDepPath` — before any continuation fires, because Promise.all sync-invokes each function and the inner `await fetchDeps(...)` only yields after the prologue completes. The recursive form has the same property top-down, so by the time *any* node's `graph[dir].children = getChildren(...)` post-recursion line runs, every node in the entire tree is already in `pkgLocationsByDepPath`. Pacquet runs synchronously, so the cleanest way to preserve that invariant is two passes: 1. Recursive walk that inserts each node with `children: {}` and pushes its dir to `pkg_locations_by_dep_path`. Recursion order matches upstream's outer body order (insert → push → recurse). 2. After the walk returns, `fill_children` iterates the graph and resolves each node's `children: alias -> dir` by looking up the node's snapshot and consulting the now-complete `pkg_locations`. A single-pass walker that computed children inline gave `children` maps missing later siblings — caught by the `walker_transitive_dep_flattens_under_root` test before the refactor. ## Tests - `walker_empty_lockfile_produces_empty_result` — no importers produces an empty graph with the root-importer key present. - `walker_single_root_dep_emits_one_node` — `root -> a`, with `a` placed under `<lockfile_dir>/node_modules/a`. - `walker_transitive_dep_flattens_under_root` — `root -> a -> b` hoists `b` to root; `a`'s `children["b"]` points at the root-level dir, not `a/node_modules/b`. - `walker_version_conflict_keeps_loser_nested` — `root -> {a@1, c}` with `c -> a@2`: `a@1` at root, `a@2` nested under `c`; `hoisted_locations` records both dirs. - `walker_honors_pre_skipped_dep_path` — depPaths in `opts.skipped` are dropped from the walk entirely. - `walker_records_directory_resolution_as_injection_target` — `directory:` resolutions populate `injection_targets_by_dep_path` for the post-install re-mirror pass.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements lockfile_to_hoisted_dep_graph: runs pacquet_real_hoist, walks the HoisterResult to assemble directory-keyed dependency graph nodes, records hoisted locations and injection targets, then resolves hoister snapshot references to populate each node’s children. Adds workspace deps and comprehensive walker tests. ChangesHoisted Dependency Graph Walker
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)
Comment |
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/hoisted_dep_graph.rs`:
- Around line 462-466: The function path_relative_to_lockfile_dir returns
OS-native separators which yields backslashes on Windows; update it to normalize
returned paths to use forward slashes for portability (so hoisted_locations
match pnpm format and tests expecting "node_modules/a"). Locate
path_relative_to_lockfile_dir and after obtaining the lossy string (both in the
Ok and fallback branches) replace Windows backslashes with forward slashes
(e.g., replace '\' with '/') or otherwise construct the relative path using '/'
as the separator before returning.
🪄 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: 530c1025-4fa2-4cd0-b9bd-ef8fa73e0969
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
crates/package-manager/Cargo.tomlcrates/package-manager/src/hoisted_dep_graph.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: copilot-pull-request-reviewer
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- 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/hoisted_dep_graph.rs
🧠 Learnings (2)
📚 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/hoisted_dep_graph.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/hoisted_dep_graph.rs
🔇 Additional comments (6)
crates/package-manager/Cargo.toml (1)
26-26: LGTM!Also applies to: 38-38
crates/package-manager/src/hoisted_dep_graph.rs (5)
25-35: LGTM!Also applies to: 184-209
225-298: LGTM!
300-340: LGTM!
448-457: LGTM!Also applies to: 475-497
499-916: LGTM!
There was a problem hiding this comment.
Pull request overview
Implements the walker (sub-slice 4b of #438) that turns a lockfile + pacquet_real_hoist::hoist output into a directory-keyed LockfileToDepGraphResult for the hoisted-linker install path. The new lockfile_to_hoisted_dep_graph function uses a two-pass design (recursive walk inserts nodes with empty children and records every directory in pkg_locations_by_dep_path; a second pass resolves each node's children: alias → dir map via SnapshotDepRef::resolve) to preserve the invariant upstream's async Promise.all walker gets for free. Single-importer only; store fetch wiring, installability filtering, and prev_graph diff are deferred to 4c/4d.
Changes:
- Adds
lockfile_to_hoisted_dep_graph,HoistedDepGraphError, and a private two-pass walker (walk_deps+fill_children) incrates/package-manager/src/hoisted_dep_graph.rs. - Wires
pacquet-real-hoistandindexmapinto thepacquet-package-managercrate. - Adds six walker tests covering empty input, single root dep, transitive flatten, version conflict, pre-skipped dep paths, and
directory:injection-target recording.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/package-manager/src/hoisted_dep_graph.rs | New walker + error type + tests on top of the 4a type skeleton. |
| crates/package-manager/Cargo.toml | Adds pacquet-real-hoist and indexmap dependencies. |
| Cargo.lock | Mirrors the new crate dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #486 +/- ##
==========================================
+ Coverage 90.30% 90.55% +0.24%
==========================================
Files 121 121
Lines 11366 12269 +903
==========================================
+ Hits 10264 11110 +846
- Misses 1102 1159 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
review) `path_relative_to_lockfile_dir` was returning OS-native path separators via `to_string_lossy()`. On Windows that produced `node_modules\a` in `hoisted_locations`, which: - Broke the three Slice 4b walker tests on Windows CI (they assert forward-slash paths to match the Slice 2 round-trip fixture convention). - Was inconsistent with `pnpm-lock.yaml` and pacquet's `.modules.yaml.hoisted_locations` Slice 2 schema test, both of which use forward slashes for cross-platform portability. Upstream's `path.relative` in JS is OS-native too, so a `.modules.yaml` written by pnpm on Windows technically holds backslashes — but that's an upstream cross-platform inconsistency rather than a choice worth mirroring. Pacquet normalizes here so the on-disk shape is portable across OSes. Caught by Coderabbit and surfaced by the Windows CI run on PR #486.
…lice 4c) (#491) Sub-slice 4c of #438. Wires `package_is_installable` into the hoisted-graph walker shipped in 4b (#486), so optional packages on unsupported platforms get filtered into `result.skipped` and engine-strict mismatches surface as typed errors. Single-importer only; store I/O and `prev_graph` diff still to come (4d, then 5). ## Behavior Mirrors upstream's `if (!opts.force && packageIsInstallable(...) === false)` gate at [lockfileToHoistedDepGraph.ts:200-211](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/lockfileToHoistedDepGraph.ts#L200-L211): | Upstream return | `package_is_installable` (Rust) | Walker action | |---|---|---| | `null` (no constraint) | `Ok(Installable)` | emit node | | `true` (warn but proceed) | `Ok(ProceedWithWarning)` | emit node (warning emit deferred) | | `false` (optional + incompatible) | `Ok(SkipOptional)` | add to `result.skipped`, skip node | | throws `UnsupportedEngineError` (strict) | `Err(InstallabilityError::Engine)` | `HoistedDepGraphError::Installability` | | throws `InvalidNodeVersionError` | `Err(InstallabilityError::InvalidNodeVersion)` | `HoistedDepGraphError::Installability` | ## New options on `LockfileToHoistedDepGraphOptions` - `force: bool` — bypass the check entirely. Used by Slice 4d's `prev_graph` walk, where the previous lockfile is replayed wholesale so the orphan diff catches packages that would now filter out. Mirrors upstream's `force` at [lockfileToHoistedDepGraph.ts:73](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/lockfileToHoistedDepGraph.ts#L73-L76). - `engine_strict`, `current_node_version`, `current_os`, `current_cpu`, `current_libc`, `supported_architectures` — host-derived axes the check consumes. ## New output on `LockfileToDepGraphResult` - `skipped: BTreeSet<String>` — the input `opts.skipped` cloned and extended with depPaths added during the walk. Upstream mutates the input set in place; pacquet returns the augmented set so the caller can persist it into `.modules.yaml.skipped` without sharing mutable state. ## Tests 15 walker tests total — the 10 from 4b survive (one extended to assert the input depPath survives into output `skipped`), plus five new ones: - `walker_skips_optional_dep_on_unsupported_platform` — Linux host, package targets darwin only, `optional: true` → added to `result.skipped`, no graph entry, no `hoisted_locations`. - `walker_emits_required_dep_with_unsupported_platform_as_warning` — same shape but `optional: false` → walker proceeds (matches upstream `packageIsInstallable === true`); warning log emit is out of scope for 4c. - `walker_errors_on_engine_strict_mismatch` — `engine_strict: true` + `engines.node = ">=99.0.0"` → `HoistedDepGraphError::Installability`. - `walker_force_bypasses_installability_check` — `force: true` emits an incompatible required dep without erroring. - `walker_emits_compatible_dep` — sanity: compatible host + no constraint mismatch → graph entry, no skip.
Summary
Sub-slice 4b of #438. Implements
lockfile_to_hoisted_dep_graph(lockfile, opts) -> LockfileToDepGraphResult— the walker that consumes the Slice 3 hoister output and produces the directory-keyed graph + hierarchy + hoisted_locations + direct-deps-by-importer-id + injection-targets-by-dep-path that 4a (#478) pinned the type shape of.Single-importer only (multi-importer lockfiles surface as
HoistError::UnsupportedWorkspacevia the underlying hoister).What's deferred
Three things are intentionally left to follow-ups so 4b stays focused:
fetching/files_index_fileon the graph node are still defaulted. 4c will wireStoreController::fetchPackage(and the skip-fetch optimization that consultscurrentHoistedLocations).opts.skippedas input but doesn't runpackageIsInstallableto add new entries. 4c will plumbpackage_is_installablefrompacquet-package-is-installable(already used by the isolated linker path).prev_graphdiff —prev_graphisNone. 4d will walk the current lockfile alongside the wanted one (usingforce: true+ emptyskippedper upstream) and produce the orphan-removal input Slice 5's linker consumes.Two-pass design
Upstream's
fetchDepswalks asynchronously viaawait Promise.all(deps.map(async (dep) => { ... })). The key invariant: each sibling's async function runs its sync prologue (insert + push topkgLocationsByDepPath) before any continuation fires, becausePromise.allsync-invokes each function and the innerawait fetchDeps(...)only yields after the prologue completes. So by the time any node's post-recursiongraph[dir].children = getChildren(...)line runs, every node in the entire tree is already inpkgLocationsByDepPath.Pacquet runs synchronously. To preserve that invariant cleanly:
children: BTreeMap::new()and push its dir topkg_locations_by_dep_path. Recursion order matches upstream's outer body (insert → push → recurse → push tohoistedLocations).fill_children). Iterate every graph node and resolvechildren: alias → dirfrom the snapshot'sdependencies+optionalDependenciesviaSnapshotDepRef::resolve, consulting the now-completepkg_locations_by_dep_path.A single-pass walker that computed children inline produced incorrect
childrenmaps for hoisted transitive deps — the canonical caseroot → a → bwithbflattened to root lefta.children["b"]empty becauseb's pkg_locations entry hadn't been recorded yet. Caught bywalker_transitive_dep_flattens_under_rootbefore the refactor.Tests
walker_empty_lockfile_produces_empty_result— empty importer → empty graph, with the"."root importer key still present.walker_single_root_dep_emits_one_node—root → a, node at<lockfile_dir>/node_modules/a.walker_transitive_dep_flattens_under_root—root → a → b:bhoists to root,a.children["b"]points at the root-level dir.walker_version_conflict_keeps_loser_nested—root → {a@1, c}withc → a@2:a@1at root,a@2nested underc;hoisted_locationsrecords both directories;c.children["a"]points at the nested copy.walker_honors_pre_skipped_dep_path— depPaths inopts.skippedare dropped from the walk entirely (and not recorded inhoisted_locations).walker_records_directory_resolution_as_injection_target—directory:resolutions populateinjection_targets_by_dep_pathfor the post-install re-mirror pass.Test plan
just ready(859 → 869 tests, all green) including thetyposcheck that caught anUnparseable→Unparsabletypo.cargo doc --document-private-items,taplo format --check,just dylintall clean.fill_childrencall leaveschildrenempty in thewalker_transitive_dep_flattens_under_rootandwalker_version_conflict_keeps_loser_nestedcases — both fail with diffs that name the missing alias.What's next
prev_graphdiff.After 4c-4d, Slice 5 (
linkHoistedModules) consumes this output to produce the on-disk tree.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Chores
Improvements
Tests