feat: persist .modules.yaml.skipped + headless seed-and-recompute (#434 slice 3)#467
Conversation
… slice 3) Slice 1 (#439) computed a `SkippedSnapshots` set on every frozen-lockfile install but never serialized it; slice 2 (#456) wired `supportedArchitectures` so the input side became complete. This slice closes the loop by mirroring the three upstream sites pinned at pnpm/pnpm@94240bc046: - **Write** — `Modules.skipped` was already declared at `modules-yaml/src/lib.rs:197` with sort-on-write at `:407` but always serialized empty. `build_modules_manifest` now takes `&SkippedSnapshots` and produces the depPath string list pnpm writes at `installing/deps-installer/src/install/index.ts:1625`. Empty set still produces an empty list — fresh installs unchanged. - **Seed** — `install_frozen_lockfile::run` reads `.modules.yaml.skipped` before computing the in-memory set and passes the parsed `PackageKey`s as the seed to `compute_skipped_snapshots`. Mirrors upstream's load at `installing/read-projects-context/src/index.ts:79` plus the early return at `deps/graph-builder/src/lockfileToDepGraph.ts:194`. - **Short-circuit** — `compute_skipped_snapshots` returns the seed verbatim on the fast path (no constraints in the lockfile) and, on the slow path, skips the per-snapshot re-check for keys already in the seed without emitting a duplicate `pnpm:skipped-optional-dependency` event. The non-seeded snapshots still re-run `package_is_installable`, covering the "host arch changed since last install" case upstream guards at `:206-215`. `InstallFrozenLockfile::run` returns the final `SkippedSnapshots` so `Install::run` can thread it into `build_modules_manifest`. Read errors on `.modules.yaml` degrade to an empty seed — the file is a cache artifact, not authoritative. Tests: - `installability/tests::seeded_snapshot_short_circuits_recheck` pins the no-re-emit behavior for a seeded key whose metadata remains incompatible (test-the-test: removing the short-circuit fires the event and the assertion catches it). - `installability/tests::fast_path_preserves_seed` covers the constraint-free path. - `installability/tests::from_strings_skips_unparsable_entries` pins the tolerance for orphan / malformed strings. - `install/tests::build_modules_manifest_serializes_skipped_set` + `_skipped_is_empty_on_empty_set` cover the write path (test-the-test: replacing the wire-up with `vec![]` flips the former to fail). Closes #463.
|
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)
📜 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)
📝 WalkthroughWalkthroughThis PR implements cross-install persistence of optional-dependency skip decisions. Previously-skipped snapshots are seeded from ChangesSkipped snapshot seeding and persistence
Sequence Diagram(s)sequenceDiagram
participant InstallCLI as Install CLI
participant InstallRun as Install::run
participant InstallFL as InstallFrozenLockfile::run
participant ReadManifest as read_modules_manifest
participant ComputeSkip as compute_skipped_snapshots
participant BuildManifest as build_modules_manifest
participant WriteManifest as write_modules_manifest
InstallCLI->>InstallRun: frozen-lockfile install
InstallRun->>InstallFL: run frozen-lockfile phase
InstallFL->>ReadManifest: load prior modules manifest
alt Cache exists
ReadManifest-->>InstallFL: SkippedSnapshots seed
else Cache missing or error
ReadManifest-->>InstallFL: empty seed
end
InstallFL->>ComputeSkip: compute with seed
ComputeSkip-->>InstallFL: SkippedSnapshots (seeded entries preserved)
InstallFL-->>InstallRun: InstallFrozenLockfileOutput
InstallRun->>BuildManifest: pass hoisted deps and skipped set
BuildManifest-->>InstallRun: Modules with skipped field
InstallRun->>WriteManifest: write modules manifest
WriteManifest-->>InstallCLI: .modules.yaml with .skipped entries
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
🧹 Nitpick comments (1)
crates/package-manager/src/install.rs (1)
472-472: ⚡ Quick winSort
Modules.skippedbefore returning the manifest.
SkippedSnapshotsiterates aHashSet, so this produces a nondeterministicModules.skippedorder. The writer may sort later, butbuild_modules_manifestis already consumed directly in tests and is now part of the skipped-state contract; sorting here keeps the returned manifest stable and matches pnpm’s sorted depPath list.Suggested change
- skipped: skipped.iter().map(ToString::to_string).collect(), + skipped: { + let mut skipped: Vec<_> = skipped.iter().map(ToString::to_string).collect(); + skipped.sort(); + skipped + },🤖 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/install.rs` at line 472, build_modules_manifest currently collects Modules.skipped from a HashSet producing nondeterministic order; change the collection to first collect into a Vec<String> (e.g., let mut skipped_vec = skipped.iter().map(ToString::to_string).collect::<Vec<_>>()), sort it (prefer sort_unstable()), and then use that sorted vector for the Modules { skipped: ... } field so Modules.skipped is stable; update the area around the existing "skipped: skipped.iter().map(ToString::to_string).collect()," expression accordingly in build_modules_manifest.
🤖 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 258-259: The seed currently uses all entries from manifest.skipped
unchecked; change the code that builds seed (the match arm using
read_modules_manifest::<RealApi> and
SkippedSnapshots::from_strings(&manifest.skipped)) to filter the parsed
SkippedSnapshots so it only keeps depPaths that exist in the current wanted
lockfile snapshots (i.e., intersect the seed with snapshots.keys()) before using
it for the fast path or writing .modules.yaml; locate the seed construction near
the read_modules_manifest call and apply the intersection operation on the
SkippedSnapshots instance so only snapshots present in snapshots.keys() are
carried forward.
---
Nitpick comments:
In `@crates/package-manager/src/install.rs`:
- Line 472: build_modules_manifest currently collects Modules.skipped from a
HashSet producing nondeterministic order; change the collection to first collect
into a Vec<String> (e.g., let mut skipped_vec =
skipped.iter().map(ToString::to_string).collect::<Vec<_>>()), sort it (prefer
sort_unstable()), and then use that sorted vector for the Modules { skipped: ...
} field so Modules.skipped is stable; update the area around the existing
"skipped: skipped.iter().map(ToString::to_string).collect()," expression
accordingly in build_modules_manifest.
🪄 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: a7735e0b-0468-476e-b4b7-fa50e0142124
📒 Files selected for processing (5)
crates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/installability.rscrates/package-manager/src/installability/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). (6)
- GitHub Check: Agent
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-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/tests.rscrates/package-manager/src/install.rscrates/package-manager/src/installability.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/installability/tests.rs
🧠 Learnings (2)
📚 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.rscrates/package-manager/src/installability/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/install/tests.rscrates/package-manager/src/install.rscrates/package-manager/src/installability.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/installability/tests.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #467 +/- ##
=======================================
Coverage 88.64% 88.65%
=======================================
Files 116 116
Lines 9952 9995 +43
=======================================
+ Hits 8822 8861 +39
- Misses 1130 1134 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Pull request overview
This PR persists installability-skipped optional dependency snapshots into .modules.yaml.skipped and seeds subsequent frozen-lockfile installs from that persisted state, completing the read/write loop for optional dependency skipping.
Changes:
- Adds
SkippedSnapshots::from_stringsand seed-aware recomputation/fast-path behavior. - Reads
.modules.yaml.skippedduring frozen installs and returns skipped snapshots alongside hoist results. - Writes skipped depPath strings into
.modules.yamland adds focused unit tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/package-manager/src/installability.rs |
Adds parsing of persisted skipped entries and seed-aware skip recomputation. |
crates/package-manager/src/installability/tests.rs |
Updates existing tests for the new seed parameter and adds seed/fast-path parsing tests. |
crates/package-manager/src/install_frozen_lockfile.rs |
Reads .modules.yaml.skipped, threads the seed through installability, and returns skipped output. |
crates/package-manager/src/install.rs |
Threads frozen skipped output into .modules.yaml generation. |
crates/package-manager/src/install/tests.rs |
Adds tests for manifest serialization of skipped snapshots. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.2347020872199996,
"stddev": 0.12873100592260467,
"median": 2.1685892009199996,
"user": 2.71464208,
"system": 2.17764532,
"min": 2.1130844779199998,
"max": 2.44447998692,
"times": [
2.18490054192,
2.33048876892,
2.1296293289199997,
2.12877257092,
2.12529847092,
2.3410072129199997,
2.1130844779199998,
2.15227785992,
2.39708165292,
2.44447998692
]
},
{
"command": "pacquet@main",
"mean": 2.16940326412,
"stddev": 0.05003676264604298,
"median": 2.1738685459199996,
"user": 2.70793048,
"system": 2.13384742,
"min": 2.10612476092,
"max": 2.27402193692,
"times": [
2.1707324839199997,
2.17700460792,
2.10612476092,
2.13387308992,
2.12264763192,
2.17788150092,
2.13175960892,
2.18108079392,
2.21890622592,
2.27402193692
]
},
{
"command": "pnpm",
"mean": 5.429281428320001,
"stddev": 0.05348084356687257,
"median": 5.44102506892,
"user": 8.785257479999999,
"system": 2.7051264199999996,
"min": 5.34333117192,
"max": 5.51140482092,
"times": [
5.40261264892,
5.34955492092,
5.454496858920001,
5.51140482092,
5.43942111892,
5.34333117192,
5.45056326192,
5.483775580920001,
5.415024880920001,
5.44262901892
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.5458054676600002,
"stddev": 0.04587033631111286,
"median": 0.5319530728600002,
"user": 0.37561345999999995,
"system": 0.9529102399999999,
"min": 0.5188880798600001,
"max": 0.6720628918600001,
"times": [
0.6720628918600001,
0.5577775688600001,
0.5299071718600001,
0.5342149708600001,
0.5188880798600001,
0.54181762186,
0.5217935758600001,
0.5339989738600001,
0.5193102688600001,
0.5282835528600001
]
},
{
"command": "pacquet@main",
"mean": 0.6124884234600001,
"stddev": 0.04274902527434668,
"median": 0.6053241468600001,
"user": 0.37803896000000003,
"system": 0.9716438399999999,
"min": 0.56801668786,
"max": 0.69837699886,
"times": [
0.69837699886,
0.5713372868600001,
0.57661586486,
0.61791578686,
0.56801668786,
0.6299573638600001,
0.5840425788600001,
0.62392832486,
0.5927325068600001,
0.6619608348600001
]
},
{
"command": "pnpm",
"mean": 2.1850401695599997,
"stddev": 0.14871175445902193,
"median": 2.13055410486,
"user": 2.86980116,
"system": 1.2879754399999999,
"min": 2.02920388086,
"max": 2.52425152586,
"times": [
2.31487102286,
2.26961426286,
2.06439958086,
2.10077574586,
2.0970925118599997,
2.52425152586,
2.1890849548599998,
2.11760624086,
2.14350196886,
2.02920388086
]
}
]
} |
…d read Adds `frozen_install_preserves_seeded_skipped_across_reinstall`: pre-writes `.modules.yaml.skipped` with two entries (one bare, one scoped), runs a frozen-lockfile install against an empty lockfile, and asserts both entries survive the round-trip verbatim — sorted alphabetically by `write_modules_manifest`. Covers the read/seed/write plumbing end-to-end (load, [`SkippedSnapshots::from_strings`] parse, threading through [`InstallFrozenLockfileOutput`], `build_modules_manifest` serialization, `write_modules_manifest` sort-on-write). The slow path is already covered by `seeded_snapshot_short_circuits_recheck` and `fast_path_preserves_seed`; this one fills the gap raised by the PR #467 review on plumbing-level coverage. Test-the-test verified: stubbing `read_modules_manifest::<RealApi>` to ignore the loaded skipped list flips the test to fail with `left: 0, right: 2`. Also tightens the comment on `build_modules_manifest_serializes_skipped_set` so it doesn't reference a non-existent read-back loop.
…e payload (#434 slice 4) (#474) Slice 4 of the [#434 umbrella](#434) (`Proper optionalDependencies support`). Slices 1–3 (#439, #456, #467) close the installability-driven skip path; this slice adds the second skip pathway upstream handles in the frozen install: an `optional: true` snapshot whose **fetch / extract** step fails at install time must not abort the install. Local-materialization errors (`CreateVirtualDir`) and config-shape errors still abort even for optional snapshots — matching upstream's `linkPkg` path which sits outside the catch. Mirrors the silent catch sites at [`deps/graph-builder/src/lockfileToDepGraph.ts:294-298`](https://github.com/pnpm/pnpm/blob/94240bc046/deps/graph-builder/src/lockfileToDepGraph.ts#L294-L298) and [`installing/deps-restorer/src/index.ts:912-921`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L912-L921): ```ts try { ... fetch ... } catch (err) { if (pkgSnapshot.optional) return; throw err } ``` ## Changes - **Reporter**: `SkippedOptionalPackage` becomes a `#[serde(untagged)]` enum with `Installed { id, name, version }` and `ResolutionFailure { name?, version?, bareSpecifier }` variants. Models upstream's discriminated union at [`core-loggers/src/skippedOptionalDependencyLogger.ts:10-31`](https://github.com/pnpm/pnpm/blob/94240bc046/core/core-loggers/src/skippedOptionalDependencyLogger.ts#L10-L31). The new variant is wire-shape-only in slice 4 — pacquet has no resolver yet, but a future resolver port at the upstream emit site ([`resolveDependencies.ts:1376-1383`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-resolver/src/resolveDependencies.ts#L1376-L1383)) lands without re-touching this type. - **`SkippedSnapshots`**: gains a `fetch_failed` subset disjoint from the existing `installability` subset. `contains` / `iter` return the union (downstream consumers treat both as absent); `iter_installability` returns only the persistent subset that `.modules.yaml.skipped` records, matching upstream's behavior of never updating `opts.skipped` at the fetch-failure catch sites. - **`CreateVirtualStore`**: cold-batch dispatch swallows per-snapshot errors when `snapshot.optional` is true **and** the error is fetch-side. A new `is_fetch_side_failure` helper restricts the swallow to `InstallPackageBySnapshotError::DownloadTarball` and `::GitFetch` — the same surface upstream wraps in `storeController.fetchPackage`. Local-materialization (`CreateVirtualDir`) and config-shape errors (`MissingTarballIntegrity` / `UnsupportedResolution`) propagate even for optional snapshots, matching upstream's `linkPkg` path which sits outside the catch. Side benefit: the warm-batch path (which only surfaces `CreateVirtualDir` errors) stays consistently abort-on-error without a parallel swallow site. The per-install `fetch_failed: HashSet<PackageKey>` rides out on `CreateVirtualStoreOutput`. - **`InstallFrozenLockfile::run`**: folds the returned `fetch_failed` into its mutable `SkippedSnapshots` so downstream phases (`SymlinkDirectDependencies`, `LinkVirtualStoreBins`, `BuildModules`, hoist) treat the dropped snapshots as absent through the same gate they already use for installability skips. ## Test plan - [x] `reporter::tests::skipped_optional_resolution_failure_event_matches_pnpm_wire_shape` — full payload (`bareSpecifier` camelCase, no `id`). - [x] `reporter::tests::skipped_optional_resolution_failure_omits_absent_name_and_version` — optional-field shape. - [x] `install::tests::frozen_install_silently_swallows_unreachable_optional_tarball` — ports [`installing/deps-restorer/test/index.ts:340-360`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/test/index.ts#L340-L360): unreachable optional tarball (`http://127.0.0.1:1/...`), install succeeds, slot not created, `.modules.yaml.skipped` left empty. **Test-the-test verified** by inverting the `optional` guard in the cold-batch match arm. Runs with `enable_global_virtual_store = false` so the slot-path assertion targets the legacy layout. - [x] `install::tests::frozen_install_propagates_non_optional_fetch_failure` — pins the polarity: same fixture, non-optional snapshot, install must error. - [x] `just ready` (typos + fmt + check + nextest + clippy). - [x] `taplo format --check`. - [x] `just dylint` (perfectionist). - [x] `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace`. ## Out of scope - Resolver-side `resolution_failure` emit site — needs a resolver, tracked separately. - `--no-optional` / `--omit=optional` plumbing — umbrella slice 5. - Surfacing fetch failures to the reporter on the frozen path — upstream itself is silent here; only the resolver-side emit fires `pnpm:skipped-optional-dependency`. Closes #471.
…#485) Slice 5 of the [#434 umbrella](#434) (`Proper optionalDependencies support`). Slices 1–4 (#439, #456, #467, #474) handle installability + the fetch-failure swallow. This slice closes the user-facing `--no-optional` flag: the CLI flag already exists and threads through `Install::dependency_groups`, but only `SymlinkDirectDependencies` and the on-disk `included` field actually honored it. The rest of the install pipeline walked `optional_dependencies` unconditionally, so the optional subtree was still extracted, linked, and built. Mirrors upstream's depNode filter at [`installing/deps-installer/src/install/link.ts:109-111`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/link.ts#L109-L111): ```ts if (!opts.include.optionalDependencies) { depNodes = depNodes.filter(({ optional }) => !optional) } ``` ## Changes - **`SkippedSnapshots`**: gains a third disjoint subset `optional_excluded` alongside `installability` (slice 1) and `fetch_failed` (slice 4). The existing `contains` / `iter` already return the union, so every downstream consumer that filters by skip-set (`CreateVirtualStore`, `SymlinkDirectDependencies`, `BuildModules`, hoist, `link_bins`) now respects `--no-optional` through the same gate. `iter_installability` still returns only the persistent subset for `.modules.yaml.skipped` writes — `--no-optional` exclusions are transient (matching upstream's behavior of never updating `opts.skipped` at the filter site). - **`InstallFrozenLockfile::run`**: iterates the lockfile snapshots once and inserts every `snap.optional == true` entry into the new subset when the dispatch list doesn't contain `DependencyGroup::Optional`. The `SnapshotEntry::optional` flag is set by the resolver only when the snapshot is reachable **exclusively** through optional edges, so a snapshot reachable through any non-optional edge carries `optional: false` and survives the filter — covers [`optionalDependencies.ts:712`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/test/install/optionalDependencies.ts#L712) (`dependency that is both optional and non-optional is installed`).
Summary
Slice 3 of the #434 umbrella (
Proper optionalDependencies support). Slice 1 (#439) computed aSkippedSnapshotsset on every frozen-lockfile install but never serialized it; slice 2 (#456) closed the input side. This PR closes the loop by mirroring three upstream sites pinned at pnpm/pnpm@94240bc046:Write —
Modules.skippedwas declared atcrates/modules-yaml/src/lib.rs:197with sort-on-write but always serialized empty.build_modules_manifestnow takes&SkippedSnapshotsand produces the depPath string list pnpm writes atinstalling/deps-installer/src/install/index.ts:1625.Seed —
install_frozen_lockfile::runreads.modules.yaml.skippedbefore computing the in-memory set and passes the parsedPackageKeys as the seed tocompute_skipped_snapshots. Mirrors upstream's load atinstalling/read-projects-context/src/index.ts:79plus the early return atdeps/graph-builder/src/lockfileToDepGraph.ts:194.Short-circuit —
compute_skipped_snapshotsreturns the seed verbatim on the fast path (no constraints in the lockfile) and, on the slow path, skips the per-snapshot re-check for keys already in the seed without emitting a duplicatepnpm:skipped-optional-dependencyevent. Non-seeded snapshots still re-runpackage_is_installable, covering the "host arch changed since last install" case upstream guards at:206-215.InstallFrozenLockfile::runnow returns a smallInstallFrozenLockfileOutput { hoisted_dependencies, skipped }struct soInstall::runcan thread both into.modules.yaml. Read errors on.modules.yamldegrade to an empty seed — the file is a cache artifact, not authoritative.Test plan
installability/tests::seeded_snapshot_short_circuits_recheck— verified with test-the-test: remove the short-circuit and the test fails on the duplicate event assertion.installability/tests::fast_path_preserves_seed— constraint-free path.installability/tests::from_strings_skips_unparsable_entries— orphan / malformed strings.install/tests::build_modules_manifest_serializes_skipped_set+_skipped_is_empty_on_empty_set— write path. Verified with test-the-test: replace the wire-up withvec![]and the former fails.just ready— typos + fmt + check + nextest (192 package-manager tests + 850 across the workspace) + clippy.taplo format --check.just dylint(perfectionist).RUSTDOCFLAGS=\"-D warnings\" cargo doc --no-deps --workspace.Out of scope
<virtual_store_dir>/lock.yaml) — umbrella slice 6.--no-optionalplumbing — umbrella slice 5.--forcebypass (pacquet doesn't expose the flag yet).Closes #463.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
New Features
Tests