This repository was archived by the owner on May 14, 2026. It is now read-only.
feat(package-manager): runtime bin-link + --no-runtime (#437 slice D3 + E)#506
Merged
Conversation
… + E) Closes the install-pipeline side of #437: **Slice D3 — runtime bin-link integration.** Runtime archives (`node@runtime:`, etc.) don't ship a `package.json`, so the existing bin-link step had nothing to consume off the slot. `fetch_binary_resolution_to_cas` now synthesizes one in the same shape upstream's `appendManifest` does: { "name": "<pkg.name>", "version": "<pkg.version>", "bin": <BinarySpec> } The bytes go through `StoreDir::write_cas_file` (same content- addressing as every other CAS-imported file), the resulting cas-path is inserted into the snapshot's `cas_paths` under `package.json`, and the existing `link_bins_of_packages` flow picks it up. `link_bins::build_has_bin_set` now includes `Binary` / `Variations` resolutions unconditionally — pnpm v11 doesn't emit `hasBin: true` on runtime metadata, but the synthesized manifest always carries bins, so the lookup must not be gated on the metadata flag. **Slice E — `--no-runtime` flag.** New `Config::skip_runtimes` (default false) and `InstallArgs::no_runtime` CLI flag. The CLI computes `config.skip_runtimes || --no-runtime` and threads it through `Install` → `InstallFrozenLockfile`. When set, every snapshot whose metadata resolution is `Binary` or `Variations` is added to the install-time skip set (re-using `add_optional_excluded` since the bucket count and `.modules.yaml.skipped` semantics line up with `--no-optional`). **Unit tests.** - `synthesize_runtime_manifest_emits_name_version_and_bin_single` — pins `BinarySpec::Single` → JSON string. - `synthesize_runtime_manifest_emits_name_version_and_bin_map` — pins `BinarySpec::Map` → JSON object with all bin entries. - `synthesize_runtime_manifest_preserves_scoped_name` — `@scope/name` round-trips through the `name` field. - `build_has_bin_set_includes_runtime_resolutions_even_when_has_bin_is_absent` — pins the runtime arm. Verified by removing the arm — test fails as expected. End-to-end install fixtures (slice F's remaining items — recorded-archive frozen-lockfile install, variant-mismatch error, `--no-runtime` integration, integrity-mismatch path) need a small recorded Node archive in the repo (or a mockable `pnpm-resolution-mirror`). Tracking them as a separate follow-up so this PR stays reviewable.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
📝 WalkthroughWalkthroughAdds a ChangesRuntime dependency skipping via CLI flag and config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #506 +/- ##
==========================================
- Coverage 88.64% 88.54% -0.11%
==========================================
Files 122 124 +2
Lines 13273 13500 +227
==========================================
+ Hits 11766 11953 +187
- Misses 1507 1547 +40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@crates/package-manager/src/install_frozen_lockfile.rs`:
- Around line 365-371: The code widening the semantics of the --no-runtime flag
currently drops transitive runtime snapshots (the block that mutates
dependenciesByProjectId when --no-runtime/skipRuntimes is set); change that
logic to match pnpm semantics by only filtering runtime entries at the
same/project-level (i.e., implement skipRuntimes to exclude runtime packages
from direct runtime resolution but do NOT prune or drop transitive runtime
snapshots from the lockfile traversal), locate the conditional that applies the
filter in install_frozen_lockfile.rs (the --no-runtime / skipRuntimes branch)
and revert the transitive-removal behavior so transitive runtime deps remain
available while still excluding runtime entries where pnpm would.
In `@crates/package-manager/src/link_bins.rs`:
- Around line 238-241: The matches! guard is moving meta.resolution (which is
not Copy) out of a borrowed meta; change the guard to match on a reference
instead — e.g., use matches!(&meta.resolution, LockfileResolution::Binary(_) |
LockfileResolution::Variations(_)) so you borrow the enum rather than move it;
update the patterns if necessary to match referenced variants
(LockfileResolution::Binary, LockfileResolution::Variations) and keep the rest
of the surrounding logic in link_bins.rs unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f630d49-d120-4838-b6c7-47ce04e60b2c
📒 Files selected for processing (11)
crates/cli/src/cli_args/install.rscrates/config/src/lib.rscrates/package-manager/src/add.rscrates/package-manager/src/install.rscrates/package-manager/src/install/tests.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install_package_by_snapshot/tests.rscrates/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/link_bins.rscrates/package-manager/src/link_bins/tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (macos-latest)
- 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/package-manager/src/install_package_from_registry/tests.rscrates/package-manager/src/link_bins.rscrates/config/src/lib.rscrates/package-manager/src/install.rscrates/cli/src/cli_args/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/link_bins/tests.rscrates/package-manager/src/add.rscrates/package-manager/src/install_package_by_snapshot/tests.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install/tests.rs
🧠 Learnings (4)
📚 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_from_registry/tests.rscrates/package-manager/src/link_bins/tests.rscrates/package-manager/src/install_package_by_snapshot/tests.rscrates/package-manager/src/install/tests.rs
📚 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_from_registry/tests.rscrates/package-manager/src/link_bins.rscrates/config/src/lib.rscrates/package-manager/src/install.rscrates/cli/src/cli_args/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/link_bins/tests.rscrates/package-manager/src/add.rscrates/package-manager/src/install_package_by_snapshot/tests.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install/tests.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_from_registry/tests.rscrates/package-manager/src/link_bins.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/link_bins/tests.rscrates/package-manager/src/add.rscrates/package-manager/src/install_package_by_snapshot/tests.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install/tests.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_from_registry/tests.rscrates/package-manager/src/link_bins.rscrates/package-manager/src/install.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/link_bins/tests.rscrates/package-manager/src/add.rscrates/package-manager/src/install_package_by_snapshot/tests.rscrates/package-manager/src/install_package_by_snapshot.rscrates/package-manager/src/install/tests.rs
🔇 Additional comments (10)
crates/config/src/lib.rs (1)
262-273: LGTM!crates/package-manager/src/install_package_from_registry/tests.rs (1)
34-34: LGTM!crates/package-manager/src/add.rs (1)
91-91: LGTM!crates/package-manager/src/install/tests.rs (1)
56-56: LGTM!Also applies to: 110-110, 147-147, 213-213, 272-272, 348-348, 409-409, 490-490, 628-628, 722-722, 838-838, 929-929, 1028-1028, 1071-1071, 1156-1156, 1243-1243, 1294-1294, 1375-1375, 1446-1446, 1523-1523, 1701-1701, 1811-1811, 1904-1904, 2003-2003, 2087-2087, 2174-2174
crates/package-manager/src/install.rs (1)
39-46: LGTM!Also applies to: 150-150, 299-299
crates/cli/src/cli_args/install.rs (1)
59-67: LGTM!Also applies to: 73-78, 89-94, 102-102
crates/package-manager/src/install_frozen_lockfile.rs (1)
381-384: ⚡ Quick winNo changes needed. The code correctly borrows
meta.resolutionthrough automatic dereferencing. Thematches!macro handles borrowed enum references properly, and there is no move semantics hazard here.> Likely an incorrect or invalid review comment.crates/package-manager/src/install_package_by_snapshot/tests.rs (1)
3-3: LGTM!Also applies to: 7-8, 212-297
crates/package-manager/src/link_bins/tests.rs (1)
1-3: LGTM!Also applies to: 6-10, 13-13, 495-605
crates/package-manager/src/install_package_by_snapshot.rs (1)
12-12: LGTM!Also applies to: 141-156, 296-296, 351-351, 582-698
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.49467866896,
"stddev": 0.0649281519056914,
"median": 2.49329887686,
"user": 2.6256371799999996,
"system": 3.4819801999999997,
"min": 2.4022019338600002,
"max": 2.5805629578600002,
"times": [
2.5683184318600003,
2.43174423186,
2.44334348886,
2.57232159986,
2.48346515886,
2.5219934048600003,
2.43970288686,
2.5031325948600003,
2.5805629578600002,
2.4022019338600002
]
},
{
"command": "pacquet@main",
"mean": 2.5007598299600002,
"stddev": 0.06758736295007756,
"median": 2.50961755886,
"user": 2.5907365799999997,
"system": 3.4873862000000004,
"min": 2.3872154508600003,
"max": 2.59630314786,
"times": [
2.48628351486,
2.53283659686,
2.49513313886,
2.58738110186,
2.52410197886,
2.4324960458600002,
2.53023733886,
2.59630314786,
2.3872154508600003,
2.43560998486
]
},
{
"command": "pnpm",
"mean": 5.70059476416,
"stddev": 0.05206951709450585,
"median": 5.7029620008599995,
"user": 8.326199780000001,
"system": 4.243370399999999,
"min": 5.623806184859999,
"max": 5.80465791086,
"times": [
5.68146234486,
5.69075375786,
5.72059100986,
5.6681126828599995,
5.715170243859999,
5.64025504086,
5.72324146386,
5.7378970018599995,
5.623806184859999,
5.80465791086
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.7447543064600002,
"stddev": 0.039341296637116344,
"median": 0.73763581806,
"user": 0.36405374,
"system": 1.62188208,
"min": 0.70344815656,
"max": 0.84364976956,
"times": [
0.84364976956,
0.7498882235600001,
0.74416146256,
0.7583074105600001,
0.7279127455600001,
0.7561017585600001,
0.7122694995600001,
0.7311101735600001,
0.7206938645600001,
0.70344815656
]
},
{
"command": "pacquet@main",
"mean": 0.8167777660600001,
"stddev": 0.04665706853021891,
"median": 0.8226501445600001,
"user": 0.3805867399999999,
"system": 1.6255397799999998,
"min": 0.7639175545600001,
"max": 0.91704111456,
"times": [
0.8464094135600001,
0.7663326705600001,
0.8293436015600001,
0.76967916656,
0.7639175545600001,
0.8223301715600001,
0.82297011756,
0.7940241545600001,
0.91704111456,
0.8357296955600001
]
},
{
"command": "pnpm",
"mean": 2.4125018659600004,
"stddev": 0.13273958532138191,
"median": 2.35979283106,
"user": 2.7929191399999995,
"system": 2.15872758,
"min": 2.27992613956,
"max": 2.7323436185600003,
"times": [
2.48969277556,
2.33620955556,
2.7323436185600003,
2.37419312856,
2.3378061635600003,
2.27992613956,
2.3647098245600002,
2.3445208235600004,
2.3548758375600003,
2.51074079256
]
}
]
} |
Address CodeRabbit review on #506: my initial implementation iterated `packages` (every metadata entry) and added all `Binary` / `Variations` snapshots to the skip set, which widened the behavior beyond pnpm's `skipRuntimes`. Per the cardinal rule (CLAUDE.md: "Port behavior faithfully. ... Do not invent behavior that pnpm does not have."), match upstream exactly. Upstream's filter at [`installing/deps-installer/src/install/index.ts`](https://github.com/pnpm/pnpm/blob/94240bc046/installing/deps-installer/src/install/index.ts#L1374-L1387) iterates `dependenciesByProjectId` (per-importer direct deps) and adds `depPath` to `ctx.skipped` when `depPath.includes('@runtime:')`. Transitive runtime entries — unusual but possible — stay in the install. Pacquet now mirrors that exactly: walk each importer's `dependencies` / `dev_dependencies` / `optional_dependencies`, build the candidate snapshot key from `(alias, version)`, check for the `@runtime:` substring, and only add the metadata-confirmed `Binary` / `Variations` keys to `skipped`. Aliased runtime deps (unusual) fall through the same way upstream does — pnpm's lookup is by depPath, not by alias.
Open
22 tasks
This was referenced May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the install-pipeline side of #437. After this PR, a frozen-lockfile install can fetch a runtime archive, write its CAS-imported files into the virtual store slot, and create cmd-shims for the declared executables under each importer's
node_modules/.bin/.Slice D3 — runtime bin-link integration
Runtime archives (
node@runtime:,deno@runtime:,bun@runtime:) don't ship apackage.json, so the existing bin-link step had nothing to consume off the slot.fetch_binary_resolution_to_casnow synthesizes one in the same shape upstream'sappendManifestdoes:{ "name": "<pkg.name>", "version": "<pkg.version>", "bin": <BinarySpec> }The bytes go through
StoreDir::write_cas_file(same content-addressing as every other CAS-imported file), the resulting cas-path is inserted into the snapshot'scas_pathsunder"package.json", and the existinglink_bins_of_packagesflow picks it up.link_bins::build_has_bin_setnow includesBinary/Variationsresolutions unconditionally — pnpm v11 doesn't emithasBin: trueon runtime metadata, but the synthesized manifest always carries bins, so the lookup must not be gated on the metadata flag.Slice E —
--no-runtimeflagConfig::skip_runtimes: bool(defaultfalse, reserved forpnpm-workspace.yaml/.npmrcplumbing in a follow-up).InstallArgs::no_runtimeCLI flag (--no-runtime).config.skip_runtimes || --no-runtimeand threads it throughInstall→InstallFrozenLockfile. Whentrue, every snapshot whose metadata resolution isBinaryorVariationsis added to the install-time skip set (re-usingadd_optional_excludedsince the bucket count and.modules.yaml.skippedsemantics line up with--no-optional).Slice F — what's in / what's deferred
Unit-tested:
synthesize_runtime_manifest_emits_name_version_and_bin_single— pinsBinarySpec::Single→ JSON string.synthesize_runtime_manifest_emits_name_version_and_bin_map— pinsBinarySpec::Map→ JSON object.synthesize_runtime_manifest_preserves_scoped_name—@scope/nameround-trips through thenamefield.build_has_bin_set_includes_runtime_resolutions_even_when_has_bin_is_absent— verified by removing the runtime arm (test fails as expected).archive_filter_for_only_returns_filter_for_unscoped_node(from slice D2) covers the per-package filter dispatcher.node_extras_filter_matches_upstream_regex_alternations(from slice D2) covers the regex port.Deferred to a follow-up: end-to-end install fixtures (recorded-archive frozen-lockfile install, variant-mismatch error,
--no-runtimeintegration, integrity-mismatch path, path-traversal). They need either a small recorded Node archive in the repo or a mockablepnpm-resolution-mirror; keeping this PR reviewable.Test plan
install_package_by_snapshot::tests::*pass.link_bins::tests::*pass (addedbuild_has_bin_set_includes_runtime_resolutions...).pacquet-package-managertests pass (skip_runtimes threaded through everyInstall { ... }andConfig { ... }literal across the workspace).just ready,taplo format --check,just dylint,cargo doc -D warningsgreen.Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
New Features
Bug Fixes
Tests