Skip to content

fix(pacquet): resolve catalog: in pnpm.overrides before freshness check#11820

Merged
zkochan merged 1 commit into
mainfrom
fix-frozen-install2
May 21, 2026
Merged

fix(pacquet): resolve catalog: in pnpm.overrides before freshness check#11820
zkochan merged 1 commit into
mainfrom
fix-frozen-install2

Conversation

@zkochan

@zkochan zkochan commented May 21, 2026

Copy link
Copy Markdown
Member

Summary

The frozen-lockfile freshness check compared the lockfile's overrides map (where pnpm has already expanded catalog: to a concrete specifier) against the raw config map (still holding the literal catalog: strings). Every catalog-backed override therefore surfaced as ERR_PNPM_OUTDATED_LOCKFILE on every install, with output like:

`overrides` in the lockfile ({"hosted-git-info@1": "npm:@pnpm/hosted-git-info@1.0.0", ...})
  doesn't match the current config ({"hosted-git-info@1": "catalog:", ...})

Fix

Mirror pnpm's parseOverrides(overrides, catalogs)createOverridesMapFromParsed pipeline:

  • pacquet-config-parse-overrides::parse_overrides[_iter] now takes &Catalogs and runs each value through resolve_from_catalogFound swaps in the resolved specifier, Unused keeps the original value, Misconfiguration surfaces as ERR_PNPM_CATALOG_IN_OVERRIDES with the resolver's own error message (closer to upstream than the old hardcoded message).
  • Added create_overrides_map_from_parsed helper mirroring upstream.
  • The frozen-lockfile branch of Install::run parses config.overrides once with the workspace catalogs before the freshness check, hands the resolved map to check_lockfile_settings, and reuses the same parsed slice for VersionsOverrider (no double parsing).

Test plan

  • Added frozen_lockfile_resolves_catalog_protocol_in_overrides_before_freshness_check: workspace catalog defines placeholder: 1.0.0, manifest declares placeholder: ^9, pnpm.overrides is placeholder: catalog:, lockfile records the resolved overrides.placeholder: 1.0.0. Install must accept the lockfile.
  • Verified the test catches the regression: temporarily reverted the fix and the test failed with the exact error users see — OverridesChanged { lockfile: {"placeholder": "1.0.0"}, config: {"placeholder": "catalog:"} }.
  • Existing parse_overrides and VersionsOverrider tests updated to pass an empty catalogs map; two new tests cover catalog: and catalog:<name> resolution.
  • cargo nextest run -p pacquet-config-parse-overrides -p pacquet-package-manager --lib → 293/293 pass.
  • cargo clippy -p pacquet-config-parse-overrides -p pacquet-package-manager --all-targets -- --deny warnings clean.

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

Summary by CodeRabbit

  • New Features
    • Package overrides configuration now fully supports catalog protocol resolution, with catalog: and catalog:<name> references being automatically resolved from workspace catalogs.
    • Frozen lockfile freshness checks now properly account for and resolve catalog references in overrides.

Review Change Stack

The frozen-lockfile freshness check compared the lockfile's overrides
map (with `catalog:` already expanded by pnpm) against the raw config
map (still containing `catalog:` strings), so every catalog-backed
override surfaced as `ERR_PNPM_OUTDATED_LOCKFILE` on every install.

Mirror pnpm's `parseOverrides(overrides, catalogs)` →
`createOverridesMapFromParsed` pipeline: thread `&Catalogs` through
`parse_overrides[_iter]`, resolve each value via `resolve_from_catalog`,
and flatten the resolved entries into the map handed to
`check_lockfile_settings`.
@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: 71fd5480-1d50-45b4-919e-aa33bf5700b1

📥 Commits

Reviewing files that changed from the base of the PR and between 5016810 and 1edae12.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (6)
  • pacquet/crates/config-parse-overrides/Cargo.toml
  • pacquet/crates/config-parse-overrides/src/lib.rs
  • pacquet/crates/config-parse-overrides/src/tests.rs
  • pacquet/crates/package-manager/src/install.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/package-manager/src/overrides/tests.rs
