This repository was archived by the owner on Jul 21, 2026. It is now read-only.
feat(lockfile): support npm-alias importer dependency versions#524
Merged
Conversation
…sions
pnpm writes importer dependency `version:` fields in three shapes:
bare semver-with-peer (`4.0.0`), `link:<path>`, or — when a specifier
(typically `catalog:`) resolves to a different package name — the full
npm-alias `<name>@<version>`. The third shape is what
`refToRelative` recognises with the same leading-`@` / `@` before
`(`/`:` test that `SnapshotDepRef` already uses, and which pnpm v11
emits for entries like:
js-yaml:
specifier: 'catalog:'
version: '@zkochan/js-yaml@0.0.11'
Pacquet's `ImporterDepVersion` only modelled `Regular` and `Link`, so
deserialising a lockfile with an aliased catalog dep failed with
"Failed to parse importer dependency version".
Add an `Alias(PkgNameVerPeer)` variant and a `resolved_key` helper
that returns the correct snapshot-map key for each shape — the
importer-map key paired with the version for `Regular`, the alias's
own `(name, suffix)` for `Alias`, and `None` for `Link`. Every site
that previously built a key from `as_regular().map(|v| PkgNameVerPeer::new(name, v))`
now goes through `resolved_key`, so aliased deps reach the snapshot,
the skipped-set, the reachability BFS, the build-sequence root walk,
the runtime exclusion check, and both hoist passes correctly.
For symlink targets, an aliased dep links the importer-key name to
`<slot>/node_modules/<alias-real-name>` (the resolved package's true
name inside its slot), matching pnpm's `linkDirectDeps`. The
`pnpm:root added` event now reports `realName` as the resolved
package name for aliases, where before it always echoed the
importer-map key.
Upstream reference: `refToRelative` in
`pnpm/pnpm@8a80235c7b/deps/path/src/index.ts:96-110`.
📝 WalkthroughWalkthroughThe PR extends Changesnpm-alias support in ImporterDepVersion and dependency key resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 |
Micro-Benchmark ResultsLinux |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #524 +/- ##
==========================================
- Coverage 89.17% 89.06% -0.11%
==========================================
Files 125 125
Lines 14220 14251 +31
==========================================
+ Hits 12680 12693 +13
- Misses 1540 1558 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/package-manager/src/install_frozen_lockfile.rs (1)
461-477:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLook up runtime metadata by the peer-stripped key.
packagesis keyed by the metadata key, not the full snapshot key. With the current lookup, a runtime dep that carries a peer suffix will miss its metadata row here and slip past--no-runtime.Suggested fix
- if let Some(meta) = pkgs.get(&key) + let metadata_key = key.without_peer(); + if let Some(meta) = pkgs.get(&metadata_key) && matches!( &meta.resolution, pacquet_lockfile::LockfileResolution::Binary(_) | pacquet_lockfile::LockfileResolution::Variations(_), )🤖 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/package-manager/src/install_frozen_lockfile.rs` around lines 461 - 477, The code is looking up package metadata in pkgs using the full snapshot key (key) which may include peer suffixes, so pkgs.get(&key) can miss entries and runtime deps bypass --no-runtime; instead compute a peer-stripped key (e.g. derive meta_key by removing the peer/peer suffix from key) and use pkgs.get(&meta_key) when checking meta in the if-let, while still using the original snapshot key for skipped.add_optional_excluded(key) and the subsequent logic; update the lookup around spec.version.resolved_key(alias) / key / pkgs.get(...) accordingly to use the peer-stripped key for metadata lookup.
🧹 Nitpick comments (1)
crates/lockfile/src/resolved_dependency.rs (1)
157-175: ⚡ Quick winExtract the alias-shape check into one shared helper.
looks_like_alias()duplicates the same discriminatorSnapshotDepRefalready uses. Keeping two copies of the dep-path classifier invites parser drift: the next edge-case fix can land in one path and silently desync the other.As per coding guidelines, "Before implementing any non-trivial helper function, search the workspace for existing functions or utilities that do the same or similar thing" and "If logic you need already exists in another crate but is not exported, refactor it into a shared crate or move it to a utility crate rather than copy-pasting."
🤖 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/resolved_dependency.rs` around lines 157 - 175, Duplicate alias-shape logic in looks_like_alias should be refactored into a single shared helper used by both looks_like_alias and SnapshotDepRef; create a new utility function (e.g., dep_path::is_alias or crate::utils::looks_like_alias_shared) that implements the current classifier (leading '@' or an '@' before any '(' or ':'), export it from a common module accessible to both crates, replace the local looks_like_alias and the duplicated classifier in SnapshotDepRef to call the shared helper, and remove the duplicated code while preserving existing behavior and tests.
🤖 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 `@crates/package-manager/src/install_frozen_lockfile.rs`:
- Around line 461-477: The code is looking up package metadata in pkgs using the
full snapshot key (key) which may include peer suffixes, so pkgs.get(&key) can
miss entries and runtime deps bypass --no-runtime; instead compute a
peer-stripped key (e.g. derive meta_key by removing the peer/peer suffix from
key) and use pkgs.get(&meta_key) when checking meta in the if-let, while still
using the original snapshot key for skipped.add_optional_excluded(key) and the
subsequent logic; update the lookup around spec.version.resolved_key(alias) /
key / pkgs.get(...) accordingly to use the peer-stripped key for metadata
lookup.
---
Nitpick comments:
In `@crates/lockfile/src/resolved_dependency.rs`:
- Around line 157-175: Duplicate alias-shape logic in looks_like_alias should be
refactored into a single shared helper used by both looks_like_alias and
SnapshotDepRef; create a new utility function (e.g., dep_path::is_alias or
crate::utils::looks_like_alias_shared) that implements the current classifier
(leading '@' or an '@' before any '(' or ':'), export it from a common module
accessible to both crates, replace the local looks_like_alias and the duplicated
classifier in SnapshotDepRef to call the shared helper, and remove the
duplicated code while preserving existing behavior and tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 98369aef-57be-4952-aa76-0d69868a636e
📒 Files selected for processing (9)
crates/lockfile/src/resolved_dependency.rscrates/lockfile/src/resolved_dependency/tests.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/current_lockfile.rscrates/package-manager/src/hoist.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/real-hoist/src/lib.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). (6)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Code Coverage
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Lint and Test (ubuntu-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/current_lockfile.rscrates/lockfile/src/resolved_dependency/tests.rscrates/real-hoist/src/lib.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoist.rscrates/lockfile/src/resolved_dependency.rs
🧠 Learnings (5)
📚 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/current_lockfile.rscrates/lockfile/src/resolved_dependency/tests.rscrates/real-hoist/src/lib.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoist.rscrates/lockfile/src/resolved_dependency.rs
📚 Learning: 2026-05-13T22:52:32.579Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 506
File: crates/package-manager/src/link_bins.rs:238-241
Timestamp: 2026-05-13T22:52:32.579Z
Learning: In Rust code using `matches!` (or `match`) on a borrowed value (e.g., `meta: &PackageMetadata` and a field access like `meta.resolution`), it is safe to use wildcard/OR patterns such as `LockfileResolution::Binary(_) | LockfileResolution::Variations(_)` when the pattern does not bind the inner payload by value. `_` wildcard patterns (and similar discriminant-only patterns) should not move non-`Copy` data; they only test the variant and avoid ownership transfer. Do not flag these patterns as move-out-of-borrow/ownership compile errors. Only patterns that bind the inner value by value (e.g., `LockfileResolution::Binary(b)`) would require ownership and may trigger move errors when matching on data behind a borrow.
Applied to files:
crates/package-manager/src/current_lockfile.rscrates/lockfile/src/resolved_dependency/tests.rscrates/real-hoist/src/lib.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoist.rscrates/lockfile/src/resolved_dependency.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/current_lockfile.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoist.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/current_lockfile.rscrates/package-manager/src/hoisted_dep_graph.rscrates/package-manager/src/symlink_direct_dependencies.rscrates/package-manager/src/build_sequence.rscrates/package-manager/src/install_frozen_lockfile.rscrates/package-manager/src/hoist.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/resolved_dependency/tests.rs
🔇 Additional comments (3)
crates/package-manager/src/symlink_direct_dependencies.rs (3)
286-290: LGTM!Also applies to: 384-388
432-442: LGTM!
493-509: LGTM!
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.10383335702,
"stddev": 0.07768764951656557,
"median": 2.09595632002,
"user": 2.71021158,
"system": 2.1258466,
"min": 1.98747116452,
"max": 2.20310305752,
"times": [
2.1835280295199997,
2.0630377105199997,
2.04975676652,
2.04435337452,
2.12887492952,
1.98747116452,
2.19401812352,
2.0312261165199996,
2.20310305752,
2.1529642975199996
]
},
{
"command": "pacquet@main",
"mean": 2.11565507312,
"stddev": 0.05963161149050938,
"median": 2.12772454852,
"user": 2.67356298,
"system": 2.1114237,
"min": 1.99849250452,
"max": 2.22307523952,
"times": [
2.1419699845199998,
1.99849250452,
2.0508251545199996,
2.22307523952,
2.0995375845199997,
2.12595055752,
2.10850671452,
2.1294985395199997,
2.14678570752,
2.13190874452
]
},
{
"command": "pnpm",
"mean": 5.25272633352,
"stddev": 0.0657257778730102,
"median": 5.26612816602,
"user": 8.42044498,
"system": 2.7222306,
"min": 5.12955710452,
"max": 5.33374910552,
"times": [
5.31404574352,
5.211223324520001,
5.29599789552,
5.25631920152,
5.24635003852,
5.29661958352,
5.16746420752,
5.12955710452,
5.33374910552,
5.27593713052
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.51291967514,
"stddev": 0.028443161145883866,
"median": 0.5064603167399999,
"user": 0.36409388,
"system": 0.93864176,
"min": 0.49177933974000004,
"max": 0.58963413674,
"times": [
0.58963413674,
0.51543580774,
0.51339810674,
0.5032491247399999,
0.49815909974000006,
0.49295029474,
0.51646506074,
0.49177933974000004,
0.50967150874,
0.49845427174000007
]
},
{
"command": "pacquet@main",
"mean": 0.5211954433400001,
"stddev": 0.053047773474671656,
"median": 0.5011180747399999,
"user": 0.35974078,
"system": 0.9525933600000002,
"min": 0.49492100774000003,
"max": 0.66940975374,
"times": [
0.66940975374,
0.49492100774000003,
0.51070865574,
0.51331359374,
0.50026366474,
0.49672461974000004,
0.49753743674000006,
0.49917050974000005,
0.50197248474,
0.52793270674
]
},
{
"command": "pnpm",
"mean": 2.1816456790400003,
"stddev": 0.06735624930631078,
"median": 2.18097251274,
"user": 2.7463425799999994,
"system": 1.2909600599999997,
"min": 2.10373171074,
"max": 2.32308117874,
"times": [
2.32308117874,
2.10373171074,
2.12023365774,
2.23174467074,
2.11917943474,
2.15667987874,
2.20655642074,
2.13836199174,
2.21162269974,
2.20526514674
]
}
]
} |
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
Alias(PkgNameVerPeer)variant toImporterDepVersionso importerversion:fields written as<name>@<version>(e.g.version: '@zkochan/js-yaml@0.0.11') parse correctly. Previously these lockfiles failed withFailed to parse importer dependency version.ImporterDepVersion::resolved_key(importer_key)and route every consumer through it — the snapshot lookup, skipped check, reachability BFS, runtime exclusion, build-sequence root walk, hoisted-dep-graph builder, and both hoist passes. For aliases the resolved key is the alias's own(name, suffix), not(importer-map key, version).<modules_dir>/<importer-key>now points to<slot>/node_modules/<alias-real-name>and thepnpm:root addedevent reportsrealNameas the resolved package name.Upstream reference:
refToRelativein pnpm/pnpm@8a80235c7b/deps/path/src/index.ts#L96-L110.Test plan
cargo nextest run -p pacquet-lockfile resolved_dependency(11 tests including new alias parse + serde round-trip +resolved_keycases)cargo nextest run -p pacquet-lockfile -p pacquet-package-manager -p pacquet-real-hoist(395 tests)just ready(1237 tests, lint clean)taplo format --checkandjust dylintclean--frozen-lockfileerror is gone; a separate runtime-specifier check now fires, unrelated to this PR)Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit
Release Notes