fix(pacquet/registry): deserialize optionalDependencies and peerDependenciesMeta#11934
Conversation
…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 reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🧰 Additional context used📓 Path-based instructions (1)pacquet/**/*.rs📄 CodeRabbit inference engine (pacquet/AGENTS.md)
Files:
🧠 Learnings (3)📚 Learning: 2026-05-20T19:40:55.051ZApplied to files:
📚 Learning: 2026-05-22T00:08:44.646ZApplied to files:
📚 Learning: 2026-05-20T23:07:58.444ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughPackageVersion is extended with ChangesOptional and Peer Dependency Metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
Integrated-Benchmark Report (Linux)Scenario: Isolated linker: fresh restore, cold cache + cold store
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
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
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
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
]
}
]
} |
|
| Branch | pr/11934 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark 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%) |
…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).
|
Ported the corresponding upstream tests to pacquet in Tests addedResolver layer — would have caught the original bug
Install integration — ports from
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pacquet/crates/package-manager/src/install/tests.rs (1)
898-916: ⚡ Quick winTrim 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
📒 Files selected for processing (3)
pacquet/crates/package-manager/src/install/tests.rspacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/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 firespnpm:<channel>events throughglobalLogger,logger.debug(), orstreamParser.write(), mirror the call site, payload, and ordering so the reporter parses pacquet's NDJSON the same way it parses pnpm's.
Declare a newtype wrapper for branded string types. Do not collapse the brand into a plainStringor&str.
If upstream always validates before construction, validate in pacquet's wrapper too. The wrapper must construct only viaTryFrom<String>and/orFromStr. Do not provide an infallible public constructor.
If upstream never validates, just brand for type-safety. Expose an infallibleFrom<String>(andFrom<&str>when convenient).
If upstream occasionally constructs without validation, exposefrom_str_uncheckedas an escape hatch alongside the validating constructor.
Match upstream serde behavior for branded types that cross JSON, YAML, or INI boundaries. Use#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization.
Use#[derive(derive_more::From)]and#[derive(derive_more::Into)]for mechanical conversion impls. Fall back to manualimplonly when conversion needs custom logic.
String-literal unions should becomeenums, not newtype wrappers. Model closed sets of valid string values as enums.
Template literal types should be treated as branded strings with validation discipline from rules 2-5.
Choose owned vs. borrowed parameters to minimize copies. Widen to the most encompassing type (&Pathover&PathBuf,&strover&String) when it doesn't force extra copies.
PreferArc::clone(&x)/Rc::clone(&x)overx.clone()for reference-counted types, so the cost is visible at the call site.
Follow Rust API Guidelines for naming conventions.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;. Two forms stay allowed: external-crate preludes likeuse rayon::prelude::*;and root-of-module re-...
Files:
pacquet/crates/resolving-npm-resolver/src/npm_resolver/tests.rspacquet/crates/package-manager/src/install/tests.rspacquet/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.rspacquet/crates/package-manager/src/install/tests.rspacquet/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.rspacquet/crates/package-manager/src/install/tests.rspacquet/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.rspacquet/crates/package-manager/src/install/tests.rspacquet/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).
Summary
PackageVersion(the per-version registry manifest the npm resolver parses) was missing theoptionalDependenciesandpeerDependenciesMetafields. The resolver buildsResolveResult.manifestviaserde_json::to_value(picked)and downstream walks it withextract_children(readsoptionalDependencies) andextract_peer_dependencies(readspeerDependenciesMeta). Without those fields on the struct, both reads always saw nothing — sooptionalDependenciesedges were silently dropped, and every optional peer was treated as required, then auto-installed via theautoInstallPeersfallback inhoist_peers.Astro cascade
On the vlt
astrofixture,unstorage(a transitive of astro) declares 19 optional peers viapeerDependenciesMeta(@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 ownoptionalDependencies(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_recursionBox-pinning, per-nodelock_recoverablemutex acquires, manifestserde_json::to_valuecost, tarball extraction) were the bottleneck.Before / after on vlt astro
pacquet installwall time (warm store)resolution:entries@azure/...,@vercel/..., ...)(rollup@4.60.4)(typescript@5.9.3)(rollup@4.60.4)(typescript@5.9.3)sharp(optionalDependencies) refs in lockfileWarm-cache hyperfine (3 runs, fresh
node_modules+ lockfile each time):Closes the astro column in #11902.
Implementation
optional_dependencies: Option<HashMap<String, String>>andpeer_dependencies_meta: Option<HashMap<String, PeerDependencyMeta>>toPackageVersion. The existing#[serde(rename_all = "camelCase")]handles wire format.PeerDependencyMetanewtype with just theoptionalfield (the only field the resolver consumes).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.pacquet-package-managerinstall::tests::fresh_install_*tests pass.cargo clippy -p pacquet-registry --all-targets -- --deny warningsclean.pacquet installon 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
Tests