Skip to content

fix(pacquet/registry): deserialize optionalDependencies and peerDependenciesMeta#11934

Merged
zkochan merged 4 commits into
mainfrom
pacquet-deserialize-optional-deps-and-peer-meta
May 25, 2026
Merged

fix(pacquet/registry): deserialize optionalDependencies and peerDependenciesMeta#11934
zkochan merged 4 commits into
mainfrom
pacquet-deserialize-optional-deps-and-peer-meta

Conversation

@zkochan

@zkochan zkochan commented May 25, 2026

Copy link
Copy Markdown
Member

Summary

PackageVersion (the per-version registry manifest the npm resolver parses) was missing the optionalDependencies and peerDependenciesMeta fields. The resolver builds ResolveResult.manifest via serde_json::to_value(picked) and downstream walks it with extract_children (reads optionalDependencies) and extract_peer_dependencies (reads peerDependenciesMeta). Without those fields on the struct, both reads always saw nothing — so optionalDependencies edges were silently dropped, and every optional peer was treated as required, then auto-installed via the autoInstallPeers fallback in hoist_peers.

Astro cascade

On the vlt astro fixture, unstorage (a transitive of astro) declares 19 optional peers via peerDependenciesMeta (@azure/*, @vercel/*, @netlify/blobs, @upstash/redis, @deno/kv, ioredis, uploadthing, …). Pacquet's resolver auto-installed every one of them and walked their transitive trees; astro's own optionalDependencies (sharp) went missing entirely.

The supposed "5.5× astro deep-tree slowdown" tracked in #11902 was almost all wasted work, not a real perf bug. None of the candidate hypotheses listed there (async_recursion Box-pinning, per-node lock_recoverable mutex acquires, manifest serde_json::to_value cost, tarball extraction) were the bottleneck.

Before / after on vlt astro

Metric Before After pnpm 11.3.0
pacquet install wall time (warm store) 39.6 s 8.5 s 7.0 s
Lockfile lines 13,364 3,037 3,444
resolution: entries 1,535 377 377
Astro root peer suffixes 30 (@azure/..., @vercel/..., ...) (rollup@4.60.4)(typescript@5.9.3) (rollup@4.60.4)(typescript@5.9.3)
sharp (optionalDependencies) refs in lockfile 0 110 85

Warm-cache hyperfine (3 runs, fresh node_modules + lockfile each time):

pacquet (patched):   670 ms ± 72 ms
pnpm 11.3.0:        1270 ms ±  7 ms
pacquet is 1.89 ± 0.20 times faster than pnpm

Closes the astro column in #11902.

Implementation

  • Add optional_dependencies: Option<HashMap<String, String>> and peer_dependencies_meta: Option<HashMap<String, PeerDependencyMeta>> to PackageVersion. The existing #[serde(rename_all = "camelCase")] handles wire format.
  • Add a PeerDependencyMeta newtype with just the optional field (the only field the resolver consumes).
  • Fix up the four struct-literal construction sites in tests + the trust-evidence projection.
  • Add a regression test that deserializes a fixture with both fields populated and asserts they round-trip through serde_json::to_value — which is what the resolver consumes.

Test plan

  • cargo nextest run -p pacquet-registry — 17/17 pass (incl. new test).
  • cargo nextest run -p pacquet-resolving-npm-resolver — 232/232 pass.
  • Affected pacquet-package-manager install::tests::fresh_install_* tests pass.
  • cargo clippy -p pacquet-registry --all-targets -- --deny warnings clean.
  • Manual: pacquet install on the vlt astro fixture produces a lockfile structurally matching pnpm's (377 resolutions, 2-peer astro suffix, sharp present).

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

Summary by CodeRabbit

  • New Features

    • Package versions now carry optionalDependencies and per-peer metadata, preserved during registry-style serialization/deserialization
  • Tests

    • Updated test fixtures and helpers to match the expanded package shape
    • Added integration tests verifying peer auto-install respects per-peer optional flags
    • Added resolver and manifest normalization tests to ensure optionalDependencies and per-peer metadata are carried and preserved

Review Change Stack

…denciesMeta from the registry

`PackageVersion` (the per-version registry manifest the npm resolver
parses) was missing the `optionalDependencies` and `peerDependenciesMeta`
fields. The resolver builds `ResolveResult.manifest` via
`serde_json::to_value(picked)` and downstream walks it with
`extract_children` (reads `optionalDependencies`) and
`extract_peer_dependencies` (reads `peerDependenciesMeta`). Without
those fields on the struct, both reads always saw nothing — so
`optionalDependencies` edges were silently dropped, and every optional
peer was treated as required, then auto-installed via the
`autoInstallPeers` fallback in `hoist_peers`.

On the vlt astro fixture this cascaded badly: `unstorage`'s 19 optional
peers (`@azure/*`, `@vercel/*`, `@netlify/blobs`, `@upstash/redis`, ...)
got auto-installed and pulled in their transitive deps, while astro's
own `optionalDependencies` (`sharp`) was missing entirely. Pacquet's
lockfile carried 1535 resolutions vs pnpm's 377 — the "5.5x astro
deep-tree slowdown" tracked in #11902 was almost all wasted work, not a
real perf bug.

After this patch on the vlt astro fixture:

- `pacquet install` warm-store: 39.6s -> 8.5s (4.7x), now within 22% of
  pnpm.
- Lockfile: 13364 lines -> 3037 lines (pnpm: 3444).
- Resolutions: 1535 -> 377 (matches pnpm exactly).
- Astro root peer suffixes: 30 (azure/vercel/...) -> `(rollup@4.60.4)(typescript@5.9.3)` (matches pnpm).
- `sharp` correctly installed via astro's `optionalDependencies`.
- Warm-cache hyperfine (3 runs): pacquet 670ms +/- 72ms vs pnpm 1.27s
  +/- 7ms — pacquet 1.89x faster.

Added a regression test that deserializes a fixture with both fields
populated and asserts they round-trip through `serde_json::to_value`,
which is what the resolver consumes.

---
Written by an agent (Claude Code, claude-opus-4-7).
@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 →

@coderabbitai

coderabbitai Bot commented May 25, 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: e32a96ae-9796-472e-95d4-b7a35bde312b

📥 Commits

Reviewing files that changed from the base of the PR and between 9c5ee14 and 37ab1ac.

📒 Files selected for processing (2)
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/tarball/src/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). (7)
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that 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/tarball/src/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/tarball/src/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

  • pacquet/crates/tarball/src/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/tarball/src/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
🔇 Additional comments (2)
pacquet/crates/package-manager/src/install/tests.rs (1)

898-901: LGTM!

pacquet/crates/tarball/src/tests.rs (1)

2701-2701: LGTM!


📝 Walkthrough

Walkthrough

PackageVersion is extended with optional_dependencies and peer_dependencies_meta plus a PeerDependencyMeta struct. Serde maps camelCase registry keys. Test helpers are updated and new resolver, installer, and tarball tests validate propagation and handling of these fields.

Changes

Optional and Peer Dependency Metadata

Layer / File(s) Summary
PackageVersion schema and PeerDependencyMeta type
pacquet/crates/registry/src/package_version.rs
Adds optional_dependencies: Option<HashMap<String, String>> and peer_dependencies_meta: Option<HashMap<String, PeerDependencyMeta>> with serde camelCase wiring; introduces PeerDependencyMeta { optional: Option<bool> } and unit test for (de)serialization.
Test helper fixtures updated
pacquet/crates/package-manager/src/build_snapshot/tests.rs, pacquet/crates/registry/src/package/tests.rs, pacquet/crates/resolving-npm-resolver/src/create_npm_resolution_verifier.rs, pacquet/crates/resolving-npm-resolver/src/pick_package_from_meta/tests.rs
All PackageVersion struct literals in test helpers are updated to set optional_dependencies: None and peer_dependencies_meta: None.
Resolver and installer tests for optional/peer metadata
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs, pacquet/crates/package-manager/src/install/tests.rs
Adds a resolver test that round-trips optionalDependencies and peerDependenciesMeta.optional into the resolved manifest, and two integration tests asserting auto-install-peers does not cascade peers marked optional via peerDependenciesMeta.
Tarball normalize_bundled_manifest tests
pacquet/crates/tarball/src/tests.rs
Adds normalize_bundled_manifest_tests verifying field inclusion/exclusion, lifecycle scripts filtering, null-field dropping, and preservation of optionalDependencies and peerDependenciesMeta.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#11919: Consumes peerDependenciesMeta.optional in lockfile-pruner BFS to recompute transitive optional flags.

"I nibble at JSON with a twitchy nose,
camelCase keys where the registry goes,
peers marked optional tucked in each map,
tests hopping through install and resolve trap,
carrots, manifests, and one happy prose."

🚥 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 precisely summarizes the main change: adding deserialization support for optionalDependencies and peerDependenciesMeta fields to PackageVersion, which is the core fix resolving the bug where optional dependencies were incorrectly auto-installed.
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 pacquet-deserialize-optional-deps-and-peer-meta

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 25, 2026

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.00      8.6±0.24ms   503.7 KB/sec    1.01      8.7±0.22ms   499.6 KB/sec

@codecov-commenter

codecov-commenter commented May 25, 2026

Copy link
Copy Markdown

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11934      +/-   ##
==========================================
+ Coverage   87.78%   87.83%   +0.05%     
==========================================
  Files         224      224              
  Lines       27395    27434      +39     
==========================================
+ Hits        24049    24097      +48     
+ Misses       3346     3337       -9     

☔ 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

github-actions Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Integrated-Benchmark Report (Linux)

Scenario: Isolated linker: fresh restore, cold cache + cold store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.994 ± 0.073 1.906 2.148 1.00
pacquet@main 1.996 ± 0.078 1.912 2.159 1.00 ± 0.05
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.9938525573599999,
      "stddev": 0.07285174102201411,
      "median": 1.9896210220600001,
      "user": 2.70562496,
      "system": 3.4255540999999994,
      "min": 1.90615812856,
      "max": 2.14785499556,
      "times": [
        1.94821307756,
        2.01224585356,
        1.90615812856,
        1.96791274756,
        2.01346805056,
        1.94606191056,
        2.14785499556,
        2.01132929656,
        2.06436273956,
        1.92091877356
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.9960040964599997,
      "stddev": 0.07752611868249408,
      "median": 1.98448472906,
      "user": 2.76433626,
      "system": 3.4041457999999998,
      "min": 1.91175636156,
      "max": 2.15918065956,
      "times": [
        1.99839151956,
        1.92322383756,
        1.96212909356,
        2.15918065956,
        2.06232497256,
        2.01565191756,
        1.91175636156,
        2.0411223815599997,
        1.91568228256,
        1.97057793856
      ]
    }
  ]
}

Scenario: Isolated linker: fresh restore, hot cache + hot store

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 626.1 ± 38.7 606.6 734.2 1.00
pacquet@main 630.5 ± 16.0 616.2 672.4 1.01 ± 0.07
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 0.6261135278800001,
      "stddev": 0.03869053174032278,
      "median": 0.61487094678,
      "user": 0.34567351999999996,
      "system": 1.3375246600000001,
      "min": 0.60656619378,
      "max": 0.73424328178,
      "times": [
        0.73424328178,
        0.62929567278,
        0.62041551778,
        0.61834236278,
        0.61616830978,
        0.60835444578,
        0.60656619378,
        0.60706045678,
        0.61357358378,
        0.60711545378
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 0.6305336395800001,
      "stddev": 0.016014435165109107,
      "median": 0.63020240978,
      "user": 0.36932912,
      "system": 1.3361702599999998,
      "min": 0.61620468478,
      "max": 0.67239671878,
      "times": [
        0.67239671878,
        0.63070780878,
        0.62969701078,
        0.63343489578,
        0.61864665778,
        0.61620468478,
        0.62354068378,
        0.61828619178,
        0.63104315878,
        0.63137858478
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, cold cache + cold store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 2.180 ± 0.027 2.136 2.234 1.00
pacquet@main 2.469 ± 0.049 2.404 2.577 1.13 ± 0.03
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 2.17965538634,
      "stddev": 0.0273242886842859,
      "median": 2.1797204186399997,
      "user": 3.7093298799999994,
      "system": 3.0991185399999996,
      "min": 2.13582293764,
      "max": 2.23412297564,
      "times": [
        2.17814669664,
        2.14978991164,
        2.18129414064,
        2.16881462764,
        2.13582293764,
        2.19156214564,
        2.19630740464,
        2.23412297564,
        2.16625568664,
        2.19443733664
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 2.46916563194,
      "stddev": 0.04850631085457987,
      "median": 2.45535188814,
      "user": 4.39180348,
      "system": 3.3236121400000003,
      "min": 2.40407909764,
      "max": 2.57705018464,
      "times": [
        2.45656566664,
        2.4908223616400003,
        2.5115931706400003,
        2.57705018464,
        2.47649765064,
        2.43263706764,
        2.40407909764,
        2.45413810964,
        2.44244510664,
        2.44582790364
      ]
    }
  ]
}

Scenario: Isolated linker: fresh install, hot cache + hot store

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.354 ± 0.019 1.329 1.385 1.00
pacquet@main 1.636 ± 0.061 1.583 1.798 1.21 ± 0.05
BENCHMARK_REPORT.json
{
  "results": [
    {
      "command": "pacquet@HEAD",
      "mean": 1.35356074386,
      "stddev": 0.01945262213975736,
      "median": 1.3578582569600002,
      "user": 1.62934138,
      "system": 1.8211012599999996,
      "min": 1.32903256596,
      "max": 1.38489332296,
      "times": [
        1.36000947796,
        1.37556975296,
        1.36032691696,
        1.32903256596,
        1.33137790896,
        1.38489332296,
        1.3312892139599999,
        1.3423478709599999,
        1.36505337196,
        1.35570703596
      ]
    },
    {
      "command": "pacquet@main",
      "mean": 1.63642330566,
      "stddev": 0.0605299775201648,
      "median": 1.61429895746,
      "user": 2.03099208,
      "system": 2.04223246,
      "min": 1.5825680209600002,
      "max": 1.7976231139599999,
      "times": [
        1.6393849729599999,
        1.5825680209600002,
        1.61213630896,
        1.6086372249599998,
        1.7976231139599999,
        1.6193940579600001,
        1.61643526496,
        1.61115294396,
        1.6121626499600001,
        1.66473849796
      ]
    }
  ]
}

@github-actions

github-actions Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/11934
Testbedpacquet
Click to view all benchmark results
BenchmarkLatencyBenchmark Result
milliseconds (ms)
(Result Δ%)
Upper Boundary
milliseconds (ms)
(Limit %)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
🚷 view threshold
2,179.66 ms
(-42.46%)Baseline: 3,788.30 ms
4,545.96 ms
(47.95%)
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
🚷 view threshold
1,353.56 ms
(-52.24%)Baseline: 2,834.22 ms
3,401.06 ms
(39.80%)
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
🚷 view threshold
1,993.85 ms
(-9.57%)Baseline: 2,204.91 ms
2,645.89 ms
(75.36%)
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
🚷 view threshold
626.11 ms
(-4.60%)Baseline: 656.32 ms
787.58 ms
(79.50%)
🐰 View full continuous benchmarking report in Bencher

…nciesMeta

Port the upstream test cases that exercise the same correctness concern
the previous commit fixed — `optionalDependencies` and
`peerDependenciesMeta` round-tripping through the resolver and the
bundled-manifest pipelines.

Tests added:

- `pacquet-resolving-npm-resolver::npm_resolver::tests::resolved_manifest_carries_optional_dependencies_and_peer_dependencies_meta` —
  mocks a registry packument that declares both fields and asserts the
  resolver-produced `ResolveResult.manifest` carries them as
  camelCase JSON keys. This is the tightest level at which the
  #11934 bug surfaces: before the fix, `serde_json::to_value(picked)`
  dropped both fields because `PackageVersion` didn't declare them.

- `pacquet-package-manager::install::tests::auto_install_peers_does_not_cascade_optional_peers` —
  ported from upstream's "warning is not reported when cannot resolve
  optional peer dependency" at
  https://github.com/pnpm/pnpm/blob/1fb8a2d5d8/installing/deps-installer/test/install/peerDependencies.ts#L1181-L1255.
  Installs `@pnpm.e2e/abc-optional-peers@1.0.0` against the real mock
  registry under default `autoInstallPeers: true`; asserts that the
  required peer (`peer-a`) lands in the virtual store while the two
  optional peers (`peer-b`, `peer-c`) stay out. Before the fix,
  `peerDependenciesMeta` was dropped and the `autoInstallPeers`
  fallback in `hoist_peers` cascaded both optional peers (plus their
  transitives) into the install.

- `pacquet-package-manager::install::tests::auto_install_peers_skips_meta_only_optional_peers` —
  ported from upstream's companion test
  https://github.com/pnpm/pnpm/blob/1fb8a2d5d8/installing/deps-installer/test/install/peerDependencies.ts#L1257-L1323.
  Same assertion shape but the optional peers are declared *only* via
  `peerDependenciesMeta` (no matching `peerDependencies` entry);
  upstream and pacquet treat such entries as optional peers with an
  implicit range of `*`.

- `pacquet-tarball::tests::normalize_bundled_manifest_tests` (8 cases) —
  ported from upstream's
  https://github.com/pnpm/pnpm/blob/1fb8a2d5d8/store/cafs/test/normalizeBundledManifest.test.ts.
  The bundled-manifest pipeline is the install-side sibling of the
  resolver-side path the previous commit fixed; pacquet's
  `normalize_bundled_manifest` already preserves the right fields,
  but had zero unit tests. Pins the field-preservation contract so a
  regression there can't replicate #11934 on the install
  side. Two upstream cases are intentionally not ported (semver.clean
  normalization and the `0.0.0` default for missing versions) because
  pacquet's port intentionally diverges from upstream on both, per the
  function's existing doc comment.

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

zkochan commented May 25, 2026

Copy link
Copy Markdown
Member Author

Ported the corresponding upstream tests to pacquet in 9423a71bd6.

Tests added

Resolver layer — would have caught the original bug

  • pacquet-resolving-npm-resolver::npm_resolver::tests::resolved_manifest_carries_optional_dependencies_and_peer_dependencies_meta — mocks a registry packument that declares both fields and asserts the resolver-produced ResolveResult.manifest carries them as camelCase JSON keys. Before the fix, serde_json::to_value(picked) silently dropped both fields because PackageVersion didn't declare them; this test fails against that state.

Install integration — ports from installing/deps-installer/test/install/peerDependencies.ts

  • pacquet-package-manager::install::tests::auto_install_peers_does_not_cascade_optional_peers — ported from upstream's warning is not reported when cannot resolve optional peer dependency. Installs @pnpm.e2e/abc-optional-peers@1.0.0 against the real mock registry; asserts the required peer (peer-a) lands in the virtual store while the two optional peers (peer-b, peer-c) stay out. Before the fix, the autoInstallPeers fallback in hoist_peers cascaded both optional peers into the install.

  • pacquet-package-manager::install::tests::auto_install_peers_skips_meta_only_optional_peers — ported from upstream's companion test. Uses @pnpm.e2e/abc-optional-peers-meta-only@1.0.0 where the optional peers are declared only via peerDependenciesMeta (no matching peerDependencies entry); pacquet treats such entries as optional peers with an implicit range of *.

Install-side sibling — port from store/cafs/test/normalizeBundledManifest.test.ts

  • pacquet-tarball::tests::normalize_bundled_manifest_tests (8 cases) — port of normalizeBundledManifest.test.ts. Pacquet's normalize_bundled_manifest already preserves optionalDependencies and peerDependenciesMeta correctly, but had zero unit tests — this pins the field-preservation contract on the install-side bundled-manifest path so a regression there can't replicate the bug on the install side. Two upstream cases intentionally not ported (semver.clean normalization and the 0.0.0 default for missing version) because pacquet diverges from upstream on both per the function's existing doc comment.

Upstream tests not ported (out of scope)

  • validatePeerDependencies.test.ts — pacquet does not yet have a validate_peer_dependencies equivalent; that is a separate feature gap.
  • transformPeerDependenciesMeta.test.ts — covers pnpm publish's manifest-export step, unrelated to install correctness.
  • hoistPeers.test.ts cases already fully covered by pacquet-resolving-deps-resolver::hoist_peers::tests.
  • resolving/npm-resolver/test/optionalDependencies.test.ts cases already covered by pacquet-resolving-npm-resolver::pick_package::tests::{default_pick_targets_abbreviated_endpoint_and_mirror, optional_opt_forces_full_metadata_endpoint, cache_key_separates_abbreviated_from_full}.

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

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

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

898-916: ⚡ Quick win

Trim test rustdoc to intent-level contract only.

Both doc blocks currently re-describe test mechanics and assertions in detail. Keep only the non-obvious “why/regression contract” and let the assertions document behavior.

[Suggestion: collapse each block to a short regression statement plus upstream reference.]

As per coding guidelines, "Tests are documentation. Do not duplicate them in prose."

Also applies to: 1012-1021

🤖 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/package-manager/src/install/tests.rs` around lines 898 - 916,
Trim the verbose rustdoc blocks that precede the test about
`@pnpm.e2e/abc-optional-peers@1.0.0` (the test describing optional peers
`peer-b`/`peer-c` and regression for pnpm/pnpm#11934) down to an intent-level
sentence: state the regression/contract (e.g., "regression: optional
peerDependenciesMeta must be preserved so optional peers are not
auto-installed") and include the upstream reference link; remove step-by-step
mechanics and assertions so the test assertions themselves remain the single
source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@pacquet/crates/package-manager/src/install/tests.rs`:
- Around line 898-916: Trim the verbose rustdoc blocks that precede the test
about `@pnpm.e2e/abc-optional-peers@1.0.0` (the test describing optional peers
`peer-b`/`peer-c` and regression for pnpm/pnpm#11934) down to an intent-level
sentence: state the regression/contract (e.g., "regression: optional
peerDependenciesMeta must be preserved so optional peers are not
auto-installed") and include the upstream reference link; remove step-by-step
mechanics and assertions so the test assertions themselves remain the single
source of truth.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fad6f5f7-0f50-46a2-9fab-18a1ae8c2f58

📥 Commits

Reviewing files that changed from the base of the PR and between 681da39 and 9423a71.

📒 Files selected for processing (3)
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/tarball/src/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (windows-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Compile & Lint
🧰 Additional context used
📓 Path-based instructions (1)
pacquet/**/*.rs

📄 CodeRabbit inference engine (pacquet/AGENTS.md)

pacquet/**/*.rs: When porting a function that 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-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/tarball/src/tests.rs
🧠 Learnings (3)
📚 Learning: 2026-05-20T19:40:55.051Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11774
File: pacquet/crates/resolving-deps-resolver/src/resolve_peers.rs:0-0
Timestamp: 2026-05-20T19:40:55.051Z
Learning: In the pacquet Rust code, ensure the semver implementation uses the `node-semver` crate (not `nodejs-semver`). `node-semver`’s public API does not include a `satisfies_with_prerelease`-style method; prerelease-tolerant matching should be implemented inline by first calling `Range::satisfies`, and when it rejects a prerelease version, retry matching against a stripped `MAJOR.MINOR.PATCH` base of the prerelease version.

Applied to files:

  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-22T00:08:44.646Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11837
File: pacquet/crates/resolving-npm-resolver/src/pick_package.rs:33-51
Timestamp: 2026-05-22T00:08:44.646Z
Learning: In the pnpm/pnpm repo’s pacquet Rust crates, do not flag Unicode ellipsis characters (U+2026, `…`) in Rust doc comments (`///` / `/** */`) as a lint violation. The pacquet crate’s `dylint.toml` only enables `perfectionist::derive_ordering`, and the Dylint `unicode-ellipsis` rule is not enabled for this project—so `…` in doc comments is an intentional, repo-consistent style.

Applied to files:

  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/tarball/src/tests.rs
📚 Learning: 2026-05-20T23:07:58.444Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 11784
File: pacquet/crates/resolving-deps-resolver/src/hoist_peers.rs:120-133
Timestamp: 2026-05-20T23:07:58.444Z
Learning: When reviewing code in this pacquet Rust port, follow the upstream pnpm compatibility rule: only match pnpm’s behavior exactly. Do not propose review changes that intentionally deviate from pnpm’s documented/observed behavior, even if pnpm appears buggy. If you identify a real bug in pnpm behavior, the review should prioritize fixing it upstream in pnpm first, and avoid implementing a pnpm-behavior workaround here unless the same fix has already landed upstream.

Applied to files:

  • pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs
  • pacquet/crates/package-manager/src/install/tests.rs
  • pacquet/crates/tarball/src/tests.rs
🧬 Code graph analysis (2)
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rs (1)
pacquet/crates/resolving-resolver-base/src/resolve.rs (2)
  • WantedDependency (82-102)
  • ResolveOptions (194-225)
pacquet/crates/package-manager/src/install/tests.rs (3)
pacquet/crates/config/src/lib.rs (2)
  • new (923-925)
  • NodeLinker (43-56)
pacquet/crates/reporter/src/lib.rs (1)
  • SilentReporter (771-771)
pacquet/tasks/registry-mock/src/mock_instance.rs (2)
  • load_or_init (126-142)
  • drop (23-29)
🔇 Additional comments (3)
pacquet/crates/package-manager/src/install/tests.rs (1)

917-1010: LGTM!

Also applies to: 1022-1099

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

331-401: LGTM!

pacquet/crates/tarball/src/tests.rs (1)

4-5: LGTM!

Also applies to: 2531-2713

- `pacquet-tarball::tests::normalize_bundled_manifest_tests::preserves_optional_dependencies_and_peer_dependencies_meta_keys`
  had a trailing comma on a single-line `assert_eq!` invocation that
  `cargo fmt` left in place; `perfectionist::macro-trailing-comma`
  rejects it under `-D warnings`. Drop the comma.
- Trim the rustdoc + inline comments on
  `auto_install_peers_does_not_cascade_optional_peers` down to the
  regression intent + upstream link; the assertions themselves carry
  the mechanics.

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants