Fix npm alias links missed from a member (#1494)

crates/socket-patch-cli/src/commands/vex_consumed.rs

@@ -641,17 +641,21 @@ mod tests {
             ..GlobalArgs::default()
         };
         let purl = "pkg:npm/left-pad@1.3.0".to_string();
-        // The resolver never takes a link as a copy.
-        assert!(installed_copies(&common, &purl)
+        let mut expected = vec![alias.clone(), peers[1].clone()];
+        expected.sort();
+        // The resolver takes a root link only for a purl with no copy
+        // (#1494), and expands it to the same copies.
+        let mut resolved = installed_copies(&common, &purl)
             .await
-            .get(&purl)
-            .is_none_or(Vec::is_empty));
+            .remove(&purl)
+            .unwrap();
+        resolved.sort();
+        assert_eq!(resolved, expected, "the resolver's own copy set");
+        // An empty installed entry still falls back to the alias identity.
         let installed = HashMap::from([(purl, Vec::new())]);
         let (mut paths, calls) = tracked_npm_hosted(&common, &installed).await;
-        assert_eq!(calls, vec![vec![alias.clone()]]);
-        let mut expected = vec![alias, peers[1].clone()];
+        assert_eq!(calls, vec![vec![alias]]);
         paths.sort();
-        expected.sort();
         assert_eq!(paths, expected);
     }
 
@@ -746,9 +750,12 @@ mod tests {
             &purls[0],
         )
         .await;
-        assert!(
-            resolved.get(&purls[0]).is_none_or(Vec::is_empty),
-            "the resolver skips symlinks: {resolved:?}"
+        // The crawler skips symlinks; the resolver takes the root link
+        // only because the purl has no other copy (#1494).
+        assert_eq!(
+            resolved.get(&purls[0]),
+            Some(&vec![root.join("node_modules/lp")]),
+            "{resolved:?}"
         );
 
         let (_, _, _, snapshot) =

crates/socket-patch-cli/src/ecosystem_dispatch.rs

@@ -619,8 +619,17 @@ pub async fn find_all_packages_for_purls(
     // `merge_qualified`'s `push_path`. Cargo keeps every registry index
     // copy (`merge_cargo_copies`); the single-copy ecosystems keep true
     // first-wins via their own `merge_first_wins` wiring in
-    // `dispatch_find`.
-    dispatch_find(partitioned, options, silent, merge_variant_copies, None).await
+    // `dispatch_find`. An npm purl with no copy can still be reached
+    // through a workspace member's alias link (#1494).
+    dispatch_find_with_npm_links(
+        partitioned,
+        options,
+        silent,
+        merge_variant_copies,
+        None,
+        NpmLinks::ForUnresolved,
+    )
+    .await
 }
 
 /// Multi-copy variant of `find_packages_for_rollback` (qualified-aware
@@ -634,42 +643,88 @@ pub async fn find_all_packages_for_rollback(
 }
 
 /// [`find_all_packages_for_rollback`], taking the npm `node_modules` roots
-/// from `prior` as [`find_packages_for_rollback_reusing`] does.
+/// from `prior` as [`find_packages_for_rollback_reusing`] does. An npm purl
+/// with no copy can still be reached through a workspace member's alias
+/// link (#1494), as for apply.
 pub async fn find_all_packages_for_rollback_reusing(
     partitioned: &HashMap<Ecosystem, Vec<String>>,
     options: &CrawlerOptions,
     silent: bool,
     prior: Option<&NpmCrawlSnapshot>,
 ) -> HashMap<String, Vec<PathBuf>> {
-    let npm_roots = prior
-        .filter(|p| p.taken_with(options))
-        .map(|p| p.roots.as_slice());
-    dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await
+    dispatch_find_with_npm_links(
+        partitioned,
+        options,
+        silent,
+        merge_qualified,
+        prior,
+        NpmLinks::ForUnresolved,
+    )
+    .await
 }
 
 /// The install paths that reach each purl, for `scan`'s and `rollback`'s
-/// path targets: [`find_all_packages_for_rollback_reusing`]'s physical
-/// copies, plus each npm package link directly in a `node_modules` root
-/// (scoped ones one level down) whose target's `package.json` names one
-/// of the npm purls asked for.
+/// path targets: [`find_all_packages_for_rollback_reusing`]'s copies plus
+/// every matching npm root link, not only those of purls with no copy
+/// (#1266). The result is for path matching only: it can name one
+/// physical copy more than once.
+pub async fn find_package_paths_for_path_scope(
+    partitioned: &HashMap<Ecosystem, Vec<String>>,
+    options: &CrawlerOptions,
+    silent: bool,
+    prior: Option<&NpmCrawlSnapshot>,
+) -> HashMap<String, Vec<PathBuf>> {
+    dispatch_find_with_npm_links(
+        partitioned,
+        options,
+        silent,
+        merge_qualified,
+        prior,
+        NpmLinks::All,
+    )
+    .await
+}
+
+/// Which npm root links [`dispatch_find_with_npm_links`] adds.
+#[derive(Clone, Copy, PartialEq, Eq)]
+enum NpmLinks {
+    /// Every link to a purl asked for: path scoping matches by install
+    /// path, so each spelling counts.
+    All,
+    /// Only links to purls the resolver found no copy of. The commands
+    /// that act per copy already reach every store peer variant of a
+    /// found copy through the apply engine's fan-out, so a link to one
+    /// would be a second visit (#633); and a link into first-party source
+    /// beside a found registry copy must not turn into a refusal.
+    ForUnresolved,
+}
+
+/// [`dispatch_find`]'s copies, plus each npm package link directly in a
+/// `node_modules` root (scoped ones one level down) whose target's
+/// `package.json` names one of the npm purls asked for (see [`NpmLinks`]
+/// for which), after the copies.
 ///
 /// The crawler records a plain-name link at its own path
 /// (`packages/a/node_modules/ms`), but never an alias link
 /// (`packages/a/node_modules/lp -> …/.pnpm/left-pad@1.3.0/node_modules/left-pad`):
-/// links are not physical copies, and apply and VEX must see each copy
-/// once (#856). Path scoping asks which paths install the package
-/// instead, and pnpm's isolated linker or npm's `install-strategy=linked`
-/// reach a workspace member's ALIAS dependency only through such a link
-/// (#1266). A link is matched by its target's own identity, not against
-/// the crawled copies, so a link to a store peer variant the crawl
-/// skipped still counts, and the cost is one pass over the root links
-/// however many purls are asked for. The result is for path matching
-/// only: it can name one physical copy more than once.
-pub async fn find_package_paths_for_path_scope(
+/// links are not physical copies, and the resolver must see each copy
+/// once (#856). Yet pnpm's isolated linker, vlt and npm's
+/// `install-strategy=linked` reach a workspace member's ALIAS dependency
+/// only through such a link. Path scoping needs it to select the member
+/// (#1266), and a run from inside the member, whose `node_modules` is the
+/// only root, has no other route to the copy at all (#1494). A link is
+/// matched by its target's own identity, not against the crawled copies,
+/// so a link to a store peer variant the crawl skipped still counts, and
+/// the cost is one pass over the root links however many purls are asked
+/// for (none when every purl already has a copy and only the unresolved
+/// ones' links are wanted).
+async fn dispatch_find_with_npm_links(
     partitioned: &HashMap<Ecosystem, Vec<String>>,
     options: &CrawlerOptions,
     silent: bool,
+    variant_merge: MergeFn,
     prior: Option<&NpmCrawlSnapshot>,
+    links: NpmLinks,
 ) -> HashMap<String, Vec<PathBuf>> {
     let mut wanted: HashMap<PurlKey, Vec<String>> = HashMap::new();
     for purl in partitioned.get(&Ecosystem::Npm).into_iter().flatten() {
@@ -687,13 +742,23 @@ pub async fn find_package_paths_for_path_scope(
         partitioned,
         options,
         silent,
-        merge_qualified,
+        variant_merge,
         npm_roots.as_deref(),
     )
     .await;
     let Some(roots) = npm_roots else {
         return found;
     };
+    if links == NpmLinks::ForUnresolved {
+        let resolved = |purl: &String| found.get(purl).is_some_and(|paths| !paths.is_empty());
+        wanted.retain(|_, purls| {
+            purls.retain(|purl| !resolved(purl));
+            !purls.is_empty()
+        });
+        if wanted.is_empty() {
+            return found;
+        }
+    }
     // A failed enumeration only loses the extra links; the copies stand.
     let links = tokio::task::spawn_blocking(move || npm_links_to(&wanted, &roots))
         .await
@@ -794,17 +859,25 @@ pub async fn find_packages_for_rollback(
 /// tree nothing has touched since) instead of walking the tree for them
 /// again. Only the root discovery is reused: each root is still searched
 /// by `find_by_purls`, so copy choice and order are unchanged. A snapshot
-/// taken with other options is ignored.
+/// taken with other options is ignored. An npm purl with no copy can still
+/// be reached through a workspace member's alias link (#1494).
 pub async fn find_packages_for_rollback_reusing(
     partitioned: &HashMap<Ecosystem, Vec<String>>,
     options: &CrawlerOptions,
     silent: bool,
     prior: Option<&NpmCrawlSnapshot>,
 ) -> HashMap<String, PathBuf> {
-    let npm_roots = prior
-        .filter(|p| p.taken_with(options))
-        .map(|p| p.roots.as_slice());
-    collapse_to_first(dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await)
+    collapse_to_first(
+        dispatch_find_with_npm_links(
+            partitioned,
+            options,
+            silent,
+            merge_qualified,
+            prior,
+            NpmLinks::ForUnresolved,
+        )
+        .await,
+    )
 }
 
 /// The npm half of one [`crawl_ecosystems_with_npm`] run: the packages
@@ -977,6 +1050,9 @@ pub(crate) fn npm_paths_by_identity_in(
 /// [`find_packages_for_rollback_reusing`]. Only the root discovery is
 /// reused: each root is still searched by `find_by_purls`, so copy choice
 /// and order are unchanged. A snapshot taken with other options is ignored.
+/// An npm purl with no copy can still be reached through a workspace
+/// member's alias link (#1494), so `apply --check` and `vex` see the copy
+/// `apply` patched.
 ///
 /// Read-only by contract (its one caller is `vex`), so it also searches the
 /// gem stores under a refused out-of-tree `.bundle/config` path
@@ -990,13 +1066,13 @@ pub async fn find_manifest_package_copies_reusing(
 ) -> HashMap<String, Vec<PathBuf>> {
     let partitioned = partition_purls(purls, common.ecosystems.as_deref());
     let crawler_options = common.crawler_options();
-    let npm_roots = prior.and_then(|p| p.roots_for(&crawler_options));
-    let mut copies = dispatch_find(
+    let mut copies = dispatch_find_with_npm_links(
         &partitioned,
         &crawler_options,
         quiet,
         merge_qualified,
-        npm_roots,
+        prior,
+        NpmLinks::ForUnresolved,
     )
     .await;
     // `apply` also writes every store variant of each npm copy (a pnpm peer
@@ -2058,6 +2134,59 @@ mod tests {
         assert_eq!(single.get("pkg:npm/dup@1.0.0"), Some(&root_copy));
     }
 
+    /// #1494: run from a workspace member, a member's alias link into the
+    /// root store (`packages/a/node_modules/lp`, vlt's layout here) is the
+    /// only route to the copy, so the copy lookups must return it; a link
+    /// to another version must not count. From the root the store copy is
+    /// found and the link is not added: the apply engine's store fan-out
+    /// already reaches a link's peer variant, and a second visit would be
+    /// #633's phantom `already_patched`. Path scoping still sees it.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn multi_copy_lookups_take_a_member_alias_link_into_the_root_store() {
+        let tmp = tempfile::tempdir().unwrap();
+        let root = tmp.path();
+        let store = root.join("node_modules/.vlt");
+        let store_copy = write_npm_package(&store.join("~npm~left-pad@1.3.0"), "left-pad", "1.3.0");
+        write_npm_package(&store.join("~npm~left-pad@1.2.0"), "left-pad", "1.2.0");
+        let member_nm = root.join("packages/a/node_modules");
+        std::fs::create_dir_all(&member_nm).unwrap();
+        std::os::unix::fs::symlink(
+            "../../../node_modules/.vlt/~npm~left-pad@1.3.0/node_modules/left-pad",
+            member_nm.join("lp"),
+        )
+        .unwrap();
+        std::os::unix::fs::symlink(
+            "../../../node_modules/.vlt/~npm~left-pad@1.2.0/node_modules/left-pad",
+            member_nm.join("old"),
+        )
+        .unwrap();
+        let purl = "pkg:npm/left-pad@1.3.0".to_string();
+        let partitioned = partition_purls(std::slice::from_ref(&purl), None);
+
+        let member = local_options(root.join("packages/a"));
+        let link = vec![member_nm.join("lp")];
+        let applied = find_all_packages_for_purls(&partitioned, &member, true).await;
+        assert_eq!(applied.get(&purl), Some(&link), "{applied:?}");
+        let rolled = find_all_packages_for_rollback(&partitioned, &member, true).await;
+        assert_eq!(rolled.get(&purl), Some(&link), "{rolled:?}");
+        let one = find_packages_for_rollback(&partitioned, &member, true).await;
+        assert_eq!(one.get(&purl), Some(&link[0]), "{one:?}");
+
+        let at_root = local_options(root.to_path_buf());
+        let store_only = vec![store_copy.clone()];
+        let applied = find_all_packages_for_purls(&partitioned, &at_root, true).await;
+        assert_eq!(applied.get(&purl), Some(&store_only), "{applied:?}");
+        let rolled = find_all_packages_for_rollback(&partitioned, &at_root, true).await;
+        assert_eq!(rolled.get(&purl), Some(&store_only), "{rolled:?}");
+        let scoped = find_package_paths_for_path_scope(&partitioned, &at_root, true, None).await;
+        assert_eq!(
+            scoped.get(&purl),
+            Some(&vec![store_copy, member_nm.join("lp")]),
+            "{scoped:?}"
+        );
+    }
+
     /// Multi-copy for gem (mirrors the npm test above): bundler's scoped
     /// (`<engine>/<abi>/gems`) and flat (`gems/`) store layouts coexist under
     /// one `vendor/bundle` root — a bundler-2 `--path` install beside a