feat(pacquet): peer-dependency resolution stage#11774
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughWalkthroughAdds a new deps-path crate (DepPath utilities and filename mapping), a two-pass resolver (tree then peer-resolution producing a DepPath-keyed graph), and installs wired to materialize packages using DepPath-derived virtual-store names. ChangesPeer Dependency Resolution and Install Integration
Sequence Diagram(s)sequenceDiagram
participant Client as Install Client
participant Resolver
participant Tree as resolve_dependency_tree
participant Peers as resolve_peers
participant Graph as DependenciesGraph
participant InstallFlow as install_subtree
Client->>Resolver: request install
Resolver->>Tree: resolve_dependency_tree(direct_specs)
Tree->>Tree: allocate NodeId per occurrence
Tree-->>Resolver: ResolvedTree
Resolver->>Peers: resolve_peers(ResolvedTree)
Peers->>Peers: walk tree post-order / match peers / compute DepPath
Peers-->>Resolver: ResolvePeersResult{graph, direct_by_alias}
Resolver->>InstallFlow: for alias in direct_by_alias
InstallFlow->>Graph: lookup(dep_path)
Graph-->>InstallFlow: DependenciesGraphNode
InstallFlow->>InstallFlow: derive slot = dep_path_to_filename(dep_path)
InstallFlow->>InstallFlow: materialize package from node.resolve_result
InstallFlow->>InstallFlow: recurse on node.children(alias→dep_path)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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 |
Ports pnpm's `resolvePeers` algorithm to pacquet's `resolving-deps-resolver` crate and wires it into `install_without_lockfile`. The depPath-keyed `DependenciesGraph` produced by the new stage replaces the flat `(name, version)` keying the install pass used to drive virtual-store slot names from. New `pacquet-deps-path` crate ports the `@pnpm/deps.path` helpers (`createPeerDepGraphHash`, `depPathToFilename`, balanced-paren suffix scan). `ResolvedTree` now carries both the flat dedup map (`packages`) and the per-occurrence tree (`dependencies_tree`, keyed by a new `NodeId`) so two parents sharing the same package can compute different peer suffixes — the whole point of the stage. Cycles in the tree pass are broken by skipping the cycled edge entirely (matches upstream's `parentIdsContainSequence` gate in `buildTree`), and peer-resolution falls back to `name@version` peer-ids when re-entering an in-progress node. Three upstream optimisations are intentionally not ported in this slice: `peersCache`, the `purePkgs` fast path, and `graph-cycles`-driven async deferment. Each is correctness-preserving — the algorithm produces the same depPaths, just without the short-circuits. `autoInstallPeers` keeps its existing "fold peers into the regular walk" behavior until the full `hoistPeers` algorithm lands.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11774 +/- ##
==========================================
+ Coverage 89.98% 90.06% +0.08%
==========================================
Files 155 163 +8
Lines 18114 18851 +737
==========================================
+ Hits 16299 16979 +680
- Misses 1815 1872 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Micro-Benchmark ResultsLinux |
Contributor
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.3619910438999994,
"stddev": 0.05875614954362005,
"median": 2.3778716586,
"user": 2.7658078,
"system": 3.5840501799999998,
"min": 2.2740831501,
"max": 2.4392318411,
"times": [
2.4392318411,
2.3294963320999997,
2.4034802841,
2.2740831501,
2.4345233990999997,
2.3773978221,
2.2781506491,
2.3783454951,
2.3240698991,
2.3811315670999997
]
},
{
"command": "pacquet@main",
"mean": 2.3777274775999997,
"stddev": 0.07650174422987759,
"median": 2.3526554591,
"user": 2.7748862999999995,
"system": 3.6055872800000004,
"min": 2.2842781560999996,
"max": 2.5192480411,
"times": [
2.3958508051,
2.5192480411,
2.2842781560999996,
2.3310021660999998,
2.3495459491,
2.3415241251,
2.3557649691,
2.4165260831,
2.4837272910999997,
2.2998071900999997
]
},
{
"command": "pnpm",
"mean": 4.586463266600001,
"stddev": 0.0345219472581763,
"median": 4.5831092481,
"user": 7.7825982,
"system": 4.0517433800000004,
"min": 4.5397146251,
"max": 4.6430763501,
"times": [
4.5842393671,
4.6105880451,
4.5819791291000005,
4.5963001271,
4.632895667100001,
4.6430763501,
4.5397146251,
4.549436662100001,
4.5598536761,
4.566549017100001
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7210467862599999,
"stddev": 0.07264737613580068,
"median": 0.6881882636600001,
"user": 0.39082986000000003,
"system": 1.58393956,
"min": 0.67966380616,
"max": 0.86468975416,
"times": [
0.86468975416,
0.68365345416,
0.68598981516,
0.68534557816,
0.67978132816,
0.85210580216,
0.67966380616,
0.69710837116,
0.69038671216,
0.69174324116
]
},
{
"command": "pacquet@main",
"mean": 0.69384038926,
"stddev": 0.04787643553817553,
"median": 0.68084482016,
"user": 0.36675095999999996,
"system": 1.5812649600000004,
"min": 0.66218540016,
"max": 0.82687791316,
"times": [
0.70046749416,
0.66720963316,
0.82687791316,
0.67508631316,
0.68165364516,
0.66218540016,
0.68222428416,
0.6856079591600001,
0.67705525516,
0.68003599516
]
},
{
"command": "pnpm",
"mean": 2.51014631436,
"stddev": 0.12840981946638252,
"median": 2.5105569446600002,
"user": 2.99844576,
"system": 2.22757086,
"min": 2.36985144216,
"max": 2.77695256116,
"times": [
2.39219830016,
2.48997537916,
2.36985144216,
2.77695256116,
2.38651660916,
2.40535117516,
2.56992493416,
2.53113851016,
2.57854545616,
2.60100877616
]
}
]
} |
- Dylint perfectionist::single-letter-let-binding: rename `i` → `cursor` in `suffix_index.rs`. - Dylint perfectionist::macro-trailing-comma: drop trailing comma in the `dep_path_to_filename` `file:` test. - rustdoc broken-intra-doc-links: link `[`DependenciesTree`]` to the newly-exported `pacquet_resolving_deps_resolver::DependenciesTree` type alias and downgrade the `pacquet_lockfile::PkgNameVerPeer` mention in `pacquet-deps-path`'s module doc to plain text (the crate deliberately doesn't depend on `pacquet-lockfile`). - rustdoc fn-vs-mod ambiguity: prefix [`resolve_dependency_tree`], [`resolve_peers`], and [`create_peer_dep_graph_hash`] doc-links with `fn@` so they bind to the function rather than the same-named module. - typos: rename `unparseable` → `unparsable` in a test name.
- `derive_ordering`: reorder `#[derive(...)]` on `DepPath` and `NodeId` to match the configured `prefix_then_alphabetical` style — `PartialOrd, Ord, Hash` rather than `Hash, PartialOrd, Ord`. See `dylint.toml`'s `[perfectionist::derive_ordering]` prefix list for the canonical order. - `single_letter_function_param`: rename `s` → `value` on `impl From<String> for DepPath`.
Review Summary by QodoImplement peer-dependency resolution stage with depPath-keyed graph
WalkthroughsDescription• Ports pnpm's peer-dependency resolution algorithm to pacquet • New pacquet-deps-path crate with depPath string manipulation helpers • Refactors ResolvedTree to carry per-occurrence tree keyed by NodeId • Implements resolve_peers stage producing depPath-keyed DependenciesGraph • Wires peer resolution into install_without_lockfile install pass Diagramflowchart LR
A["resolve_dependency_tree<br/>per-occurrence tree"] --> B["resolve_peers<br/>match peer requirements"]
B --> C["DependenciesGraph<br/>depPath-keyed"]
C --> D["install_subtree<br/>virtual-store slots"]
E["pacquet-deps-path<br/>string helpers"] -.-> B
E -.-> D
File Changes1. pacquet/crates/deps-path/src/lib.rs
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
50 rules 1. Peer issues logged via tracing::warn!
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
pacquet/crates/resolving-deps-resolver/src/dependencies_graph.rs (1)
7-30: ⚡ Quick winMake
DepPath’s inner string private.
pub struct DepPath(pub String)lets callers bypass the brand entirely, which undercuts the type-safety this wrapper is meant to add and makes future validation impossible to enforce. The existingFrom<String>andas_str()surface already covers normal construction and access.Suggested change
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct DepPath(pub String); +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct DepPath(String);As per coding guidelines, "Declare a newtype wrapper for branded string types. Do not collapse the brand into a plain
Stringor&str."🤖 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/dependencies_graph.rs` around lines 7 - 30, The DepPath newtype currently exposes its inner String (pub struct DepPath(pub String)), which breaks the branding; change it to a private field (pub struct DepPath(String)) so callers cannot bypass the brand, keep the existing impls (impl From<String> for DepPath, as_str, and Display) as-is since they live in the same module and will still construct and access the inner String; verify public API still uses DepPath::from(...) or as_str() for access.pacquet/crates/resolving-deps-resolver/src/lib.rs (1)
56-59: ⚡ Quick winRe-export
ResolvePeersResultwith the new root API.
resolve_peersis now exposed from the crate root, but its concrete return type is still trapped in a private module. Callers can use inference, but they can't spell the type in their own signatures from the root module.Suggested fix
-pub use resolve_peers::{ResolvePeersOptions, resolve_peers}; +pub use resolve_peers::{ResolvePeersOptions, ResolvePeersResult, resolve_peers};🤖 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/lib.rs` around lines 56 - 59, The root module re-exports resolve_peers and ResolvePeersOptions but not the concrete return type ResolvePeersResult, preventing callers from naming the type; export ResolvePeersResult from the crate root by adding it to the existing pub use line (pub use resolve_peers::{ResolvePeersOptions, ResolvePeersResult, resolve_peers};) and confirm that ResolvePeersResult is declared pub in the resolve_peers module so consumers can reference it in signatures.pacquet/crates/resolving-deps-resolver/src/tests.rs (1)
314-327: ⚡ Quick winAssert the graph edge, not just the depPath suffix.
This test still passes if
react-domgets the right depPath string but itsgraph.children["react"]edge is missing. Please also assert the peer child edge itself, ideally with the manifest order reversed so the peer target is visited later.🤖 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 314 - 327, The test currently only asserts the depPath string for "react-dom" but not that the graph edge to its peer "react" exists; update the test after computing result (from resolve_peers and ResolvePeersOptions::default()) to locate the react-dom node (e.g. DepPath::from("react-dom@18.0.0(react@18.0.0)")) and assert that result.graph.children[react_dom_dep_path] contains the react node (e.g. DepPath::from("react@18.0.0")), and also reverse the manifest input order in the test so the peer target is visited later to exercise the edge creation logic. Ensure you still keep the existing asserts on result.direct_dependencies_by_alias and peer_dependency_issues.
🤖 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/deps-path/src/dep_path_to_filename.rs`:
- Around line 44-50: The current logic slices trimmed.as_bytes()[1..] which will
panic for empty or single-byte inputs; before creating after_first, guard that
trimmed has length > 1 (e.g., check trimmed.len() > 1) and handle the
short/empty-case by returning trimmed.to_string() (or the appropriate fallback)
so the position search only runs on valid slices; update the code that computes
after_first and the subsequent position match (variables trimmed, after_first,
rel) to use this length check to avoid out-of-bounds slicing.
In `@pacquet/crates/deps-path/src/peer_id.rs`:
- Around line 11-14: The PeerId enum currently uses a raw String in the DepPath
variant which loses the branded type-safety; introduce a new DepPath newtype
(e.g., pub struct DepPath(String)) and replace the enum variant from
DepPath(String) to DepPath(DepPath); update any constructors, pattern matches,
and usages of PeerId::DepPath, plus impls (From/Into/AsRef/Display/Debug) as
needed to preserve behavior and ergonomics while keeping the brand encapsulated.
In `@pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs`:
- Around line 264-273: extract_children currently appends peerDependencies
unconditionally which conflicts with extract_peer_dependencies that treats names
from dependencies and optionalDependencies as "own", causing child edges to be
lost or overwritten; update extract_children (and its use of collect_deps) to
first collect both "dependencies" and "optionalDependencies" into out (or at
least ensure those names are considered "own"), then when auto_install_peers is
true either collect "peerDependencies" after that or filter peer entries by
skipping any names already present in out (so peer entries do not override
existing children in the BTreeMap by alias); refer to extract_children,
collect_deps, extract_peer_dependencies, peerDependenciesWithoutOwn, children
and BTreeMap in your change.
In `@pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs`:
- Around line 373-378: The code currently skips inserting peer edges when
self.node_dep_paths.get(peer_node_id) is None, dropping edges for peers that
point to nodes not yet walked; change the loop in resolve_peers so that if
node_dep_paths lacks peer_node_id you create a dep-path for the peer and still
insert the alias into graph_children — e.g., check
self.node_dep_paths.get(peer_node_id).cloned().or_else(||
Some(self.build_peer_id(peer_node_id))) and insert that value for peer_alias
into graph_children (referencing graph_children, child_dep_paths,
all_resolved_peers, self.node_dep_paths.get, peer_alias, peer_node_id, and
build_peer_id).
- Around line 555-565: The helper satisfies_with_prereleases currently uses
parsed_version.satisfies(&parsed_range) which excludes prereleases; update it to
call the semver prerelease-inclusive API instead (use the crate's
satisfies_with_prerelease variant) so prerelease versions like 18.0.0-rc.1 match
^18.0.0; specifically, inside satisfies_with_prereleases replace the final
parsed_version.satisfies(&parsed_range) call with the appropriate
satisfies_with_prerelease call (passing parsed_version and parsed_range) and set
include_prerelease = true so parsed_range/...satisfies_with_prerelease(...,
true) is used.
---
Nitpick comments:
In `@pacquet/crates/resolving-deps-resolver/src/dependencies_graph.rs`:
- Around line 7-30: The DepPath newtype currently exposes its inner String (pub
struct DepPath(pub String)), which breaks the branding; change it to a private
field (pub struct DepPath(String)) so callers cannot bypass the brand, keep the
existing impls (impl From<String> for DepPath, as_str, and Display) as-is since
they live in the same module and will still construct and access the inner
String; verify public API still uses DepPath::from(...) or as_str() for access.
In `@pacquet/crates/resolving-deps-resolver/src/lib.rs`:
- Around line 56-59: The root module re-exports resolve_peers and
ResolvePeersOptions but not the concrete return type ResolvePeersResult,
preventing callers from naming the type; export ResolvePeersResult from the
crate root by adding it to the existing pub use line (pub use
resolve_peers::{ResolvePeersOptions, ResolvePeersResult, resolve_peers};) and
confirm that ResolvePeersResult is declared pub in the resolve_peers module so
consumers can reference it in signatures.
In `@pacquet/crates/resolving-deps-resolver/src/tests.rs`:
- Around line 314-327: The test currently only asserts the depPath string for
"react-dom" but not that the graph edge to its peer "react" exists; update the
test after computing result (from resolve_peers and
ResolvePeersOptions::default()) to locate the react-dom node (e.g.
DepPath::from("react-dom@18.0.0(react@18.0.0)")) and assert that
result.graph.children[react_dom_dep_path] contains the react node (e.g.
DepPath::from("react@18.0.0")), and also reverse the manifest input order in the
test so the peer target is visited later to exercise the edge creation logic.
Ensure you still keep the existing asserts on
result.direct_dependencies_by_alias and peer_dependency_issues.
🪄 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: e4969e3e-694a-428b-980f-ae29e8016750
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
Cargo.tomlpacquet/crates/deps-path/Cargo.tomlpacquet/crates/deps-path/src/create_peer_dep_graph_hash.rspacquet/crates/deps-path/src/dep_path_to_filename.rspacquet/crates/deps-path/src/lib.rspacquet/crates/deps-path/src/peer_id.rspacquet/crates/deps-path/src/suffix_index.rspacquet/crates/package-manager/Cargo.tomlpacquet/crates/package-manager/src/install_without_lockfile.rspacquet/crates/resolving-deps-resolver/Cargo.tomlpacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/node_id.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.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). (2)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Run benchmark on ubuntu-latest
🧰 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/node_id.rspacquet/crates/deps-path/src/peer_id.rspacquet/crates/deps-path/src/lib.rspacquet/crates/deps-path/src/suffix_index.rspacquet/crates/deps-path/src/dep_path_to_filename.rspacquet/crates/deps-path/src/create_peer_dep_graph_hash.rspacquet/crates/resolving-deps-resolver/src/dependencies_graph.rspacquet/crates/resolving-deps-resolver/src/lib.rspacquet/crates/resolving-deps-resolver/src/resolve_peers.rspacquet/crates/resolving-deps-resolver/src/tests.rspacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rspacquet/crates/resolving-deps-resolver/src/resolved_tree.rspacquet/crates/package-manager/src/install_without_lockfile.rs
🔇 Additional comments (10)
Cargo.toml (1)
34-34: LGTM!pacquet/crates/deps-path/Cargo.toml (1)
1-17: LGTM!pacquet/crates/deps-path/src/create_peer_dep_graph_hash.rs (1)
1-33: LGTM!Also applies to: 35-93
pacquet/crates/deps-path/src/dep_path_to_filename.rs (1)
25-43: LGTM!Also applies to: 52-114
pacquet/crates/deps-path/src/lib.rs (1)
1-25: LGTM!pacquet/crates/deps-path/src/suffix_index.rs (1)
1-81: LGTM!Also applies to: 83-135
pacquet/crates/package-manager/Cargo.toml (1)
32-32: LGTM!pacquet/crates/resolving-deps-resolver/Cargo.toml (1)
14-25: LGTM!pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs (1)
1-124: LGTM!pacquet/crates/resolving-deps-resolver/src/node_id.rs (1)
1-29: LGTM!
Round of correctness fixes flagged by CodeRabbit and qodo-code-review on #11774. - **`dep_path_to_filename_unescaped`**: guard against empty / single-byte input. The previous `trimmed.as_bytes()[1..]` slice panicked when `trimmed.len() < 2`; the function now short-circuits to `trimmed.to_string()` in that case, mirroring upstream's `indexOf` with `fromIndex = 1` returning -1 on out-of-range scan. - **`extract_children` / `extract_peer_dependencies` mismatch**: the child walker added every `peerDependencies` entry when `auto_install_peers` was on, but the peer-resolution stage's `peerDependenciesWithoutOwn` filter skips peer names also in `dependencies` / `optionalDependencies`. `BTreeMap` collection of the result by alias could silently drop the optional edge for a name that appeared in both. Fix: collect `optionalDependencies` into children too, and dedupe peer entries against own deps before appending. - **Dropped peer edges in `graph_children`**: when a peer pointed at a later sibling direct dep (e.g., manifest order `{ react-dom: …, react: … }`), the peer's `DepPath` wasn't in `node_dep_paths` yet at the time the parent's `graph_children` was built, so the symlink edge was silently dropped. Install would then walk react-dom's slot without finding react. Add `pending_peer_edges` + a `patch_pending_peer_edges` post-pass that runs after every direct dep is walked, with regression test. - **Aliased child peer misfilter**: the "is this peer one of my own children?" check compared the peer alias against `tree_node.children` keys, but `children` is keyed by install alias while peers can match by real name via the dual-keyed `ParentRefs`. Switch both filter sites to compare by `NodeId` so an npm-aliased child satisfying a peer is correctly classified as internal. - **`DepPath` newtype consolidation**: `PeerId::DepPath(String)` collapsed the depPath brand. Move the `DepPath` newtype from `resolving-deps-resolver::dependencies_graph` into the lower-level `pacquet-deps-path` crate so the `PeerId` enum carries the branded type instead of `String`. `resolving-deps-resolver` re-exports `DepPath` from `pacquet_deps_path` to keep existing imports working. - **Prerelease-tolerant semver match**: `node-semver`'s `Range::satisfies` rejects prereleases against non-prerelease comparators (`18.0.0-rc.1` vs `^18.0.0` returns `false`). Add a fallback in `satisfies_with_prereleases`: if the straight check fails and the candidate is a prerelease, retry against the stripped `MAJOR.MINOR.PATCH` base. Approximates Yarn's `satisfiesWithPrereleases` for the cases pnpm cares about without importing the full per-comparator algorithm. The `tracing::warn!` peer-issue emission flagged by qodo and the deeper prerelease semantics gap (per-comparator) remain documented follow-ups; the slice's `resolve_peers.rs` module doc lists what's intentionally not ported in this PR.
Three sites where the previous edits left non-canonical wrapping: - `create_peer_dep_graph_hash.rs` test calls now fit one line. - `lib.rs` re-export ordering (alphabetical). - `tests.rs` whitespace. CI Format check is `cargo fmt --all -- --check`; the local pre-push hook didn't catch these because the edits landed via `Edit` without a follow-up `cargo fmt`.
…added `cargo fmt` collapsed the regression test's `assert_eq!` onto one line and kept the trailing comma, which tripped the same `perfectionist::macro_trailing_comma` rule that #11774's earlier commit fixed elsewhere. The lint forbids a trailing comma on single-line macro calls; rustfmt leaves it alone. Drop the comma to satisfy both.
zkochan
added a commit
that referenced
this pull request
May 20, 2026
…gResolutionId The peer-resolution stage landed on main (#11774) using the old `.id.name` / `.id.suffix` field accesses. Round-trip the opaque id back through `PkgNameVer` at the five call sites via a small `npm_pkg_name_ver` helper. Pulls `pacquet-lockfile` from dev-deps to regular deps.
This was referenced May 20, 2026
4 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.
Summary
Ports pnpm's
resolvePeersalgorithm to pacquet'sresolving-deps-resolvercrate and wires it intoinstall_without_lockfile. The depPath-keyedDependenciesGraphproduced by the new stage replaces the flat(name, version)keying the install pass used to derive virtual-store slot names from, so two parents sharing the same package can now get different peer suffixes.What's new
pacquet-deps-pathcrate — ports@pnpm/deps.pathhelpers:create_peer_dep_graph_hash,dep_path_to_filename, balanced-paren suffix scan (index_of_dep_path_suffix/remove_suffix/get_pkg_id_with_patch_hash), and thePeerIdunion (Pair { name, version }|DepPath(String)).resolving-deps-resolverdata-shape refactor:NodeIdnewtype (monotonic counter) for per-occurrence tree identity, mirroring upstream'snextNodeId.ResolvedTreenow carries both the flat dedup map (packageskeyed bypkgIdWithPatchHash) and the per-occurrence tree (dependencies_treekeyed byNodeId), plusall_peer_dep_names.ResolvedPackagerecordspeer_dependenciesextracted withpeerDependenciesMetafolding andpeerDependenciesWithoutOwnfiltering.Ok(None)fromresolve_node), matching upstream'sparentIdsContainSequencegate inbuildTree.resolve_peersalgorithm:DependenciesGraphkeyed byDepPath, with children, transitive peer set, depth, andis_pure.ParentRefspropagation — descend with parent + own-children view; resolve own peers against the parent view only.node-semver, withworkspace:prefix stripping,*shortcut, and a literal-equality fallback for unparseable ranges.in_progressset +name@versionfallback peer-id (synchronous analog of upstream'sanalyzeGraph-driven async deferment).install_without_lockfilerewiring: runsresolve_dependency_tree → resolve_peersand walks theDependenciesGraphbyDepPath. Virtual-store slot names flow throughdep_path_to_filename.Scope limits (explicit follow-ups)
autoInstallPeerskeeps its existing fold behavior. Upstream'shoistPeerspre-pass is not ported in this PR — peer resolution itself runs regardless, but auto-installed peers don't get hoisted to the importer level yet, so their peer-suffix construction is limited until that lands.resolve_peers.rs:peersCache(always recomputes)purePkgsfast path (general path produces same answer)graph-cycles-driven async deferment (replaced by syncin_progressset + name@version fallback)tracing::warn!for now; porting the issue renderer is its own slice.Test plan
cargo nextest run --workspace— 1608 passed, 3 skipped, 0 failedcargo clippy --workspace --all-targets -- --deny warningscargo fmt --checkcargo check --workspace --all-targetsshould_install_circular_dependencies(cycle-break regression) passespacquet-deps-path(helpers + balanced-paren scan)resolving-deps-resolver::tests::peers: pure package → depPath equals pkgId; peer resolved against sibling at parent level; missing peer is reported + node stays pure; bad peer version is reported + install still proceeds with picked candidate.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests