feat(lockfile): select_platform_variant + PlatformSelector (#437 slice B)#466
Conversation
…e B)
Add the variant-picking logic for `VariationsResolution` ahead of
Slice D's install-pipeline dispatch:
- `PlatformSelector { os, cpu, libc: Option<String> }` mirrors
upstream's [`PlatformSelector`](https://github.com/pnpm/pnpm/blob/94240bc046/resolving/resolver-base/src/index.ts#L78-L83).
The `libc` tri-state encodes pnpm's `string | null | undefined`
shape: `None` (host doesn't care about libc — macOS/Windows/BSD)
and `Some("glibc")` collapse to the same matching arm (variant
must have no `libc:` annotation); `Some("musl")` requires an exact
`libc: "musl"` annotation so the glibc default doesn't silently
win on a musl host.
- `select_platform_variant(variants, selector)` ports
[`selectPlatformVariant`](https://github.com/pnpm/pnpm/blob/94240bc046/resolving/resolver-base/src/index.ts#L92-L98).
Iterates `variants` in declaration order and returns the first
variant whose `targets[]` contains an `(os, cpu, libc)` triple
matching the selector. Targets-array scan is linear because real
archives ship 1–3 target entries per variant; quadratic cost is
immaterial.
- `libc_matches(variant_libc, requested_libc)` is the asymmetric
helper from
[`libcMatches`](https://github.com/pnpm/pnpm/blob/94240bc046/resolving/resolver-base/src/index.ts#L100-L107).
Crate-private so the matching policy isn't part of the public API.
Lives in `pacquet-lockfile` next to the variant types (mirrors
upstream's `resolver-base` package boundary). The host-platform side
(constructing the `PlatformSelector` from `pacquet-graph-hasher`'s
existing `host_platform/arch/libc` helpers) lands at the install
dispatcher in Slice D — keeps `pacquet-lockfile` free of the
`graph-hasher` dep and lets tests drive the picker with synthetic
hosts.
Tests: first-match wins, multi-target variant matches any host triple,
no-match returns `None`, musl-host rejects glibc-default variant,
musl-host matches musl-annotated variant, and a six-row `libc_matches`
truth table (None / "glibc" / "musl" / unknown-future-libc on both
sides) pinning the asymmetric contract from upstream.
Part of #437.
---
Written by an agent (Claude Code, claude-opus-4-7).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
📝 WalkthroughWalkthroughAdds host-based platform variant selection to the lockfile resolver: a public ChangesPlatform Variant Selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/lockfile/src/resolution/tests.rs (2)
568-582: 💤 Low valueConsider using distinct URLs in test fixtures for clearer assertions.
The integrity check on lines 577-580 doesn't actually verify which variant was picked since both variants share the same hash. The targets assertion on line 581 is the real discriminator. Consider giving each variant a distinct URL to make the test intent clearer, or remove the redundant integrity assertion.
🤖 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 `@crates/lockfile/src/resolution/tests.rs` around lines 568 - 582, The test pick_first_matching_variant uses two variants with identical integrity hashes so the integrity assertion on picked.resolution.integrity() cannot distinguish which variant was chosen; update the test fixture for variant("darwin-arm64", ...) and variant("linux-x64", ...) to use distinct download URLs (or distinct integrity values) so the integrity assertion becomes meaningful, or alternatively remove the redundant integrity assertion and rely on the existing picked.targets assertion and select_platform_variant call to verify the chosen variant.
638-661: 💤 Low valueConsider adding explicit
(Some("glibc"), Some("glibc"))case to truth table.The truth table is thorough, but adding an explicit assertion for
libc_matches(Some("glibc"), Some("glibc"))returningfalsewould document the perhaps-surprising behavior that a variant with explicitlibc: "glibc"annotation is rejected even when the selector requests"glibc". This matches the "pnpm only annotates non-glibc builds" contract but is worth pinning.📝 Suggested addition
// Selector says "glibc" (Linux glibc host): same rule as None. assert!(libc_matches(None, Some("glibc"))); assert!(!libc_matches(Some("musl"), Some("glibc"))); + // Explicit "glibc" annotation on variant is rejected (pnpm never emits it). + assert!(!libc_matches(Some("glibc"), Some("glibc")));🤖 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 `@crates/lockfile/src/resolution/tests.rs` around lines 638 - 661, Add an explicit assertion in the libc_matches_truth_table test that libc_matches(Some("glibc"), Some("glibc")) is false to document that an explicitly annotated glibc variant is rejected when the selector is "glibc"; update the test function libc_matches_truth_table to include this case alongside the existing assertions so the behavior of libc_matches is clearly pinned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/lockfile/src/resolution/tests.rs`:
- Around line 568-582: The test pick_first_matching_variant uses two variants
with identical integrity hashes so the integrity assertion on
picked.resolution.integrity() cannot distinguish which variant was chosen;
update the test fixture for variant("darwin-arm64", ...) and
variant("linux-x64", ...) to use distinct download URLs (or distinct integrity
values) so the integrity assertion becomes meaningful, or alternatively remove
the redundant integrity assertion and rely on the existing picked.targets
assertion and select_platform_variant call to verify the chosen variant.
- Around line 638-661: Add an explicit assertion in the libc_matches_truth_table
test that libc_matches(Some("glibc"), Some("glibc")) is false to document that
an explicitly annotated glibc variant is rejected when the selector is "glibc";
update the test function libc_matches_truth_table to include this case alongside
the existing assertions so the behavior of libc_matches is clearly pinned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4cd39a10-3ad0-49de-8946-5af20db47c5c
📒 Files selected for processing (2)
crates/lockfile/src/resolution.rscrates/lockfile/src/resolution/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: Agent
- GitHub Check: Lint and Test (macos-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Dylint
- GitHub Check: Code Coverage
- GitHub Check: Run benchmark on ubuntu-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 andpipe-traitchains; do not break them into intermediateletbindings 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 (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse 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::*;inlib.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 plainStringor&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 viaTryFrom<String>and/orFromStr; 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 infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/lockfile/src/resolution.rscrates/lockfile/src/resolution/tests.rs
🧠 Learnings (2)
📚 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/lockfile/src/resolution.rscrates/lockfile/src/resolution/tests.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/lockfile/src/resolution/tests.rs
🔇 Additional comments (7)
crates/lockfile/src/resolution.rs (3)
176-199: LGTM!The
PlatformSelectorstruct correctly models the tri-statelibcsemantics documented in the PR objectives. The doc comment thoroughly explains theNone/Some("glibc")/Some("musl")behavior and links to upstream.
201-222: LGTM!The function correctly implements declaration-order iteration with linear target scanning, matching upstream's
selectPlatformVariantsemantics. The borrowing is efficient with no unnecessary allocations.
233-238: 💤 Low valueNo changes needed. The implementation is correct and already well-documented.
The doc comment at lines 224–232 and supporting comments at lines 132–136 and 186–190 clearly explain that pnpm only writes
libcfor non-glibc (musl) variants and never emitslibc: "glibc"explicitly. The asymmetric matching logic—whereNoneandSome("glibc")both requirevariant_libc.is_none()—is the intended behavior and faithfully ports upstream'slibcMatches. The code is correct.crates/lockfile/src/resolution/tests.rs (4)
1-6: LGTM!
540-562: LGTM!Test helpers are concise and make the test cases readable.
588-596: LGTM!
601-631: LGTM!Good coverage of the critical musl/glibc rejection semantics. The descriptive assertion message on line 617-618 clearly explains the expected behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #466 +/- ##
==========================================
+ Coverage 88.64% 88.67% +0.02%
==========================================
Files 116 116
Lines 9952 9969 +17
==========================================
+ Hits 8822 8840 +18
+ Misses 1130 1129 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
There was a problem hiding this comment.
Pull request overview
Adds platform-variant selection logic to pacquet-lockfile to support pnpm v11 runtime dependency “variations” resolutions (to be wired into the install pipeline in later slices).
Changes:
- Introduces
PlatformSelectorplusselect_platform_variant()for declaration-order variant selection. - Adds
libc_matches()implementing pnpm’s asymmetric libc matching semantics (glibc/none vs musl). - Adds unit tests covering variant selection and a libc matching truth table.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/lockfile/src/resolution.rs | Adds PlatformSelector, select_platform_variant, and libc_matches to pick a matching variant for a host triple. |
| crates/lockfile/src/resolution/tests.rs | Adds unit tests for variant selection behavior and libc_matches truth table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Two follow-ups from Copilot review on PR #466: - `resolution.rs:152-154` claimed "the variant picker checks at runtime that the resolved inner is atomic", but `select_platform_variant` does not. Reword to describe the actual contract: pacquet's `PlatformAssetResolution.resolution` is typed as the full `LockfileResolution` for serde uniformity, and the lockfile is trusted to honor upstream's atomic-inner invariant. No infinite-recursion risk because the install dispatcher doesn't call back into `select_platform_variant` for non-`Variations` inputs. - `resolution.rs:169` referenced `pick_variant` in `pacquet-package-manager`, but the picker actually lives in this module as `select_platform_variant`. Updated the pointer. - `pick_returns_first_when_multiple_variants_match` test: two variants both list the same `(darwin, arm64)` target; the test asserts the first one wins, pinning the `Array.prototype.find` semantics. Pnpm-written lockfiles can rely on declaration order (e.g., listing a preferred build before a fallback) — without this test, a future refactor that switched the iteration to a triple-keyed `BTreeMap` would silently break that. No behavior change; doc-comment text + test addition. --- Written by an agent (Claude Code, claude-opus-4-7).
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5340172155,
"stddev": 0.10963977588197407,
"median": 2.5164822383,
"user": 2.6147036200000007,
"system": 3.4788639199999998,
"min": 2.4311761583,
"max": 2.7824665832999997,
"times": [
2.4773330013,
2.4313701262999996,
2.7824665832999997,
2.5794632043,
2.5312517082999997,
2.4391692023,
2.5278384472999997,
2.6349776943,
2.5051260293,
2.4311761583
]
},
{
"command": "pacquet@main",
"mean": 2.5073567108,
"stddev": 0.069179338497146,
"median": 2.5044753452999995,
"user": 2.61120632,
"system": 3.4735682199999998,
"min": 2.3802799633,
"max": 2.6181038172999997,
"times": [
2.6181038172999997,
2.5428317283,
2.4651078912999997,
2.5447016892999996,
2.4781126243,
2.5844229882999996,
2.5154226082999998,
2.4935280822999997,
2.4510557153,
2.3802799633
]
},
{
"command": "pnpm",
"mean": 5.7165902826999995,
"stddev": 0.08160274211751391,
"median": 5.697518129300001,
"user": 8.231976819999998,
"system": 4.25350042,
"min": 5.6126869443,
"max": 5.9181224143,
"times": [
5.6748836143000005,
5.696908173300001,
5.7390982713000005,
5.6126869443,
5.727785558300001,
5.6981280853000005,
5.6709963383,
5.754415567300001,
5.6728778603,
5.9181224143
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7438194066,
"stddev": 0.0366374428046,
"median": 0.7306011190000001,
"user": 0.38341786000000005,
"system": 1.6060912799999998,
"min": 0.713017443,
"max": 0.8316096700000001,
"times": [
0.8316096700000001,
0.7327237160000001,
0.761813846,
0.7303984330000001,
0.713017443,
0.7191516450000001,
0.720193696,
0.7308038050000001,
0.7227557400000001,
0.7757260720000001
]
},
{
"command": "pacquet@main",
"mean": 0.8401774978000001,
"stddev": 0.06180675785855487,
"median": 0.8345190720000001,
"user": 0.37649425999999997,
"system": 1.6498870799999998,
"min": 0.761033285,
"max": 0.9875251770000001,
"times": [
0.8324675530000001,
0.815393255,
0.776402514,
0.8374711890000001,
0.761033285,
0.9875251770000001,
0.8365705910000001,
0.8290418150000001,
0.847371132,
0.8784984670000001
]
},
{
"command": "pnpm",
"mean": 2.4223954651,
"stddev": 0.17882402829440816,
"median": 2.349069174,
"user": 2.76740636,
"system": 2.1537488799999993,
"min": 2.2695770669999997,
"max": 2.808101723,
"times": [
2.444223671,
2.365849011,
2.808101723,
2.379809629,
2.2695770669999997,
2.331243712,
2.3016782,
2.3322893369999997,
2.310555531,
2.68062677
]
}
]
} |
Summary
Slice B of #437. Adds the pure-logic variant-picker ahead of Slice D's install-pipeline dispatch. Builds on the lockfile types from #457 (slice A). No new workspace deps; logic lives in
pacquet-lockfilenext to the variant types.PlatformSelector { os, cpu, libc: Option<String> }mirrors upstream'sPlatformSelector. Thelibctri-state encodes pnpm'sstring | null | undefinedshape:None(macOS / Windows / BSD — libc constraint is irrelevant) andSome("glibc")collapse to the same matching arm (variant must have nolibc:annotation);Some("musl")requires an exactlibc: "musl"annotation, so the glibc-default variant doesn't silently win on a musl host.select_platform_variant(variants, selector) -> Option<&PlatformAssetResolution>portsselectPlatformVariant. Declaration-order match; the targets-array scan is linear because real Node / Deno / Bun archives ship 1–3 target entries per variant.libc_matches(variant_libc, requested_libc) -> boolis the asymmetric helper fromlibcMatches. Crate-private so the matching policy isn't part of the public API.The host-platform side (constructing the
PlatformSelectorfrompacquet-graph-hasher's existinghost_platform/arch/libchelpers, plus--cpu/--os/--libc/supportedArchitecturesoverrides) lands at the install dispatcher in Slice D — keepspacquet-lockfilefree of thegraph-hasherdep and lets the picker's tests drive synthetic hosts. The--cpu/--os/--libcCLI flags themselves belong to #453.Test plan
just ready— 920 tests pass (2 skipped)taplo format --check— cleanjust dylint(perfectionist) — cleanNone, musl host rejects the glibc-default variant, musl host matches a musl-annotated variant, and a six-rowlibc_matchestruth table (None / "glibc" / "musl" / unknown-future-libc on both sides) pinning the asymmetric contract.Follow-up slices on #437
ignore_file_pattern; zip withzip = "5"workspace dep + path-traversal validation +NODE_EXTRAS_IGNORE_PATTERN).Binary/Variations+ bin linking viapacquet-cmd-shim+ host-selector plumbing (pacquet-graph-hasher→PlatformSelector).--no-runtimeCLI flag +Config.skip_runtimes.plans/TEST_PORTING.mdInstallation Of Runtimes section.Upstream
pnpm/pnpm@
94240bc046.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Tests