Skip to content

feat(pacquet): attach patch hashes to resolved pkg ids#11791

Merged
zkochan merged 3 commits into
mainfrom
patched-deps
May 21, 2026
Merged

feat(pacquet): attach patch hashes to resolved pkg ids#11791
zkochan merged 3 commits into
mainfrom
patched-deps

Conversation

@zkochan

@zkochan zkochan commented May 21, 2026

Copy link
Copy Markdown
Member

Summary

  • Threads patchedDependencies (already parsed and hashed by pacquet-patching) into the resolver's tree walker so every matched package's pkgIdWithPatchHash carries the (patch_hash=<hash>) suffix upstream's resolveDependencies.ts:1502-1507 produces.
  • Surfaces ResolvedTree::applied_patches for the post-walk ERR_PNPM_UNUSED_PATCH check, and propagates ERR_PNPM_PATCH_KEY_CONFLICT from get_patch_info through the resolver's error surface.
  • Wires install_without_lockfile to pull config.resolved_patched_dependencies() and hand it to the resolver.

The peer-resolution stage already concatenates the peer suffix onto pkg.id, so the patched id flows naturally into the DepPath keys the install layer consumes — the installer picks the patched virtual-store slot without further changes.

Test plan

  • Added 4 unit tests in pacquet-resolving-deps-resolver covering exact match, range match, unused patch, and ERR_PNPM_PATCH_KEY_CONFLICT.
  • cargo check --locked --workspace --all-targets
  • cargo clippy --locked --workspace --all-targets -- --deny warnings
  • cargo nextest run — 1835/1835 passing
  • cargo fmt --check
  • End-to-end install with a real patchedDependencies entry against the mocked registry — the existing frozen-lockfile path already covers this via install_frozen_lockfile.rs; the without-lockfile path now uses the same resolved_patched_dependencies() source.

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

Summary by CodeRabbit

  • New Features

    • Resolving and applying configured package patches during install via patchedDependencies.
    • Tracking which configured patches were actually applied to resolved packages.
    • Detecting conflicts when multiple patches target the same package version.
  • Tests

    • Added tests covering exact, range, no-match, and conflicting patch scenarios.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 21, 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: 02d57bad-ee2a-479d-bec1-e57a4ae9944f

📥 Commits

Reviewing files that changed from the base of the PR and between 9dbb69a and 3f8b355.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (7)
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/Cargo.toml
  • 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/resolved_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.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/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.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/resolve_importer/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.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/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs
  • pacquet/crates/package-manager/src/install_without_lockfile.rs
  • pacquet/crates/resolving-deps-resolver/src/tests.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
  • pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs
🔇 Additional comments (7)
pacquet/crates/package-manager/src/install_without_lockfile.rs (1)

92-107: LGTM!

Also applies to: 142-146, 199-200, 334-349, 356-356, 361-363

pacquet/crates/resolving-deps-resolver/Cargo.toml (1)

18-18: LGTM!

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

24-24: LGTM!

Also applies to: 30-30, 79-85, 133-133, 138-138

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

91-91: LGTM!

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

2-2: LGTM!

Also applies to: 14-14, 40-47, 74-87, 116-116, 135-143, 156-157, 161-171, 184-184, 199-199, 294-294, 399-445

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

39-46: LGTM!

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

116-119: LGTM!

Also applies to: 183-186, 213-216, 256-259, 306-309, 354-357, 405-408, 464-467, 486-669


📝 Walkthrough

Walkthrough

This PR wires pnpm's patchedDependencies through install -> importer -> dependency-tree resolver, computes patch-aware package IDs (appending patch hashes), records applied patch keys in the resolved tree, and surfaces ambiguous patch-range conflicts with tests and an install error variant.

Changes

Patch resolution and application

Layer / File(s) Summary
Install entry point and error handling
pacquet/crates/package-manager/src/install_without_lockfile.rs, pacquet/crates/resolving-deps-resolver/Cargo.toml
Install resolves patchedDependencies once and passes the result to resolver options; a new ResolvePatchedDependencies error variant handles resolution failures; pacquet-patching dependency added.
Importer integration and options threading
pacquet/crates/resolving-deps-resolver/src/resolve_importer.rs, pacquet/crates/resolving-deps-resolver/src/resolve_importer/tests.rs
ResolveImporterOptions gains patched_dependencies field; importer destructures and wires it to tree context via with_patched_dependencies builder; test helper updated.
Tree options and conflict detection
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
ResolveDependencyTreeOptions accepts patched dependencies; PatchKeyConflict error variant surfaces patch-range ambiguities; supporting imports added.
Tree context state and initialization
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
TreeCtx gains patch storage and applied_patches set; builder method attaches patched dependencies; state initialized in resolver entry point and threaded into snapshots/final tree.
Patch-aware package ID computation
pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
New build_pkg_id_with_patch_hash helper prefixes package names when needed, looks up patch info, appends hash signatures to ids on match, and records matched patch keys; resolve_node uses it for dedup IDs.
Applied patches in resolved tree output
pacquet/crates/resolving-deps-resolver/src/resolved_tree.rs, pacquet/crates/resolving-deps-resolver/src/resolve_dependency_tree.rs
ResolvedTree includes applied_patches set for later diagnostics; threaded through into_resolved_tree and snapshot.
Test updates and patch behavior validation
pacquet/crates/resolving-deps-resolver/src/tests.rs
Existing tests updated to set patched_dependencies: None; new patched_dependencies test module covers exact/range patching, hash suffixing, applied-patches tracking, and conflict detection.

Sequence Diagram

sequenceDiagram
  participant InstallWithoutLockfile
  participant pacquet_patching
  participant ResolveImporter
  participant TreeCtx
  participant ResolveDependencyTree
  participant ResolvedTree

  InstallWithoutLockfile->>pacquet_patching: resolve_patched_dependencies
  pacquet_patching-->>InstallWithoutLockfile: PatchGroupRecord or Error
  InstallWithoutLockfile->>ResolveImporter: set patched_dependencies
  ResolveImporter->>TreeCtx: with_patched_dependencies
  TreeCtx->>ResolveDependencyTree: walk nodes
  ResolveDependencyTree->>TreeCtx: build_pkg_id_with_patch_hash
  ResolveDependencyTree->>ResolvedTree: produce resolved tree
Loading

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs:

  • pnpm/pnpm#11784: Modifies resolver/importer wiring similar to this PR; both touch ResolveImporterOptions and TreeCtx integration.

"I'm a rabbit with a tiny quill,
I hop through patches, byte and thrill,
From config root to resolved tree,
Hashes bloom where names should be—hip, hooray! 🐇"

🚥 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 accurately summarizes the main change: attaching patch hashes to resolved package IDs, which is the core objective across multiple files.
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 patched-deps

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.

@github-actions

github-actions Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01      7.5±0.22ms   581.1 KB/sec    1.00      7.4±0.45ms   584.1 KB/sec

@zkochan zkochan marked this pull request as ready for review May 21, 2026 00:39
@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 →

zkochan added 3 commits May 21, 2026 02:49
Thread `patchedDependencies` into the tree walker so each matched
package's `pkgIdWithPatchHash` gains the `(patch_hash=<hash>)` suffix
upstream's `resolveDependencies.ts` produces. The peer resolver
concatenates the peer suffix onto the patched id, so the install
layer's depPath-keyed lookups land on the patched virtual-store slot
without further changes.

Surfaces `ResolvedTree::applied_patches` for the post-walk
`ERR_PNPM_UNUSED_PATCH` check, and propagates
`ERR_PNPM_PATCH_KEY_CONFLICT` from `get_patch_info` through the
resolver error surface.
The two `pub` items pointed at `resolve_node`, which is private —
fine under `--document-private-items` but rejected when CI runs
`cargo doc` with `-D rustdoc::private-intra-doc-links`. Rephrase to
plain prose; the call site is obvious from the surrounding context.
Dylint's `perfectionist::macro_trailing_comma` rejects a trailing
comma on a single-line macro invocation.
@codecov-commenter