📜 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). (2)
  • GitHub Check: ubuntu-latest / Node.js 26.0.0 / 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 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/overrides/tests.rs
  • pacquet/crates/config-parse-overrides/src/lib.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config-parse-overrides/src/tests.rs
  • pacquet/crates/package-manager/src/install.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/package-manager/src/overrides/tests.rs
  • pacquet/crates/config-parse-overrides/src/lib.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config-parse-overrides/src/tests.rs
  • pacquet/crates/package-manager/src/install.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/package-manager/src/overrides/tests.rs
  • pacquet/crates/config-parse-overrides/src/lib.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/config-parse-overrides/src/tests.rs
  • pacquet/crates/package-manager/src/install.rs
🔇 Additional comments (15)
pacquet/crates/config-parse-overrides/Cargo.toml (1)

16-17: LGTM!

pacquet/crates/config-parse-overrides/src/lib.rs (7)

22-30: LGTM!


34-35: LGTM!


55-58: LGTM!


88-98: LGTM!


110-116: LGTM!

Also applies to: 122-144


146-159: LGTM!


202-228: LGTM!

pacquet/crates/config-parse-overrides/src/tests.rs (4)

1-6: LGTM!


36-36: LGTM!

Also applies to: 43-43, 53-53, 71-71, 92-92, 111-111, 124-124


143-156: LGTM!


158-208: LGTM!

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

448-509: LGTM!

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

2-14: LGTM!

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

1655-1757: LGTM!


📝 Walkthrough

Walkthrough

This PR integrates workspace catalog resolution into the pnpm.overrides parsing pipeline, enabling catalog: protocol references to be resolved at parse time and applied during frozen-lockfile freshness checks. The core change updates the public parse_overrides API to accept a &Catalogs parameter and implements a new resolver-based catalog-resolution helper, replacing the prior in-file protocol detection.

Changes

Catalog-aware Override Parsing and Integration

Layer / File(s) Summary
Dependencies and API contracts
pacquet/crates/config-parse-overrides/Cargo.toml, pacquet/crates/config-parse-overrides/src/lib.rs
Add pacquet-catalogs-resolver and pacquet-catalogs-types dependencies; update module documentation to describe catalog: resolution semantics; clarify that VersionOverride.new_bare_specifier stores resolved specifiers and that CatalogInOverrides error reflects resolver misconfiguration.
Override parsing with catalog resolution
pacquet/crates/config-parse-overrides/src/lib.rs
Update parse_overrides and parse_overrides_iter signatures to accept &Catalogs parameter; modify parse_overrides_iter to resolve each override's new_bare_specifier via a new resolve_catalog_in_value helper that builds a WantedDependency, calls resolve_from_catalog, and maps resolver errors to ParseOverridesError::CatalogInOverrides.
Override parser test suite
pacquet/crates/config-parse-overrides/src/tests.rs
Update all existing parse_overrides test calls to pass &Catalogs::new(); rewrite catalog error test to validate resolver-based misconfiguration messaging; add three new tests for catalog: resolution to default/named catalogs and for the create_overrides_map_from_parsed flattening.
Frozen-lockfile freshness check integration
pacquet/crates/package-manager/src/install.rs
Parse config.overrides through catalog-aware parse_overrides_iter to produce the overrides_map used in settings freshness comparison; conditionally build the manifest for freshness checks by cloning and applying VersionsOverrider from parsed overrides.
Integration test and supporting updates
pacquet/crates/package-manager/src/overrides/tests.rs, pacquet/crates/package-manager/src/install/tests.rs
Update overrides test helper to pass Catalogs::new() to parse_overrides; add regression test verifying Install::run accepts catalog: overrides under --frozen-lockfile without triggering OutdatedLockfile error.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#11787: Main PR integrates the newly introduced pacquet-catalogs-resolver and pacquet-catalogs-types into config-parse-overrides, which directly depends on the catalogs port work from PR #11787.
  • pnpm/pnpm#11793: Main PR extends the pnpm overrides parsing and install pipeline by changing parse_overrides from rejecting catalog: to resolving it via Catalogs, then applying resolved overrides to frozen-lockfile freshness checks.
  • pnpm/pnpm#11811: Both PRs modify the frozen-lockfile freshness flow in install.rs; main PR changes how config.overrides is parsed and applied before the manifest check, while retrieved PR adds ignore_manifest_check to conditionally skip that gate.

Poem

🐇 Catalogs resolve with a hop and a bound,
Overrides now know where specifiers are found,
No more catalog: left unresolved in the check,
Frozen lockfiles pass—what a wonderful wreck! ✨

🚥 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 pull request title clearly and concisely describes the main fix: resolving catalog protocol entries in pnpm.overrides before the frozen-lockfile freshness check.
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 fix-frozen-install2

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.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.50%. Comparing base (f279d77) to head (1edae12).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11820      +/-   ##
==========================================
+ Coverage   87.48%   87.50%   +0.01%     
==========================================
  Files         202      202              
  Lines       23704    23706       +2     
==========================================
+ Hits        20737    20743       +6     
+ Misses       2967     2963       -4     

☔ 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 zkochan marked this pull request as ready for review May 21, 2026 14:41
@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

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      5.1±0.58ms   858.2 KB/sec    1.05      5.3±1.04ms   814.7 KB/sec

@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.378 ± 0.058 2.292 2.469 1.02 ± 0.04
pacquet@main 2.343 ± 0.077 2.268 2.471 1.00
pnpm 4.481 ± 0.051 4.405 4.599 1.91 ± 0.07
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.3782681426000005,
      "stddev": 0.05768248618204182,
      "median": 2.4039782557000002,
      "user": 2.7692648399999995,
      "system": 3.5912003599999998,
      "min": 2.2921900712000003,
      "max": 2.4694069072000002,
      "times": [
        2.4694069072000002,
        2.3218523252,
        2.2921900712000003,
        2.3991416852,
        2.3282957122,
        2.4093059022000003,
        2.3208332692,
        2.4183735732000002,
        2.4144671542,
        2.4088148262
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.3427117605000003,
      "stddev": 0.07661538173271001,
      "median": 2.3022536247000005,
      "user": 2.8017439399999993,
      "system": 3.5520532599999997,
      "min": 2.2676004262,
      "max": 2.4707277362,
      "times": [
        2.4707277362,
        2.3655274242,
        2.2924404462,
        2.2961008172,
        2.3030020982000003,
        2.4614883472,
        2.3015051512,
        2.3978329312,
        2.2708922272,
        2.2676004262
      ]
    },
    {
      "command": "pnpm",
      "mean": 4.4808144297000005,
      "stddev": 0.050710333085436425,
      "median": 4.471877983200001,
      "user": 7.541953339999999,
      "system": 4.004540260000001,
      "min": 4.4049835822,
      "max": 4.5987212402,
      "times": [
        4.4539627322,
        4.4771616502,
        4.5137415802000005,
        4.4049835822,
        4.4556919622,
        4.4665943162,
        4.4597151472,
        4.5003474202,
        4.5987212402,
        4.4772246662
      ]
    }
  ]
}

Scenario: Frozen Lockfile (Hot Cache)

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 707.1 ± 46.9 684.8 838.8 1.00
pacquet@main 719.5 ± 53.4 671.0 826.2 1.02 ± 0.10
pnpm 2538.2 ± 112.8 2397.3 2826.1 3.59 ± 0.29
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.7070619548000001,
      "stddev": 0.04685218367037381,
      "median": 0.6898108915000001,
      "user": 0.37054997999999995,
      "system": 1.58553824,
      "min": 0.684830967,
      "max": 0.8388487800000001,
      "times": [
        0.8388487800000001,
        0.6900566180000001,
        0.695436151,
        0.689394822,
        0.689565165,
        0.684830967,
        0.7086005790000001,
        0.68668375,
        0.699582159,
        0.687620557
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.7194739968999999,
      "stddev": 0.05342156001019935,
      "median": 0.694613791,
      "user": 0.37726297999999997,
      "system": 1.58806384,
      "min": 0.670987467,
      "max": 0.8262223710000001,
      "times": [
        0.762012986,
        0.670987467,
        0.8262223710000001,
        0.699475209,
        0.685848785,
        0.697306024,
        0.79024463,
        0.687803091,
        0.682917848,
        0.691921558
      ]
    },
    {
      "command": "pnpm",
      "mean": 2.5381664750999997,
      "stddev": 0.11275012779958085,
      "median": 2.5202176465,
      "user": 2.94659998,
      "system": 2.20554694,
      "min": 2.3973100179999998,
      "max": 2.826099866,
      "times": [
        2.4876655100000002,
        2.523814514,
        2.464574399,
        2.826099866,
        2.562361825,
        2.516620779,
        2.3973100179999998,
        2.500008711,
        2.533881537,
        2.569327592
      ]
    }
  ]
}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants