Skip to content

feat(pacquet): port auto-install-peers algorithm#11784

Merged
zkochan merged 6 commits into
mainfrom
auto-install-peer-desp
May 20, 2026
Merged

feat(pacquet): port auto-install-peers algorithm#11784
zkochan merged 6 commits into
mainfrom
auto-install-peer-desp

Conversation

@zkochan

@zkochan zkochan commented May 20, 2026

Copy link
Copy Markdown
Member

Summary

  • Replaces pacquet's placeholder peer-folding (which folded peer deps in as nested children of every consumer) with a faithful port of pnpm's hoistPeers algorithm — missing peers are hoisted to the importer's direct deps, shared across consumers and deduplicated against the preferred-versions map.
  • Adds the multi-pass loop (resolveRootDependencies shape): inner loop hoists missing required peers and re-resolves until none remain; outer loop hoists optional peers whose version is already in scope, then re-enters the inner loop.
  • Adds the auto-install-peers-from-highest-match setting mirroring upstream's flag for range-merge behavior across conflicting consumers.

What changed

  • New crate pacquet-lockfile-preferred-versions — port of @pnpm/lockfile.preferred-versions. Seeds the version-picker tie-break table from manifest + lockfile snapshots, with weight-bump on dual-source matches. Includes a get_version_selector_type helper that mirrors the version-selector-type npm package (semver version → range → URI-safe tag → None).
  • New hoist_peers module in pacquet-resolving-deps-resolver — ports hoistPeers + getHoistableOptionalPeers from upstream's hoistPeers.ts with all 10 upstream test cases (PR fix: handle non-string version selectors in hoistPeers #11048, fix: invalid specifiers for peers on all non-exact version selectors #11049 regressions, workspace: protocol, exact override pinning, weighted selectors, etc.).
  • New resolve_importer orchestrator in pacquet-resolving-deps-resolver — public entry point that runs the tree pass, then drives the hoist loop, then runs the final peer pass. Returns { resolved_tree, peers_result } for the install layer. Mirrors upstream's resolveRootDependencies.
  • resolve_dependency_tree refactor: removed the placeholder peer-folding in extract_children; split the entry point into reusable building blocks (TreeCtx::new, extend_tree, into_resolved_tree, snapshot, resolved_versions) so the orchestrator can extend the tree incrementally without re-walking already-resolved subtrees.
  • Config: added auto_install_peers_from_highest_match: bool (default false, matching pnpm) to pacquet-config with yaml deserializer, env-overlay key AUTO_INSTALL_PEERS_FROM_HIGHEST_MATCH, and clear_workspace_only_fields entry.
  • install_without_lockfile now calls resolve_importer instead of resolve_dependency_tree + resolve_peers directly, seeding all_preferred_versions from the importer manifest via get_preferred_versions_from_lockfile_and_manifests(None, &[manifest]).

Architecture notes

  • The orchestrator's hoist loop runs resolve_peers over a snapshot of the in-flight tree each iteration to find missing peers, then calls hoist_peers (or get_hoistable_optional_peers for the outer pass) to pick specifiers, then calls extend_tree to walk those new deps. The existing per-id dedup gate in resolve_node means re-entering walked packages costs only a cache lookup — equivalent to upstream's inline pkgAddresses.push(...) extension. all_preferred_versions is mutated as each new package resolves, mirroring upstream's per-resolve allPreferredVersions[name][version] = 'version' assignment at resolveDependencies.ts:1440.
  • get_hoistable_optional_peers faithfully ports upstream's specType === 'version' filter — which only matches plain Version selectors, not weighted ones from lockfile/manifest seeding. This means it picks up versions added to the preferred-versions map during the walk (which we now do, per the bullet above), matching upstream behavior including its known limitation.
  • merge_ranges is a simplification of upstream's mergePkgsDeps + safeIntersect: single-range and all-identical-range cases pass through; non-identical multi-range cases drop on intersection (matching upstream's intersection === null arm) or ||-join when auto_install_peers_from_highest_match is set. Full semver-range intersection (via a port of semver-range-intersect) is a follow-up.
  • Single-importer slice; workspace support waits on pacquet's workspace work. resolve_peers_from_workspace_root flag is plumbed and uses the importer's direct deps as workspace_root_deps when set (single-importer ⇒ importer is root).

Test plan

  • cargo nextest run -p pacquet-lockfile-preferred-versions — 9 tests
  • cargo nextest run -p pacquet-resolving-deps-resolver hoist_peers — 10 ported upstream tests
  • cargo nextest run -p pacquet-resolving-deps-resolver resolve_importer — 4 orchestrator tests (auto-installs missing required peer, doesn't hoist when disabled, transitive peer hoisting, preferred-version reuse)
  • cargo nextest run -p pacquet-resolving-deps-resolver -p pacquet-lockfile-preferred-versions -p pacquet-config — 238 tests pass
  • cargo nextest run -p pacquet-cli auto_install_peers_hoists — E2E install against mock registry, asserts peer-a / peer-b / peer-c land in node_modules/.pnpm/ and node_modules/@pnpm.e2e/ symlinks
  • cargo clippy --workspace --all-targets -- --deny warnings clean
  • cargo fmt --check + taplo format --check clean
  • typos clean on new files
  • CI green

Out of scope (follow-ups)

  • Full semver-range-intersect port for merge_ranges non-identical-range case.
  • Workspace support — multi-importer hoist loop, resolve_peers_from_workspace_root for cross-importer peer satisfaction.
  • Pre-seeding all_preferred_versions from the wanted lockfile in install_without_lockfile's sibling install paths (frozen-lockfile doesn't re-resolve, so no change needed there).

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

Summary by CodeRabbit

  • New Features
    • Added a setting to prefer highest-match when auto-installing peers.
    • Enhanced peer auto-installation and hoisting (required and optional peers) with multi-pass resolution and improved preferred-version handling.
    • Better seeding of preferred versions from lockfiles and manifests to influence installs.
  • Tests
    • Expanded unit and end-to-end tests covering peer hoisting, optional-peer selection, preferred-version logic, and an auto-install-peers e2e scenario.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0909d5ef-65ec-42fb-9d3d-e62ce208eed6

📥 Commits

Reviewing files that changed from the base of the PR and between 4161c0d and 97422da.

📒 Files selected for processing (1)
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
🧠 Learnings (2)
📚 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/hoist_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/hoist_peers.rs
🔇 Additional comments (1)
pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs (1)

207-214: LGTM!


📝 Walkthrough

Walkthrough

Implements importer-level multi-pass peer auto-install hoisting seeded by preferred-versions. Adds a preferred-versions crate, config/workspace/env wiring, resolver refactor (TreeCtx), hoist decision logic, resolve_importer orchestration, install integration, and unit + e2e tests.

Changes

Peer Auto-Install with Preferred Versions

