fix(pacquet): port pnpm's lockfile-pruner BFS to re-derive transitive optional#11919
Merged
Conversation
…ansitive optional The resolver's per-node AND-fold updates only the directly-revisited package, so descendants walked first via an `optionalDependencies` edge stay stuck at `optional: true` even when a later non-optional path reaches them transitively. Upstream hides this from users by re-deriving the flag in `copyDependencySubGraph`; pacquet had no equivalent pass, so a fetch or build failure on a transitively-required package was silently tolerated as if it were optional. Port the BFS into `dependencies_graph_to_lockfile`: walk from the importer's direct deps (classified by manifest dep-group), recurse through each node's children with parent-inherited optional for regular edges and forced `optional: true` for `optionalDependencies` edges, then override `SnapshotEntry.optional` to `false` for any package reached by an all-non-optional path. Refs #11916.
📝 WalkthroughWalkthroughThis PR implements a BFS-based recomputation of per-DepPath ChangesOptional flag recomputation for lockfile snapshots
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Contributor
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11919 +/- ##
==========================================
+ Coverage 87.66% 87.70% +0.03%
==========================================
Files 215 215
Lines 25458 25517 +59
==========================================
+ Hits 22319 22380 +61
+ Misses 3139 3137 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
527-537: ⚡ Quick winTrim scenario narration in test comments.
These comment blocks restate behavior already encoded by the fixture and assertions. Prefer a short intent line and keep details in test code to avoid drift.
Suggested simplification
-/// Scenario from -/// [pnpm/pnpm#11916](https://github.com/pnpm/pnpm/issues/11916): root -/// declares `optionalDependencies.a` and `dependencies.b`; `a.dependencies = {c}` -/// and `b.dependencies = {a}`. The resolver's [`DependenciesGraphNode::optional`] -/// field is stale on `c` — the tree walker marks it `optional: true` -/// on the `optional → a → c` descent and then misses the AND-fold on -/// the `prod → b → a` revisit because the lazy-children path doesn't -/// re-traverse `a`'s subtree. The lockfile-pruner BFS re-derives the -/// flag from the importer-rooted graph, so `c` ends up `optional: -/// false` (reachable through the all-non-optional path `prod → b → -/// a → c`). +/// Regression for pnpm/pnpm#11916: ensures transitive optionality is +/// recomputed when a subgraph is later reachable via a non-optional path.As per coding guidelines, "Tests are documentation. Do not duplicate them in prose."
Also applies to: 547-562, 623-627
🤖 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 `@pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs` around lines 527 - 537, Shorten the verbose scenario comment above the test to a single intent line that states what the test verifies (e.g., "Verifies optional flag is derived correctly for node c when reachable via optional and prod paths"), remove the long narrative reproduction of the fixture behavior, and leave the concrete details in the test fixture and assertions; apply the same trimming to the other comment blocks referenced (the blocks around lines 547-562 and 623-627) so each becomes a concise purpose line rather than a paragraph of scenario narration.
🤖 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 `@pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs`:
- Around line 527-537: Shorten the verbose scenario comment above the test to a
single intent line that states what the test verifies (e.g., "Verifies optional
flag is derived correctly for node c when reachable via optional and prod
paths"), remove the long narrative reproduction of the fixture behavior, and
leave the concrete details in the test fixture and assertions; apply the same
trimming to the other comment blocks referenced (the blocks around lines 547-562
and 623-627) so each becomes a concise purpose line rather than a paragraph of
scenario narration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e961718-4fef-4da6-91d3-9c4c53efdc05
📒 Files selected for processing (2)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile/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: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: When porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.
Applied to files:
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.
Applied to files:
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.
Applied to files:
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rspacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs
🔇 Additional comments (2)
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile.rs (1)
76-78: LGTM!Also applies to: 248-255, 263-264, 382-394, 415-425, 427-484, 486-516
pacquet/crates/package-manager/src/dependencies_graph_to_lockfile/tests.rs (1)
538-621: LGTM!Also applies to: 629-725
Contributor
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.18726364562,
"stddev": 0.12391139374295311,
"median": 2.1506586568199997,
"user": 2.76661484,
"system": 3.45294446,
"min": 2.06176456532,
"max": 2.4339835963199996,
"times": [
2.37045595132,
2.4339835963199996,
2.09496634332,
2.2153360633199997,
2.1237581623199997,
2.06176456532,
2.1886691153199997,
2.11710076832,
2.1775591513199997,
2.08904273932
]
},
{
"command": "pacquet@main",
"mean": 2.09307631792,
"stddev": 0.0608400244059164,
"median": 2.08198835432,
"user": 2.7666918399999996,
"system": 3.40404006,
"min": 2.0047331523199996,
"max": 2.21965732432,
"times": [
2.21965732432,
2.1375123013199997,
2.10869323932,
2.0047331523199996,
2.13031758232,
2.0775120073199997,
2.0671557313199997,
2.0864647013199997,
2.07281739932,
2.02589974032
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6800963158,
"stddev": 0.021658110936869514,
"median": 0.6767633227000001,
"user": 0.37250715999999995,
"system": 1.4864576999999999,
"min": 0.6586653527,
"max": 0.7294931357000001,
"times": [
0.7294931357000001,
0.6848073297,
0.6996018047000001,
0.6757000217000001,
0.6725952687000001,
0.6593722217000001,
0.6828513167000001,
0.6778266237,
0.6600500827000001,
0.6586653527
]
},
{
"command": "pacquet@main",
"mean": 0.708843722,
"stddev": 0.0731396415005988,
"median": 0.6799125017000001,
"user": 0.3812505599999999,
"system": 1.4852541000000001,
"min": 0.6600369087000001,
"max": 0.8871574997000001,
"times": [
0.7925060027,
0.6600369087000001,
0.6710586627,
0.6707974937000001,
0.6686691377,
0.6776414027000001,
0.6859124777000001,
0.6821836007000001,
0.8871574997000001,
0.6924740337
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.6416440223600004,
"stddev": 0.053658827348005644,
"median": 2.64954447636,
"user": 4.415928439999999,
"system": 3.4444034199999995,
"min": 2.55823641586,
"max": 2.73561494086,
"times": [
2.64911246886,
2.65706840286,
2.56905821286,
2.61671691386,
2.73561494086,
2.64997648386,
2.55823641586,
2.68028450086,
2.68360652286,
2.61676536086
]
},
{
"command": "pacquet@main",
"mean": 2.63507434026,
"stddev": 0.03347283309333166,
"median": 2.6324899888599997,
"user": 4.42524874,
"system": 3.44452792,
"min": 2.58608990586,
"max": 2.69969181286,
"times": [
2.63571496886,
2.65875500786,
2.59552356986,
2.65977888086,
2.62667315686,
2.62926500886,
2.6121682648599998,
2.64708282586,
2.69969181286,
2.58608990586
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.7594251656600002,
"stddev": 0.025366411581126656,
"median": 1.75750965796,
"user": 2.06481134,
"system": 2.20893822,
"min": 1.72107799996,
"max": 1.7890045109600001,
"times": [
1.7759344299600002,
1.78757070796,
1.72660289196,
1.7650818919600002,
1.74993742396,
1.7890045109600001,
1.72107799996,
1.78744074296,
1.74223178296,
1.74936927396
]
},
{
"command": "pacquet@main",
"mean": 1.7730699185600003,
"stddev": 0.02088614397407783,
"median": 1.7683012154600002,
"user": 2.06760734,
"system": 2.20198382,
"min": 1.73854559396,
"max": 1.81624207196,
"times": [
1.7595970169600001,
1.77800096696,
1.81624207196,
1.79644783396,
1.76733847896,
1.7706561689600002,
1.76856800396,
1.76726862296,
1.73854559396,
1.7680344269600001
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/11919 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,641.64 ms(-43.35%)Baseline: 4,663.18 ms | 5,595.81 ms (47.21%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,759.43 ms(-51.63%)Baseline: 3,637.38 ms | 4,364.85 ms (40.31%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,187.26 ms(-5.97%)Baseline: 2,326.21 ms | 2,791.45 ms (78.36%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 680.10 ms(+3.34%)Baseline: 658.13 ms | 789.76 ms (86.11%) |
This was referenced May 25, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Pacquet's resolver inherits a longstanding pnpm-CLI quirk: on revisit of a package,
ResolvedPackage.optionalis AND-folded only on the directly-visited package — not on its transitive descendants. Walked first via an optional path and then revisited via a non-optional path, the direct package getsoptional: falsecorrectly, but its already-walked children stay stuck atoptional: true.Walk order:
optional → A → C(setsC.optional = true), thenprod → B → A(revisits A, setsA.optional = false). C should now beoptional: false(reachable through the all-non-optional pathprod → B → A → C) but the in-memoryResolvedPackage.optionalstaystrue.How pnpm CLI hides this from users
lockfile/pruner/src/index.ts:160-205(copyDependencySubGraph) does a BFS from importers through the dep graph, tracking anonOptionalset. Any package reached by an all-non-optional path is stampedoptional: falseon the lockfile snapshot. Install-time consumers read from the post-pruner snapshot, so they never see the stale resolver value.Why pacquet was exposed
Pacquet had no equivalent pruner pass.
dependencies_graph_to_lockfile.rswrote the rawnode.optional(sourced from the resolver) straight to the lockfile snapshot, and the install-time gates (create_virtual_store.rsfor fetch-side failures,installability.rsfor build-side,link_hoisted_modules.rs/hoisted_dep_graph.rsfor the hoisted linker) all read from that snapshot. A fetch or build failure on a transitively-required package was silently tolerated as if it were optional.Fix
Port
copyDependencySubGraph's BFS into pacquet:compute_corrected_optionalindependencies_graph_to_lockfile.rs. Iterative BFS from the importer's direct deps, classified by manifest dep-group (dev / optional / prod), inheriting the parent'soptionalfor regular edges and forcingoptional: trueforoptionalDependencies/ optional-peer edges.build_snapshot_entryconsults the resultingHashMap<DepPath, bool>and falls back toDependenciesGraphNode::optionalfor nodes the BFS never reached (graph entries unreachable from any importer, matching upstream's "untouched depLockfile keeps its existing flag" arm).Tests
Two new tests in
dependencies_graph_to_lockfile/tests.rs:transitive_optional_is_recomputed_for_packages_reachable_via_a_non_optional_path— the exact scenario from the issue. Verified failing on the un-ported code before the fix landed.shared_subdep_reached_through_dev_optional_and_prod_paths_is_marked_non_optional— ported from upstream pnpm's'subdependency is both optional and dev'pruner test.Test plan
cargo nextest run -p pacquet-package-manager --lib dependencies_graph_to_lockfile::tests— 8 tests passcargo nextest run -p pacquet-package-manager— 305 tests passcargo nextest run -p pacquet-resolving-deps-resolver— 56 tests passjust checkandjust lint(-- --deny warnings) cleanCloses #11916.
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Bug Fixes
Tests