Conversation
Code Review by Qodo
Context used 1. Optional tarballs fetched before filtering
|
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 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). (9)
📝 WalkthroughWalkthroughInfers missing os/cpu/libc for optional dependencies from package names and uses those inferred fields in installability checks so platform-specific optional packages are skipped when incompatible. ChangesPlatform Field Inference for Optional Dependencies
Sequence DiagramsequenceDiagram
participant Client
participant PackageRequester
participant effectivePlatform
participant inferPlatformFromPackageName
participant checkPackage
Client->>PackageRequester: request optional dep (package name)
PackageRequester->>PackageRequester: obtain manifest (os/cpu/libc missing)
PackageRequester->>effectivePlatform: build effective manifest for optional dep
effectivePlatform->>inferPlatformFromPackageName: infer {os,cpu,libc} from name
inferPlatformFromPackageName->>effectivePlatform: return inferred platform
effectivePlatform->>checkPackage: run checkPackage with enriched manifest
checkPackage->>PackageRequester: return installability verdict
PackageRequester->>Client: skip download if not installable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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 |
PR Summary by QodoSkip platform-specific optional deps when metadata/lockfile lacks os/cpu/libc WalkthroughsDescription• Re-check optional dependency installability using tarball manifest when metadata lacks platform fields. • Heal lockfiles by writing os/cpu/libc so subsequent installs skip incompatible binaries. • Add unit + e2e coverage for metadata-stripping registries and broken lockfiles. Diagramgraph TD
REG(["Registry / proxy"]) --> PR["Package requester"] --> SF["Store fetchPackage"] --> BM["Tarball manifest"] --> IC["Platform check"] --> LF[("Lockfile entry")]
LF --> GB["Headless dep graph"] --> NM["Link node_modules"]
IC -->|"not installable"| SK["Mark skipped"]
SK -.-> NM
subgraph Legend
direction LR
_ext(["External"]) ~~~ _svc["Component"] ~~~ _db[("Data")]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Always fetch bundled manifest for all optional deps
2. Hard-fail when optional dep lacks platform fields
Recommendation: Keep the PR’s conditional strategy: only consult the tarball manifest for optional deps that appear platform-unrestricted due to missing os/cpu/libc. This minimizes overhead on healthy registries while correctly skipping incompatible binaries and repairing lockfiles so future installs become fast and deterministic. File ChangesBug fix (3)
Tests (2)
Documentation (1)
|
…name Some registries strip the os/cpu/libc fields (or just libc) from the version objects of the packuments they serve. Resolution then saw every platform-specific optional dependency as platform-unrestricted, so pnpm downloaded and installed the binaries of every platform regardless of supportedArchitectures, and wrote lockfile entries without the platform fields, which broke installs from that lockfile on every machine. Platform-specific binary packages encode their platform in the package name (e.g. @nx/nx-win32-arm64-msvc), so packageIsInstallable now fills the missing platform fields of an optional dependency from the name's tokens. Since every install path decides installability through that check before fetching, foreign-platform binaries are skipped without even downloading them, in fresh resolution and in headless installs with both node linkers alike. A package that declares no platform fields at all is treated as platform-specific only when an operating system is recognized in its name, so a generic name segment (such as 'arm' on its own) never gets a package skipped. Fixes #11702 Fixes #9940
|
Code review by qodo was updated up to the latest commit 23f55eb |
…l deps from the package name Port of pnpm commit 34875b2d7c (PR #12312). Some registries strip the os/cpu/libc fields (or just libc) from the version objects of the packuments they serve, and lockfile entries written from such metadata lack the fields too, so every platform's binaries were installed regardless of supportedArchitectures. Platform-specific binary packages encode their platform in the package name (e.g. @nx/nx-win32-arm64-msvc), so the installability check now fills the missing platform fields of an optional dependency from the name's tokens: infer_platform_from_package_name + inferred_platform in pacquet-package-is-installable, applied inside package_is_installable (hoisted linker) and in compute_skipped_snapshots (isolated linker, with the check cache keyed by the snapshot's optional flag since the verdicts can differ). The any_installability_constraint fast path now also considers optional snapshots whose names infer a platform their metadata row does not declare, so the inference is reachable on lockfiles without any declared constraint. Same guard rails as upstream: declared fields always win (each field is filled only when missing — a missing libc alone is inferred, disambiguating -gnu vs -musl), and a package declaring no platform fields at all engages the inference only when an operating-system token is recognized in its name, so a generic name segment such as 'arm' on its own never gets a package skipped. Fixes #11702 Fixes #9940
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pacquet/crates/env-installer/src/install_config_deps.rs (1)
289-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOptional subdeps with stripped platform fields still bypass name-based platform inference.
Line 289 exits before installability evaluation when
os/cpu/libcare missing, so the stripped-metadata case never reaches inference. Addingmanifest.nameon Line 293 alone does not activate inference in this path.Suggested fix
use pacquet_package_is_installable::{ - InstallabilityOptions, PackageInstallabilityManifest, check_package, + InstallabilityOptions, PackageInstallabilityManifest, WantedPlatformRef, check_package, + inferred_platform, }; @@ fn is_compatible<Reporter: self::Reporter>( @@ ) -> bool { - if subdep.os.is_none() && subdep.cpu.is_none() && subdep.libc.is_none() { - return true; - } - let manifest = PackageInstallabilityManifest { + let mut manifest = PackageInstallabilityManifest { name: subdep.name.clone(), engines: None, cpu: subdep.cpu.clone(), os: subdep.os.clone(), libc: subdep.libc.clone(), }; + if let Some(platform) = inferred_platform( + &manifest.name, + WantedPlatformRef { + os: manifest.os.as_deref(), + cpu: manifest.cpu.as_deref(), + libc: manifest.libc.as_deref(), + }, + ) { + manifest.os = platform.os; + manifest.cpu = platform.cpu; + manifest.libc = platform.libc; + } + if manifest.os.is_none() && manifest.cpu.is_none() && manifest.libc.is_none() { + return true; + }Based on learnings: follow upstream pnpm behavior exactly for parity-sensitive ports.
🤖 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/env-installer/src/install_config_deps.rs` around lines 289 - 307, The early return that immediately returns true when subdep.os, subdep.cpu, and subdep.libc are all None prevents name-based platform inference from running; remove that early-return and instead always build the PackageInstallabilityManifest (using subdep.name, subdep.cpu, subdep.os, subdep.libc) and run the existing installability evaluation that uses id and InstallabilityOptions so stripped metadata still triggers name-based inference consistent with upstream pnpm behavior; ensure id = format!("{}@{}", subdep.name, subdep.version) and the constructed InstallabilityOptions (current_node_version/current_os/current_cpu/current_libc/supported_architectures) are passed into the evaluation path.Source: Learnings
🤖 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.
Outside diff comments:
In `@pacquet/crates/env-installer/src/install_config_deps.rs`:
- Around line 289-307: The early return that immediately returns true when
subdep.os, subdep.cpu, and subdep.libc are all None prevents name-based platform
inference from running; remove that early-return and instead always build the
PackageInstallabilityManifest (using subdep.name, subdep.cpu, subdep.os,
subdep.libc) and run the existing installability evaluation that uses id and
InstallabilityOptions so stripped metadata still triggers name-based inference
consistent with upstream pnpm behavior; ensure id = format!("{}@{}",
subdep.name, subdep.version) and the constructed InstallabilityOptions
(current_node_version/current_os/current_cpu/current_libc/supported_architectures)
are passed into the evaluation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bbff8cd2-c8d4-4c68-ba12-ba32ec9e775f
📒 Files selected for processing (11)
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/package-is-installable/src/infer_platform_from_package_name.rspacquet/crates/package-is-installable/src/lib.rspacquet/crates/package-is-installable/src/package_is_installable.rspacquet/crates/package-is-installable/src/tests.rspacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/installability.rspacquet/crates/package-manager/src/installability/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). (8)
- GitHub Check: ubuntu-latest / Node.js 24 / Test
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Doc
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (2)
pacquet/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/*.rs: Log emissions are part of matching pnpm — when porting a function that firespnpm:<channel>events throughglobalLogger,logger.debug(...), orstreamParser.write(...), mirror the call site, payload, and ordering so@pnpm/cli.default-reporterparses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plainStringor&strin Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too viaTryFrom<String>and/orFromStrand do not provide an infallible public constructor
If upstream TypeScript never validates a branded string, just brand for type-safety in Rust by exposing an infallibleFrom<String>constructor
If upstream TypeScript occasionally constructs a branded string without validation, exposefrom_str_uncheckedin Rust as an escape hatch alongside the validating constructor
Match upstream serde behavior for branded strings crossing JSON, YAML, or INI boundaries by using#[serde(try_from = "String")]for deserialization and#[serde(into = "String")]for serialization
Derive simple conversions for branded strings using#[derive(derive_more::From)]and#[derive(derive_more::Into)]instead of handwritingimplblocks; use manualimplonly when conversion needs custom logic
Model TypeScript string literal unions (like'auto' | 'always' | 'never') as Rustenums instead of newtype wrappers, since the set of valid values is closed
Treat TypeScript string template literal types (like`${string}@${string}`) the same as branded string types in Rust, using a newtype wrapper with validation
Follow the code style guide inCODE_STYLE_GUIDE.md— imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching,pipe-trait, error handling, test layout, and cloning ofArcandRc
Choose owned vs. borrowed parameters to minimize copies; widen to t...
Files:
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/package-is-installable/src/tests.rspacquet/crates/package-is-installable/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-is-installable/src/package_is_installable.rspacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rspacquet/crates/package-is-installable/src/infer_platform_from_package_name.rspacquet/crates/package-manager/src/installability.rspacquet/crates/package-manager/src/installability/tests.rs
pacquet/**/tests/**/*.rs
📄 CodeRabbit inference engine (pacquet/AGENTS.md)
pacquet/**/tests/**/*.rs: Prefer real fixtures over dependency-injection seams — usetempfile::TempDir, the mocked registry, or integration tests spawning the actual binary for happy paths and most error paths; use the DI seam only for filesystem error kinds, deterministic time, shared process-global state, or external-service happy paths
Follow test-logging guidance — log before non-assert_eq!assertions,dbg!complex structures, skip logging for simple scalarassert_eq!
Tests must not be tolerant of missing build/runtime environment by silently returning early when a tool isn't found — let the test panic when a required tool is absent
Prefer#[cfg_attr(target_os = "windows", ignore = "...")](or matching#[cfg(unix)]gates) over runtime probe-and-skip helpers for platform-locked tools
Port relevant pnpm tests to Rust tests whenever they translate when porting behavior from pnpm
Use snapshot tests withinstaand carefully review diffs when intentional changes alter snapshots; accept withcargo insta reviewonly after careful review
Tests that need the mocked registry should startpnprthroughpacquet-testing-utils;cargo test/cargo nextest runshould not require a separatejust registry-mock launchstep
Files:
pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
🧠 Learnings (4)
📚 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/env-installer/src/install_config_deps.rspacquet/crates/package-is-installable/src/tests.rspacquet/crates/package-is-installable/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-is-installable/src/package_is_installable.rspacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rspacquet/crates/package-is-installable/src/infer_platform_from_package_name.rspacquet/crates/package-manager/src/installability.rspacquet/crates/package-manager/src/installability/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/env-installer/src/install_config_deps.rspacquet/crates/package-is-installable/src/tests.rspacquet/crates/package-is-installable/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-is-installable/src/package_is_installable.rspacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rspacquet/crates/package-is-installable/src/infer_platform_from_package_name.rspacquet/crates/package-manager/src/installability.rspacquet/crates/package-manager/src/installability/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/env-installer/src/install_config_deps.rspacquet/crates/package-is-installable/src/tests.rspacquet/crates/package-is-installable/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-is-installable/src/package_is_installable.rspacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rspacquet/crates/package-is-installable/src/infer_platform_from_package_name.rspacquet/crates/package-manager/src/installability.rspacquet/crates/package-manager/src/installability/tests.rs
📚 Learning: 2026-06-06T18:58:37.156Z
Learnt from: zkochan
Repo: pnpm/pnpm PR: 12243
File: pacquet/crates/package-manager/src/install_package_by_snapshot.rs:319-322
Timestamp: 2026-06-06T18:58:37.156Z
Learning: When reviewing Rust code, do not assume `matches!(expr, Pattern(_))` will move out of `expr` if `Pattern(_)` contains no by-value bindings. `matches!` desugars to a `match` that auto-borrows the scrutinee for discrimination, so even if `expr` is a non-`Copy` value behind a shared reference (e.g., `&T`), the macro should not move-out of the borrowed data purely due to `matches!`. Treat `matches!(&expr, Pattern(_))` as a readability/clarity improvement, not a correctness requirement. Only flag potential move-out-of-borrow risks when the pattern includes by-value bindings that would require moving the matched value.
Applied to files:
pacquet/crates/env-installer/src/install_config_deps.rspacquet/crates/package-is-installable/src/tests.rspacquet/crates/package-is-installable/src/lib.rspacquet/crates/package-manager/src/install_with_fresh_lockfile.rspacquet/crates/package-manager/src/hoisted_dep_graph.rspacquet/crates/package-manager/src/install_frozen_lockfile.rspacquet/crates/package-is-installable/src/package_is_installable.rspacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rspacquet/crates/package-is-installable/src/infer_platform_from_package_name.rspacquet/crates/package-manager/src/installability.rspacquet/crates/package-manager/src/installability/tests.rs
🔇 Additional comments (10)
pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs (1)
1-108: LGTM!pacquet/crates/package-is-installable/src/lib.rs (1)
21-21: LGTM!Also applies to: 34-34
pacquet/crates/package-is-installable/src/package_is_installable.rs (1)
11-11: LGTM!Also applies to: 19-25, 211-235
pacquet/crates/package-manager/src/hoisted_dep_graph.rs (1)
667-667: LGTM!Also applies to: 773-788
pacquet/crates/package-is-installable/src/tests.rs (1)
4-4: LGTM!Also applies to: 7-7, 11-11
pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs (1)
1-172: LGTM!pacquet/crates/package-manager/src/installability.rs (1)
28-29: LGTM!Also applies to: 319-320, 363-413, 465-486, 520-526
pacquet/crates/package-manager/src/install_frozen_lockfile.rs (1)
332-334: LGTM!pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs (1)
1468-1472: LGTM!pacquet/crates/package-manager/src/installability/tests.rs (1)
266-307: LGTM!Also applies to: 319-320, 604-739
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 10.184068852360003,
"stddev": 0.10541900502246532,
"median": 10.135112506759999,
"user": 3.1252711800000004,
"system": 3.25514054,
"min": 10.08806012726,
"max": 10.38867267526,
"times": [
10.27129918326,
10.38867267526,
10.121321054260001,
10.14878406926,
10.12144094426,
10.11168505726,
10.08806012726,
10.32734172826,
10.110619379260001,
10.151464305260001
]
},
{
"command": "pacquet@main",
"mean": 10.20081311476,
"stddev": 0.15237127323756286,
"median": 10.14690171376,
"user": 3.10317048,
"system": 3.2437137400000005,
"min": 10.10560520126,
"max": 10.60700606626,
"times": [
10.15725106826,
10.10576898726,
10.14947149526,
10.13901636526,
10.10560520126,
10.14396766126,
10.60700606626,
10.14433193226,
10.29723111026,
10.15848126026
]
},
{
"command": "pnpr@HEAD",
"mean": 5.421939813460001,
"stddev": 0.14984016215992194,
"median": 5.36109958326,
"user": 2.4564257800000004,
"system": 2.89725554,
"min": 5.320967529260001,
"max": 5.71966667426,
"times": [
5.399460528260001,
5.71966667426,
5.336396645260001,
5.320967529260001,
5.37773347126,
5.6836011312600005,
5.3308600442600005,
5.34446569526,
5.377810961260001,
5.32843545426
]
},
{
"command": "pnpr@main",
"mean": 5.431507422159999,
"stddev": 0.15715606271562757,
"median": 5.37091424776,
"user": 2.3942955800000005,
"system": 2.9104699399999996,
"min": 5.32303831026,
"max": 5.84038836726,
"times": [
5.44791247626,
5.32303831026,
5.33909461426,
5.33112228726,
5.34030273626,
5.35403961826,
5.38778887726,
5.52212066326,
5.84038836726,
5.42926627126
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.63479453576,
"stddev": 0.008942263661510656,
"median": 0.6316761939600001,
"user": 0.36614715999999997,
"system": 1.2930994999999998,
"min": 0.6262104044600001,
"max": 0.6506167984600001,
"times": [
0.6294313034600001,
0.6268335744600001,
0.6392051524600001,
0.6262104044600001,
0.6339210844600001,
0.6291224044600001,
0.6506167984600001,
0.6491345714600001,
0.6353926924600001,
0.6280773714600001
]
},
{
"command": "pacquet@main",
"mean": 0.6534678733600001,
"stddev": 0.03432029750011654,
"median": 0.6393050069600001,
"user": 0.36240066000000004,
"system": 1.305308,
"min": 0.6282373774600001,
"max": 0.7387372204600001,
"times": [
0.6594339964600001,
0.6402915034600001,
0.6349765264600001,
0.64879750546,
0.7387372204600001,
0.63104544246,
0.6834687414600001,
0.6383185104600001,
0.6313719094600001,
0.6282373774600001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.75934443496,
"stddev": 0.07027763231879888,
"median": 0.7338524774600002,
"user": 0.36419795999999993,
"system": 1.3408778,
"min": 0.7160381574600001,
"max": 0.9526584014600001,
"times": [
0.7638853504600001,
0.74241064046,
0.72484582346,
0.72302114646,
0.7160381574600001,
0.9526584014600001,
0.7296085304600001,
0.7316146534600001,
0.7360903014600001,
0.7732713444600001
]
},
{
"command": "pnpr@main",
"mean": 0.77174734076,
"stddev": 0.07925925636578598,
"median": 0.7498277359600001,
"user": 0.3904150599999999,
"system": 1.3131698,
"min": 0.7226569224600001,
"max": 0.9902314004600001,
"times": [
0.7716507704600001,
0.7594352614600001,
0.73465650946,
0.7277208004600001,
0.7288263724600001,
0.9902314004600001,
0.7826398984600001,
0.7515093784600001,
0.74814609346,
0.7226569224600001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 9.19006800318,
"stddev": 0.04406301440274203,
"median": 9.196061198879999,
"user": 3.45843032,
"system": 3.2380225999999994,
"min": 9.10056051338,
"max": 9.24517547838,
"times": [
9.21548741038,
9.220811768379999,
9.17195429738,
9.24517547838,
9.10056051338,
9.178750279379999,
9.17922724138,
9.14472288138,
9.23109500538,
9.21289515638
]
},
{
"command": "pacquet@main",
"mean": 9.17509698318,
"stddev": 0.05491666827956955,
"median": 9.166673483379999,
"user": 3.44941272,
"system": 3.2195758999999997,
"min": 9.10700317538,
"max": 9.292305452379999,
"times": [
9.17739964238,
9.21776597738,
9.15553419738,
9.12790005038,
9.292305452379999,
9.124133454379999,
9.21153083938,
9.10700317538,
9.18144971838,
9.15594732438
]
},
{
"command": "pnpr@HEAD",
"mean": 5.1066546625800004,
"stddev": 0.06188783824742778,
"median": 5.102007861380001,
"user": 2.2908288199999993,
"system": 2.7817852999999997,
"min": 5.03742895738,
"max": 5.23732691038,
"times": [
5.109522725380001,
5.1225827413800005,
5.07708649038,
5.040542701380001,
5.05205200738,
5.03742895738,
5.130600564380001,
5.16491053038,
5.094492997380001,
5.23732691038
]
},
{
"command": "pnpr@main",
"mean": 5.10060914468,
"stddev": 0.040465343815842096,
"median": 5.09221807138,
"user": 2.23425962,
"system": 2.8097554999999996,
"min": 5.04449443038,
"max": 5.17388379838,
"times": [
5.10355848838,
5.08087765438,
5.05846814638,
5.12367191638,
5.074491619380001,
5.12602470738,
5.07871230138,
5.04449443038,
5.17388379838,
5.141908384380001
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.3524236141800003,
"stddev": 0.02262628752942512,
"median": 1.3485359675800002,
"user": 1.49814392,
"system": 1.7480705399999998,
"min": 1.32034933058,
"max": 1.40450769158,
"times": [
1.3361763185800002,
1.3592444645800001,
1.34791572458,
1.34902233058,
1.33873871458,
1.40450769158,
1.32034933058,
1.3480496045800001,
1.3700881255800001,
1.35014383658
]
},
{
"command": "pacquet@main",
"mean": 1.36379516268,
"stddev": 0.029692102025681733,
"median": 1.3528696980800001,
"user": 1.50514372,
"system": 1.7448346399999999,
"min": 1.3389545145800001,
"max": 1.4284723215800001,
"times": [
1.3991732455800001,
1.4284723215800001,
1.3552620555800001,
1.38081778958,
1.35197158058,
1.3449246705800002,
1.3537678155800001,
1.34236280758,
1.3389545145800001,
1.3422448255800001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6554959817800001,
"stddev": 0.015084472170422228,
"median": 0.6519150045800001,
"user": 0.32198292,
"system": 1.26739514,
"min": 0.6369141465800001,
"max": 0.6884203845800001,
"times": [
0.6717441905800001,
0.6884203845800001,
0.6541078895800001,
0.6490685485800001,
0.64972211958,
0.6424818655800001,
0.64598304658,
0.6369141465800001,
0.6570628395800001,
0.6594547865800001
]
},
{
"command": "pnpr@main",
"mean": 0.6577964299800001,
"stddev": 0.018462329719777834,
"median": 0.6549334955800001,
"user": 0.33036602000000004,
"system": 1.2566042400000001,
"min": 0.6370240615800001,
"max": 0.7028316915800001,
"times": [
0.7028316915800001,
0.6492920765800001,
0.6666441005800001,
0.6553617355800001,
0.65856066958,
0.6447157925800001,
0.64339110858,
0.6656378075800001,
0.6545052555800001,
0.6370240615800001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.99634751698,
"stddev": 0.01859246387768725,
"median": 4.99965839408,
"user": 1.6796988000000002,
"system": 1.9367650599999997,
"min": 4.96134330708,
"max": 5.01975080208,
"times": [
4.99337286008,
5.00871326708,
4.99496559808,
5.00294673108,
5.01340801708,
4.99637005708,
5.00416303108,
4.96134330708,
4.96844149908,
5.01975080208
]
},
{
"command": "pacquet@main",
"mean": 4.9613617113799995,
"stddev": 0.019200088057126304,
"median": 4.96764494308,
"user": 1.6787161000000002,
"system": 1.9031656599999998,
"min": 4.92981033508,
"max": 4.98158496608,
"times": [
4.94546705708,
4.93450359708,
4.97595010808,
4.98066910908,
4.96636961208,
4.98158496608,
4.95415982708,
4.92981033508,
4.96892027408,
4.97618222808
]
},
{
"command": "pnpr@HEAD",
"mean": 0.66947007588,
"stddev": 0.01942013919890236,
"median": 0.66507403858,
"user": 0.32101759999999996,
"system": 1.2953178600000002,
"min": 0.64316620708,
"max": 0.70856818008,
"times": [
0.70856818008,
0.66634882208,
0.66141591308,
0.6787119150800001,
0.67635395208,
0.6503894180800001,
0.64316620708,
0.66379925508,
0.65645792608,
0.68948917008
]
},
{
"command": "pnpr@main",
"mean": 0.65728331168,
"stddev": 0.022280948939094884,
"median": 0.6528852995800001,
"user": 0.33490909999999996,
"system": 1.2430801599999999,
"min": 0.6363002840800001,
"max": 0.70112633908,
"times": [
0.69143064408,
0.65591359508,
0.6380942310800001,
0.65510484408,
0.64725379708,
0.6363002840800001,
0.63696276308,
0.70112633908,
0.65066575508,
0.65998086408
]
}
]
} |
|
| Branch | pr/12312 |
| 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 | 9,190.07 ms(+6.87%)Baseline: 8,599.00 ms | 10,318.80 ms (89.06%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 4,996.35 ms(-0.52%)Baseline: 5,022.30 ms | 6,026.76 ms (82.90%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,352.42 ms(-4.92%)Baseline: 1,422.37 ms | 1,706.85 ms (79.24%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 10,184.07 ms(+1.38%)Baseline: 10,045.52 ms | 12,054.63 ms (84.48%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 634.79 ms(-2.88%)Baseline: 653.59 ms | 784.31 ms (80.94%) |
|
| Branch | pr/12312 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 5,106.65 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 669.47 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 655.50 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 5,421.94 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 759.34 ms |
|
Code review by qodo was updated up to the latest commit 9227d34 |
|
Re CodeRabbit's suggestion to apply the name inference in Written by an agent (Claude Code, claude-fable-5). |
Micro-Benchmark ResultsLinux |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12312 +/- ##
==========================================
+ Coverage 87.70% 87.75% +0.05%
==========================================
Files 288 289 +1
Lines 35177 35312 +135
==========================================
+ Hits 30852 30989 +137
+ Misses 4325 4323 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Integrate the 9 commits main gained (#12271, #12294, #12301, #12303, #12305, #12312, #12315, #12316, and the release/version bumps). Conflict resolution: all four conflicts (record_lockfile_verified, build_modules, hoisted_dep_graph, install) were between this branch's lint edits and main's feature changes — took main's authoritative versions; lint compliance is re-derived by re-running clippy in the follow-up commit.
Problem
Closes #11702
Closes #9940
Some registries (and registry proxies) strip the
os/cpu/libcfields — or justlibc— from the version objects of the packuments they serve. pnpm's resolution then saw every platform-specific optional dependency (e.g. the@nx/nx-*,@esbuild/*binaries) as platform-unrestricted, with two consequences:supportedArchitecturesor the current platform.supportedArchitecturedon't works as expected #9940 — it hasenginesbut nocpu/oson any@esbuild/*entry). Such a lockfile then breaks installs for every machine that uses it, even ones talking to a healthy registry, because headless installs decide installability from the lockfile's platform fields.Reproduced end-to-end by putting a metadata-stripping proxy in front of registry.npmjs.org:
pnpm add -D nxwithsupportedArchitectures: {os: [linux], cpu: [x64], libc: [glibc]}downloaded and installed all 10@nx/nx-*platform binaries and wrote a lockfile with zerocpu/osfields.Fix
Platform-specific binary packages encode their platform in the package name (
@nx/nx-win32-arm64-msvc,@esbuild/linux-x64,turbo-windows-64,bun-linux-aarch64, …), sopackageIsInstallablenow fills the missing platform fields of an optional dependency from the name's tokens. Every install path — fresh resolution (package requester), headless install with the isolated linker (lockfileToDepGraph), and the hoisted linker (lockfileToHoistedDepGraph) — already decides installability through that one check before fetching, so foreign-platform binaries are skipped without downloading them at all, and existing lockfiles that lack the fields work too.Inference is deliberately conservative:
libcalone is inferred from the name (…-linux-x64-muslvs…-linux-x64-gnu), which also covers registries that strip onlylibc(the pnpm install multiple bindings of different libc #7362 / CodeArtifact shape).arminis-arm) never gets a cross-platform package skipped.Tests
config/package-is-installable: unit tests for the inference over real-world names (nx, esbuild, biome, turbo, bun, sharp, …) and for the precedence/gating rules, including the libc-only case.package-requester: a resolver wrapper that strips the platform fields; asserts the package is reported not installable and no fetch is started.deps-installere2e: a real metadata-stripping HTTP proxy in front of the registry mock; asserts only the matching binary is installed and the foreign ones are not even in the store.deps-installere2e: lockfile entries hand-stripped of platform fields + frozen install, for bothisolatedandhoistednode linkers.All of these fail without the fix. Verified end-to-end in Docker against a stripping proxy:
pnpm add -D nxfetches exactly one binary (@nx/nx-linux-x64-gnu), and a frozen install from a lockfile with all platform fields removed materializes only that binary — including the musl/gnu disambiguation, which comes from the name.pacquet
Ported in the same PR (
fix(package-is-installable): infer missing platform fields of optional deps from the package name):infer_platform_from_package_name+inferred_platforminpacquet-package-is-installable, applied insidepackage_is_installable(hoisted linker) and incompute_skipped_snapshots(isolated linker, with the check cache keyed by the snapshot'soptionalflag since the verdicts can differ). Theany_installability_constraintfast path now also considers optional snapshots whose names infer a platform their metadata row doesn't declare, so the inference is reachable on lockfiles without any declared constraint. Unit tests port the same table and gating cases;installabilitypass tests cover the skip, the libc-only disambiguation, the non-optional exemption, and the fast-path interaction.Written by an agent (Claude Code, claude-fable-5).
Summary by CodeRabbit
Bug Fixes
Tests
Chores