Skip to content
This repository was archived by the owner on May 14, 2026. It is now read-only.

feat(package-manager): dispatch Binary/Variations to runtime fetchers (#437 slice D1)#492

Merged
zkochan merged 1 commit into
mainfrom
feat/437-slice-d
May 13, 2026
Merged

feat(package-manager): dispatch Binary/Variations to runtime fetchers (#437 slice D1)#492
zkochan merged 1 commit into
mainfrom
feat/437-slice-d

Conversation

@zkochan

@zkochan zkochan commented May 13, 2026

Copy link
Copy Markdown
Member

Summary

Slice D1 of #437 — wires the runtime fetchers landed in slice C (#472) into the install dispatcher. Replaces the Binary / Variations arms in install_package_by_snapshot.rs and create_virtual_store.rs::snapshot_cache_key. After this slice a frozen-lockfile install can dispatch a runtime resolution end-to-end through the existing CAS-write pipeline (just without the per-archive ignore filter or bin-link cmd-shims yet — see D2 below).

  • LockfileResolution::Binary(b) dispatches on b.archive: tarball uses DownloadTarballToStore, zip uses DownloadZipArchiveToStore with archive_prefix: b.prefix.as_deref().
  • LockfileResolution::Variations(v) builds a PlatformSelector from pacquet_graph_hasher::{host_platform, host_arch, host_libc}, runs select_platform_variant, and routes the matched variant's inner BinaryResolution through the same dispatcher. A non-Binary inner shape (corrupt lockfile or future shape) raises a typed error rather than silently routing.
  • snapshot_cache_key returns the proper warm-cache key for both new arms — Binary uses store_index_key(integrity, pkg_id); Variations runs the same selector and keys off the picked variant.
  • Two new error variants on InstallPackageBySnapshotError:
    • NoMatchingPlatformVariant — carries the host triple + rendered available-target list so the user can see at a glance why selection failed.
    • VariantHasNonBinaryResolution — defensive guard.

What's not in this slice

Splitting the issue's Slice D into smaller PRs (per the established cadence). Follow-up slices:

  • D2NODE_EXTRAS_IGNORE_PATTERN filter for pkg.name == "node" (strip bundled npm / corepack); bin-link cmd-shims for the runtime executables (bin field on BinaryResolution); @runtime: substring handling in skip lists / reporter prefixes.
  • E--no-runtime flag.
  • F — end-to-end install fixtures (host-match, variant mismatch, --no-runtime, integrity mismatch, path-traversal).

Test plan

  • host_platform_selector_omits_libc_on_non_linux_hosts — pins the host_libc() == "unknown"selector.libc.is_none() relationship (covers both Linux and non-Linux hosts via the same assertion).
  • render_variant_targets_formats_each_triple_with_optional_libc — locks in the os/cpu[+libc] rendering used by NoMatchingPlatformVariant.
  • just ready green
  • taplo format --check, just dylint, cargo doc -D warnings green

End-to-end runtime install tests live in slice F (the slice's whole point is to fixture them once the rest of the pipeline is in place).


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

Summary by CodeRabbit

  • New Features

    • Added support for binary and variation-based package resolutions during installation
    • Implemented platform-aware variant selection for multi-platform compatibility
  • Improvements

    • Enhanced cache prefetching for additional lockfile resolution types
    • Improved error reporting when no matching platform variant is found, displaying available targets and host specifications

Review Change Stack

…#437 slice D1)

Replace the cold-path `UnsupportedResolution` arms for
`LockfileResolution::{Binary, Variations}` with actual fetcher
dispatch. Slice C wired the per-archive download
(`DownloadTarballToStore` / `DownloadZipArchiveToStore`); this
slice routes lockfile resolutions through them.

- `Binary(b)` dispatches on `b.archive` (`Tarball` / `Zip`):
  - `Tarball` → `DownloadTarballToStore` (same shape as
    registry / tarball entries minus the `package_unpacked_size`
    hint).
  - `Zip` → `DownloadZipArchiveToStore` with
    `archive_prefix: b.prefix.as_deref()` so the runtime
    archive's top-level wrapper (e.g. `node-vX.Y.Z-darwin-arm64/`)
    is stripped before the CAS keys are written.
- `Variations(v)` runs `select_platform_variant` against a
  freshly-built `PlatformSelector` (`pacquet_graph_hasher`'s
  `host_platform` / `host_arch` / `host_libc`, mapping
  `"unknown"` → `None` per upstream's
  `process.platform === 'linux' ? family : null` convention).
  The picked variant's inner resolution must be `Binary`; any
  other shape (corrupt lockfile or a future shape pacquet hasn't
  learned about) raises the typed
  `VariantHasNonBinaryResolution` error rather than silently
  routing.
- `create_virtual_store::snapshot_cache_key` returns the proper
  warm-cache key for both new arms: `Binary` uses
  `store_index_key(integrity, pkg_id)`; `Variations` runs the
  same selector and keys off the picked variant. A miss on
  variant selection or a non-`Binary` inner shape returns
  `Ok(None)` and lets the cold path raise the typed error.
- Two new error variants on `InstallPackageBySnapshotError`:
  - `NoMatchingPlatformVariant { package_key, host_os, host_cpu,
    host_libc, available_targets }` — fires when no variant
    matches the host; carries the host triple + the rendered
    available target list so the user can see at a glance why.
  - `VariantHasNonBinaryResolution { package_key, inner_kind }`
    — defensive guard for malformed lockfiles.

The Node-runtime `NODE_EXTRAS_IGNORE_PATTERN` filter (strips
bundled `npm` / `corepack` from the archive) and bin-link
cmd-shims for runtime executables will land in Slice D2; the
filter slot stays `None` for now and `BinaryResolution::bin` is
read but not yet acted on. Slice E adds `--no-runtime` and
Slice F adds the end-to-end install fixtures.

Two new unit tests cover the helpers:
- `host_platform_selector_omits_libc_on_non_linux_hosts` —
  pins the `host_libc() == "unknown"` ⇔ `selector.libc.is_none()`
  relationship without needing platform-gated tests.
- `render_variant_targets_formats_each_triple_with_optional_libc`
  — locks in the `os/cpu[+libc]` rendering used by the
  `NoMatchingPlatformVariant` error message.
Copilot AI review requested due to automatic review settings May 13, 2026 20:54
@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3addcc26-5d77-4381-9b36-e2574f143985

📥 Commits

Reviewing files that changed from the base of the PR and between ee05f22 and 67df55b.

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

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Preserve existing method chains and pipe-trait chains; do not break them into intermediate let bindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Path over &PathBuf, &str over &String) when it does not force extra copies.
Prefer Arc::clone(&x) and Rc::clone(&x) over x.clone() for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Write use super::{Foo, bar} instead of use super::*; for any glob whose target is a module you control. External-crate preludes (e.g., use rayon::prelude::*;) and root-of-module re-exports (e.g., pub use submodule::*; in lib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plain String or &str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only via TryFrom<String> and/or FromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallible From<String> and From<&str> constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bare as assertion), add a from_str_unchecked (or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...

Files:

  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_by_snapshot/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
🧠 Learnings (4)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.

Applied to files:

  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_by_snapshot/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
📚 Learning: 2026-05-13T19:22:48.951Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 478
File: crates/package-manager/src/hoisted_dep_graph.rs:51-55
Timestamp: 2026-05-13T19:22:48.951Z
Learning: When reviewing this Rust codebase, avoid introducing/using a newtype like `PkgIdWithPatchHash` in only one module (e.g., `hoisted_dep_graph.rs`) if other related pacquet modules still represent the upstream pnpm “pkg id with patch hash” as a plain `String` (e.g., `virtual_store_layout`). For type consistency, either keep `pkg_id_with_patch_hash` as `String` here, or require a workspace-wide sweep that defines/extracts the `PkgIdWithPatchHash` newtype once and updates all call sites together (otherwise defer the refactor to a coordinated follow-up PR).

Applied to files:

  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_by_snapshot/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
📚 Learning: 2026-05-13T20:09:22.171Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 486
File: crates/package-manager/src/hoisted_dep_graph.rs:472-476
Timestamp: 2026-05-13T20:09:22.171Z
Learning: In pnpm/pacquet, when generating serialized `hoisted_locations` strings (used for `.modules.yaml`), normalize path separators to forward slashes (e.g., replace `\\` with `/`). This preserves cross-platform portability: the output must not use OS-native separators because upstream pnpm’s `path.relative` can return platform-specific separators, which would break consistency with pnpm/pacquet’s other serialized formats on Windows.

Applied to files:

  • crates/package-manager/src/install_package_by_snapshot.rs
  • crates/package-manager/src/install_package_by_snapshot/tests.rs
  • crates/package-manager/src/create_virtual_store.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.

Applied to files:

  • crates/package-manager/src/install_package_by_snapshot/tests.rs
🔇 Additional comments (13)
crates/package-manager/src/create_virtual_store.rs (3)

3-3: LGTM!

Also applies to: 11-11


792-801: LGTM!


802-827: LGTM!

crates/package-manager/src/install_package_by_snapshot.rs (7)

10-14: LGTM!

Also applies to: 21-23


103-139: LGTM!


262-282: LGTM!


283-336: LGTM!


425-442: LGTM!


444-519: LGTM!


521-536: LGTM!

crates/package-manager/src/install_package_by_snapshot/tests.rs (3)

1-6: LGTM!


38-68: LGTM!


70-108: LGTM!


📝 Walkthrough

Walkthrough

This PR extends the package manager to support binary and platform-variant lockfile resolutions by adding runtime installation logic and warm cache key generation. Binary archives are downloaded into the content-addressed store, platform variants are selected using host OS/CPU/libc matching, and new error diagnostics capture host-triple mismatches and unsupported inner resolutions.

Changes

Binary and Variant Lockfile Resolution Support

Layer / File(s) Summary
Error handling for platform-variant resolution failures
crates/package-manager/src/install_package_by_snapshot.rs
InstallPackageBySnapshotError gains NoMatchingPlatformVariant and VariantHasNonBinaryResolution variants to capture host-triple mismatch (with rendered available targets) and non-binary inner resolutions.
Host platform selector and binary archive download helpers
crates/package-manager/src/install_package_by_snapshot.rs, crates/package-manager/src/install_package_by_snapshot/tests.rs
host_platform_selector() normalizes host libc to Option, fetch_binary_resolution_to_cas() downloads tarball/zip archives to CAS, and render_variant_targets() formats advertised targets for errors; unit tests verify libc omission on non-Linux and correct target formatting.
Runtime resolution dispatcher with variant selection
crates/package-manager/src/install_package_by_snapshot.rs
InstallPackageBySnapshot::run handles Binary and Variations resolutions by building a host PlatformSelector, selecting the matching variant, downloading binaries to CAS, and rejecting unmatched or non-binary variants.
Warm cache key generation for binary and variant resolutions
crates/package-manager/src/create_virtual_store.rs
snapshot_cache_key generates warm CAS keys for Binary resolutions and selects host-matching variants for Variations, returning warm keys only when the selected variant is Binary.

Sequence Diagram

sequenceDiagram
  participant InstallPackageBySnapshot
  participant HostPlatformSelector
  participant BinaryFetch
  participant CAS
  
  InstallPackageBySnapshot->>HostPlatformSelector: host_platform_selector()
  HostPlatformSelector-->>InstallPackageBySnapshot: PlatformSelector (with os/cpu/libc)
  
  alt Binary resolution
    InstallPackageBySnapshot->>BinaryFetch: fetch_binary_resolution_to_cas(binary)
    BinaryFetch->>CAS: download and store archive
    CAS-->>BinaryFetch: relative-path → CAS-path map
    BinaryFetch-->>InstallPackageBySnapshot: snapshot map
  else Variations resolution
    InstallPackageBySnapshot->>HostPlatformSelector: select_platform_variant(variations)
    HostPlatformSelector-->>InstallPackageBySnapshot: matching variant
    alt Variant is Binary
      InstallPackageBySnapshot->>BinaryFetch: fetch_binary_resolution_to_cas(binary)
      BinaryFetch->>CAS: download and store archive
      CAS-->>InstallPackageBySnapshot: snapshot map
    else Variant is not Binary
      InstallPackageBySnapshot-->>InstallPackageBySnapshot: error: VariantHasNonBinaryResolution
    end
  else No matching variant
    InstallPackageBySnapshot-->>InstallPackageBySnapshot: error: NoMatchingPlatformVariant
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

  • pnpm/pacquet#437: Both implement the same code-level features—selecting host-matching platform variants and handling Binary/Variations lockfile resolutions with binary fetch-to-CAS.
  • pnpm/pacquet#433: Both touch the same install_package_by_snapshot code paths extended for per-snapshot behavior control.

Possibly related PRs

  • pnpm/pacquet#457: Main PR implements warm-cache key generation and fetch/variant selection for LockfileResolution::Binary/Variations, which the retrieved PR initially left as "unsupported/none."
  • pnpm/pacquet#466: Main PR directly uses the PlatformSelector/select_platform_variant API introduced in the retrieved PR to resolve LockfileResolution::Variations.
  • pnpm/pacquet#472: Main PR's zip-based binary archive downloading is directly enabled by the retrieved PR adding the zip fetch/extract pipeline.

Poem

🐰 Hark, binary variants now take flight,
With host-matched platforms shining bright,
Cache keys warm as fresh-baked bread,
While archives fetch to CAS ahead,
Platform puzzles solved with glee! 🎯

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: dispatching Binary and Variations lockfile resolutions to runtime fetchers, with the PR slice reference providing context.
Description check ✅ Passed The description comprehensively covers the summary, linked issue (#437), upstream behavior alignment, test plan, and checklist items, following the template structure.
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 feat/437-slice-d

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

@zkochan

zkochan commented May 13, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 13, 2026

Copy link
Copy Markdown
✅ Actions performed

Full review triggered.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions

Copy link
Copy Markdown

Micro-Benchmark Results

Linux

group                          main                                   pr
-----                          ----                                   --
tarball/download_dependency    1.01     16.4±1.07ms   264.9 KB/sec    1.00     16.2±0.19ms   267.0 KB/sec

@codecov

codecov Bot commented May 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 12.29508% with 107 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.32%. Comparing base (5397931) to head (67df55b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...package-manager/src/install_package_by_snapshot.rs 13.39% 97 Missing ⚠️
crates/package-manager/src/create_virtual_store.rs 0.00% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
- Coverage   88.82%   88.32%   -0.50%     
==========================================
  Files         121      121              
  Lines       12597    12738     +141     
==========================================
+ Hits        11189    11251      +62     
- Misses       1408     1487      +79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@zkochan zkochan merged commit c359dd7 into main May 13, 2026
14 of 17 checks passed
@zkochan zkochan deleted the feat/437-slice-d branch May 13, 2026 21:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants