Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat: persist .modules.yaml.skipped + headless seed-and-recompute (#434 slice 3)#467

Merged
zkochan merged 2 commits into
mainfrom
feat-modules-yaml-skipped
May 13, 2026
Merged

feat: persist .modules.yaml.skipped + headless seed-and-recompute (#434 slice 3)#467
zkochan merged 2 commits into
mainfrom
feat-modules-yaml-skipped

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Slice 3 of the #434 umbrella (Proper optionalDependencies support). Slice 1 (#439) computed a SkippedSnapshots set 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:

  • WriteModules.skipped was declared at crates/modules-yaml/src/lib.rs:197 with sort-on-write 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.

  • Seedinstall_frozen_lockfile::run reads .modules.yaml.skipped before computing the in-memory set and passes the parsed PackageKeys 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-circuitcompute_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. 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 now returns a small InstallFrozenLockfileOutput { hoisted_dependencies, skipped } struct so Install::run can thread both into .modules.yaml. Read errors on .modules.yaml degrade 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 with vec![] 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

  • Current-lockfile write (<virtual_store_dir>/lock.yaml) — umbrella slice 6.
  • --no-optional plumbing — umbrella slice 5.
  • --force bypass (pacquet doesn't expose the flag yet).

Closes #463.


Written by an agent (Claude Code, claude-opus-4-7).

Summary by CodeRabbit

  • Bug Fixes

    • Skipped optional dependencies are preserved across reinstalls, including frozen installs, preventing unnecessary rechecks or reinstalls.
  • New Features

    • Installation metadata now records a persisted "skipped" list in the modules manifest, and writes are sorted for deterministic output.
  • Tests

    • Added tests to verify serialization, persistence, preservation, and behavior of seeded skipped entries across installs.

Review Change Stack

… 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.
Copilot AI review requested due to automatic review settings May 13, 2026 16:17
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 33eaca81-6d47-4405-ad4b-2408f2899d18

📥 Commits

Reviewing files that changed from the base of the PR and between 7bae53b and 439b1b3.

📒 Files selected for processing (1)
  • crates/package-manager/src/install/tests.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/package-manager/src/install/tests.rs
📜 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)
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest

📝 Walkthrough

Walkthrough

This PR implements cross-install persistence of optional-dependency skip decisions. Previously-skipped snapshots are seeded from .modules.yaml.skipped on subsequent installs, short-circuiting rechecks and preserving skip state; the skipped set is written back into .modules.yaml after install.

Changes

Skipped snapshot seeding and persistence

Layer / File(s) Summary
Seeded skip computation in installability
crates/package-manager/src/installability.rs, crates/package-manager/src/installability/tests.rs
SkippedSnapshots::from_strings() parses serialized strings into PackageKey. compute_skipped_snapshots now accepts seed: SkippedSnapshots, initializes working skipped from seed, returns the seed unchanged on the fast path (no constraints), and short-circuits per-snapshot re-checks for seeded entries. Tests updated to pass explicit seed; new tests validate seeded short-circuiting and fast-path preservation.
Load and thread skip seed in frozen-lockfile install
crates/package-manager/src/install_frozen_lockfile.rs
InstallFrozenLockfile::run reads .modules.yaml via read_modules_manifest::<RealApi> to obtain a SkippedSnapshots seed (falls back to empty with a warning on read error), threads the seed into compute_skipped_snapshots, and returns InstallFrozenLockfileOutput { hoisted_dependencies, skipped }.
Capture and persist skipped set through Install
crates/package-manager/src/install.rs, crates/package-manager/src/install/tests.rs
Install::run now receives both hoisted_dependencies and SkippedSnapshots from dispatch, passes &skipped into build_modules_manifest, and Modules.skipped is populated from the skipped set (as sorted dependency-path strings). Tests cover non-empty/empty serialization and frozen reinstall preservation and sorting.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pacquet#439: Adds compute_skipped_snapshots plumbing this PR builds on; closely connected to the skip logic.
  • pnpm/pacquet#456: Also modifies compute_skipped_snapshots and frozen install path (supported_architectures); complementary changes.
  • pnpm/pacquet#407: Related changes to .modules.yaml generation which this PR populates with skipped entries.

Poem

🐰 I nibble seeds from last night's write,
tuck skipped snapshots safe and tight.
Across installs the memory stays,
no recheck dance, no extra plays.
The manifest hums — all sorted, right.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title precisely describes the main change: persisting .modules.yaml.skipped and implementing seed-and-recompute logic as part of #434 slice 3.
Description check ✅ Passed The description fully covers the scope, implementation details, and testing strategy, and follows the repository template with Summary, Linked issue, Upstream reference, and Checklist sections.
Linked Issues check ✅ Passed All coding requirements from issue #463 are met: write-path serialization of skipped snapshots into Modules.skipped, read-seed path to load and parse .modules.yaml.skipped, short-circuit logic in compute_skipped_snapshots, and comprehensive test coverage.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing the three core requirements (write, seed, short-circuit) from issue #463 with appropriate test updates; out-of-scope items are explicitly listed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat-modules-yaml-skipped

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/package-manager/src/install.rs (1)

472-472: ⚡ Quick win

Sort Modules.skipped before returning the manifest.

SkippedSnapshots iterates a HashSet, so this produces a nondeterministic Modules.skipped order. The writer may sort later, but build_modules_manifest is 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

📥 Commits

Reviewing files that changed from the base of the PR and between c614d64 and 7bae53b.

📒 Files selected for processing (5)
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/install/tests.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/installability.rs
  • crates/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 and pipe-trait chains; do not break them into intermediate let bindings 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 (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use 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::*; in lib.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 plain String or &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 via TryFrom<String> and/or FromStr; 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 infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_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.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/installability.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/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.rs
  • crates/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.rs
  • crates/package-manager/src/install.rs
  • crates/package-manager/src/installability.rs
  • crates/package-manager/src/install_frozen_lockfile.rs
  • crates/package-manager/src/installability/tests.rs

Comment thread crates/package-manager/src/install_frozen_lockfile.rs
@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.77108% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.65%. Comparing base (c614d64) to head (439b1b3).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...tes/package-manager/src/install_frozen_lockfile.rs 66.66% 4 Missing ⚠️
crates/package-manager/src/install.rs 96.55% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.03     16.9±0.49ms   256.2 KB/sec    1.00     16.4±0.30ms   264.2 KB/sec

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_strings and seed-aware recomputation/fast-path behavior.
  • Reads .modules.yaml.skipped during frozen installs and returns skipped snapshots alongside hoist results.
  • Writes skipped depPath strings into .modules.yaml and 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.

Comment thread crates/package-manager/src/install/tests.rs Outdated
Comment thread crates/package-manager/src/install_frozen_lockfile.rs
@github-actions

github-actions Bot commented May 13, 2026

Copy link
Copy Markdown

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.235 ± 0.129 2.113 2.444 1.03 ± 0.06
pacquet@main 2.169 ± 0.050 2.106 2.274 1.00
pnpm 5.429 ± 0.053 5.343 5.511 2.50 ± 0.06
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)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 545.8 ± 45.9 518.9 672.1 1.00
pacquet@main 612.5 ± 42.7 568.0 698.4 1.12 ± 0.12
pnpm 2185.0 ± 148.7 2029.2 2524.3 4.00 ± 0.43
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.
@zkochan zkochan merged commit 4882312 into main May 13, 2026
23 of 24 checks passed
@zkochan zkochan deleted the feat-modules-yaml-skipped branch May 13, 2026 16:53
zkochan added a commit that referenced this pull request May 13, 2026
…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.
zkochan added a commit that referenced this pull request May 13, 2026
…#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`).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Persist .modules.yaml.skipped + headless seed-and-recompute (#434 slice 3)

2 participants