Skip to content

fix: infer missing platform fields of optional dependencies from the package name#12312

Merged
zkochan merged 4 commits into
mainfrom
fix/11702
Jun 10, 2026
Merged

fix: infer missing platform fields of optional dependencies from the package name#12312
zkochan merged 4 commits into
mainfrom
fix/11702

Conversation

@zkochan

@zkochan zkochan commented Jun 10, 2026

Copy link
Copy Markdown
Member

Problem

Closes #11702
Closes #9940

Some registries (and registry proxies) strip the os/cpu/libc fields — or just libc — 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:

  1. Every platform's binaries were downloaded and installed, regardless of supportedArchitectures or the current platform.
  2. The lockfile was written without the platform fields (this is visible in the lockfile committed to the reproduction repo of supportedArchitecture don't works as expected #9940 — it has engines but no cpu/os on 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 nx with supportedArchitectures: {os: [linux], cpu: [x64], libc: [glibc]} downloaded and installed all 10 @nx/nx-* platform binaries and wrote a lockfile with zero cpu/os fields.

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, …), so packageIsInstallable now 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:

  • It only runs for optional dependencies, and declared manifest/lockfile fields always take precedence — each field is filled only when missing. In particular, a missing libc alone is inferred from the name (…-linux-x64-musl vs …-linux-x64-gnu), which also covers registries that strip only libc (the pnpm install multiple bindings of different libc #7362 / CodeArtifact shape).
  • A package that declares no platform fields at all is treated as platform-specific only when an operating system token is recognized in its name, so a generic name segment (e.g. arm in is-arm) never gets a cross-platform package skipped.
  • Packages whose names carry no platform tokens (fsevents-shaped) simply keep today's behavior.

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-installer e2e: 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-installer e2e: lockfile entries hand-stripped of platform fields + frozen install, for both isolated and hoisted node linkers.

All of these fail without the fix. Verified end-to-end in Docker against a stripping proxy: pnpm add -D nx fetches 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_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 doesn't declare, so the inference is reachable on lockfiles without any declared constraint. Unit tests port the same table and gating cases; installability pass 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

    • Optional, platform-specific dependencies are now inferred from package names and correctly skipped when incompatible even if registry metadata omits platform fields, preventing unwanted downloads.
  • Tests

    • Added end-to-end and unit tests covering name-based platform inference, install skipping across link modes, registry/lockfile stripping scenarios, and related edge cases.
  • Chores

    • Spell-check dictionary updated and a patch-level changeset/version bump added.

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (6) 📘 Rule violations (0) 📎 Requirement gaps (1)

Context used

Grey Divider


Action required

1. Optional tarballs fetched before filtering 📎 Requirement gap ➹ Performance
Description
The new logic explicitly awaits fetching() to read the bundled manifest for optional dependencies
whose registry/lockfile metadata lacks os/cpu/libc, which can still download non-matching
platform binaries even when supportedArchitectures is configured. This violates the requirement to
avoid fetching/downloading platform-incompatible optional binaries.
Code

installing/package-requester/src/packageRequester.ts[R316-336]

+    wantedDependency.optional === true &&
+    manifest.os == null && manifest.cpu == null && manifest.libc == null
+  ) {
+    // Registry metadata is not guaranteed to carry os/cpu/libc — some
+    // registries strip them from packument version objects. For an optional
+    // dependency that looks platform-unrestricted, trust the manifest bundled
+    // in the fetched tarball instead: without this, platform-specific optional
+    // dependencies would be installed on every platform and the lockfile would
+    // be written without the platform fields, breaking installs everywhere.
+    // https://github.com/pnpm/pnpm/issues/11702
+    const { bundledManifest } = await fetchResult.fetching()
+    if (bundledManifest != null && (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null)) {
+      manifest = { ...manifest, os: bundledManifest.os, cpu: bundledManifest.cpu, libc: bundledManifest.libc }
+      recheckInstallability = true
+    }
}
-  // Check installability now that we have the manifest (for git/tarball packages without registry metadata)
-  if (isInstallable === undefined && manifest != null) {
+  // Check installability using the fetched manifest — either the registry
+  // metadata had none (git/tarball packages), or it was missing the
+  // platform fields that the bundled manifest declares.
+  if (recheckInstallability && manifest != null) {
  isInstallable = ctx.force === true || packageIsInstallable(id, manifest, {
Evidence
PR Compliance IDs 1 and 2 require that when supportedArchitectures is set, pnpm must not
download/fetch platform-incompatible optional binaries. The added code explicitly triggers
fetchRawManifest and then awaits fetching() to consult the bundled manifest for optional deps
missing platform fields, meaning the tarball may be downloaded before installability is determined
and the dep is skipped.

Respect supportedArchitectures during dependency installation (do not fetch non-matching platform binaries)
Filter optionalDependencies (e.g., esbuild) according to supportedArchitectures
installing/package-requester/src/packageRequester.ts[316-336]
deps/graph-builder/src/lockfileToDepGraph.ts[286-327]
installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[248-290]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New code paths fetch optional dependency tarballs (via `fetching()`) to discover missing `os`/`cpu`/`libc` fields, which can still download platform-incompatible binaries under `supportedArchitectures`.
## Issue Context
Compliance requires pnpm to not fetch/download optional, platform-specific binaries that do not match the configured `supportedArchitectures`.
## Fix Focus Areas
- installing/package-requester/src/packageRequester.ts[316-336]
- deps/graph-builder/src/lockfileToDepGraph.ts[286-327]
- installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[248-290]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Fast-path skips platform recheck 🐞 Bug ≡ Correctness
Description
In lockfileToDepGraph, the bundled-manifest platform verification only runs inside the fetch path,
so when a package hits the “present/unchanged” fast paths (or early-returns), an optional dep whose
lockfile entry lacks os/cpu/libc won’t be revalidated and can remain installed. This undermines the
intended healing behavior for existing broken lockfiles when node_modules is reused across installs.
Code

deps/graph-builder/src/lockfileToDepGraph.ts[R286-328]

+        // A lockfile entry of an optional dependency without any platform
+        // fields may have been written from registry metadata that strips
+        // os/cpu/libc, so the bundled manifest of the fetched package has to
+        // be consulted before the package is linked to node_modules.
+        // https://github.com/pnpm/pnpm/issues/11702
+        const verifyPlatformOfFetchedPkg = !opts.force &&
+          !isDirectoryDep &&
+          pkgSnapshot.optional === true &&
+          pkgSnapshot.os == null && pkgSnapshot.cpu == null && pkgSnapshot.libc == null
      try {
        fetchResponse = await opts.storeController.fetchPackage({
          allowBuild: opts.allowBuild,
          force: false,
+            fetchRawManifest: verifyPlatformOfFetchedPkg,
          lockfileDir: opts.lockfileDir,
          ignoreScripts: opts.ignoreScripts,
          pkg: { name: pkgName, version: pkgVersion, id: packageId, resolution },
          supportedArchitectures: opts.supportedArchitectures,
        })
+          if (verifyPlatformOfFetchedPkg && fetchResponse.fetching != null) {
+            const { bundledManifest } = await fetchResponse.fetching()
+            if (
+              bundledManifest != null &&
+              (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null) &&
+              packageIsInstallable(packageId, {
+                name: pkgName,
+                version: pkgVersion,
+                os: bundledManifest.os,
+                cpu: bundledManifest.cpu,
+                libc: bundledManifest.libc,
+              }, {
+                engineStrict: opts.engineStrict,
+                lockfileDir: opts.lockfileDir,
+                nodeVersion: opts.nodeVersion,
+                optional: true,
+                supportedArchitectures: opts.supportedArchitectures,
+              }) === false
+            ) {
+              delete locationByDepPath[depPath]
+              opts.skipped.add(depPath)
+              return
+            }
+          }
Evidence
The code sets fetchResponse = {} (and may return early) when the package is already
present/unchanged, which prevents entering the if (!fetchResponse) block where the new
bundled-manifest platform verification is implemented. Also, missing platform fields are treated as
unrestricted by installability checks, so without the bundled-manifest recheck these deps appear
installable.

deps/graph-builder/src/lockfileToDepGraph.ts[196-215]
deps/graph-builder/src/lockfileToDepGraph.ts[246-280]
deps/graph-builder/src/lockfileToDepGraph.ts[282-333]
config/package-is-installable/src/index.ts[68-93]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`lockfileToDepGraph` only performs the new bundled-manifest platform recheck when it calls `storeController.fetchPackage()`. If the package is considered already present/unchanged (fast-path sets `fetchResponse = {}`) or the function returns early, the recheck never happens, so optional deps with missing `os/cpu/libc` in the lockfile may stay installed even though their bundled manifest would mark them unsupported.
### Issue Context
This PR adds platform verification using the bundled manifest for optional deps when lockfile snapshot lacks `os/cpu/libc`. However, existing fast paths can prevent any fetch/read of the bundled manifest and thus skip the new verification.
### Fix Focus Areas
- deps/graph-builder/src/lockfileToDepGraph.ts[246-333]
### Implementation notes
- When `pkgSnapshot.optional === true` and `pkgSnapshot.os/cpu/libc` are all missing, bypass the “present/unchanged” early-return and `{}` fetchResponse fast paths so that the bundled manifest can be consulted.
- Prefer a cheap read:
- If the package is already in the store, calling `storeController.fetchPackage({ fetchRawManifest: true, ... })` should read from store without re-downloading.
- Alternatively, if the package directory is present, read `package.json` from that directory to obtain `os/cpu/libc` and run `packageIsInstallable`.
- If unsupported, ensure the depPath is added to `opts.skipped` and any precomputed location mapping is removed so pruning can clean it up.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Hoisted skipFetch bypasses check 🐞 Bug ≡ Correctness
Description
In lockfileToHoistedDepGraph, the new bundled-manifest platform verification is only executed when
skipFetch is false; if a package is already present in the expected hoisted location, it skips
fetching and therefore skips the platform revalidation. With a broken lockfile entry missing
os/cpu/libc, this can leave incompatible optional packages hoisted across subsequent installs.
Code

installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[R248-290]

+      // A lockfile entry of an optional dependency without any platform
+      // fields may have been written from registry metadata that strips
+      // os/cpu/libc, so the bundled manifest of the fetched package has to
+      // be consulted before the package is linked to node_modules.
+      // https://github.com/pnpm/pnpm/issues/11702
+      const verifyPlatformOfFetchedPkg = !opts.force &&
+        !isDirectoryDep &&
+        pkgSnapshot.optional === true &&
+        pkgSnapshot.os == null && pkgSnapshot.cpu == null && pkgSnapshot.libc == null
    try {
      fetchResponse = opts.storeController.fetchPackage({
        allowBuild: opts.allowBuild,
        force: false,
+          fetchRawManifest: verifyPlatformOfFetchedPkg,
        lockfileDir: opts.lockfileDir,
        ignoreScripts: opts.ignoreScripts,
        pkg: pkgResolution,
        supportedArchitectures: opts.supportedArchitectures,
      }) as unknown as ReturnType<FetchPackageToStoreFunction>
      if (fetchResponse instanceof Promise) fetchResponse = await fetchResponse
+        if (verifyPlatformOfFetchedPkg && fetchResponse.fetching != null) {
+          const { bundledManifest } = await fetchResponse.fetching()
+          if (
+            bundledManifest != null &&
+            (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null) &&
+            packageIsInstallable(packageId, {
+              name: pkgName,
+              version: pkgVersion,
+              os: bundledManifest.os,
+              cpu: bundledManifest.cpu,
+              libc: bundledManifest.libc,
+            }, {
+              engineStrict: opts.engineStrict,
+              lockfileDir: opts.lockfileDir,
+              nodeVersion: opts.nodeVersion,
+              optional: true,
+              supportedArchitectures: opts.supportedArchitectures,
+            }) === false
+          ) {
+            opts.skipped.add(depPath)
+            return
+          }
+        }
Evidence
skipFetch is computed and if true the code avoids the fetch branch entirely. The new
bundled-manifest platform recheck lives only in that fetch branch, so it never runs for reused
hoisted packages, while missing platform fields are treated as unrestricted.

installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[233-291]
config/package-is-installable/src/index.ts[68-93]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In `fetchDeps` (hoisted linker), `verifyPlatformOfFetchedPkg` is only applied in the `else` branch when `skipFetch` is false. When `skipFetch` is true (package already present), platform constraints are not rechecked, so optional deps whose lockfile snapshots lack `os/cpu/libc` can remain installed even if their bundled manifest is incompatible.
### Issue Context
This PR aims to make installs resilient to stripped `os/cpu/libc` in lockfiles by consulting the bundled manifest. The hoisted reuse fast path currently bypasses this consultation.
### Fix Focus Areas
- installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[233-295]
### Implementation notes
- For `pkgSnapshot.optional === true` and missing `os/cpu/libc`, perform verification even when `skipFetch` is true:
- Option A: read `package.json` from the existing hoisted directory and extract `os/cpu/libc`.
- Option B: call `storeController.fetchPackage({ fetchRawManifest: true, ... })` even on skipFetch (should be store-local if already present), then run `packageIsInstallable` using `bundledManifest`.
- If unsupported, add `depPath` to `opts.skipped` so later pruning/linking logic can remove it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

4. Misses '-64' CPU tokens 🐞 Bug ≡ Correctness ⭐ New
Description
inferPlatformFromPackageName does not treat a bare "64" token as x64, so packages whose names use
the “-<os>-64” convention (e.g. turbo-windows-64, esbuild-darwin-64) infer only an OS and remain
installable on any CPU for that OS when cpu metadata is missing. Since checkPlatform only validates
CPU when wantedPlatform.cpu is present, supportedArchitectures.cpu may not filter these optional
binaries on mismatched architectures.
Code

config/package-is-installable/src/inferPlatformFromPackageName.ts[R17-33]

+const CPU_BY_TOKEN = new Map([
+  ['arm', 'arm'],
+  ['armv6', 'arm'],
+  ['armv7', 'arm'],
+  ['arm64', 'arm64'],
+  ['aarch64', 'arm64'],
+  ['ia32', 'ia32'],
+  ['loong64', 'loong64'],
+  ['mips64el', 'mips64el'],
+  ['ppc64', 'ppc64'],
+  ['ppc64le', 'ppc64'],
+  ['riscv64', 'riscv64'],
+  ['s390x', 's390x'],
+  ['x64', 'x64'],
+  ['amd64', 'x64'],
+  ['wasm32', 'wasm32'],
+])
Evidence
The CPU token map has no entry for a bare 64, while tests include turbo-windows-64 and
esbuild-darwin-64 and therefore currently infer no CPU. checkPlatform() only checks CPU when
wantedPlatform.cpu is provided, so missing CPU inference bypasses CPU filtering when
registry/lockfile metadata is stripped.

config/package-is-installable/src/inferPlatformFromPackageName.ts[17-33]
config/package-is-installable/test/inferPlatformFromPackageName.ts[15-25]
config/package-is-installable/src/checkPlatform.ts[29-40]
pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs[22-35]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Some platform-specific optional binary packages encode x64 as a bare `64` token in their name (e.g. `turbo-windows-64`, `esbuild-darwin-64`). The current token map does not recognize `64`, so inference omits `cpu`, and CPU compatibility is never checked.

### Issue Context
- `checkPlatform()` only enforces CPU when `wantedPlatform.cpu` is set.
- The test suite already includes real-world `*-*-64` names, which currently infer OS only.

### Fix Focus Areas
- Add a conservative heuristic to infer `cpu: ['x64']` from a `64` token **only when** an OS token is also present (to avoid generic false positives), and when no other CPU token is present.
- Apply the same logic in both the TypeScript implementation and the pacquet Rust port.
- Extend/adjust unit tests to cover the new behavior.

- config/package-is-installable/src/inferPlatformFromPackageName.ts[17-33]
- config/package-is-installable/test/inferPlatformFromPackageName.ts[15-25]
- pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs[22-35]
- pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs[24-55]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


5. Server close not awaited 🐞 Bug ☼ Reliability
Description
The new optional-deps e2e test calls http.Server.close() in a finally block without awaiting
completion, which can leave open handles and cause Jest to warn or flake depending on timing. This
is inconsistent with other repo tests that explicitly await server shutdown.
Code

installing/deps-installer/test/install/optionalDependencies.ts[R152-167]

+  await new Promise<void>((resolve) => {
+    server.listen(0, resolve)
+  })
+  const registryProxy = `http://localhost:${(server.address() as AddressInfo).port}/`
+  try {
+    await install({
+      dependencies: {
+        '@pnpm.e2e/has-many-optional-deps': '1.0.0',
+      },
+    }, testDefaults({
+      registries: { default: registryProxy },
+      supportedArchitectures: { os: ['darwin'], cpu: ['arm64'] },
+    }))
+  } finally {
+    server.close()
+  }
Evidence
The new test does not await server closure, while other tests in-repo explicitly await
server.close() to avoid leaked handles.

installing/deps-installer/test/install/optionalDependencies.ts[148-167]
pnpm/test/install/pnpmRegistry.ts[54-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test `skip optional dependencies ... when the registry metadata has no platform fields` starts an `http.Server` and calls `server.close()` in `finally` without waiting for the close to complete. This may keep open handles briefly after the test finishes.
### Issue Context
`http.Server.close()` is asynchronous; other tests in this repo wrap `server.close()` in a Promise and `await` it.
### Fix Focus Areas
- installing/deps-installer/test/install/optionalDependencies.ts[148-167]
### Suggested change
In the `finally` block, replace `server.close()` with:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

6. Proxy drops request method 🐞 Bug ⚙ Maintainability
Description
createMetadataStrippingRegistryProxy() forwards all incoming requests upstream using fetch() without
preserving req.method, so HEAD/other methods are silently converted into GET. This makes the proxy
semantically incorrect vs real registry interactions and could break if the install path uses HEAD
requests (which pnpm does in other resolution flows).
Code

installing/deps-installer/test/install/optionalDependencies.ts[R186-204]

+  return http.createServer((req, res) => {
+    (async () => {
+      const upstream = await fetch(`http://localhost:${REGISTRY_MOCK_PORT}${req.url}`, {
+        headers: { accept: req.headers.accept ?? '*/*' },
+      })
+      const contentType = upstream.headers.get('content-type') ?? ''
+      if (contentType.includes('json')) {
+        const doc = await upstream.json() as { versions?: Record<string, Record<string, unknown>> }
+        for (const versionMeta of Object.values(doc.versions ?? {})) {
+          delete versionMeta.os
+          delete versionMeta.cpu
+          delete versionMeta.libc
+        }
+        res.writeHead(upstream.status, { 'content-type': 'application/json' })
+        res.end(JSON.stringify(doc))
+      } else {
+        res.writeHead(upstream.status, { 'content-type': contentType })
+        res.end(Buffer.from(await upstream.arrayBuffer()))
+      }
Evidence
The proxy currently issues an upstream fetch without preserving the inbound HTTP method. pnpm
codebase contains HEAD-based registry interactions, and other in-repo proxy code forwards `method:
req.method`, demonstrating the expected proxying behavior.

installing/deps-installer/test/install/optionalDependencies.ts[185-204]
resolving/tarball-resolver/src/index.ts[10-28]
pnpm/test/install/pnpmRegistry.ts[34-37]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test registry proxy forwards requests to the upstream registry mock using `fetch(...)` but does not include `method: req.method`. This means any HEAD request becomes a GET upstream.
### Issue Context
pnpm does use HEAD requests in some registry/tarball resolution flows, so the proxy should be correct and future-proof.
### Fix Focus Areas
- installing/deps-installer/test/install/optionalDependencies.ts[185-204]
### Suggested change
Pass through the incoming method (and optionally body for non-GET/HEAD) when calling `fetch`:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


7. Unnormalized Accept header 🐞 Bug ⚙ Maintainability
Description
The metadata-stripping proxy passes req.headers.accept directly into fetch() headers, but Node
types it as string | string[] | undefined and it may be an array at runtime. Normalizing to a
string avoids type-safety issues and prevents sending an unexpected header value.
Code

installing/deps-installer/test/install/optionalDependencies.ts[R182-184]

+      const upstream = await fetch(`http://localhost:${REGISTRY_MOCK_PORT}${req.url}`, {
+        headers: { accept: req.headers.accept ?? '*/*' },
+      })
Evidence
The proxy currently forwards req.headers.accept without handling the string[] case, which is a
valid shape for Node incoming headers and can also be rejected by stricter typing/tooling.

installing/deps-installer/test/install/optionalDependencies.ts[177-199]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`req.headers.accept` can be `string | string[] | undefined`, but the proxy forwards it into `fetch()` as if it were always a string.
### Issue Context
This is test-only code, but normalizing prevents TS friction and makes the proxy behavior explicit.
### Fix Focus Areas
- installing/deps-installer/test/install/optionalDependencies.ts[179-184]
### Implementation notes
Example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 9227d34

Results up to commit 23f55eb


🐞 Bugs (5) 📘 Rule violations (0) 📎 Requirement gaps (1)

Context used

Action required
1. Optional tarballs fetched before filtering 📎 Requirement gap ➹ Performance
Description
The new logic explicitly awaits fetching() to read the bundled manifest for optional dependencies
whose registry/lockfile metadata lacks os/cpu/libc, which can still download non-matching
platform binaries even when supportedArchitectures is configured. This violates the requirement to
avoid fetching/downloading platform-incompatible optional binaries.
Code

installing/package-requester/src/packageRequester.ts[R316-336]

+    wantedDependency.optional === true &&
+    manifest.os == null && manifest.cpu == null && manifest.libc == null
+  ) {
+    // Registry metadata is not guaranteed to carry os/cpu/libc — some
+    // registries strip them from packument version objects. For an optional
+    // dependency that looks platform-unrestricted, trust the manifest bundled
+    // in the fetched tarball instead: without this, platform-specific optional
+    // dependencies would be installed on every platform and the lockfile would
+    // be written without the platform fields, breaking installs everywhere.
+    // https://github.com/pnpm/pnpm/issues/11702
+    const { bundledManifest } = await fetchResult.fetching()
+    if (bundledManifest != null && (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null)) {
+      manifest = { ...manifest, os: bundledManifest.os, cpu: bundledManifest.cpu, libc: bundledManifest.libc }
+      recheckInstallability = true
+    }
 }
-  // Check installability now that we have the manifest (for git/tarball packages without registry metadata)
-  if (isInstallable === undefined && manifest != null) {
+  // Check installability using the fetched manifest — either the registry
+  // metadata had none (git/tarball packages), or it was missing the
+  // platform fields that the bundled manifest declares.
+  if (recheckInstallability && manifest != null) {
   isInstallable = ctx.force === true || packageIsInstallable(id, manifest, {
Evidence
PR Compliance IDs 1 and 2 require that when supportedArchitectures is set, pnpm must not
download/fetch platform-incompatible optional binaries. The added code explicitly triggers
fetchRawManifest and then awaits fetching() to consult the bundled manifest for optional deps
missing platform fields, meaning the tarball may be downloaded before installability is determined
and the dep is skipped.

Respect supportedArchitectures during dependency installation (do not fetch non-matching platform binaries)
Filter optionalDependencies (e.g., esbuild) according to supportedArchitectures
installing/package-requester/src/packageRequester.ts[316-336]
deps/graph-builder/src/lockfileToDepGraph.ts[286-327]
installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[248-290]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New code paths fetch optional dependency tarballs (via `fetching()`) to discover missing `os`/`cpu`/`libc` fields, which can still download platform-incompatible binaries under `supportedArchitectures`.
## Issue Context
Compliance requires pnpm to not fetch/download optional, platform-specific binaries that do not match the configured `supportedArchitectures`.
## Fix Focus Areas
- installing/package-requester/src/packageRequester.ts[316-336]
- deps/graph-builder/src/lockfileToDepGraph.ts[286-327]
- installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[248-290]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Fast-path skips platform recheck 🐞 Bug ≡ Correctness
Description
In lockfileToDepGraph, the bundled-manifest platform verification only runs inside the fetch path,
so when a package hits the “present/unchanged” fast paths (or early-returns), an optional dep whose
lockfile entry lacks os/cpu/libc won’t be revalidated and can remain installed. This undermines the
intended healing behavior for existing broken lockfiles when node_modules is reused across installs.
Code

deps/graph-builder/src/lockfileToDepGraph.ts[R286-328]

+        // A lockfile entry of an optional dependency without any platform
+        // fields may have been written from registry metadata that strips
+        // os/cpu/libc, so the bundled manifest of the fetched package has to
+        // be consulted before the package is linked to node_modules.
+        // https://github.com/pnpm/pnpm/issues/11702
+        const verifyPlatformOfFetchedPkg = !opts.force &&
+          !isDirectoryDep &&
+          pkgSnapshot.optional === true &&
+          pkgSnapshot.os == null && pkgSnapshot.cpu == null && pkgSnapshot.libc == null
       try {
         fetchResponse = await opts.storeController.fetchPackage({
           allowBuild: opts.allowBuild,
           force: false,
+            fetchRawManifest: verifyPlatformOfFetchedPkg,
           lockfileDir: opts.lockfileDir,
           ignoreScripts: opts.ignoreScripts,
           pkg: { name: pkgName, version: pkgVersion, id: packageId, resolution },
           supportedArchitectures: opts.supportedArchitectures,
         })
+          if (verifyPlatformOfFetchedPkg && fetchResponse.fetching != null) {
+            const { bundledManifest } = await fetchResponse.fetching()
+            if (
+              bundledManifest != null &&
+              (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null) &&
+              packageIsInstallable(packageId, {
+                name: pkgName,
+                version: pkgVersion,
+                os: bundledManifest.os,
+                cpu: bundledManifest.cpu,
+                libc: bundledManifest.libc,
+              }, {
+                engineStrict: opts.engineStrict,
+                lockfileDir: opts.lockfileDir,
+                nodeVersion: opts.nodeVersion,
+                optional: true,
+                supportedArchitectures: opts.supportedArchitectures,
+              }) === false
+            ) {
+              delete locationByDepPath[depPath]
+              opts.skipped.add(depPath)
+              return
+            }
+          }
Evidence
The code sets fetchResponse = {} (and may return early) when the package is already
present/unchanged, which prevents entering the if (!fetchResponse) block where the new
bundled-manifest platform verification is implemented. Also, missing platform fields are treated as
unrestricted by installability checks, so without the bundled-manifest recheck these deps appear
installable.

deps/graph-builder/src/lockfileToDepGraph.ts[196-215]
deps/graph-builder/src/lockfileToDepGraph.ts[246-280]
deps/graph-builder/src/lockfileToDepGraph.ts[282-333]
config/package-is-installable/src/index.ts[68-93]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`lockfileToDepGraph` only performs the new bundled-manifest platform recheck when it calls `storeController.fetchPackage()`. If the package is considered already present/unchanged (fast-path sets `fetchResponse = {}`) or the function returns early, the recheck never happens, so optional deps with missing `os/cpu/libc` in the lockfile may stay installed even though their bundled manifest would mark them unsupported.
### Issue Context
This PR adds platform verification using the bundled manifest for optional deps when lockfile snapshot lacks `os/cpu/libc`. However, existing fast paths can prevent any fetch/read of the bundled manifest and thus skip the new verification.
### Fix Focus Areas
- deps/graph-builder/src/lockfileToDepGraph.ts[246-333]
### Implementation notes
- When `pkgSnapshot.optional === true` and `pkgSnapshot.os/cpu/libc` are all missing, bypass the “present/unchanged” early-return and `{}` fetchResponse fast paths so that the bundled manifest can be consulted.
- Prefer a cheap read:
- If the package is already in the store, calling `storeController.fetchPackage({ fetchRawManifest: true, ... })` should read from store without re-downloading.
- Alternatively, if the package directory is present, read `package.json` from that directory to obtain `os/cpu/libc` and run `packageIsInstallable`.
- If unsupported, ensure the depPath is added to `opts.skipped` and any precomputed location mapping is removed so pruning can clean it up.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Hoisted skipFetch bypasses check 🐞 Bug ≡ Correctness
Description
In lockfileToHoistedDepGraph, the new bundled-manifest platform verification is only executed when
skipFetch is false; if a package is already present in the expected hoisted location, it skips
fetching and therefore skips the platform revalidation. With a broken lockfile entry missing
os/cpu/libc, this can leave incompatible optional packages hoisted across subsequent installs.
Code

installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[R248-290]

+      // A lockfile entry of an optional dependency without any platform
+      // fields may have been written from registry metadata that strips
+      // os/cpu/libc, so the bundled manifest of the fetched package has to
+      // be consulted before the package is linked to node_modules.
+      // https://github.com/pnpm/pnpm/issues/11702
+      const verifyPlatformOfFetchedPkg = !opts.force &&
+        !isDirectoryDep &&
+        pkgSnapshot.optional === true &&
+        pkgSnapshot.os == null && pkgSnapshot.cpu == null && pkgSnapshot.libc == null
     try {
       fetchResponse = opts.storeController.fetchPackage({
         allowBuild: opts.allowBuild,
         force: false,
+          fetchRawManifest: verifyPlatformOfFetchedPkg,
         lockfileDir: opts.lockfileDir,
         ignoreScripts: opts.ignoreScripts,
         pkg: pkgResolution,
         supportedArchitectures: opts.supportedArchitectures,
       }) as unknown as ReturnType<FetchPackageToStoreFunction>
       if (fetchResponse instanceof Promise) fetchResponse = await fetchResponse
+        if (verifyPlatformOfFetchedPkg && fetchResponse.fetching != null) {
+          const { bundledManifest } = await fetchResponse.fetching()
+          if (
+            bundledManifest != null &&
+            (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null) &&
+            packageIsInstallable(packageId, {
+              name: pkgName,
+              version: pkgVersion,
+              os: bundledManifest.os,
+              cpu: bundledManifest.cpu,
+              libc: bundledManifest.libc,
+            }, {
+              engineStrict: opts.engineStrict,
+              lockfileDir: opts.lockfileDir,
+              nodeVersion: opts.nodeVersion,
+              optional: true,
+              supportedArchitectures: opts.supportedArchitectures,
+            }) === false
+          ) {
+            opts.skipped.add(depPath)
+            return
+          }
+        }
Evidence
skipFetch is computed and if true the code avoids the fetch branch entirely. The new
bundled-manifest platform recheck lives only in that fetch branch, so it never runs for reused
hoisted packages, while missing platform fields are treated as unrestricted.

installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[233-291]
config/package-is-installable/src/index.ts[68-93]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
In `fetchDeps` (hoisted linker), `verifyPlatformOfFetchedPkg` is only applied in the `else` branch when `skipFetch` is false. When `skipFetch` is true (package already present), platform constraints are not rechecked, so optional deps whose lockfile snapshots lack `os/cpu/libc` can remain installed even if their bundled manifest is incompatible.
### Issue Context
This PR aims to make installs resilient to stripped `os/cpu/libc` in lockfiles by consulting the bundled manifest. The hoisted reuse fast path currently bypasses this consultation.
### Fix Focus Areas
- installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[233-295]
### Implementation notes
- For `pkgSnapshot.optional === true` and missing `os/cpu/libc`, perform verification even when `skipFetch` is true:
- Option A: read `package.json` from the existing hoisted directory and extract `os/cpu/libc`.
- Option B: call `storeController.fetchPackage({ fetchRawManifest: true, ... })` even on skipFetch (should be store-local if already present), then run `packageIsInstallable` using `bundledManifest`.
- If unsupported, add `depPath` to `opts.skipped` so later pruning/linking logic can remove it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended
4. Server close not awaited 🐞 Bug ☼ Reliability ⭐ New
Description
The new optional-deps e2e test calls http.Server.close() in a finally block without awaiting
completion, which can leave open handles and cause Jest to warn or flake depending on timing. This
is inconsistent with other repo tests that explicitly await server shutdown.
Code

installing/deps-installer/test/install/optionalDependencies.ts[R152-167]

+  await new Promise<void>((resolve) => {
+    server.listen(0, resolve)
+  })
+  const registryProxy = `http://localhost:${(server.address() as AddressInfo).port}/`
+  try {
+    await install({
+      dependencies: {
+        '@pnpm.e2e/has-many-optional-deps': '1.0.0',
+      },
+    }, testDefaults({
+      registries: { default: registryProxy },
+      supportedArchitectures: { os: ['darwin'], cpu: ['arm64'] },
+    }))
+  } finally {
+    server.close()
+  }
Evidence
The new test does not await server closure, while other tests in-repo explicitly await
server.close() to avoid leaked handles.

installing/deps-installer/test/install/optionalDependencies.ts[148-167]
pnpm/test/install/pnpmRegistry.ts[54-56]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test `skip optional dependencies ... when the registry metadata has no platform fields` starts an `http.Server` and calls `server.close()` in `finally` without waiting for the close to complete. This may keep open handles briefly after the test finishes.

### Issue Context
`http.Server.close()` is asynchronous; other tests in this repo wrap `server.close()` in a Promise and `await` it.

### Fix Focus Areas
- installing/deps-installer/test/install/optionalDependencies.ts[148-167]

### Suggested change
In the `finally` block, replace `server.close()` with:
```ts
await new Promise<void>((resolve, reject) => server.close((err) => err ? reject(err) : resolve()))
```
(Or an equivalent helper.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
5. Proxy drops request method 🐞 Bug ⚙ Maintainability ⭐ New
Description
createMetadataStrippingRegistryProxy() forwards all incoming requests upstream using fetch() without
preserving req.method, so HEAD/other methods are silently converted into GET. This makes the proxy
semantically incorrect vs real registry interactions and could break if the install path uses HEAD
requests (which pnpm does in other resolution flows).
Code

installing/deps-installer/test/install/optionalDependencies.ts[R186-204]

+  return http.createServer((req, res) => {
+    (async () => {
+      const upstream = await fetch(`http://localhost:${REGISTRY_MOCK_PORT}${req.url}`, {
+        headers: { accept: req.headers.accept ?? '*/*' },
+      })
+      const contentType = upstream.headers.get('content-type') ?? ''
+      if (contentType.includes('json')) {
+        const doc = await upstream.json() as { versions?: Record<string, Record<string, unknown>> }
+        for (const versionMeta of Object.values(doc.versions ?? {})) {
+          delete versionMeta.os
+          delete versionMeta.cpu
+          delete versionMeta.libc
+        }
+        res.writeHead(upstream.status, { 'content-type': 'application/json' })
+        res.end(JSON.stringify(doc))
+      } else {
+        res.writeHead(upstream.status, { 'content-type': contentType })
+        res.end(Buffer.from(await upstream.arrayBuffer()))
+      }
Evidence
The proxy currently issues an upstream fetch without preserving the inbound HTTP method. pnpm
codebase contains HEAD-based registry interactions, and other in-repo proxy code forwards `method:
req.method`, demonstrating the expected proxying behavior.

installing/deps-installer/test/install/optionalDependencies.ts[185-204]
resolving/tarball-resolver/src/index.ts[10-28]
pnpm/test/install/pnpmRegistry.ts[34-37]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test registry proxy forwards requests to the upstream registry mock using `fetch(...)` but does not include `method: req.method`. This means any HEAD request becomes a GET upstream.

### Issue Context
pnpm does use HEAD requests in some registry/tarball resolution flows, so the proxy should be correct and future-proof.

### Fix Focus Areas
- installing/deps-installer/test/install/optionalDependencies.ts[185-204]

### Suggested change
Pass through the incoming method (and optionally body for non-GET/HEAD) when calling `fetch`:
```ts
const upstream = await fetch(url, {
 method: req.method,
 headers: { /* existing headers */ },
})
```
Optionally, if `req.method === 'HEAD'`, avoid reading/forwarding a body.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


6. Unnormalized Accept header 🐞 Bug ⚙ Maintainability
Description
The metadata-stripping proxy passes req.headers.accept directly into fetch() headers, but Node
types it as string | string[] | undefined and it may be an array at runtime. Normalizing to a
string avoids type-safety issues and prevents sending an unexpected header value.
Code

installing/deps-installer/test/install/optionalDependencies.ts[R182-184]

+      const upstream = await fetch(`http://localhost:${REGISTRY_MOCK_PORT}${req.url}`, {
+        headers: { accept: req.headers.accept ?? '*/*' },
+      })
Evidence
The proxy currently forwards req.headers.accept without handling the string[] case, which is a
valid shape for Node incoming headers and can also be rejected by stricter typing/tooling.

installing/deps-installer/test/install/optionalDependencies.ts[177-199]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`req.headers.accept` can be `string | string[] | undefined`, but the proxy forwards it into `fetch()` as if it were always a string.
### Issue Context
This is test-only code, but normalizing prevents TS friction and makes the proxy behavior explicit.
### Fix Focus Areas
- installing/deps-installer/test/install/optionalDependencies.ts[179-184]
### Implementation notes
Example:

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 94b904d


🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (1)

Context used

Action required
1. Optional tarballs fetched before filtering 📎 Requirement gap ➹ Performance
Description
The new logic explicitly awaits fetching() to read the bundled manifest for optional dependencies
whose registry/lockfile metadata lacks os/cpu/libc, which can still download non-matching
platform binaries even when supportedArchitectures is configured. This violates the requirement to
avoid fetching/downloading platform-incompatible optional binaries.
Code

installing/package-requester/src/packageRequester.ts[R316-336]

+    wantedDependency.optional === true &&
+    manifest.os == null && manifest.cpu == null && manifest.libc == null
+  ) {
+    // Registry metadata is not guaranteed to carry os/cpu/libc — some
+    // registries strip them from packument version objects. For an optional
+    // dependency that looks platform-unrestricted, trust the manifest bundled
+    // in the fetched tarball instead: without this, platform-specific optional
+    // dependencies would be installed on every platform and the lockfile would
+    // be written without the platform fields, breaking installs everywhere.
+    // https://github.com/pnpm/pnpm/issues/11702
+    const { bundledManifest } = await fetchResult.fetching()
+    if (bundledManifest != null && (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null)) {
+      manifest = { ...manifest, os: bundledManifest.os, cpu: bundledManifest.cpu, libc: bundledManifest.libc }
+      recheckInstallability = true
+    }
  }
-  // Check installability now that we have the manifest (for git/tarball packages without registry metadata)
-  if (isInstallable === undefined && manifest != null) {
+  // Check installability using the fetched manifest — either the registry
+  // metadata had none (git/tarball packages), or it was missing the
+  // platform fields that the bundled manifest declares.
+  if (recheckInstallability && manifest != null) {
    isInstallable = ctx.force === true || packageIsInstallable(id, manifest, {
Evidence
PR Compliance IDs 1 and 2 require that when supportedArchitectures is set, pnpm must not
download/fetch platform-incompatible optional binaries. The added code explicitly triggers
fetchRawManifest and then awaits fetching() to consult the bundled manifest for optional deps
missing platform fields, meaning the tarball may be downloaded before installability is determined
and the dep is skipped.

Respect supportedArchitectures during dependency installation (do not fetch non-matching platform binaries)
Filter optionalDependencies (e.g., esbuild) according to supportedArchitectures
installing/package-requester/src/packageRequester.ts[316-336]
deps/graph-builder/src/lockfileToDepGraph.ts[286-327]
installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[248-290]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New code paths fetch optional dependency tarballs (via `fetching()`) to discover missing `os`/`cpu`/`libc` fields, which can still download platform-incompatible binaries under `supportedArchitectures`.

## Issue Context
Compliance requires pnpm to not fetch/download optional, platform-specific binaries that do not match the configured `supportedArchitectures`.

## Fix Focus Areas
- installing/package-requester/src/packageRequester.ts[316-336]
- deps/graph-builder/src/lockfileToDepGraph.ts[286-327]
- installing/deps-restorer/src/lockfileToHoistedDepGraph.ts[248-290]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Fast-path skips platform recheck 🐞 Bug ≡ Correctness
Description
In lockfileToDepGraph, the bundled-manifest platform verification only runs inside the fetch path,
so when a package hits the “present/unchanged” fast paths (or early-returns), an optional dep whose
lockfile entry lacks os/cpu/libc won’t be revalidated and can remain installed. This undermines the
intended healing behavior for existing broken lockfiles when node_modules is reused across installs.
Code

deps/graph-builder/src/lockfileToDepGraph.ts[R286-328]

+        // A lockfile entry of an optional dependency without any platform
+        // fields may have been written from registry metadata that strips
+        // os/cpu/libc, so the bundled manifest of the fetched package has to
+        // be consulted before the package is linked to node_modules.
+        // https://github.com/pnpm/pnpm/issues/11702
+        const verifyPlatformOfFetchedPkg = !opts.force &&
+          !isDirectoryDep &&
+          pkgSnapshot.optional === true &&
+          pkgSnapshot.os == null && pkgSnapshot.cpu == null && pkgSnapshot.libc == null
        try {
          fetchResponse = await opts.storeController.fetchPackage({
            allowBuild: opts.allowBuild,
            force: false,
+            fetchRawManifest: verifyPlatformOfFetchedPkg,
            lockfileDir: opts.lockfileDir,
            ignoreScripts: opts.ignoreScripts,
            pkg: { name: pkgName, version: pkgVersion, id: packageId, resolution },
            supportedArchitectures: opts.supportedArchitectures,
          })
+          if (verifyPlatformOfFetchedPkg && fetchResponse.fetching != null) {
+            const { bundledManifest } = await fetchResponse.fetching()
+            if (
+              bundledManifest != null &&
+              (bundledManifest.os != null || bundledManifest.cpu != null || bundledManifest.libc != null) &&
+              packageIsInstallable(packageId, {
+                name: pkgName,
+                version: pkgVersion,
+                os: bundledManifest.os,
+                cpu: bundledManifest...

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

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: 2fff251f-e1bc-4556-a698-c021f52db0e4

📥 Commits

Reviewing files that changed from the base of the PR and between fe44450 and 9227d34.

📒 Files selected for processing (1)
  • installing/deps-installer/test/install/optionalDependencies.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • installing/deps-installer/test/install/optionalDependencies.ts
📜 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)
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Compile & Lint
  • GitHub Check: Dylint
  • GitHub Check: Run benchmark on ubuntu-latest
  • GitHub Check: Lint and Test (macos-latest)
  • GitHub Check: Code Coverage
  • GitHub Check: Lint and Test (ubuntu-latest)
  • GitHub Check: Lint and Test (windows-latest)

📝 Walkthrough

Walkthrough

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

Changes

Platform Field Inference for Optional Dependencies

Layer / File(s) Summary
Release Notes and Version Bumps
.changeset/spicy-pots-wonder.md
Patch version bumps for pnpm and @pnpm/config.package-is-installable and documents the behavioral change for optional dependency platform handling.
Platform Inference from Package Names (JS)
config/package-is-installable/src/inferPlatformFromPackageName.ts
Core inferPlatformFromPackageName maps package-name tokens to canonical os/cpu/libc arrays and returns only recognized fields or null.
packageIsInstallable Integration (JS)
config/package-is-installable/src/index.ts
Re-exports inference helper and adds effectivePlatform to enrich optional dependency manifests with inferred platform fields when metadata is incomplete before calling checkPackage.
JS unit and integration tests
config/package-is-installable/test/*, installing/package-requester/test/index.ts, installing/deps-installer/test/install/optionalDependencies.ts
Parametrized unit tests for token mappings and precedence; a requester test that strips platform fields at resolve-time; installer tests that proxy/strip packuments and mutate lockfiles to assert skipped optional deps across nodeLinker modes.
Spell Check Config
cspell.json
Adds platform-related tokens (gnueabihf, loong, musleabihf, openharmony, riscv) to the spell-check allowlist.
Rust inference and wiring
pacquet/crates/package-is-installable/*, pacquet/crates/package-manager/*, pacquet/crates/env-installer/src/install_config_deps.rs
Adds infer_platform_from_package_name/inferred_platform, re-exports them, extends manifests with name, integrates inference into per-snapshot checks and any_installability_constraint, updates caches and gating, and adds tests.

Sequence Diagram

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

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

  • pnpm/pnpm#11725: Updates config/package-is-installable for optional dependency platform inference and is directly related to these changes.

"🐰 I hop through names and tokens clear,
I sniff for win, linux, arm, or mear;
If your bin won't run on this machine,
I skip the fetch and keep things clean —
No extra weight, just what belongs here."

🚥 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 directly and specifically describes the core change: inferring missing platform fields for optional dependencies from package names, which is the primary purpose of this entire changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/11702

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Skip platform-specific optional deps when metadata/lockfile lacks os/cpu/libc
🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

Walkthroughs

Description
• 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.
Diagram
graph 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
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Always fetch bundled manifest for all optional deps
  • ➕ Simpler rules; eliminates reliance on registry metadata correctness entirely.
  • ➕ Consistent behavior across registries/proxies.
  • ➖ More network/IO: forces tarball fetch even when metadata already indicates incompatibility.
  • ➖ Increases latency for common cases (platform-specific binaries on unsupported platforms).
2. Hard-fail when optional dep lacks platform fields
  • ➕ Avoids downloading incompatible tarballs when metadata is incomplete.
  • ➕ Makes registry/proxy misbehavior immediately visible.
  • ➖ Breaks installs that currently succeed (optional deps should not be fatal).
  • ➖ Does not heal existing lockfiles; worsens UX for consumers of committed lockfiles.

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.

Grey Divider

File Changes

Bug fix (3)
lockfileToDepGraph.ts Verify platform for optional deps when lockfile snapshot lacks os/cpu/libc +34/-0

Verify platform for optional deps when lockfile snapshot lacks os/cpu/libc

• During dep graph construction, detects optional lockfile entries missing all platform fields and requests the raw manifest during store fetch. If the bundled manifest declares platform constraints and the package is not installable, the dep path is removed from locations and added to the skipped set before linking.

deps/graph-builder/src/lockfileToDepGraph.ts


lockfileToHoistedDepGraph.ts Re-check optional dep installability from fetched manifest in hoisted restorer +33/-0

Re-check optional dep installability from fetched manifest in hoisted restorer

• For hoisted headless installs, detects optional snapshots missing platform fields and fetches the raw bundled manifest before linking. Uses packageIsInstallable to skip incompatible optionals and records them in the skipped set, preventing incorrect node_modules materialization from broken lockfiles.

installing/deps-restorer/src/lockfileToHoistedDepGraph.ts


packageRequester.ts Merge bundled manifest platform fields and re-check optional installability +21/-2

Merge bundled manifest platform fields and re-check optional installability

• When an optional dependency’s resolved manifest lacks os/cpu/libc, awaits the tarball fetch and merges platform fields from the bundled manifest into the returned manifest. Re-runs packageIsInstallable using the fetched/merged manifest so incompatible optionals are marked not installable and the lockfile receives the correct platform fields.

installing/package-requester/src/packageRequester.ts


Tests (2)
optionalDependencies.ts Add e2e coverage for metadata-stripping registries and broken lockfiles +128/-0

Add e2e coverage for metadata-stripping registries and broken lockfiles

• Adds an HTTP proxy that strips os/cpu/libc from registry JSON responses and verifies incompatible optional deps are skipped while the lockfile gains the missing platform fields. Adds frozen-install tests (isolated + hoisted) where lockfile snapshots are manually stripped, ensuring only matching-platform optionals are materialized and others are recorded as skipped.

installing/deps-installer/test/install/optionalDependencies.ts


index.ts Unit test: optional installability determined from fetched manifest when metadata stripped +39/-0

Unit test: optional installability determined from fetched manifest when metadata stripped

• Adds a test wrapper around the resolver that deletes os/cpu/libc from the resolved manifest. Asserts the package requester marks the optional dependency as not installable and returns platform fields sourced from the fetched tarball manifest so they can be written into the lockfile.

installing/package-requester/test/index.ts


Documentation (1)
spicy-pots-wonder.md Add changeset documenting optional-dep platform-field healing +8/-0

Add changeset documenting optional-dep platform-field healing

• Adds a patch changeset for the affected packages and pnpm. Documents the behavior change: use tarball manifest to skip incompatible optional deps and write os/cpu/libc into the lockfile even when registry metadata is stripped.

.changeset/spicy-pots-wonder.md


Grey Divider

Qodo Logo

Comment thread installing/package-requester/src/packageRequester.ts Outdated
Comment thread deps/graph-builder/src/lockfileToDepGraph.ts Outdated
Comment thread installing/deps-restorer/src/lockfileToHoistedDepGraph.ts Outdated
zkochan added 2 commits June 10, 2026 16:36
…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
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 23f55eb

@zkochan zkochan changed the title fix: skip platform-specific optional deps when metadata lacks platform fields fix: infer missing platform fields of optional dependencies from the package name Jun 10, 2026
…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

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

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 win

Optional subdeps with stripped platform fields still bypass name-based platform inference.

Line 289 exits before installability evaluation when os/cpu/libc are missing, so the stripped-metadata case never reaches inference. Adding manifest.name on 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

📥 Commits

Reviewing files that changed from the base of the PR and between 23f55eb and fe44450.

📒 Files selected for processing (11)
  • pacquet/crates/env-installer/src/install_config_deps.rs
  • pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs
  • pacquet/crates/package-is-installable/src/lib.rs
  • pacquet/crates/package-is-installable/src/package_is_installable.rs
  • pacquet/crates/package-is-installable/src/tests.rs
  • pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/installability.rs
  • pacquet/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 fires pnpm:<channel> events through globalLogger, logger.debug(...), or streamParser.write(...), mirror the call site, payload, and ordering so @pnpm/cli.default-reporter parses pacquet's NDJSON the same way
Declare a newtype wrapper for branded string types instead of collapsing the brand into a plain String or &str in Rust
If upstream TypeScript always validates before construction of a branded string, validate in the Rust wrapper too via TryFrom<String> and/or FromStr and 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 infallible From<String> constructor
If upstream TypeScript occasionally constructs a branded string without validation, expose from_str_unchecked in 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 handwriting impl blocks; use manual impl only when conversion needs custom logic
Model TypeScript string literal unions (like 'auto' | 'always' | 'never') as Rust enums 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 in CODE_STYLE_GUIDE.md — imports, modules, naming, ownership and borrowing, parameter type selection, trait bounds, pattern matching, pipe-trait, error handling, test layout, and cloning of Arc and Rc
Choose owned vs. borrowed parameters to minimize copies; widen to t...

Files:

  • pacquet/crates/env-installer/src/install_config_deps.rs
  • pacquet/crates/package-is-installable/src/tests.rs
  • pacquet/crates/package-is-installable/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-is-installable/src/package_is_installable.rs
  • pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
  • pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs
  • pacquet/crates/package-manager/src/installability.rs
  • pacquet/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 — use tempfile::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 scalar assert_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 with insta and carefully review diffs when intentional changes alter snapshots; accept with cargo insta review only after careful review
Tests that need the mocked registry should start pnpr through pacquet-testing-utils; cargo test / cargo nextest run should not require a separate just registry-mock launch step

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.rs
  • pacquet/crates/package-is-installable/src/tests.rs
  • pacquet/crates/package-is-installable/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-is-installable/src/package_is_installable.rs
  • pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
  • pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs
  • pacquet/crates/package-manager/src/installability.rs
  • pacquet/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.rs
  • pacquet/crates/package-is-installable/src/tests.rs
  • pacquet/crates/package-is-installable/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-is-installable/src/package_is_installable.rs
  • pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
  • pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs
  • pacquet/crates/package-manager/src/installability.rs
  • pacquet/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.rs
  • pacquet/crates/package-is-installable/src/tests.rs
  • pacquet/crates/package-is-installable/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-is-installable/src/package_is_installable.rs
  • pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
  • pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs
  • pacquet/crates/package-manager/src/installability.rs
  • pacquet/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.rs
  • pacquet/crates/package-is-installable/src/tests.rs
  • pacquet/crates/package-is-installable/src/lib.rs
  • pacquet/crates/package-manager/src/install_with_fresh_lockfile.rs
  • pacquet/crates/package-manager/src/hoisted_dep_graph.rs
  • pacquet/crates/package-manager/src/install_frozen_lockfile.rs
  • pacquet/crates/package-is-installable/src/package_is_installable.rs
  • pacquet/crates/package-is-installable/src/tests/infer_platform_from_package_name.rs
  • pacquet/crates/package-is-installable/src/infer_platform_from_package_name.rs
  • pacquet/crates/package-manager/src/installability.rs
  • pacquet/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

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 10.184 ± 0.105 10.088 10.389 1.88 ± 0.06
pacquet@main 10.201 ± 0.152 10.106 10.607 1.88 ± 0.06
pnpr@HEAD 5.422 ± 0.150 5.321 5.720 1.00
pnpr@main 5.432 ± 0.157 5.323 5.840 1.00 ± 0.04
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

Command Mean [ms] Min [ms] Max [ms] Relative
pacquet@HEAD 634.8 ± 8.9 626.2 650.6 1.00
pacquet@main 653.5 ± 34.3 628.2 738.7 1.03 ± 0.06
pnpr@HEAD 759.3 ± 70.3 716.0 952.7 1.20 ± 0.11
pnpr@main 771.7 ± 79.3 722.7 990.2 1.22 ± 0.13
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 9.190 ± 0.044 9.101 9.245 1.80 ± 0.02
pacquet@main 9.175 ± 0.055 9.107 9.292 1.80 ± 0.02
pnpr@HEAD 5.107 ± 0.062 5.037 5.237 1.00 ± 0.01
pnpr@main 5.101 ± 0.040 5.044 5.174 1.00
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 1.352 ± 0.023 1.320 1.405 2.06 ± 0.06
pacquet@main 1.364 ± 0.030 1.339 1.428 2.08 ± 0.07
pnpr@HEAD 0.655 ± 0.015 0.637 0.688 1.00
pnpr@main 0.658 ± 0.018 0.637 0.703 1.00 ± 0.04
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

Command Mean [s] Min [s] Max [s] Relative
pacquet@HEAD 4.996 ± 0.019 4.961 5.020 7.60 ± 0.26
pacquet@main 4.961 ± 0.019 4.930 4.982 7.55 ± 0.26
pnpr@HEAD 0.669 ± 0.019 0.643 0.709 1.02 ± 0.05
pnpr@main 0.657 ± 0.022 0.636 0.701 1.00
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
      ]
    }
  ]
}

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12312
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
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%)
🐰 View full continuous benchmarking report in Bencher

@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

🐰 Bencher Report

Branchpr/12312
Testbedpnpr

⚠️ 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-thresholds flag.

Click to view all benchmark results
BenchmarkLatencymilliseconds (ms)
isolated-linker.fresh-install.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
5,106.65 ms
isolated-linker.fresh-install.cold-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
669.47 ms
isolated-linker.fresh-install.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
655.50 ms
isolated-linker.fresh-restore.cold-cache.cold-store📈 view plot
⚠️ NO THRESHOLD
5,421.94 ms
isolated-linker.fresh-restore.hot-cache.hot-store📈 view plot
⚠️ NO THRESHOLD
759.34 ms
🐰 View full continuous benchmarking report in Bencher

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Jun 10, 2026

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 9227d34

@zkochan

zkochan commented Jun 10, 2026

Copy link
Copy Markdown
Member Author

Re CodeRabbit's suggestion to apply the name inference in pacquet/crates/env-installer/src/install_config_deps.rs: that gap is intentional. The config-dependencies path deliberately uses check_package (no inference), mirroring pnpm's installConfigDeps, which calls checkPackage rather than packageIsInstallable — the env lockfile records all platform variants on purpose and pnpm applies no name inference there either. Per pacquet's cardinal rule, pnpm is the source of truth; if we ever want inference in the config-deps path, it should land in pnpm first.


Written by an agent (Claude Code, claude-fable-5).

@github-actions

Copy link
Copy Markdown
Contributor

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.02      7.7±0.27ms   565.7 KB/sec    1.00      7.5±0.14ms   577.0 KB/sec

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.75%. Comparing base (bf1b731) to head (9227d34).
⚠️ Report is 4 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zkochan zkochan merged commit 52be454 into main Jun 10, 2026
30 of 32 checks passed
@zkochan zkochan deleted the fix/11702 branch June 10, 2026 19:51
KSXGitHub pushed a commit that referenced this pull request Jun 10, 2026
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.
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.

pnpm downloads binaries for other platforms even with explicit supportedArchitectures supportedArchitecture don't works as expected

2 participants