codecov-commenter commented May 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.12195% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.40%. Comparing base (71dfccc) to head (3f8b355).

Files with missing lines Patch % Lines
...lving-deps-resolver/src/resolve_dependency_tree.rs 94.11% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11791   +/-   ##
=======================================
  Coverage   87.40%   87.40%           
=======================================
  Files         198      198           
  Lines       23081    23119   +38     
=======================================
+ Hits        20173    20208   +35     
- Misses       2908     2911    +3     

☔ 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.

@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.392 ± 0.042 2.320 2.473 1.00
pacquet@main 2.413 ± 0.052 2.327 2.512 1.01 ± 0.03
pnpm 4.825 ± 0.063 4.740 4.941 2.02 ± 0.04
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.39203637396,
      "stddev": 0.04235123621870255,
      "median": 2.38906087526,
      "user": 2.7293383399999995,
      "system": 3.7284531199999997,
      "min": 2.3195400997599998,
      "max": 2.47317259976,
      "times": [
        2.39880246076,
        2.43131172576,
        2.40201159876,
        2.37931928976,
        2.37054934176,
        2.47317259976,
        2.41313627076,
        2.35580640876,
        2.3195400997599998,
        2.37671394376
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.41299451416,
      "stddev": 0.051862667021599966,
      "median": 2.40556182126,
      "user": 2.7080303399999996,
      "system": 3.6974539199999996,
      "min": 2.32699536476,
      "max": 2.51202276976,
      "times": [
        2.4547949287599997,
        2.36994820276,
        2.45651773176,
        2.38971373576,
        2.51202276976,
        2.38558387276,
        2.42324489276,
        2.40462670076,
        2.32699536476,
        2.40649694176
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.825326063660001,
      "stddev": 0.06331963561203664,
      "median": 4.81030275576,
      "user": 8.14648884,
      "system": 4.21497802,
      "min": 4.7395447207600006,
      "max": 4.941348056760001,
      "times": [
        4.777850029760001,
        4.7395447207600006,
        4.78880660276,
        4.831132359760001,
        4.897916982760001,
        4.840549927760001,
        4.7740812337600005,
        4.78947315176,
        4.941348056760001,
        4.872557570760001
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 687.5 ± 25.2 665.4 754.5 1.00
pacquet@main 706.7 ± 59.7 664.2 849.3 1.03 ± 0.09
pnpm 2576.2 ± 96.4 2446.1 2795.4 3.75 ± 0.20
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6875353973000001,
      "stddev": 0.025236436406210778,
      "median": 0.6802416312,
      "user": 0.39243204,
      "system": 1.56175918,
      "min": 0.6654421787,
      "max": 0.7544787167,
      "times": [
        0.7544787167,
        0.6787467077,
        0.6741226357,
        0.6743591347,
        0.6817365547,
        0.6941768937,
        0.6950901037,
        0.6654421787,
        0.6742906237,
        0.6829104237
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.7067191785,
      "stddev": 0.059722988163716774,
      "median": 0.6836056511999999,
      "user": 0.37288354,
      "system": 1.56221788,
      "min": 0.6642020377,
      "max": 0.8493336127,
      "times": [
        0.7721672327,
        0.6715828447,
        0.6956284577,
        0.6709565787,
        0.6642020377,
        0.6969589797,
        0.8493336127,
        0.7092768587,
        0.6655218707,
        0.6715633117
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.5762384227000004,
      "stddev": 0.09643510802418613,
      "median": 2.5632368352,
      "user": 3.2212885399999998,
      "system": 2.2468199799999997,
      "min": 2.4461038007,
      "max": 2.7953722467,
      "times": [
        2.6583593487,
        2.5636903517,
        2.5666748777,
        2.7953722467,
        2.5192518727,
        2.5627833187,
        2.5176520907,
        2.4461038007,
        2.6146717087,
        2.5178246107
      ]
    }
  ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants