feat(package-manager): hoisted dep-graph installability check (#438 slice 4c)#491
Conversation
…lice 4c) Wires `pacquet_package_is_installable::package_is_installable` into the hoisted-graph walker introduced by 4b (#486). Mirrors upstream's `if (!opts.force && packageIsInstallable(...) === false)` gate at installing/deps-restorer/src/lockfileToHoistedDepGraph.ts. Behavior - `Installable` / `ProceedWithWarning` → emit node as before. - `SkipOptional` (optional + unsupported platform or engine) → add depPath to `result.skipped`, don't emit. Mirrors upstream's `opts.skipped.add(depPath); return`. - `Err(InstallabilityError)` (engine_strict + engine mismatch, or invalid node version) → propagate as `HoistedDepGraphError::Installability { dep_path, source }` so callers keep upstream's `ERR_PNPM_UNSUPPORTED_ENGINE` / `ERR_PNPM_INVALID_NODE_VERSION` diagnostic codes. New options on `LockfileToHoistedDepGraphOptions` - `force` — 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). - `engine_strict`, `current_node_version`, `current_os`, `current_cpu`, `current_libc`, `supported_architectures` — thread the 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 Pre-existing 4b tests adjusted to the new option shape via `..Default::default()`. Four new tests: - `walker_skips_optional_dep_on_unsupported_platform` — Linux host, package targets darwin only, snapshot is optional → 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`), the warning emit is out of scope for this slice. - `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. `walker_honors_pre_skipped_dep_path` also extended to assert the input depPath survives into `result.skipped`.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds installability-aware filtering to the hoisted dependency graph computation. ChangesInstallability-aware dependency graph and filtering
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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Sub-slice 4c of the hoisted node-linker work: wires package_is_installable into the hoisted-graph walker (added in #486), so optional packages on unsupported platforms are filtered into result.skipped, and engine-strict mismatches surface as typed HoistedDepGraphError::Installability errors. Single-importer only; store I/O and prev_graph diff remain for slices 4d/5.
Changes:
- Adds
force,engine_strict,current_node_version,current_os,current_cpu,current_libc,supported_architecturesoptions onLockfileToHoistedDepGraphOptionsand askipped: BTreeSet<String>field onLockfileToDepGraphResult. - Inserts an installability check inside
walk_depsmappingInstallable/ProceedWithWarning→ emit,SkipOptional→ add toskippedand continue,Err→HoistedDepGraphError::Installability. - Adds 5 new walker tests covering optional skip, required-warn, engine-strict error, force bypass, and a compatible-host sanity case; carries over the 10 prior walker tests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #491 +/- ##
==========================================
- Coverage 90.55% 88.92% -1.63%
==========================================
Files 121 121
Lines 12269 12732 +463
==========================================
+ Hits 11110 11322 +212
- Misses 1159 1410 +251 ☔ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/package-manager/src/hoisted_dep_graph.rs (1)
179-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not expose an impossible host via
Default.
lockfile_to_hoisted_dep_graph()now consults these fields on every installability check, butDefaultleaves the host identity empty. Any caller still building opts via..Default::default()will evaluate platform/engine compatibility against"", which can wrongly skip packages or fail on otherwise valid lockfiles. Please make these fields explicit, or provide a constructor that fills real host values instead of locking in the empty state here.Also applies to: 731-744
🤖 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/hoisted_dep_graph.rs` around lines 179 - 225, Default for LockfileToHoistedDepGraphOptions leaves host identity fields empty which breaks installability checks; change it so callers can't get an "impossible" host via ..Default::default(). Either remove the #[derive(Default)] and require construction via a new constructor (e.g. LockfileToHoistedDepGraphOptions::new(...)) that sets current_node_version, current_os, current_cpu, current_libc from the actual host (or from InstallabilityOptions helper), or implement Default manually to populate those four host fields with real runtime values while keeping other fields as sensible defaults; update callers (and mention lockfile_to_hoisted_dep_graph usage) to use the new constructor or explicit values instead of relying on the empty-derived default.
🧹 Nitpick comments (1)
crates/package-manager/src/hoisted_dep_graph.rs (1)
1063-1221: ⚡ Quick winAdd one regression test for
supported_architectures.This new option is threaded into the installability check, but the added coverage only exercises current-host matching. A case like
current_os = "linux"plussupported_architectures.os = ["darwin"]would catch regressions where the override is ignored and compatible packages get skipped anyway.🤖 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/hoisted_dep_graph.rs` around lines 1063 - 1221, Add a regression test that exercises LockfileToHoistedDepGraphOptions.supported_architectures so the installability check respects the override instead of only using the host; update host_aware_opts() or create a new opts variant that sets supported_architectures.os = vec!["darwin".to_string()] while current_os remains "linux", then call lockfile_to_hoisted_dep_graph(...) and assert behavior (e.g., an incompatible package for the overridden architectures is skipped or emitted per expectation). Reference LockfileToHoistedDepGraphOptions.supported_architectures, host_aware_opts(), lockfile_to_hoisted_dep_graph, and the existing metadata_with_os helper to construct the test case that would fail if the override is ignored.
🤖 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 253-271: Replace the explicit diagnostic code on the
Installability enum variant with a transparent diagnostic so the inner
InstallabilityError's codes are preserved: remove or change
#[diagnostic(code(ERR_PACQUET_HOISTED_GRAPH_UNINSTALLABLE))] and use
#[diagnostic(transparent)] on the Installability variant (which has dep_path:
String and source: Box<InstallabilityError>) so callers see the inner
UnsupportedEngineError/UnsupportedPlatformError/InvalidNodeVersionError
diagnostics, matching InstallFrozenLockfileError::Installability's pattern.
---
Outside diff comments:
In `@crates/package-manager/src/hoisted_dep_graph.rs`:
- Around line 179-225: Default for LockfileToHoistedDepGraphOptions leaves host
identity fields empty which breaks installability checks; change it so callers
can't get an "impossible" host via ..Default::default(). Either remove the
#[derive(Default)] and require construction via a new constructor (e.g.
LockfileToHoistedDepGraphOptions::new(...)) that sets current_node_version,
current_os, current_cpu, current_libc from the actual host (or from
InstallabilityOptions helper), or implement Default manually to populate those
four host fields with real runtime values while keeping other fields as sensible
defaults; update callers (and mention lockfile_to_hoisted_dep_graph usage) to
use the new constructor or explicit values instead of relying on the
empty-derived default.
---
Nitpick comments:
In `@crates/package-manager/src/hoisted_dep_graph.rs`:
- Around line 1063-1221: Add a regression test that exercises
LockfileToHoistedDepGraphOptions.supported_architectures so the installability
check respects the override instead of only using the host; update
host_aware_opts() or create a new opts variant that sets
supported_architectures.os = vec!["darwin".to_string()] while current_os remains
"linux", then call lockfile_to_hoisted_dep_graph(...) and assert behavior (e.g.,
an incompatible package for the overridden architectures is skipped or emitted
per expectation). Reference
LockfileToHoistedDepGraphOptions.supported_architectures, host_aware_opts(),
lockfile_to_hoisted_dep_graph, and the existing metadata_with_os helper to
construct the test case that would fail if the override is ignored.
🪄 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: eba86143-d15e-4145-8b71-894a22bbec5d
📒 Files selected for processing (1)
crates/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 (ubuntu-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-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 (3)
📚 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
📚 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/hoisted_dep_graph.rs
…ty (#491 review) The `Installability` variant declared `#[diagnostic(code(...))]` on the wrapper, which made the wrapper's code take precedence over the inner `InstallabilityError`'s — defeating the doc-comment's stated promise that callers see `ERR_PNPM_UNSUPPORTED_ENGINE` / `ERR_PNPM_UNSUPPORTED_PLATFORM` / `ERR_PNPM_INVALID_NODE_VERSION` directly. Switch to `#[diagnostic(transparent)]` on a tuple variant, matching `InstallFrozenLockfileError::Installability`'s pattern. The `dep_path` context dropped from the struct variant was redundant — every inner error variant already carries `package_id`, which is the same value the walker would have put on `dep_path`. Test updated to pattern-match the inner `Engine` variant and assert its `package_id` instead of the wrapper's `dep_path`. Caught by Coderabbit.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.56887029128,
"stddev": 0.1547578843619181,
"median": 2.5181263525799995,
"user": 2.6102551600000004,
"system": 3.54318098,
"min": 2.42910314908,
"max": 2.97530362808,
"times": [
2.5190462380799996,
2.63121563608,
2.60866183508,
2.97530362808,
2.51720646708,
2.50207395208,
2.51369079408,
2.4635132720799997,
2.42910314908,
2.52888794108
]
},
{
"command": "pacquet@main",
"mean": 2.51233745668,
"stddev": 0.054199647479093706,
"median": 2.50474829058,
"user": 2.60328646,
"system": 3.51684918,
"min": 2.44668111808,
"max": 2.59272657108,
"times": [
2.56538025808,
2.48045539608,
2.46879376808,
2.44668111808,
2.5290411850799996,
2.47848029108,
2.59272657108,
2.5706292300799998,
2.4478691010799998,
2.54331764808
]
},
{
"command": "pnpm",
"mean": 5.74153028388,
"stddev": 0.059604422291320694,
"median": 5.73723111008,
"user": 8.357885360000001,
"system": 4.24263398,
"min": 5.66860595408,
"max": 5.84689216308,
"times": [
5.739399050079999,
5.82654428708,
5.7626462920799995,
5.717510069079999,
5.66860595408,
5.66995520608,
5.73506317008,
5.75213471608,
5.69655193108,
5.84689216308
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7570171057,
"stddev": 0.03711516351815759,
"median": 0.7475677633,
"user": 0.3640733599999999,
"system": 1.6280382600000003,
"min": 0.7243913328,
"max": 0.8560511258000001,
"times": [
0.8560511258000001,
0.7539597778,
0.7453800278,
0.7336881438,
0.7364198848000001,
0.7497554988,
0.7417472488000001,
0.7243913328,
0.7673719518000001,
0.7614060648000001
]
},
{
"command": "pacquet@main",
"mean": 0.8132927077,
"stddev": 0.04614057083221063,
"median": 0.8035926158000001,
"user": 0.37612035999999993,
"system": 1.63884576,
"min": 0.7495858418000001,
"max": 0.9003141008000001,
"times": [
0.7648482508000001,
0.8555196708,
0.7495858418000001,
0.8068652618000001,
0.8500751858000001,
0.7759363598000001,
0.8295148408,
0.9003141008000001,
0.7999475948000001,
0.8003199698000001
]
},
{
"command": "pnpm",
"mean": 2.4440673198,
"stddev": 0.1045737683722865,
"median": 2.3880856907999997,
"user": 2.8191742599999996,
"system": 2.1831681599999997,
"min": 2.3345456308,
"max": 2.6246845778,
"times": [
2.5527763028,
2.6246845778,
2.3408487337999997,
2.5532828108,
2.3795716358,
2.3726730998,
2.3345456308,
2.3918667298,
2.5061190248,
2.3843046518
]
}
]
} |
Summary
Sub-slice 4c of #438. Wires
package_is_installableinto the hoisted-graph walker shipped in 4b (#486), so optional packages on unsupported platforms get filtered intoresult.skippedand engine-strict mismatches surface as typed errors. Single-importer only; store I/O andprev_graphdiff still to come (4d, then 5).Behavior
Mirrors upstream's
if (!opts.force && packageIsInstallable(...) === false)gate at lockfileToHoistedDepGraph.ts:200-211:package_is_installable(Rust)null(no constraint)Ok(Installable)true(warn but proceed)Ok(ProceedWithWarning)false(optional + incompatible)Ok(SkipOptional)result.skipped, skip nodeUnsupportedEngineError(strict)Err(InstallabilityError::Engine)HoistedDepGraphError::InstallabilityInvalidNodeVersionErrorErr(InstallabilityError::InvalidNodeVersion)HoistedDepGraphError::InstallabilityNew options on
LockfileToHoistedDepGraphOptionsforce: bool— bypass the check entirely. Used by Slice 4d'sprev_graphwalk, where the previous lockfile is replayed wholesale so the orphan diff catches packages that would now filter out. Mirrors upstream'sforceat lockfileToHoistedDepGraph.ts:73.engine_strict,current_node_version,current_os,current_cpu,current_libc,supported_architectures— host-derived axes the check consumes.New output on
LockfileToDepGraphResultskipped: BTreeSet<String>— the inputopts.skippedcloned 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.skippedwithout 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 toresult.skipped, no graph entry, nohoisted_locations.walker_emits_required_dep_with_unsupported_platform_as_warning— same shape butoptional: false→ walker proceeds (matches upstreampackageIsInstallable === 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: trueemits an incompatible required dep without erroring.walker_emits_compatible_dep— sanity: compatible host + no constraint mismatch → graph entry, no skip.Test plan
walker_honors_pre_skipped_dep_pathupdated to use..Default::default()for the new option fields and to assertresult.skippedis the input-extended set.just ready(869 → 874 tests, all green) — including the typos check.cargo doc --document-private-items,taplo format --check,just dylintclean.if !state.opts.force { ... }block re-failswalker_skips_optional_dep_on_unsupported_platformandwalker_errors_on_engine_strict_mismatch.Note on upstream's verdict semantics
I initially scoped a test as
walker_errors_on_required_dep_with_unsupported_platform(expecting an error), but upstream'spackageIsInstallableonlythrows onengineStrict && warn instanceof UnsupportedEngineErrororInvalidNodeVersionError— required + platform mismatch returnstrue(warn but proceed). Adjusted the test to match upstream's behavior before pushing.What's next
prev_graphdiff. Walk the current lockfile alongside the wanted one (force: true, emptyskipped) and populateresult.prev_graphso Slice 5's linker can compute orphans.After 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
New Features
Bug Fixes