Layer / File(s) Summary
Configuration contracts for highest-match peer merging
pacquet/crates/config/src/lib.rs, pacquet/crates/config/src/workspace_yaml.rs, pacquet/crates/config/src/env_overlay.rs, pacquet/crates/package-manager/src/install_package_from_registry/tests.rs, pacquet/crates/cli/tests/install.rs
auto_install_peers_from_highest_match boolean added to Config and WorkspaceSettings, parsed from workspace YAML and environment; package-manager test config default updated; CLI e2e test added.
Preferred versions seeding crate
pacquet/crates/lockfile-preferred-versions/Cargo.toml, pacquet/crates/lockfile-preferred-versions/src/lib.rs, pacquet/crates/lockfile-preferred-versions/src/version_selector_type.rs, pacquet/crates/lockfile-preferred-versions/src/tests.rs
New crate builds PreferredVersions from manifests and optional lockfile snapshots, classifies version selectors (Version/Range/Tag), and applies weight-based tie-breaking with unit tests.
Resolver context extraction and child-edge simplification
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs, pacquet/crates/resolving-deps-resolver/src/tests.rs
Introduces public TreeCtx, refactors resolve_dependency_tree/extend_tree to use it, and stops folding peerDependencies into child edges (delegates hoisting to importer).
Peer hoisting decision functions
pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs, pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs
Adds hoist_peers and get_hoistable_optional_peers implementing workspace-root matching, preferred-version tie-breaks, semver/prerelease handling, and selection logic; includes comprehensive tests.
Importer-level multi-pass peer resolution
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs, pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
Adds resolve_importer that runs nested fixed-point loops to hoist required and optional peers, merges ranges per config, updates preferred-versions from TreeCtx, and returns resolved tree + peer results; extensive tests added.
Integration into install-without-lockfile flow
pacquet/crates/package-manager/src/install_without_lockfile.rs
Replaces the two-step resolve_dependency_tree + resolve_peers with resolve_importer, seeds preferred-versions from manifest, updates error variant, and uses importer peer results for later install steps.
Crate dependency and API surface updates
Cargo.toml, pacquet/crates/package-manager/Cargo.toml, pacquet/crates/resolving-deps-resolver/src/lib.rs
Workspace dependency added for pacquet-lockfile-preferred-versions. package-manager dependencies updated. resolving-deps-resolver re-exports expanded to include hoist/importer types and TreeCtx/extend_tree.
End-to-end and test coverage
pacquet/crates/cli/tests/install.rs, pacquet/crates/resolving-deps-resolver/src/*/tests.rs
Adds CLI e2e test validating missing peer auto-install and hoisting at importer resolution; adds extensive unit tests for preferred-versions, hoist logic, and importer scenarios.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#11779: Overlaps on resolver core changes to resolve_dependency_tree.
  • pnpm/pnpm#11774: Related to install orchestration and peer-resolution wiring in install_without_lockfile.
  • pnpm/pnpm#11760: Touches the resolution/install pipeline this PR refactors.

"🐰
I hopped through manifests and lockfile seeds,
Chose preferred versions like tasty weeds.
Hoisted peers to the importer's door,
Tests and config tucked in, I hop off once more."

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(pacquet): port auto-install-peers algorithm' accurately and concisely describes the main objective of the pull request, which is porting pnpm's auto-install-peers algorithm into pacquet.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch auto-install-peer-desp

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.

❤️ Share

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

Replaces the placeholder peer-folding behavior with a faithful port of
pnpm's `hoistPeers` algorithm. Missing required peers are hoisted to
the importer's direct deps (shared across consumers, not nested), with
a multi-pass loop that re-resolves until no required peer remains and
the optional-peer pass picks already-available versions from the
preferred-versions map.

- New `pacquet-lockfile-preferred-versions` crate seeds the version-
  picker tie-break table from manifest + lockfile snapshots.
- New `hoist_peers` module ports `hoistPeers` and
  `getHoistableOptionalPeers` with the full upstream test suite.
- New `resolve_importer` orchestrator drives the multi-pass hoist
  loop and threads `parent_pkg_aliases` / `all_preferred_versions`
  across iterations.
- `resolve_dependency_tree` exposes `TreeCtx` / `extend_tree` /
  `snapshot` so the orchestrator can extend the tree incrementally
  without re-walking already-resolved subtrees.
- `auto-install-peers-from-highest-match` config setting added,
  mirroring upstream's flag for range-merge behavior.

Ported from pnpm's installing/deps-resolver `resolveRootDependencies`
and hoistPeers.ts at commit 097983f.

---
Written by an agent (Claude Code, claude-opus-4-7).
@zkochan zkochan force-pushed the auto-install-peer-desp branch from 29c3fcd to 6a082b8 Compare May 20, 2026 22:12
Pacquet changes don't get changesets.

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

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01      7.5±0.25ms   578.0 KB/sec    1.00      7.4±0.32ms   583.2 KB/sec

@codecov-commenter

codecov-commenter commented May 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.81641% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.62%. Comparing base (a8a8cbc) to head (97422da).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...es/resolving-deps-resolver/src/resolve_importer.rs 91.77% 13 Missing ⚠️
.../crates/resolving-deps-resolver/src/hoist_peers.rs 93.75% 8 Missing ⚠️
...quet/crates/lockfile-preferred-versions/src/lib.rs 95.58% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11784      +/-   ##
==========================================
- Coverage   89.69%   89.62%   -0.07%     
==========================================
  Files         174      179       +5     
  Lines       20781    21540     +759     
==========================================
+ Hits        18640    19306     +666     
- Misses       2141     2234      +93     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

zkochan added 2 commits May 21, 2026 00:28
Port nine portable scenarios from
installing/deps-installer/test/install/autoInstallPeers.ts as
orchestrator-level unit tests in resolve_importer/tests.rs:

- skip optional peers without a preferred-version hint
- dedupe via range intersection when consumers agree
- drop the peer when ranges conflict (default)
- install via `||` join when autoInstallPeersFromHighestMatch is on
- reuse a peer already brought by a sibling (preferred-versions)
- skip hoisting when the root already has the dep as a direct
- collapse the same missing peer across a transitive chain
- prefer the version pinned in the importer's own peerDependencies
- reuse a regular-dep version over re-resolving via the peer arm

The last case exposed a gap: the orchestrator wasn't including the
importer's own `peerDependencies` as initial wanted deps when
auto-install-peers is on, mirroring upstream's
`getAllDependenciesFromManifest({ autoInstallPeers: true })`. Fixed
in resolve_importer alongside the test ports.

Workspace, frozen-lockfile mutation, hook-using, override-using,
and webpack-circular-deps tests from the upstream file remain
un-ported pending the matching features in pacquet.

---
Written by an agent (Claude Code, claude-opus-4-7).
CI's Dylint job (Perfectionist lints) and the Doc job both fail on
the prior commits' style. Cleanups:

- Rename single-letter closure params (`|c|`, `|d|`, `|e|`, `|r|`)
  to descriptive names in resolve_importer, hoist_peers,
  version_selector_type, and the install + orchestrator test files.
  Triggered `perfectionist::single-letter-closure-param`.
- Add trailing commas to multi-line `assert!` invocations in
  resolve_importer/tests.rs. Triggered
  `perfectionist::macro-trailing-comma`.
- Disambiguate `[`crate::resolve_importer`]` and friends with the
  `[`fn@crate::resolve_importer`]` form — `resolve_importer`,
  `resolve_peers`, `hoist_peers`, and `get_hoistable_optional_peers`
  are each both a module and a re-exported function, which rustdoc
  flags as ambiguous.
- Replace the broken `[`resolve_dependency_tree`]` and
  `[`resolve_peers`]` intra-doc links in install_without_lockfile.rs
  with prose now that those symbols are no longer in scope there
  (the orchestrator wraps them).
- Drop the link to the private `add_weight_to_version_selector`
  helper from `get_preferred_versions_from_lockfile_and_manifests`'
  public docs and the link to the private `resolve_node` from
  `extend_tree`'s public docs.
- Remove the redundant explicit `(crate::extend_tree)` link target
  in lib.rs's module doc.

No behavior change; tests pass.

---
Written by an agent (Claude Code, claude-opus-4-7).
@zkochan zkochan marked this pull request as ready for review May 20, 2026 22:52
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@github-actions

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Frozen Lockfile

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.416 ± 0.081 2.305 2.542 1.04 ± 0.04
pacquet@main 2.330 ± 0.059 2.241 2.421 1.00
pnpm 4.535 ± 0.072 4.451 4.670 1.95 ± 0.06
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.4163893410999995,
      "stddev": 0.08110956945001237,
      "median": 2.3985466618999998,
      "user": 2.88233636,
      "system": 3.6451205799999995,
      "min": 2.3048469174,
      "max": 2.5424662774,
      "times": [
        2.5313044024,
        2.3984464124,
        2.3986469113999997,
        2.3048469174,
        2.5424662774,
        2.3490470874,
        2.3840019094,
        2.4411022574,
        2.4800195694,
        2.3340116664
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.3300494469999995,
      "stddev": 0.058589129665680174,
      "median": 2.3187627599,
      "user": 2.8117994600000005,
      "system": 3.6234116800000002,
      "min": 2.2413363964,
      "max": 2.4209025234,
      "times": [
        2.2963213103999998,
        2.3258661694,
        2.3116593504,
        2.4209025234,
        2.3402613514,
        2.3072336144,
        2.3963413264,
        2.2660326734,
        2.2413363964,
        2.3945397543999998
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.534833480500001,
      "stddev": 0.07182000731404627,
      "median": 4.5272255234,
      "user": 7.59248716,
      "system": 4.04474208,
      "min": 4.4509111884,
      "max": 4.6701781234,
      "times": [
        4.554055085400001,
        4.4898705644,
        4.4509111884,
        4.457876247400001,
        4.542195756400001,
        4.5220738334,
        4.4909915354,
        4.5323772134,
        4.6701781234,
        4.6378052574
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 698.8 ± 26.6 679.2 770.6 1.00
pacquet@main 724.7 ± 61.9 662.9 852.9 1.04 ± 0.10
pnpm 2449.3 ± 125.8 2353.3 2703.9 3.51 ± 0.22
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6988071763600001,
      "stddev": 0.02660125506300929,
      "median": 0.6915126127600001,
      "user": 0.37689414,
      "system": 1.59372332,
      "min": 0.6791670912600001,
      "max": 0.77062211826,
      "times": [
        0.77062211826,
        0.7044259742600001,
        0.6908895292600001,
        0.6791670912600001,
        0.68011071326,
        0.6961760482600001,
        0.69213569626,
        0.6830784312600001,
        0.7013890212600001,
        0.6900771402600001
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.72465910676,
      "stddev": 0.06191050679351998,
      "median": 0.7037780142600001,
      "user": 0.37686733999999994,
      "system": 1.5786784199999997,
      "min": 0.6629467102600001,
      "max": 0.8528714612600001,
      "times": [
        0.8528714612600001,
        0.7117298162600001,
        0.6629467102600001,
        0.67234057826,
        0.7037242452600001,
        0.7038317832600001,
        0.7019744922600001,
        0.6698881992600001,
        0.78416508626,
        0.7831186952600001
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.4493426161599996,
      "stddev": 0.12575504214472605,
      "median": 2.38783714576,
      "user": 2.9241101400000002,
      "system": 2.17693202,
      "min": 2.35331689626,
      "max": 2.7038990512599996,
      "times": [
        2.40851200826,
        2.37861332726,
        2.58996005126,
        2.3970609642599996,
        2.57637329026,
        2.3602561562599997,
        2.35331689626,
        2.36779367226,
        2.35764074426,
        2.7038990512599996
      ]
    }
  ]
}

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs (1)

65-72: ⚡ Quick win

Add a regression case for exact range + no satisfying preferred version with auto_install_peers = false.

That branch should currently omit hoisting, and this specific scenario is not covered yet.

Also applies to: 90-97

🤖 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/hoist_peers/tests.rs` around lines
65 - 72, Add regression tests that cover the case where a dependency has an
exact-range requirement and no preferred version satisfies it while
auto_install_peers is false: create a test (similar to
falls_back_to_range_when_no_preferred_version_satisfies_it) that calls
hoist_peers with opts(auto_install_peers=false, &preferred) and a
missing("chai","4.3.0") input, and assert that the result is empty (no
hoisting). Repeat the same pattern for the other pair of tests referenced (lines
90-97) to ensure both branches omit hoisting when auto_install_peers is false
and the preferred set cannot satisfy the exact selector.
🤖 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/hoist_peers.rs`:
- Around line 120-133: The current branch lets an exact peer range
(is_exact_version true) fall through to the max_satisfying_any logic when
opts.auto_install_peers is false, which can choose an incompatible preferred
version; instead, detect the exact-range case when auto_install_peers is false
and insert the original exact range into dependencies (using
dependencies.insert(peer_name.clone(), range.clone())) rather than running
max_satisfying_any/parts logic. Update the conditional around
is_exact_version/opts.auto_install_peers (and the else branch that uses
max_satisfying_any, non_versions, parts) so that exact ranges are preserved when
auto_install_peers is false, referencing is_exact_version,
opts.auto_install_peers, max_satisfying_any, non_versions, peer_name and range.
- Around line 170-172: The current max_satisfying implementation uses
Range::satisfies (called on the ranges iterator and later on the candidate
check) which applies strict prerelease rules and thus rejects valid prerelease
versions; update both uses to follow the prerelease-strip-and-retry pattern
implemented in resolve_peers.rs (satisfies_with_prereleases): attempt
Range::satisfies normally, and if it fails and the candidate version is a
prerelease while the range has no prerelease identifiers, strip or ignore the
prerelease and retry the check (i.e., mirror semver.maxSatisfying(..., {
includePrerelease: true })); replace the direct Range::satisfies calls in
max_satisfying with a call to the same helper logic (or extract and reuse
satisfies_with_prereleases) so peer hoisting includes valid prerelease
candidates.

---

Nitpick comments:
In `@pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs`:
- Around line 65-72: Add regression tests that cover the case where a dependency
has an exact-range requirement and no preferred version satisfies it while
auto_install_peers is false: create a test (similar to
falls_back_to_range_when_no_preferred_version_satisfies_it) that calls
hoist_peers with opts(auto_install_peers=false, &preferred) and a
missing("chai","4.3.0") input, and assert that the result is empty (no
hoisting). Repeat the same pattern for the other pair of tests referenced (lines
90-97) to ensure both branches omit hoisting when auto_install_peers is false
and the preferred set cannot satisfy the exact selector.
🪄 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: 9af3a1fa-23f7-4600-9424-fa07b8161c2e

📥 Commits

Reviewing files that changed from the base of the PR and between a8a8cbc and 352abc5.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • pacquet/crates/package-manager/src/install/snapshots/pacquet_package_manager__install__tests__should_install_dependencies.snap is excluded by !**/*.snap
📒 Files selected for processing (19)
  • Cargo.toml
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/lockfile-preferred-versions/Cargo.toml
  • pacquet/crates/lockfile-preferred-versions/src/lib.rs
  • pacquet/crates/lockfile-preferred-versions/src/tests.rs
  • pacquet/crates/lockfile-preferred-versions/src/version_selector_type.rs
  • pacquet/crates/package-manager/Cargo.toml
  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • 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). (4)
  • GitHub Check: Lint and Test (windows-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 (2)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that fires pnpm:<channel> events through globalLogger, logger.debug(), or streamParser.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 plain String or &str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only via TryFrom<String> and/or FromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallible From<String> (and From<&str> when convenient).
If upstream occasionally constructs without validation, expose from_str_unchecked as 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 manual impl only when conversion needs custom logic.
String-literal unions should become enums, 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 (&Path over &PathBuf, &str over &String) when it doesn't force extra copies.
Prefer Arc::clone(&x) / Rc::clone(&x) over x.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. Write use super::{Foo, bar} instead of use super::*;. Two forms stay allowed: external-crate preludes like use rayon::prelude::*; and root-of-module re-...

Files:

  • pacquet/crates/package-manager/src/install_package_from_registry/tests.rs
  • pacquet/crates/lockfile-preferred-versions/src/version_selector_type.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/lockfile-preferred-versions/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/lockfile-preferred-versions/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
pacquet/**/{tests,test}/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/{tests,test}/**/*.rs: Prefer real fixtures; reach for the dependency-injection seam only when they can't cover the branch. Most happy paths and error paths should be tested with tempfile::TempDir, the mocked registry, or an integration test that spawns the actual binary.
Use the DI seam — a capability trait on the Host provider, threaded as Sys: <Bounds> — only for branches a real fixture can't reach portably: filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths.
Log before non-assert_eq! assertions, dbg! complex structures, skip logging for simple scalar assert_eq!. Follow the test-logging guidance in CODE_STYLE_GUIDE.md.
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found. If the test needs a tool, let it panic when the tool is absent.
Prefer #[cfg_attr(target_os = "windows", ignore = "...")] (or #[cfg(unix)]) over a runtime probe-and-skip helper for platform-locked tools.
Port relevant tests from pnpm when porting behavior. Matching test coverage is the easiest way to prove behavioral parity.
When porting behavior from pnpm, consult plans/TEST_PORTING.md before adding ported tests. Use allow_known_failure! at the not-yet-implemented boundary and update checkboxes as items land.

Files:

  • pacquet/crates/cli/tests/install.rs
🧠 Learnings (1)
📚 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/install_package_from_registry/tests.rs
  • pacquet/crates/lockfile-preferred-versions/src/version_selector_type.rs
  • pacquet/crates/config/src/env_overlay.rs
  • pacquet/crates/config/src/lib.rs
  • pacquet/crates/cli/tests/install.rs
  • pacquet/crates/config/src/workspace_yaml.rs
  • pacquet/crates/lockfile-preferred-versions/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
  • pacquet/crates/resolving-deps-resolver/src/lib.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
  • pacquet/crates/resolving-deps-resolver/src/hoist_peers/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
  • pacquet/crates/lockfile-preferred-versions/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
🔇 Additional comments (22)
pacquet/crates/config/src/lib.rs (1)

412-417: LGTM!

pacquet/crates/config/src/workspace_yaml.rs (1)

125-125: LGTM!

Also applies to: 345-345, 415-416

pacquet/crates/config/src/env_overlay.rs (1)

140-140: LGTM!

pacquet/crates/package-manager/src/install_package_from_registry/tests.rs (1)

43-43: LGTM!

Cargo.toml (1)

27-27: LGTM!

pacquet/crates/package-manager/Cargo.toml (1)

14-44: LGTM!

pacquet/crates/resolving-deps-resolver/src/lib.rs (1)

4-6: LGTM!

Also applies to: 32-40, 47-50, 53-53, 56-56, 64-66, 70-76

pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs (6)

1-42: LGTM!


44-96: LGTM!


98-215: LGTM!


217-266: LGTM!


268-293: LGTM!


295-335: LGTM!

pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs (3)

1-90: LGTM!


92-260: LGTM!


262-768: LGTM!

pacquet/crates/package-manager/src/install_without_lockfile.rs (5)

16-19: LGTM!


50-62: LGTM!

Also applies to: 99-105


237-270: LGTM!


305-332: LGTM!


434-443: LGTM!

pacquet/crates/cli/tests/install.rs (1)

313-366: LGTM!

Comment thread pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
Comment thread pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs
Upstream's `hoistPeers` and `getHoistableOptionalPeers` both pass
`{ includePrerelease: true }` to `semver.maxSatisfying`, so a
prerelease candidate from the preferred-versions table (e.g.
`18.0.0-rc.1`) satisfies a regular range (e.g. `^18.0.0`). Rust's
`node_semver::Range::satisfies` follows strict semver semantics and
rejects prereleases when the range has none of its own, which
silently dropped valid picks in the hoist loop.

Mirror the strip-and-retry pattern already used by
`satisfies_with_prereleases` in `resolve_peers.rs`: a new
`satisfies_including_prerelease` helper in `hoist_peers.rs` retries
with the prerelease tag stripped, and `max_satisfying` /
`get_hoistable_optional_peers` now both go through it.

Add two regression tests covering an `^18.0.0` range against an
`18.0.0-rc.1` preferred-versions entry for each function.

Reported by CodeRabbit on PR #11784.

---
Written by an agent (Claude Code, claude-opus-4-7).
The doc comment I added for `satisfies_including_prerelease` linked
to `crate::resolve_peers`, which rustdoc flags as ambiguous (both a
module and a re-exported function). Replace the link with prose
since the helper this paragraph compares to is module-internal
anyway — no link can resolve to it from outside the module.

---
Written by an agent (Claude Code, claude-opus-4-7).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants