perf(pacquet): lazy children realization in dependency tree#11915
Conversation
… until peer resolution
Mirrors upstream pnpm's lazy `children` thunk on `DependenciesTreeNode`:
revisits of a `pkgIdWithPatchHash` no longer recurse to fan out a fresh
NodeId subtree eagerly. Instead the tree node records
`TreeChildren::Lazy { parent_ids }` and the peer resolver allocates
per-occurrence child NodeIds on first descent via `realize_children`,
applying the same `parentIdsContainSequence` cycle break upstream uses
in `buildTree`.
Pure subtrees that the peer resolver already short-circuits through
`purePkgs` (ported in #11906) now skip realization entirely — the tree
never gets walked past the first occurrence of those packages.
Bench (astro deep tree, cold store, single resolver phase):
- tree nodes: 74,940 → 4,069 (~18× smaller)
- `resolve_importer`: 11.6s → 8.2s (~1.42× faster)
Refs #11907.
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? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe resolver now caches per-package ChildEdge vectors and emits TreeChildren::Lazy on revisits; resolve_peers accepts a mutable ResolvedTree and realizes lazy children on-demand during traversal, updating the tree in-place and preserving deterministic NodeId/leaf behavior. ChangesLazy child realization for dependency resolution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs`:
- Around line 543-607: The revisit branch returns TreeChildren::Lazy which stops
walking the cached subtree and thus never re-applies ChildEdge.optional to
descendants; fix by recording and reusing the per-package children_by_id
(Vec<crate::resolved_tree::ChildEdge>) when creating the lazy entry and, in the
revisit path (where TreeChildren::Lazy is constructed/consumed), iterate the
cached children_by_id for this package and propagate each ChildEdge.optional
combined with current_is_optional into the cached subtree (updating
ResolvedPackage.optional for descendant pkg ids) before returning so transitive
optional flags reflect the revisiting path; key symbols: TreeChildren::Lazy,
children_by_id, ChildEdge.optional, child_specs_by_id, resolve_node,
ResolvedPackage.optional.
In `@pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs`:
- Around line 861-867: The lazy-realization branch currently recomputes
leaf-ness using children_by_id and peer_dependencies (edge.pkg_id,
tree.children_by_id, tree.packages) which is weaker than the eager walk's
pkg_is_leaf logic and causes inconsistent NodeId assignment (NodeId::next vs
NodeId::leaf). Change this branch to reuse the first-walk leaf classification
instead of recomputing: call or consult the same
resolve_dependency_tree::pkg_is_leaf result used during the eager walk (or read
the persisted leaf-flag recorded during the first walk) and base the
child_node_id decision on that value so NodeId::leaf / NodeId::next remain
consistent.
🪄 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: 22b24531-e170-4c97-bfde-a487ceec604e
📒 Files selected for processing (5)
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/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). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Compile & Lint
🧰 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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.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/resolving-deps-resolver/src/resolve_importer.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.rs
🔇 Additional comments (2)
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs (1)
52-78: LGTM!Also applies to: 169-170, 182-232
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (1)
175-177: LGTM!Also applies to: 256-257
- Re-export `TreeChildren` and `ChildEdge` from the crate root so the
intra-doc links from public docs (`resolve_peers`, `ResolvedTree`)
resolve. They were `pub` on the enum/struct but unreachable because
`resolved_tree` is a private module.
- Drop the `[Walker::realize_children]` / `[Walker::pure_pkgs]`
intra-doc references from `resolve_peers`'s public doc — `Walker`
is private, so the links failed under `--document-private-items`
with `-D rustdoc::private_intra_doc_links`. The prose still names
the items in plain backticks.
- Rename closure params `p` and let binding `v` in `realize_children`
to satisfy `perfectionist::single-letter-{closure-param,let-binding}`.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11915 +/- ##
=======================================
Coverage 87.66% 87.66%
=======================================
Files 214 215 +1
Lines 25390 25458 +68
=======================================
+ Hits 22258 22319 +61
- Misses 3132 3139 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…lization Mirrors upstream's [`ResolvedPackage.isLeaf`](https://github.com/pnpm/pnpm/blob/b9de85dcb6/installing/deps-resolver/src/resolveDependencies.ts#L250) field: `pkg_is_leaf(&result)` is computed once on the first walk and stored on `ResolvedPackage::is_leaf`; the peer-resolver's `realize_children` reads it back instead of inferring leaf-ness from `children_by_id.is_empty() + peer_dependencies.is_empty()`. The inferred check was a weaker approximation — a package with a missing manifest (e.g. a git/tarball/local resolution where `result. manifest` is None) lands on `pkg_is_leaf == false` in the eager walk but on `is_leaf == true` in the realize path, which would collapse distinct per-occurrence `NodeId`s onto a shared `NodeId::leaf` and break the peer resolver's per-call-site state. Matches upstream's [`buildTree` consumption](https://github.com/pnpm/pnpm/blob/b9de85dcb6/installing/deps-resolver/src/resolveDependencyTree.ts#L381): `ctx.resolvedPkgsById[child.id].isLeaf` is the source of truth, not recomputed per realization.
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.0724993834800003,
"stddev": 0.08733909898777982,
"median": 2.04032864608,
"user": 2.81616024,
"system": 3.4754287400000003,
"min": 1.9727952250799998,
"max": 2.27714572908,
"times": [
2.12439873208,
1.9727952250799998,
2.02690741808,
2.05940645108,
2.02747512008,
2.0167917500800003,
2.03007836008,
2.27714572908,
2.05057893208,
2.13941611708
]
},
{
"command": "pacquet@main",
"mean": 2.06675633228,
"stddev": 0.06554897301297306,
"median": 2.05729976258,
"user": 2.78829304,
"system": 3.44529774,
"min": 1.97921768108,
"max": 2.17138608608,
"times": [
2.13321695608,
2.13195336808,
2.01292955808,
2.10430668108,
2.05149099108,
2.02394829208,
1.9960051750799999,
2.17138608608,
2.06310853408,
1.97921768108
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6866358929599999,
"stddev": 0.022617507807309097,
"median": 0.67882440156,
"user": 0.38959242000000005,
"system": 1.47196304,
"min": 0.65768104906,
"max": 0.73808318306,
"times": [
0.73808318306,
0.70413134306,
0.69404325406,
0.6823931280600001,
0.69507332306,
0.67304573306,
0.6735556300600001,
0.65768104906,
0.6730966110600001,
0.6752556750600001
]
},
{
"command": "pacquet@main",
"mean": 0.6789324803600001,
"stddev": 0.018924720516200633,
"median": 0.67245956756,
"user": 0.3810710199999999,
"system": 1.47232374,
"min": 0.65960619406,
"max": 0.72749298206,
"times": [
0.72749298206,
0.67247185606,
0.67815497106,
0.66716788106,
0.67244727906,
0.67223493006,
0.66832146106,
0.65960619406,
0.6888322130600001,
0.68259503606
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.54167332082,
"stddev": 0.03692783265335108,
"median": 2.5402220891200002,
"user": 4.43551684,
"system": 3.4118865,
"min": 2.4893130811199997,
"max": 2.60099777812,
"times": [
2.60099777812,
2.53544211112,
2.54500206712,
2.5966539481199997,
2.55468830912,
2.5098279781199997,
2.55609349512,
2.51962142312,
2.4893130811199997,
2.5090930171199997
]
},
{
"command": "pacquet@main",
"mean": 3.3233058410200003,
"stddev": 0.07472949593106847,
"median": 3.31704954912,
"user": 5.198449039999999,
"system": 3.4780583,
"min": 3.2387801501199998,
"max": 3.50248396312,
"times": [
3.31627152112,
3.25879454312,
3.26159121612,
3.2387801501199998,
3.31782757712,
3.35927914912,
3.2940062491199997,
3.34938777112,
3.33463627012,
3.50248396312
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.7650360763800002,
"stddev": 0.026859128791892582,
"median": 1.76569994678,
"user": 2.1080864199999994,
"system": 2.19720212,
"min": 1.7240956897800002,
"max": 1.7979722017800002,
"times": [
1.74456809278,
1.7240956897800002,
1.72961782278,
1.75818211678,
1.7582205077800002,
1.77461872478,
1.79785080578,
1.77317938578,
1.7979722017800002,
1.79205541578
]
},
{
"command": "pacquet@main",
"mean": 2.54494658798,
"stddev": 0.13164197808309358,
"median": 2.54518480128,
"user": 2.8288161199999995,
"system": 2.25516972,
"min": 2.3999916687800003,
"max": 2.73059210178,
"times": [
2.72264589078,
2.55845184078,
2.53191776178,
2.6806241607800003,
2.4074811467800004,
2.3999916687800003,
2.4173258097800003,
2.57427033678,
2.73059210178,
2.42616516178
]
}
]
} |
|
Bencher Report| Branch | pr/11915 |
| 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,541.67 ms(-46.76%)Baseline: 4,773.69 ms | 5,728.43 ms (44.37%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,765.04 ms(-52.80%)Baseline: 3,739.58 ms | 4,487.50 ms (39.33%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 2,072.50 ms(-11.44%)Baseline: 2,340.20 ms | 2,808.24 ms (73.80%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 686.64 ms(+4.46%)Baseline: 657.30 ms | 788.76 ms (87.05%) |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (1)
553-618: 💤 Low valueConsider importing
TreeChildrenandChildEdgeto reduce qualified-path verbosity.Lines 554, 603, 609, 617 use fully qualified
crate::resolved_tree::TreeChildrenandcrate::resolved_tree::ChildEdgewhile nearby types (DependenciesTreeNode,DirectDep, etc.) are imported at line 32. Adding these to the existing import would be consistent.♻️ Suggested import consolidation
use crate::{ node_id::NodeId, - resolved_tree::{DependenciesTreeNode, DirectDep, PeerDep, ResolvedPackage, ResolvedTree}, + resolved_tree::{ChildEdge, DependenciesTreeNode, DirectDep, PeerDep, ResolvedPackage, ResolvedTree, TreeChildren}, };Then simplify usages:
- crate::resolved_tree::TreeChildren::Lazy { parent_ids: Arc::new(next_ancestors.clone()) } + TreeChildren::Lazy { parent_ids: Arc::new(next_ancestors.clone()) }- let mut by_id: Vec<crate::resolved_tree::ChildEdge> = Vec::new(); + let mut by_id: Vec<ChildEdge> = Vec::new();- by_id.push(crate::resolved_tree::ChildEdge { + by_id.push(ChildEdge {- crate::resolved_tree::TreeChildren::Realized(realized) + TreeChildren::Realized(realized)🤖 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/resolving-deps-resolver/src/resolve_dependency_tree.rs` around lines 553 - 618, The code uses fully qualified paths for crate::resolved_tree::TreeChildren and crate::resolved_tree::ChildEdge several times (e.g., the children assignment and by_id construction) while other resolved_tree types are imported; add TreeChildren and ChildEdge to the existing imports from resolved_tree and then replace occurrences of crate::resolved_tree::TreeChildren and crate::resolved_tree::ChildEdge with the shorter TreeChildren and ChildEdge identifiers (affecting the let children = ... assignment and the by_id.push(...) construction, and the final TreeChildren::Realized(...) return), leaving the surrounding logic (children_specs_by_id, children_by_id, extract_children, resolve_node) unchanged.
🤖 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/resolving-deps-resolver/src/resolve_dependency_tree.rs`:
- Around line 553-618: The code uses fully qualified paths for
crate::resolved_tree::TreeChildren and crate::resolved_tree::ChildEdge several
times (e.g., the children assignment and by_id construction) while other
resolved_tree types are imported; add TreeChildren and ChildEdge to the existing
imports from resolved_tree and then replace occurrences of
crate::resolved_tree::TreeChildren and crate::resolved_tree::ChildEdge with the
shorter TreeChildren and ChildEdge identifiers (affecting the let children = ...
assignment and the by_id.push(...) construction, and the final
TreeChildren::Realized(...) return), leaving the surrounding logic
(children_specs_by_id, children_by_id, extract_children, resolve_node)
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b0152077-1a02-4c12-a1ba-68aa459972b9
📒 Files selected for processing (3)
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
📜 Review details
🧰 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/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
🧠 Learnings (5)
📚 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/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
🔇 Additional comments (10)
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs (2)
857-865: LGTM!
833-893: LGTM!pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs (5)
52-62: LGTM!
65-79: LGTM!
146-159: LGTM!
183-184: LGTM!
196-246: LGTM!pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs (3)
237-245: LGTM!Also applies to: 262-262
306-306: LGTM!Also applies to: 322-322
486-500: LGTM!Also applies to: 527-527
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs (1)
865-868:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAppend the current package id to the lazy ancestor chain.
child_parent_idsshould extendparent_idswithpkg_id, notedge.pkg_id. As written, a lazyB -> Crealization records[..., C], so whenCis expanded later a back-edge toBis no longer recognized as a cycle and pacquet materializes an extra occurrence that upstream skips.Suggested fix
let child_parent_ids = { let mut next_ids = (*parent_ids).clone(); - next_ids.push(edge.pkg_id.clone()); + next_ids.push(pkg_id.clone()); Arc::new(next_ids) };Based on learnings: only match pnpm’s behavior exactly.
🤖 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/resolving-deps-resolver/src/resolve_peers.rs` around lines 865 - 868, The lazy ancestor chain is built from the wrong id: replace the use of edge.pkg_id when extending parent_ids so child_parent_ids appends the current pkg_id instead; locate the block creating child_parent_ids (the let child_parent_ids = { ... } expression) and change the push of edge.pkg_id to push(pkg_id.clone()) so cycles are detected correctly during lazy realization.
🤖 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.
Outside diff comments:
In `@pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs`:
- Around line 865-868: The lazy ancestor chain is built from the wrong id:
replace the use of edge.pkg_id when extending parent_ids so child_parent_ids
appends the current pkg_id instead; locate the block creating child_parent_ids
(the let child_parent_ids = { ... } expression) and change the push of
edge.pkg_id to push(pkg_id.clone()) so cycles are detected correctly during lazy
realization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4c20d1d-9a11-4910-b35b-8b0465e0caff
📒 Files selected for processing (1)
pacquet/crates/resolving-deps-resolver/src/resolve_peers.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). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
- 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: Compile & Lint
🧰 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/resolving-deps-resolver/src/resolve_peers.rs
🧠 Learnings (5)
📚 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/resolving-deps-resolver/src/resolve_peers.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/resolving-deps-resolver/src/resolve_peers.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/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs
Adds two regression tests for the lazy-children mechanism introduced in #11915 that the existing coverage didn't hit: 1. `revisit_with_no_manifest_child_keeps_per_occurrence_node_id` — a child whose first walk produced `result.manifest == None` (the shape git / tarball / local resolvers return) must keep the non-leaf classification on every lazy realisation. Without the `ResolvedPackage::is_leaf` persistence the realizer would mis- classify it as a leaf and collapse distinct occurrences onto a shared `NodeId::Leaf`, breaking per-call-site state. 2. `pure_revisit_leaves_lazy_children_unrealized` — a pure pkg reached through multiple parents only realises its children for the occurrence the peer resolver walks first. Subsequent occurrences hit the `purePkgs` short-circuit before `realize_children` runs, so their `TreeChildren::Lazy` stays Lazy. Regression guard against accidentally moving the realise call above the short-circuit. Both tests were validated by breaking the relevant subject (swapping `is_leaf` back to the inferred check; moving `realize_children` above the `purePkgs` gate) and confirming they fail cleanly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
1053-1177: 💤 Low valueWell-structured regression test for manifest-less package handling.
The test correctly validates that packages resolved without a manifest body (as returned by git/tarball resolvers) preserve their non-leaf classification across lazy revisits. The setup forces the peer resolver to descend into every occurrence via the peer dependency, ensuring the lazy realization path is exercised.
Minor: The spell checker flags "mis-classify" (line 1110) and "mis-classified" (line 1158). While the hyphenated form is acceptable, "misclassify" (no hyphen) is the more common modern spelling.
🤖 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/resolving-deps-resolver/src/tests.rs` around lines 1053 - 1177, Update the spelling in the test function revisit_with_no_manifest_child_keeps_per_occurrence_node_id: replace the hyphenated words "mis-classify" and "mis-classified" with the modern unhyphenated forms "misclassify" and "misclassified" in the comment text so the test's documentation uses consistent spelling.
🤖 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/resolving-deps-resolver/src/tests.rs`:
- Around line 1053-1177: Update the spelling in the test function
revisit_with_no_manifest_child_keeps_per_occurrence_node_id: replace the
hyphenated words "mis-classify" and "mis-classified" with the modern
unhyphenated forms "misclassify" and "misclassified" in the comment text so the
test's documentation uses consistent spelling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a46d5f91-5802-419f-ae64-7cc74e35f8b8
📒 Files selected for processing (1)
pacquet/crates/resolving-deps-resolver/src/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). (7)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Compile & Lint
🧰 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/resolving-deps-resolver/src/tests.rs
🧠 Learnings (5)
📚 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/resolving-deps-resolver/src/tests.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/resolving-deps-resolver/src/tests.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/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: When implementing dependency “revisit”/optional-folding in pacquet, mirror pnpm’s `resolveDependencies.ts` behavior: on revisit, update the `optional` flag only for the directly visited package, and do not automatically change `optional` for transitive descendants. pnpm corrects stale optional flags later via `copyDependencySubGraph` (a BFS in `lockfile/pruner/src/index.ts`/`copyDependencySubGraph`). Until pacquet has an equivalent pruner/copy-subgraph step, be aware that the raw `node.optional` can flow through to the lockfile/virtual store via `dependencies_graph_to_lockfile.rs` → `create_virtual_store.rs` → `installability.rs`, so review changes that affect optional flag propagation for this mismatch.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/tests.rs
📚 Learning: 2026-05-24T21:10:50.292Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11915
File: pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs:553-617
Timestamp: 2026-05-24T21:10:50.292Z
Learning: In pacquet’s Rust resolver layer, the behavior of `ResolvedPackage.optional` “AND-folding”/revisit updates is intentionally aligned with pnpm: when a package is directly revisited, only that package’s `optional` flag is updated; transitive descendants are not re-walked/rescored in this stage. pnpm later corrects any stale optional flags downstream via its pruner/BFS logic (`copyDependencySubGraph` + `nonOptional` reachability stamping in the lockfile pruner). Since pacquet does not yet have that pruner equivalent, review should NOT flag the downstream/stale optional propagation as a bug in pacquet PRs; treat the resolver-layer optional propagation gap as expected parity until the pruning/stamping step is ported.
Applied to files:
pacquet/crates/resolving-deps-resolver/src/tests.rs
🪛 GitHub Check: Spell Check
pacquet/crates/resolving-deps-resolver/src/tests.rs
[warning] 1158-1158:
"mis" should be "miss" or "mist".
[warning] 1110-1110:
"mis" should be "miss" or "mist".
🔇 Additional comments (3)
pacquet/crates/resolving-deps-resolver/src/tests.rs (3)
131-133: LGTM!Also applies to: 202-203
458-470: LGTM!Also applies to: 508-521, 556-568, 607-619, 666-678, 761-781, 854-866, 959-971, 1026-1038, 1333-1350
1179-1286: LGTM!
CI's typos pass at .typos.toml flags `mis-` (suggests "miss" / "mist"). Use "misclassify" instead of "mis-classify" — same word, no hyphen, no typo hit.
|
The outside-diff comment on The misreading is in CodeRabbit's claim that "a lazy
By induction, every stored Walking the claimed scenario explicitly. Say
The suggested fix (
Empirically I tried CodeRabbit's swap locally; the 56-test suite still passes because every cycle that the misaligned chain would miss is one that the eager walker already pruned out of Skipping this finding. The cspell + Written by an agent (Claude Code, claude-opus-4-7). |
Summary
Ports upstream pnpm's lazy
childrenthunk onDependenciesTreeNodeto pacquet. Picks up the third optimization from #11907 — the one explicitly deferred from #11906 (purePkgs + peersCache).Upstream's
DependenciesTreeNode.childrenis a(() => ChildrenMap) | ChildrenMapsum type. The first walk of apkgIdWithPatchHashrecords its children inchildrenByParentIdand seeds the lazy thunk; every revisit reuses that entry and defers the per-occurrence subtree expansion tobuildTreeduring peer resolution.Pacquet was eagerly fanning out every revisit, producing one fresh NodeId for every reachable occurrence even though peer resolution only ever descends into the ones it can't short-circuit through
purePkgs. On deep trees this dominatedresolve_importer.Approach
TreeChildrenenum withRealized(BTreeMap<String, NodeId>)andLazy { parent_ids: Arc<Vec<String>> }arms, mirroring upstream's union.children_by_id: HashMap<String, Arc<Vec<ChildEdge>>>onResolvedTree(upstream'schildrenByParentId), populated by the first walk and immutable thereafter.ChildEdgecarries the install alias, resolved childpkgIdWithPatchHash, and theoptionalbit so per-occurrence realization keepsResolvedPackage::optionalAND-folded correctly.resolve_nodewritesTreeChildren::Lazy { parent_ids: ancestor chain }on revisits and short-circuits the recursive descent.Walkergainsrealize_children(&mut self, NodeId) -> &BTreeMap<String, NodeId>which expands a lazy node on demand, allocating per-occurrence NodeIds (NodeId::leaffor pure leaves,NodeId::nextotherwise) and applying upstream'sparentIdsContainSequencecycle break against the storedparent_idschain.resolve_peerstakes&mut ResolvedTreeso realization can flip Lazy → Realized in place. Pure subtrees that the peer resolver short-circuits viapurePkgsare never realized.Bench (astro deep tree, cold store)
resolve_importerphaseWall-clock total is in the noise —
create_virtual_storeis network/IO-bound and varies more than the resolver win. The optimization targets the resolver phase specifically, which is what #11907 calls out as the remaining hotspot.Tests
All 54 tests in
pacquet-resolving-deps-resolverpass, including the peer-resolution coverage added in #11906 (revisit_resolves_peer_in_one_occurrence_misses_in_other,cyclic_peer_dependencies_resolve_cleanly, etc.) which exercise both Realized and Lazy descent paths.Refs #11907.
Test plan
cargo nextest run -p pacquet-resolving-deps-resolverjust checkjust lintcargo fmt --checktypos pacquet registryWritten by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Performance Improvements
Refactor
New Features
Tests