From d08e9abfb9dc1d3c8f8a5104f7e56b46db94c337 Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Thu, 21 May 2026 01:30:32 +0200 Subject: [PATCH] fix(pacquet/lockfile): accept link: refs in snapshot dependencies (#11788) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lockfile): accept link: refs in snapshot dependencies An injected workspace package's snapshot can carry a `link:` dependency value when the dep is a workspace sibling. Pnpm accepts the shape — `refToRelative` short-circuits to `null` for `link:` — but pacquet's `SnapshotDepRef` only handled the plain/alias shapes and rejected `link:` at parse time. Add a `SnapshotDepRef::Link(String)` variant mirroring `ImporterDepVersion::Link`, return `None` from `resolve` / `ver_peer` for it, and skip it at every consumer (matching upstream's `if (childDepPath)` guards in `getChildren` / `lockfileDepsToGraphChildren`). Fixes https://github.com/pnpm/pnpm/issues/11775. * test(lockfile): trim redundant doc comments on link snapshot dep tests Drop test doc-comments that restated the assertion. The test name + asserts already convey the intent; the upstream-parity reason lives at the `SnapshotDepRef::resolve` / `Link` variant docs. --- .../lockfile/src/load_lockfile/tests.rs | 66 +++++++++++++++++++ .../lockfile/src/resolved_dependency.rs | 6 +- .../crates/lockfile/src/snapshot_dep_ref.rs | 59 ++++++++++++++--- .../lockfile/src/snapshot_dep_ref/tests.rs | 30 +++++++-- .../package-manager/src/build_sequence.rs | 4 +- .../src/create_symlink_layout.rs | 7 +- .../package-manager/src/current_lockfile.rs | 5 +- .../crates/package-manager/src/deps_graph.rs | 8 ++- pacquet/crates/package-manager/src/hoist.rs | 6 +- .../package-manager/src/hoisted_dep_graph.rs | 8 ++- .../src/install_frozen_lockfile.rs | 6 +- .../crates/package-manager/src/link_bins.rs | 6 +- .../src/virtual_store_layout.rs | 5 +- pacquet/crates/real-hoist/src/lib.rs | 9 ++- 14 files changed, 197 insertions(+), 28 deletions(-) diff --git a/pacquet/crates/lockfile/src/load_lockfile/tests.rs b/pacquet/crates/lockfile/src/load_lockfile/tests.rs index 49fc71d70a..454e67edcb 100644 --- a/pacquet/crates/lockfile/src/load_lockfile/tests.rs +++ b/pacquet/crates/lockfile/src/load_lockfile/tests.rs @@ -1,5 +1,6 @@ use crate::{ DirectoryResolution, ImporterDepVersion, Lockfile, LockfileResolution, PackageKey, PkgName, + SnapshotDepRef, }; use pretty_assertions::assert_eq; use tempfile::tempdir; @@ -215,3 +216,68 @@ snapshots: let snapshots = lockfile.snapshots.as_ref().expect("snapshots present"); assert!(snapshots.contains_key(&key)); } + +/// Regression test for . +/// An injected workspace package's snapshot can hold a `link:` +/// value in its `dependencies:` map when the dep is a workspace +/// sibling. Pnpm's own parser accepts the shape — `refToRelative` +/// short-circuits to `null` for `link:` references at use time — so +/// pacquet must too. +#[test] +fn parses_link_dep_in_injected_snapshot() { + let lockfile_text = text_block! { + "lockfileVersion: '9.0'" + "" + "settings:" + " autoInstallPeers: true" + " excludeLinksFromLockfile: false" + "" + "importers:" + "" + " .: {}" + "" + " packages/a:" + " dependencies:" + " b:" + " specifier: workspace:^" + " version: file:packages/b" + " dependenciesMeta:" + " b:" + " injected: true" + "" + " packages/b:" + " dependencies:" + " c:" + " specifier: workspace:^" + " version: link:../c" + "" + " packages/c: {}" + "" + "packages:" + "" + " b@file:packages/b:" + " resolution: {directory: packages/b, type: directory}" + "" + "snapshots:" + "" + " b@file:packages/b:" + " dependencies:" + " c: link:packages/c" + }; + let tmp = write_lockfile(lockfile_text); + let virtual_store_dir = tmp.path().join("node_modules").join(".pacquet"); + + let lockfile = Lockfile::load_current_from_virtual_store_dir(&virtual_store_dir) + .expect("load lockfile with link: snapshot dep") + .expect("lockfile should be present"); + + let snapshots = lockfile.snapshots.as_ref().expect("snapshots present"); + let b_key: PackageKey = "b@file:packages/b".parse().expect("parse b key"); + let b_snapshot = snapshots.get(&b_key).expect("b snapshot present"); + let deps = b_snapshot.dependencies.as_ref().expect("b deps present"); + + let c_name = PkgName::parse("c").expect("parse c"); + let c_ref = deps.get(&c_name).expect("c entry present"); + assert_eq!(c_ref, &SnapshotDepRef::Link("packages/c".to_string())); + assert_eq!(c_ref.resolve(&c_name), None); +} diff --git a/pacquet/crates/lockfile/src/resolved_dependency.rs b/pacquet/crates/lockfile/src/resolved_dependency.rs index bf1c4946ea..3c2d924775 100644 --- a/pacquet/crates/lockfile/src/resolved_dependency.rs +++ b/pacquet/crates/lockfile/src/resolved_dependency.rs @@ -48,8 +48,10 @@ pub struct ResolvedDependencySpec { /// /// Snapshot-level dependencies (the values inside `snapshots.*.dependencies`) /// use [`crate::SnapshotDepRef`] instead, which carries the same -/// plain/alias distinction but never holds a `link:` value — `link:` -/// only appears at the importer level. +/// plain / alias / link distinction. `link:` can appear at the snapshot +/// level too, for injected workspace packages whose own dependencies +/// resolve to other workspace projects (see +/// [`crate::SnapshotDepRef::Link`]). #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub enum ImporterDepVersion { /// Bare semver-with-peer; resolves to a snapshot in `snapshots:` diff --git a/pacquet/crates/lockfile/src/snapshot_dep_ref.rs b/pacquet/crates/lockfile/src/snapshot_dep_ref.rs index 53ae4178e7..3fe583b0e6 100644 --- a/pacquet/crates/lockfile/src/snapshot_dep_ref.rs +++ b/pacquet/crates/lockfile/src/snapshot_dep_ref.rs @@ -6,7 +6,7 @@ use std::{borrow::Cow, str::FromStr}; /// Value of a single entry in [`SnapshotEntry::dependencies`](crate::SnapshotEntry::dependencies) /// (or `optional_dependencies`). /// -/// A snapshot dependency can be written in one of two forms: +/// A snapshot dependency can be written in one of three forms: /// /// * A bare version with an optional peer-dependency suffix — the dependency /// resolves to `@` in the `snapshots:` map. @@ -27,9 +27,24 @@ use std::{borrow::Cow, str::FromStr}; /// string-width-cjs: string-width@4.2.3 /// ``` /// -/// Detection mirrors pnpm's `refToRelative`: a reference is an alias when a -/// package name appears before the version separator (either the first `@` -/// occurs before any `(` and `:`, or the reference begins with `@`). +/// * A `link:` value — the dependency is a workspace sibling at +/// `` relative to the lockfile root. Pnpm writes this shape for +/// injected workspace packages whose own dependencies resolve to other +/// workspace projects (and for which `excludeLinksFromLockfile` is off); +/// the dependency lives outside the virtual store and gets a direct +/// directory symlink at install time. +/// +/// ```yaml +/// b@file:packages/b: +/// dependencies: +/// c: link:packages/c +/// ``` +/// +/// Detection mirrors pnpm's `refToRelative`: a reference starting with +/// `link:` short-circuits to the link variant (and `refToRelative` returns +/// `null`); a reference is an alias when a package name appears before the +/// version separator (either the first `@` occurs before any `(` and `:`, +/// or the reference begins with `@`). /// /// Reference: #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -37,26 +52,46 @@ use std::{borrow::Cow, str::FromStr}; pub enum SnapshotDepRef { Plain(PkgVerPeer), Alias(PkgNameVerPeer), + Link(String), } impl SnapshotDepRef { /// Resolve this reference to the `snapshots:` / `packages:` key it points /// to. `alias_name` is the key of the dependency entry (the name under /// which the package is linked into `node_modules`). - pub fn resolve(&self, alias_name: &PkgName) -> PkgNameVerPeer { + /// + /// Returns `None` for [`SnapshotDepRef::Link`] entries — `link:` deps + /// don't live in the virtual store and have no snapshot key. Mirrors + /// upstream's `refToRelative` returning `null` for `link:` references. + pub fn resolve(&self, alias_name: &PkgName) -> Option { match self { SnapshotDepRef::Plain(ver_peer) => { - PkgNameVerPeer::new(alias_name.clone(), ver_peer.clone()) + Some(PkgNameVerPeer::new(alias_name.clone(), ver_peer.clone())) } - SnapshotDepRef::Alias(key) => key.clone(), + SnapshotDepRef::Alias(key) => Some(key.clone()), + SnapshotDepRef::Link(_) => None, } } /// Accessor for the version-with-peer part of this reference. - pub fn ver_peer(&self) -> &'_ PkgVerPeer { + /// + /// Returns `None` for [`SnapshotDepRef::Link`] entries — a `link:` dep + /// has no version slot. + pub fn ver_peer(&self) -> Option<&'_ PkgVerPeer> { match self { - SnapshotDepRef::Plain(ver_peer) => ver_peer, - SnapshotDepRef::Alias(key) => &key.suffix, + SnapshotDepRef::Plain(ver_peer) => Some(ver_peer), + SnapshotDepRef::Alias(key) => Some(&key.suffix), + SnapshotDepRef::Link(_) => None, + } + } + + /// `Some(target)` when this reference is a `link:` workspace sibling; + /// `None` otherwise. The returned string is the path portion *without* + /// the `link:` prefix. + pub fn as_link_target(&self) -> Option<&'_ str> { + match self { + SnapshotDepRef::Plain(_) | SnapshotDepRef::Alias(_) => None, + SnapshotDepRef::Link(target) => Some(target.as_str()), } } } @@ -66,6 +101,7 @@ impl std::fmt::Display for SnapshotDepRef { match self { SnapshotDepRef::Plain(ver_peer) => ver_peer.fmt(f), SnapshotDepRef::Alias(key) => key.fmt(f), + SnapshotDepRef::Link(target) => write!(f, "link:{target}"), } } } @@ -97,6 +133,9 @@ pub enum ParseSnapshotDepRefError { impl FromStr for SnapshotDepRef { type Err = ParseSnapshotDepRefError; fn from_str(value: &str) -> Result { + if let Some(target) = value.strip_prefix("link:") { + return Ok(SnapshotDepRef::Link(target.to_string())); + } if looks_like_alias(value) { let key = value.parse::().map_err(ParseSnapshotDepRefError::ParseAlias)?; diff --git a/pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs b/pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs index f40c30ed54..d992176d3c 100644 --- a/pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs +++ b/pacquet/crates/lockfile/src/snapshot_dep_ref/tests.rs @@ -47,17 +47,23 @@ fn parse_alias_with_peer_suffix() { #[test] fn resolve_plain_uses_alias_key_as_target_name() { let dep: SnapshotDepRef = "5.1.2".parse().unwrap(); - let resolved = dep.resolve(&pkg_name("string-width")); + let resolved = dep.resolve(&pkg_name("string-width")).expect("plain resolves"); assert_eq!(resolved.to_string(), "string-width@5.1.2"); } #[test] fn resolve_alias_uses_alias_target_name_not_key() { let dep: SnapshotDepRef = "string-width@4.2.3".parse().unwrap(); - let resolved = dep.resolve(&pkg_name("string-width-cjs")); + let resolved = dep.resolve(&pkg_name("string-width-cjs")).expect("alias resolves"); assert_eq!(resolved.to_string(), "string-width@4.2.3"); } +#[test] +fn resolve_link_returns_none() { + let dep: SnapshotDepRef = "link:packages/c".parse().unwrap(); + assert_eq!(dep.resolve(&pkg_name("c")), None); +} + #[test] fn display_roundtrip() { for input in [ @@ -66,6 +72,8 @@ fn display_roundtrip() { "string-width@4.2.3", "@types/react@17.0.49", "react-dom@17.0.2(react@17.0.2)", + "link:packages/c", + "link:../sibling", ] { let dep: SnapshotDepRef = input.parse().unwrap(); assert_eq!(dep.to_string(), input); @@ -78,12 +86,20 @@ fn deserialize_ok() { ("5.1.2", "5.1.2"), ("string-width@4.2.3", "string-width@4.2.3"), (r#""17.0.2(react@17.0.2)""#, "17.0.2(react@17.0.2)"), + ("link:packages/c", "link:packages/c"), ] { let dep: SnapshotDepRef = serde_saphyr::from_str(yaml).unwrap(); assert_eq!(dep.to_string(), expected); } } +#[test] +fn parse_link_workspace_path() { + let dep: SnapshotDepRef = "link:packages/c".parse().unwrap(); + assert_eq!(dep, SnapshotDepRef::Link("packages/c".to_string())); + assert_eq!(dep.as_link_target(), Some("packages/c")); +} + #[test] fn looks_like_alias_rules() { for (input, expected) in [ @@ -104,14 +120,18 @@ fn looks_like_alias_rules() { /// `ver_peer` accesses the version-with-peer portion of either /// variant: the inner `PkgVerPeer` for `Plain`, and the alias's /// `suffix` for `Alias`. Used by snapshot lookups that only care -/// about the version slot. +/// about the version slot. Returns `None` for the `Link` variant — +/// `link:` deps have no version slot. #[test] fn ver_peer_returns_inner_version_for_each_variant() { let plain: SnapshotDepRef = "17.0.2(react@17.0.2)".parse().unwrap(); - assert_eq!(plain.ver_peer().to_string(), "17.0.2(react@17.0.2)"); + assert_eq!(plain.ver_peer().map(ToString::to_string), Some("17.0.2(react@17.0.2)".to_string())); let alias: SnapshotDepRef = "react-dom@17.0.2(react@17.0.2)".parse().unwrap(); - assert_eq!(alias.ver_peer().to_string(), "17.0.2(react@17.0.2)"); + assert_eq!(alias.ver_peer().map(ToString::to_string), Some("17.0.2(react@17.0.2)".to_string())); + + let link: SnapshotDepRef = "link:packages/c".parse().unwrap(); + assert_eq!(link.ver_peer(), None); } /// `From` is the infallible promotion path used by diff --git a/pacquet/crates/package-manager/src/build_sequence.rs b/pacquet/crates/package-manager/src/build_sequence.rs index 1f7dbb90a9..4cfff0ccbb 100644 --- a/pacquet/crates/package-manager/src/build_sequence.rs +++ b/pacquet/crates/package-manager/src/build_sequence.rs @@ -102,7 +102,9 @@ fn build_children_map( [snap.dependencies.as_ref(), snap.optional_dependencies.as_ref()].into_iter().flatten() { for (alias, dep_ref) in deps { - let resolved = dep_ref.resolve(alias); + let Some(resolved) = dep_ref.resolve(alias) else { + continue; + }; if snapshots.contains_key(&resolved) { child_keys.push(resolved); } diff --git a/pacquet/crates/package-manager/src/create_symlink_layout.rs b/pacquet/crates/package-manager/src/create_symlink_layout.rs index 34655c0241..84f6fc1aa2 100644 --- a/pacquet/crates/package-manager/src/create_symlink_layout.rs +++ b/pacquet/crates/package-manager/src/create_symlink_layout.rs @@ -60,7 +60,12 @@ pub fn create_symlink_layout( if alias_name == self_name { return Ok(()); } - let target = dep_ref.resolve(alias_name); + // `link:` deps point at a workspace sibling outside the + // virtual store; the symlink-direct-dependencies stage + // installs those for the importer, not here. + let Some(target) = dep_ref.resolve(alias_name) else { + return Ok(()); + }; if skipped.contains(&target) { return Ok(()); } diff --git a/pacquet/crates/package-manager/src/current_lockfile.rs b/pacquet/crates/package-manager/src/current_lockfile.rs index 71438fcf4e..f8f6e3ad92 100644 --- a/pacquet/crates/package-manager/src/current_lockfile.rs +++ b/pacquet/crates/package-manager/src/current_lockfile.rs @@ -211,7 +211,10 @@ fn resolve_child( snapshots: &HashMap, skipped: &SkippedSnapshots, ) -> Option { - let resolved = dep_ref.resolve(alias); + // `link:` deps live outside the virtual store and have no + // snapshot to reach — they aren't part of the reachable-snapshot + // graph this helper computes. + let resolved = dep_ref.resolve(alias)?; if skipped.contains(&resolved) { return None; } diff --git a/pacquet/crates/package-manager/src/deps_graph.rs b/pacquet/crates/package-manager/src/deps_graph.rs index f33b9e4eee..2d03402b79 100644 --- a/pacquet/crates/package-manager/src/deps_graph.rs +++ b/pacquet/crates/package-manager/src/deps_graph.rs @@ -150,8 +150,12 @@ fn build_children(snapshot: &SnapshotEntry) -> HashMap { for (alias, dep_ref) in dep_entries { // `SnapshotDepRef::resolve` returns the `PkgNameVerPeer` // (= `PackageKey`) the alias points at in the `snapshots:` - // map. - let resolved: PackageKey = dep_ref.resolve(alias); + // map. `link:` deps don't have a snapshot key — skip them, + // mirroring upstream's `if (childDepPath)` guard in + // `lockfileDepsToGraphChildren`. + let Some(resolved) = dep_ref.resolve(alias) else { + continue; + }; children.insert(alias.to_string(), resolved); } children diff --git a/pacquet/crates/package-manager/src/hoist.rs b/pacquet/crates/package-manager/src/hoist.rs index ec6896c6a2..bee30a064f 100644 --- a/pacquet/crates/package-manager/src/hoist.rs +++ b/pacquet/crates/package-manager/src/hoist.rs @@ -86,7 +86,11 @@ pub fn build_hoist_graph( .flat_map(|m| m.iter()) .chain(snapshot.optional_dependencies.iter().flat_map(|m| m.iter())); let children: HashMap = dep_entries - .map(|(alias, dep_ref)| (alias.to_string(), dep_ref.resolve(alias))) + // `dep_ref.resolve` is `None` for `link:` deps — + // workspace siblings that live outside the virtual + // store. Mirrors upstream's `if (childDepPath)` + // check in `getChildren`. + .filter_map(|(alias, dep_ref)| Some((alias.to_string(), dep_ref.resolve(alias)?))) .collect(); Some(( key.clone(), diff --git a/pacquet/crates/package-manager/src/hoisted_dep_graph.rs b/pacquet/crates/package-manager/src/hoisted_dep_graph.rs index f683cc5d8f..55b2839c9a 100644 --- a/pacquet/crates/package-manager/src/hoisted_dep_graph.rs +++ b/pacquet/crates/package-manager/src/hoisted_dep_graph.rs @@ -816,7 +816,13 @@ fn compute_children( .flatten() .chain(snapshot.optional_dependencies.iter().flatten()); for (alias_name, dep_ref) in dep_iter { - let child_key = dep_ref.resolve(alias_name); + // `link:` deps return `None` here — they live outside the + // virtual store and don't show up in `pkg_locations`. + // Mirrors upstream's `if (childDepPath && pkgLocations...)` + // guard in `getChildren`. + let Some(child_key) = dep_ref.resolve(alias_name) else { + continue; + }; let child_dep_path = child_key.to_string(); if let Some(locations) = pkg_locations.get(&child_dep_path) && let Some(first) = locations.first() diff --git a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs index 27537c3532..073a305b69 100644 --- a/pacquet/crates/package-manager/src/install_frozen_lockfile.rs +++ b/pacquet/crates/package-manager/src/install_frozen_lockfile.rs @@ -1334,7 +1334,11 @@ pub(crate) fn find_own_runtime_node_major(snapshot: &SnapshotEntry) -> Option = children .filter_map(|(alias, dep_ref)| { - let child_key = dep_ref.resolve(alias); + // `link:` deps live outside the virtual store and + // expose their bins via the workspace project's + // own `package.json`, not through a snapshot — skip + // them here. + let child_key = dep_ref.resolve(alias)?; let metadata_key = child_key.without_peer(); let keep = match has_bin_set { Some(set) => set.contains(&metadata_key), diff --git a/pacquet/crates/package-manager/src/virtual_store_layout.rs b/pacquet/crates/package-manager/src/virtual_store_layout.rs index 3722d56a23..a5dcb21b68 100644 --- a/pacquet/crates/package-manager/src/virtual_store_layout.rs +++ b/pacquet/crates/package-manager/src/virtual_store_layout.rs @@ -342,7 +342,10 @@ fn merge_into_children( deps: &HashMap, ) { for (alias, dep_ref) in deps { - let resolved = dep_ref.resolve(alias); + // `link:` deps have no snapshot key — skip them. + let Some(resolved) = dep_ref.resolve(alias) else { + continue; + }; children.insert(alias.to_string(), resolved); } } diff --git a/pacquet/crates/real-hoist/src/lib.rs b/pacquet/crates/real-hoist/src/lib.rs index c1a911ad75..acb75537e4 100644 --- a/pacquet/crates/real-hoist/src/lib.rs +++ b/pacquet/crates/real-hoist/src/lib.rs @@ -501,7 +501,14 @@ fn collect_snapshot_deps( // snapshot lookup hits the right entry. The node's exposed // `name` stays `alias`; only the lookup uses the resolved // target name. - let dep_key = dep_ref.resolve(alias); + // + // `link:` deps return `None` — they have no snapshot to + // hoist (the install layer materialises them as direct + // directory symlinks), so we skip them here, mirroring + // upstream's `if (childDepPath)` check in `getChildren`. + let Some(dep_key) = dep_ref.resolve(alias) else { + continue; + }; let node = build_dep_node(alias, &dep_key, lockfile, opts, nodes)?; out.insert(RcByPtr(node)); }