feat: filter the wanted lockfile when writing <virtual_store_dir>/lock.yaml (#434 slice 6)#495
Conversation
…k.yaml (#434 slice 6) Pacquet wrote the raw wanted lockfile as the current lockfile. Upstream pnpm writes a filtered version with optional + skipped subtrees pruned and `include` flags applied, so the next install diffs against what was actually materialized rather than the resolver's full ambition. Without the prune, dropped snapshots (slice 1 installability, slice 4 fetch failures, slice 5 `--no-optional`) were claimed present in the current lockfile and the follow-up install would skip work that should have run. Ports <https://github.com/pnpm/pnpm/blob/94240bc046/lockfile/filtering/src/filterLockfileByImportersAndEngine.ts#L46-L94> → <https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-restorer/src/index.ts#L687-L695>. Pacquet-specific simplification: instead of re-running the engine + supportedArchitectures + skipped checks at filter time, `filter_lockfile_for_current` reuses the `SkippedSnapshots` set already produced by the install pipeline. Its three subsets (`installability`, `fetch_failed`, `optional_excluded`) are the exact set upstream's filter would drop — same observable result, no duplicated walk. ## Changes - **`pacquet-lockfile`**: `Clone` derives on `Lockfile`, `LockfileSettings`, `ProjectSnapshot`, `SnapshotEntry`, `PackageMetadata`, `PeerDependencyMeta`, `ResolvedDependencySpec`. Needed to build a filtered lockfile by clone-and-mutate. - **`pacquet-package-manager/src/current_lockfile.rs`** (new): `filter_lockfile_for_current(lockfile, included, skipped) -> Lockfile`. Three-phase filter: 1. BFS the snapshot graph from filtered importer roots, skipping keys in `skipped`. Produces the reachable set. 2. Per-importer: clear dep maps whose `include` flag is false; trim `optional_dependencies` to entries whose target survived. 3. `snapshots:` / `packages:` filtered to the reachable set (packages key off `without_peer()` so peer-variant survivors keep their shared metadata row). - **`install.rs`**: replace the raw `save_current_to_virtual_store_dir` call with `filter_lockfile_for_current(...).save_current_to_virtual_store_dir(...)`. ## Tests 8 unit tests covering each filter behavior: - `skipped_snapshot_pruned_from_snapshots_and_importer_optional` - `include_optional_false_clears_importer_section` - `transitive_under_skipped_snapshot_is_pruned` - `snapshot_reachable_via_kept_path_survives` (mirrors upstream's `:712` shared-dep case at the filter level) - `packages_filtered_to_surviving_metadata_keys` (peer-variant metadata sharing) - `link_optional_entries_survive_post_filter` (workspace link: deps don't get post-filtered) - `empty_skipped_and_full_include_is_identity_for_reachables` (baseline) - `orphan_snapshots_are_pruned` (snapshots unreachable from any importer get dropped) Test-the-test verified by breaking the BFS walker — two tests fail. ## Out of scope - Hoisted-linker current-lockfile variant (`:633`) — pacquet's hoisted node-linker isn't fully wired through the lockfile-write path yet; tracked separately under #438. - `pnpm install --filter` slicing — pacquet has no `--filter` yet. Closes #493.
📝 WalkthroughWalkthroughThe PR implements filtered current-lockfile generation: Clone derives enable lockfile types to be cloned, a new ChangesFiltered Current Lockfile Generation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/package-manager/src/current_lockfile/tests.rs (3)
69-73: ⚡ Quick winUse
blob/mainfor pnpm upstream referencesLine [72], Line [112], Line [185], and Line [358] link to a pinned pnpm SHA. Please switch these rustdoc links to
https://github.com/pnpm/pnpm/blob/main/...so cross-references stay current with upstream behavior.As per coding guidelines: "When porting features... open files from
https://github.com/pnpm/pnpm/blob/main/...rather than from a permalinked SHA."Also applies to: 108-113, 183-186, 355-359
🤖 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/current_lockfile/tests.rs` around lines 69 - 73, Replace the pinned SHA URLs in the rustdoc links inside crates/package-manager/src/current_lockfile/tests.rs (the comments referencing filterLockfileByImportersAndEngine.ts and other pnpm upstream files) so they use the live branch path "https://github.com/pnpm/pnpm/blob/main/..." instead of the permalinked SHA; update each occurrence that currently points to a specific commit (the links near the comment mentioning filterLockfileByImportersAndEngine.ts and the other three referenced links) to the blob/main form to keep cross-references current with upstream.
96-106: ⚡ Quick winAdd diagnostic logging around non-
assert_eq!assertionsThese
assert!checks don’t print enough state on failure (e.g., keys present/missing). Addeprintln!context immediately before the assertions so failures are diagnosable without reruns.Based on learnings: "In Rust test code... add logging (e.g.,
eprintln!) for assertions likeassert!/assert_ne!unless the assertion isassert_eq!."Also applies to: 175-180, 214-219, 282-285, 316-319, 346-353, 380-383
🤖 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/current_lockfile/tests.rs` around lines 96 - 106, The failing assertions lack diagnostic output; before each non-assert_eq! check (e.g., the checks using snaps, key("keep","1.0.0"), key("drop","1.0.0"), and importer fields imp.optional_dependencies / imp.dependencies and pkg("keep")) add eprintln! lines that print the current snapshots keys (snaps.keys().collect::<Vec<_>>()), the presence/absence of the specific keys, and the importer optional/dependency maps so test failures show state; apply the same pattern to the other ranges mentioned (lines around the other assertions) so each assert! has a preceding eprintln! that prints the relevant variables (snaps, imp.optional_dependencies, imp.dependencies) and the keys being asserted.
221-286: ⚡ Quick winExercise real peer-variant keys in the metadata-filtering test
This test says it verifies peer-variant collapse via
without_peer(), but fixtures only use plain1.0.0versions. Add at least one actual peer-variant snapshot/package key pair so the test truly validates that metadata rows are retained/pruned correctly for peer variants.🤖 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/current_lockfile/tests.rs` around lines 221 - 286, The test packages_filtered_to_surviving_metadata_keys currently only uses plain version keys and doesn't exercise peer-variant collapse; update the fixtures to include at least one peer-variant snapshot/package pair (i.e., add an extra entry in snapshots and packages whose key is the peer-variant form of an existing key so it shares the same without_peer() metadata key as the surviving snapshot) and keep the same assertions so the test verifies that metadata rows for pruned peer-variants are removed while peer-variants that collapse to a surviving snapshot are retained; locate and modify the snapshots HashMap and packages HashMap construction inside the test function to insert that peer-variant key matching the naming convention used elsewhere (so filter_lockfile_for_current sees a real peer variant).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/package-manager/src/current_lockfile/tests.rs`:
- Around line 69-73: Replace the pinned SHA URLs in the rustdoc links inside
crates/package-manager/src/current_lockfile/tests.rs (the comments referencing
filterLockfileByImportersAndEngine.ts and other pnpm upstream files) so they use
the live branch path "https://github.com/pnpm/pnpm/blob/main/..." instead of the
permalinked SHA; update each occurrence that currently points to a specific
commit (the links near the comment mentioning
filterLockfileByImportersAndEngine.ts and the other three referenced links) to
the blob/main form to keep cross-references current with upstream.
- Around line 96-106: The failing assertions lack diagnostic output; before each
non-assert_eq! check (e.g., the checks using snaps, key("keep","1.0.0"),
key("drop","1.0.0"), and importer fields imp.optional_dependencies /
imp.dependencies and pkg("keep")) add eprintln! lines that print the current
snapshots keys (snaps.keys().collect::<Vec<_>>()), the presence/absence of the
specific keys, and the importer optional/dependency maps so test failures show
state; apply the same pattern to the other ranges mentioned (lines around the
other assertions) so each assert! has a preceding eprintln! that prints the
relevant variables (snaps, imp.optional_dependencies, imp.dependencies) and the
keys being asserted.
- Around line 221-286: The test packages_filtered_to_surviving_metadata_keys
currently only uses plain version keys and doesn't exercise peer-variant
collapse; update the fixtures to include at least one peer-variant
snapshot/package pair (i.e., add an extra entry in snapshots and packages whose
key is the peer-variant form of an existing key so it shares the same
without_peer() metadata key as the surviving snapshot) and keep the same
assertions so the test verifies that metadata rows for pruned peer-variants are
removed while peer-variants that collapse to a surviving snapshot are retained;
locate and modify the snapshots HashMap and packages HashMap construction inside
the test function to insert that peer-variant key matching the naming convention
used elsewhere (so filter_lockfile_for_current sees a real peer variant).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9352d197-7323-4943-b8f5-f04b20c084f0
📒 Files selected for processing (9)
crates/lockfile/src/lib.rscrates/lockfile/src/package_metadata.rscrates/lockfile/src/project_snapshot.rscrates/lockfile/src/resolved_dependency.rscrates/lockfile/src/snapshot_entry.rscrates/package-manager/src/current_lockfile.rscrates/package-manager/src/current_lockfile/tests.rscrates/package-manager/src/install.rscrates/package-manager/src/lib.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: copilot-pull-request-reviewer
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- 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/lib.rscrates/lockfile/src/project_snapshot.rscrates/package-manager/src/install.rscrates/lockfile/src/package_metadata.rscrates/lockfile/src/resolved_dependency.rscrates/lockfile/src/lib.rscrates/lockfile/src/snapshot_entry.rscrates/package-manager/src/current_lockfile.rscrates/package-manager/src/current_lockfile/tests.rs
🧠 Learnings (4)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/package-manager/src/lib.rscrates/lockfile/src/project_snapshot.rscrates/package-manager/src/install.rscrates/lockfile/src/package_metadata.rscrates/lockfile/src/resolved_dependency.rscrates/lockfile/src/lib.rscrates/lockfile/src/snapshot_entry.rscrates/package-manager/src/current_lockfile.rscrates/package-manager/src/current_lockfile/tests.rs
📚 Learning: 2026-05-13T19:22:48.951Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 478
File: crates/package-manager/src/hoisted_dep_graph.rs:51-55
Timestamp: 2026-05-13T19:22:48.951Z
Learning: When reviewing this Rust codebase, avoid introducing/using a newtype like `PkgIdWithPatchHash` in only one module (e.g., `hoisted_dep_graph.rs`) if other related pacquet modules still represent the upstream pnpm “pkg id with patch hash” as a plain `String` (e.g., `virtual_store_layout`). For type consistency, either keep `pkg_id_with_patch_hash` as `String` here, or require a workspace-wide sweep that defines/extracts the `PkgIdWithPatchHash` newtype once and updates all call sites together (otherwise defer the refactor to a coordinated follow-up PR).
Applied to files:
crates/package-manager/src/lib.rscrates/package-manager/src/install.rscrates/package-manager/src/current_lockfile.rscrates/package-manager/src/current_lockfile/tests.rs
📚 Learning: 2026-05-13T20:09:22.171Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 486
File: crates/package-manager/src/hoisted_dep_graph.rs:472-476
Timestamp: 2026-05-13T20:09:22.171Z
Learning: In pnpm/pacquet, when generating serialized `hoisted_locations` strings (used for `.modules.yaml`), normalize path separators to forward slashes (e.g., replace `\\` with `/`). This preserves cross-platform portability: the output must not use OS-native separators because upstream pnpm’s `path.relative` can return platform-specific separators, which would break consistency with pnpm/pacquet’s other serialized formats on Windows.
Applied to files:
crates/package-manager/src/lib.rscrates/package-manager/src/install.rscrates/package-manager/src/current_lockfile.rscrates/package-manager/src/current_lockfile/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/package-manager/src/current_lockfile/tests.rs
🔇 Additional comments (10)
crates/lockfile/src/snapshot_entry.rs (1)
15-15: LGTM!crates/lockfile/src/project_snapshot.rs (1)
7-7: LGTM!crates/lockfile/src/resolved_dependency.rs (1)
17-17: LGTM!crates/lockfile/src/lib.rs (1)
44-45: LGTM!Also applies to: 55-56
crates/lockfile/src/package_metadata.rs (1)
11-12: LGTM!Also applies to: 39-40
crates/package-manager/src/current_lockfile.rs (3)
43-90: LGTM!
102-134: LGTM!
140-213: LGTM!crates/package-manager/src/lib.rs (1)
8-8: LGTM!Also applies to: 36-36
crates/package-manager/src/install.rs (1)
399-412: LGTM!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #495 +/- ##
==========================================
- Coverage 88.90% 88.48% -0.42%
==========================================
Files 121 122 +1
Lines 12625 13079 +454
==========================================
+ Hits 11224 11573 +349
- Misses 1401 1506 +105 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.620024602,
"stddev": 0.08539266116813497,
"median": 2.6108966879000004,
"user": 2.59746296,
"system": 3.65112346,
"min": 2.5134523114,
"max": 2.7913605054,
"times": [
2.6765784504,
2.5584775854000004,
2.7913605054,
2.5413588014000004,
2.5652137024000004,
2.6721834024,
2.5134523114,
2.6565796734000005,
2.6602972024000002,
2.5647443854
]
},
{
"command": "pacquet@main",
"mean": 2.5502189904000003,
"stddev": 0.054076563122840496,
"median": 2.5482732304000004,
"user": 2.5174872600000002,
"system": 3.6526803599999993,
"min": 2.4596345114,
"max": 2.6640314664,
"times": [
2.5542789344,
2.5159323724,
2.5228391924,
2.6640314664,
2.5422675264000003,
2.4596345114,
2.5844927194,
2.5818447304000003,
2.5571701914,
2.5196982594
]
},
{
"command": "pnpm",
"mean": 5.9253374058,
"stddev": 0.06116807052240114,
"median": 5.9353805749,
"user": 8.70724066,
"system": 4.386755559999999,
"min": 5.816929266400001,
"max": 5.9927085644,
"times": [
5.8954008424,
5.9033511554,
5.9207519384000005,
5.816929266400001,
5.9500092114,
5.9927085644,
5.992316859400001,
5.9663106174000005,
5.8422502854000005,
5.973345317400001
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7404869149,
"stddev": 0.04334773501364333,
"median": 0.7277069756000001,
"user": 0.36929613999999994,
"system": 1.5900794799999995,
"min": 0.7018904786000001,
"max": 0.8555270486000001,
"times": [
0.8555270486000001,
0.7268486156,
0.7380787336000001,
0.7223031306000001,
0.7285653356,
0.7395587856000001,
0.7593761576000001,
0.7156888646,
0.7018904786000001,
0.7170319986
]
},
{
"command": "pacquet@main",
"mean": 0.7762306870000001,
"stddev": 0.04907570879102925,
"median": 0.7630729426,
"user": 0.37653573999999995,
"system": 1.6009176799999998,
"min": 0.7094241036000001,
"max": 0.8583833686000001,
"times": [
0.7927511446000001,
0.7357743636,
0.7479882606,
0.8583833686000001,
0.7854339446,
0.7710499296000001,
0.7094241036000001,
0.8567899886000001,
0.7496158106,
0.7550959556000001
]
},
{
"command": "pnpm",
"mean": 2.560263691,
"stddev": 0.10914608317259146,
"median": 2.5151799376,
"user": 3.0445330399999997,
"system": 2.1535446800000004,
"min": 2.4386195296,
"max": 2.8171876046,
"times": [
2.5048899886,
2.5070981316,
2.6240611746,
2.8171876046,
2.5085753316,
2.4386195296,
2.5217845436,
2.6106579156,
2.5999398926,
2.4698227976
]
}
]
} |
Summary
Slice 6 of the #434 umbrella (
Proper optionalDependencies support). Slices 1–5 (#439, #456, #467, #474, #485) handle installability,--no-optional, the fetch-failure swallow, and theSkippedSnapshotsplumbing. Pacquet was writing the raw wanted lockfile as the current lockfile at<virtual_store_dir>/lock.yaml. Upstream pnpm writes a filtered version — optional + skipped subtrees pruned,includeflags applied — so a follow-up install diffs against what was actually materialized rather than the resolver's full ambition. Without this prune, dropped snapshots were claimed present and the next install skipped work that should run.Ports
filterLockfileByImportersAndEngine→writeCurrentLockfile.Pacquet-specific simplification: instead of re-running engine + supportedArchitectures + skipped checks at filter time,
filter_lockfile_for_currentreuses theSkippedSnapshotsset the install pipeline already produced. Its three subsets (installability,fetch_failed,optional_excluded) are the exact set upstream's filter would drop — same observable result, no duplicated walk.Changes
pacquet-lockfile:Clonederives onLockfile,LockfileSettings,ProjectSnapshot,SnapshotEntry,PackageMetadata,PeerDependencyMeta,ResolvedDependencySpec. Needed to build a filtered lockfile by clone-and-mutate.pacquet-package-manager/src/current_lockfile.rs(new module):filter_lockfile_for_current(lockfile, included, skipped) -> Lockfile. Three phases:skipped. Produces the reachable set.includeflag is false; trimoptional_dependenciesto entries whose target survived (matchesfilterLockfileByImportersAndEngine.ts:75-83).snapshots:/packages:filtered to the reachable set. Packages key offwithout_peer()so peer-variant survivors keep their shared metadata row.install.rs: replace the rawsave_current_to_virtual_store_dircall withfilter_lockfile_for_current(...).save_current_to_virtual_store_dir(...).Test plan
8 unit tests covering each filter behavior:
skipped_snapshot_pruned_from_snapshots_and_importer_optionalinclude_optional_false_clears_importer_sectiontransitive_under_skipped_snapshot_is_prunedsnapshot_reachable_via_kept_path_survives(mirrors upstream's:712shared-dep case at the filter level)packages_filtered_to_surviving_metadata_keys(peer-variant metadata sharing)link_optional_entries_survive_post_filter(workspacelink:deps don't get post-filtered)empty_skipped_and_full_include_is_identity_for_reachables(baseline)orphan_snapshots_are_prunedsnapshot_reachable_via_kept_path_survives+empty_skipped_and_full_include_is_identity_for_reachablesfail.just ready(typos + fmt + check + nextest + clippy).taplo format --check.just dylint(perfectionist).RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --workspace.Out of scope
:633) — pacquet's hoisted node-linker isn't fully wired through the lockfile-write path yet; tracked separately under Support nodeLinker: 'hoisted' — umbrella #438.pnpm install --filterslicing — pacquet has no--filteryet.Closes #493.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Improvements
Refactor