Skip to content

Commit f7ef1f3

Browse files
Fix npm alias links missed from a member (#1494) (#1500)
* Start fix for #1494 Assisted-by: Claude Code:claude-opus-5-5 * Patch npm alias links from a workspace member Run from inside a vlt, pnpm or npm-linked workspace member, apply reported a member's npm alias dependency ("lp": "npm:left-pad@1.3.0") as package_not_installed and exited 1, and rollback could not restore it, although scan had offered the patch. The member's node_modules is the only root walked there, and its alias entry is only a link into the root store, which the resolver never takes as a copy. Apply's and rollback's lookups now add the root links whose target is a requested package, the same pass path scoping got in #1470. Both commands already act on each physical copy once, so a run from the root still patches the store copy a single time. Fixes #1494 Assisted-by: Claude Code:claude-opus-5-5 * Skip alias links for already-found packages Apply, rollback, get and vendor now take a workspace member's alias link only for a package they found no other copy of. When a copy was found, apply already patches every store variant of it, so taking the link as well visited one variant twice and reported a phantom already_patched (or already-original on rollback). It also kept a first-party alias link beside a patched registry copy from turning a clean run into a refusal, and skips the link pass whenever every package resolves. Path-scoped scan and rollback still match every link. Assisted-by: Claude Code:claude-opus-5-5 * Let apply --check and vex see member alias links Run from a workspace member, apply now patches an alias dependency through the member's link into the root store, but apply --check and vex looked copies up without that link. The check reported the package as not installed and exited 0. They now use the same lookup as apply, which takes a root link only for a package with no other copy. Two vex_consumed tests assumed the resolver never returns a link. They now check that it returns the same copies the identity fallback finds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0195pcxXZ2GXtByX1L8egMhJ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 422e8c6 commit f7ef1f3

3 files changed

Lines changed: 398 additions & 40 deletions

File tree

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

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -641,17 +641,21 @@ mod tests {
641641
..GlobalArgs::default()
642642
};
643643
let purl = "pkg:npm/left-pad@1.3.0".to_string();
644-
// The resolver never takes a link as a copy.
645-
assert!(installed_copies(&common, &purl)
644+
let mut expected = vec![alias.clone(), peers[1].clone()];
645+
expected.sort();
646+
// The resolver takes a root link only for a purl with no copy
647+
// (#1494), and expands it to the same copies.
648+
let mut resolved = installed_copies(&common, &purl)
646649
.await
647-
.get(&purl)
648-
.is_none_or(Vec::is_empty));
650+
.remove(&purl)
651+
.unwrap();
652+
resolved.sort();
653+
assert_eq!(resolved, expected, "the resolver's own copy set");
654+
// An empty installed entry still falls back to the alias identity.
649655
let installed = HashMap::from([(purl, Vec::new())]);
650656
let (mut paths, calls) = tracked_npm_hosted(&common, &installed).await;
651-
assert_eq!(calls, vec![vec![alias.clone()]]);
652-
let mut expected = vec![alias, peers[1].clone()];
657+
assert_eq!(calls, vec![vec![alias]]);
653658
paths.sort();
654-
expected.sort();
655659
assert_eq!(paths, expected);
656660
}
657661

@@ -746,9 +750,12 @@ mod tests {
746750
&purls[0],
747751
)
748752
.await;
749-
assert!(
750-
resolved.get(&purls[0]).is_none_or(Vec::is_empty),
751-
"the resolver skips symlinks: {resolved:?}"
753+
// The crawler skips symlinks; the resolver takes the root link
754+
// only because the purl has no other copy (#1494).
755+
assert_eq!(
756+
resolved.get(&purls[0]),
757+
Some(&vec![root.join("node_modules/lp")]),
758+
"{resolved:?}"
752759
);
753760

754761
let (_, _, _, snapshot) =

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

Lines changed: 159 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -619,8 +619,17 @@ pub async fn find_all_packages_for_purls(
619619
// `merge_qualified`'s `push_path`. Cargo keeps every registry index
620620
// copy (`merge_cargo_copies`); the single-copy ecosystems keep true
621621
// first-wins via their own `merge_first_wins` wiring in
622-
// `dispatch_find`.
623-
dispatch_find(partitioned, options, silent, merge_variant_copies, None).await
622+
// `dispatch_find`. An npm purl with no copy can still be reached
623+
// through a workspace member's alias link (#1494).
624+
dispatch_find_with_npm_links(
625+
partitioned,
626+
options,
627+
silent,
628+
merge_variant_copies,
629+
None,
630+
NpmLinks::ForUnresolved,
631+
)
632+
.await
624633
}
625634

626635
/// Multi-copy variant of `find_packages_for_rollback` (qualified-aware
@@ -634,42 +643,88 @@ pub async fn find_all_packages_for_rollback(
634643
}
635644

636645
/// [`find_all_packages_for_rollback`], taking the npm `node_modules` roots
637-
/// from `prior` as [`find_packages_for_rollback_reusing`] does.
646+
/// from `prior` as [`find_packages_for_rollback_reusing`] does. An npm purl
647+
/// with no copy can still be reached through a workspace member's alias
648+
/// link (#1494), as for apply.
638649
pub async fn find_all_packages_for_rollback_reusing(
639650
partitioned: &HashMap<Ecosystem, Vec<String>>,
640651
options: &CrawlerOptions,
641652
silent: bool,
642653
prior: Option<&NpmCrawlSnapshot>,
643654
) -> HashMap<String, Vec<PathBuf>> {
644-
let npm_roots = prior
645-
.filter(|p| p.taken_with(options))
646-
.map(|p| p.roots.as_slice());
647-
dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await
655+
dispatch_find_with_npm_links(
656+
partitioned,
657+
options,
658+
silent,
659+
merge_qualified,
660+
prior,
661+
NpmLinks::ForUnresolved,
662+
)
663+
.await
648664
}
649665

650666
/// The install paths that reach each purl, for `scan`'s and `rollback`'s
651-
/// path targets: [`find_all_packages_for_rollback_reusing`]'s physical
652-
/// copies, plus each npm package link directly in a `node_modules` root
653-
/// (scoped ones one level down) whose target's `package.json` names one
654-
/// of the npm purls asked for.
667+
/// path targets: [`find_all_packages_for_rollback_reusing`]'s copies plus
668+
/// every matching npm root link, not only those of purls with no copy
669+
/// (#1266). The result is for path matching only: it can name one
670+
/// physical copy more than once.
671+
pub async fn find_package_paths_for_path_scope(
672+
partitioned: &HashMap<Ecosystem, Vec<String>>,
673+
options: &CrawlerOptions,
674+
silent: bool,
675+
prior: Option<&NpmCrawlSnapshot>,
676+
) -> HashMap<String, Vec<PathBuf>> {
677+
dispatch_find_with_npm_links(
678+
partitioned,
679+
options,
680+
silent,
681+
merge_qualified,
682+
prior,
683+
NpmLinks::All,
684+
)
685+
.await
686+
}
687+
688+
/// Which npm root links [`dispatch_find_with_npm_links`] adds.
689+
#[derive(Clone, Copy, PartialEq, Eq)]
690+
enum NpmLinks {
691+
/// Every link to a purl asked for: path scoping matches by install
692+
/// path, so each spelling counts.
693+
All,
694+
/// Only links to purls the resolver found no copy of. The commands
695+
/// that act per copy already reach every store peer variant of a
696+
/// found copy through the apply engine's fan-out, so a link to one
697+
/// would be a second visit (#633); and a link into first-party source
698+
/// beside a found registry copy must not turn into a refusal.
699+
ForUnresolved,
700+
}
701+
702+
/// [`dispatch_find`]'s copies, plus each npm package link directly in a
703+
/// `node_modules` root (scoped ones one level down) whose target's
704+
/// `package.json` names one of the npm purls asked for (see [`NpmLinks`]
705+
/// for which), after the copies.
655706
///
656707
/// The crawler records a plain-name link at its own path
657708
/// (`packages/a/node_modules/ms`), but never an alias link
658709
/// (`packages/a/node_modules/lp -> …/.pnpm/left-pad@1.3.0/node_modules/left-pad`):
659-
/// links are not physical copies, and apply and VEX must see each copy
660-
/// once (#856). Path scoping asks which paths install the package
661-
/// instead, and pnpm's isolated linker or npm's `install-strategy=linked`
662-
/// reach a workspace member's ALIAS dependency only through such a link
663-
/// (#1266). A link is matched by its target's own identity, not against
664-
/// the crawled copies, so a link to a store peer variant the crawl
665-
/// skipped still counts, and the cost is one pass over the root links
666-
/// however many purls are asked for. The result is for path matching
667-
/// only: it can name one physical copy more than once.
668-
pub async fn find_package_paths_for_path_scope(
710+
/// links are not physical copies, and the resolver must see each copy
711+
/// once (#856). Yet pnpm's isolated linker, vlt and npm's
712+
/// `install-strategy=linked` reach a workspace member's ALIAS dependency
713+
/// only through such a link. Path scoping needs it to select the member
714+
/// (#1266), and a run from inside the member, whose `node_modules` is the
715+
/// only root, has no other route to the copy at all (#1494). A link is
716+
/// matched by its target's own identity, not against the crawled copies,
717+
/// so a link to a store peer variant the crawl skipped still counts, and
718+
/// the cost is one pass over the root links however many purls are asked
719+
/// for (none when every purl already has a copy and only the unresolved
720+
/// ones' links are wanted).
721+
async fn dispatch_find_with_npm_links(
669722
partitioned: &HashMap<Ecosystem, Vec<String>>,
670723
options: &CrawlerOptions,
671724
silent: bool,
725+
variant_merge: MergeFn,
672726
prior: Option<&NpmCrawlSnapshot>,
727+
links: NpmLinks,
673728
) -> HashMap<String, Vec<PathBuf>> {
674729
let mut wanted: HashMap<PurlKey, Vec<String>> = HashMap::new();
675730
for purl in partitioned.get(&Ecosystem::Npm).into_iter().flatten() {
@@ -687,13 +742,23 @@ pub async fn find_package_paths_for_path_scope(
687742
partitioned,
688743
options,
689744
silent,
690-
merge_qualified,
745+
variant_merge,
691746
npm_roots.as_deref(),
692747
)
693748
.await;
694749
let Some(roots) = npm_roots else {
695750
return found;
696751
};
752+
if links == NpmLinks::ForUnresolved {
753+
let resolved = |purl: &String| found.get(purl).is_some_and(|paths| !paths.is_empty());
754+
wanted.retain(|_, purls| {
755+
purls.retain(|purl| !resolved(purl));
756+
!purls.is_empty()
757+
});
758+
if wanted.is_empty() {
759+
return found;
760+
}
761+
}
697762
// A failed enumeration only loses the extra links; the copies stand.
698763
let links = tokio::task::spawn_blocking(move || npm_links_to(&wanted, &roots))
699764
.await
@@ -794,17 +859,25 @@ pub async fn find_packages_for_rollback(
794859
/// tree nothing has touched since) instead of walking the tree for them
795860
/// again. Only the root discovery is reused: each root is still searched
796861
/// by `find_by_purls`, so copy choice and order are unchanged. A snapshot
797-
/// taken with other options is ignored.
862+
/// taken with other options is ignored. An npm purl with no copy can still
863+
/// be reached through a workspace member's alias link (#1494).
798864
pub async fn find_packages_for_rollback_reusing(
799865
partitioned: &HashMap<Ecosystem, Vec<String>>,
800866
options: &CrawlerOptions,
801867
silent: bool,
802868
prior: Option<&NpmCrawlSnapshot>,
803869
) -> HashMap<String, PathBuf> {
804-
let npm_roots = prior
805-
.filter(|p| p.taken_with(options))
806-
.map(|p| p.roots.as_slice());
807-
collapse_to_first(dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await)
870+
collapse_to_first(
871+
dispatch_find_with_npm_links(
872+
partitioned,
873+
options,
874+
silent,
875+
merge_qualified,
876+
prior,
877+
NpmLinks::ForUnresolved,
878+
)
879+
.await,
880+
)
808881
}
809882

810883
/// The npm half of one [`crawl_ecosystems_with_npm`] run: the packages
@@ -977,6 +1050,9 @@ pub(crate) fn npm_paths_by_identity_in(
9771050
/// [`find_packages_for_rollback_reusing`]. Only the root discovery is
9781051
/// reused: each root is still searched by `find_by_purls`, so copy choice
9791052
/// and order are unchanged. A snapshot taken with other options is ignored.
1053+
/// An npm purl with no copy can still be reached through a workspace
1054+
/// member's alias link (#1494), so `apply --check` and `vex` see the copy
1055+
/// `apply` patched.
9801056
///
9811057
/// Read-only by contract (its one caller is `vex`), so it also searches the
9821058
/// gem stores under a refused out-of-tree `.bundle/config` path
@@ -990,13 +1066,13 @@ pub async fn find_manifest_package_copies_reusing(
9901066
) -> HashMap<String, Vec<PathBuf>> {
9911067
let partitioned = partition_purls(purls, common.ecosystems.as_deref());
9921068
let crawler_options = common.crawler_options();
993-
let npm_roots = prior.and_then(|p| p.roots_for(&crawler_options));
994-
let mut copies = dispatch_find(
1069+
let mut copies = dispatch_find_with_npm_links(
9951070
&partitioned,
9961071
&crawler_options,
9971072
quiet,
9981073
merge_qualified,
999-
npm_roots,
1074+
prior,
1075+
NpmLinks::ForUnresolved,
10001076
)
10011077
.await;
10021078
// `apply` also writes every store variant of each npm copy (a pnpm peer
@@ -2058,6 +2134,59 @@ mod tests {
20582134
assert_eq!(single.get("pkg:npm/dup@1.0.0"), Some(&root_copy));
20592135
}
20602136

2137+
/// #1494: run from a workspace member, a member's alias link into the
2138+
/// root store (`packages/a/node_modules/lp`, vlt's layout here) is the
2139+
/// only route to the copy, so the copy lookups must return it; a link
2140+
/// to another version must not count. From the root the store copy is
2141+
/// found and the link is not added: the apply engine's store fan-out
2142+
/// already reaches a link's peer variant, and a second visit would be
2143+
/// #633's phantom `already_patched`. Path scoping still sees it.
2144+
#[cfg(unix)]
2145+
#[tokio::test]
2146+
async fn multi_copy_lookups_take_a_member_alias_link_into_the_root_store() {
2147+
let tmp = tempfile::tempdir().unwrap();
2148+
let root = tmp.path();
2149+
let store = root.join("node_modules/.vlt");
2150+
let store_copy = write_npm_package(&store.join("~npm~left-pad@1.3.0"), "left-pad", "1.3.0");
2151+
write_npm_package(&store.join("~npm~left-pad@1.2.0"), "left-pad", "1.2.0");
2152+
let member_nm = root.join("packages/a/node_modules");
2153+
std::fs::create_dir_all(&member_nm).unwrap();
2154+
std::os::unix::fs::symlink(
2155+
"../../../node_modules/.vlt/~npm~left-pad@1.3.0/node_modules/left-pad",
2156+
member_nm.join("lp"),
2157+
)
2158+
.unwrap();
2159+
std::os::unix::fs::symlink(
2160+
"../../../node_modules/.vlt/~npm~left-pad@1.2.0/node_modules/left-pad",
2161+
member_nm.join("old"),
2162+
)
2163+
.unwrap();
2164+
let purl = "pkg:npm/left-pad@1.3.0".to_string();
2165+
let partitioned = partition_purls(std::slice::from_ref(&purl), None);
2166+
2167+
let member = local_options(root.join("packages/a"));
2168+
let link = vec![member_nm.join("lp")];
2169+
let applied = find_all_packages_for_purls(&partitioned, &member, true).await;
2170+
assert_eq!(applied.get(&purl), Some(&link), "{applied:?}");
2171+
let rolled = find_all_packages_for_rollback(&partitioned, &member, true).await;
2172+
assert_eq!(rolled.get(&purl), Some(&link), "{rolled:?}");
2173+
let one = find_packages_for_rollback(&partitioned, &member, true).await;
2174+
assert_eq!(one.get(&purl), Some(&link[0]), "{one:?}");
2175+
2176+
let at_root = local_options(root.to_path_buf());
2177+
let store_only = vec![store_copy.clone()];
2178+
let applied = find_all_packages_for_purls(&partitioned, &at_root, true).await;
2179+
assert_eq!(applied.get(&purl), Some(&store_only), "{applied:?}");
2180+
let rolled = find_all_packages_for_rollback(&partitioned, &at_root, true).await;
2181+
assert_eq!(rolled.get(&purl), Some(&store_only), "{rolled:?}");
2182+
let scoped = find_package_paths_for_path_scope(&partitioned, &at_root, true, None).await;
2183+
assert_eq!(
2184+
scoped.get(&purl),
2185+
Some(&vec![store_copy, member_nm.join("lp")]),
2186+
"{scoped:?}"
2187+
);
2188+
}
2189+
20612190
/// Multi-copy for gem (mirrors the npm test above): bundler's scoped
20622191
/// (`<engine>/<abi>/gems`) and flat (`gems/`) store layouts coexist under
20632192
/// one `vendor/bundle` root — a bundler-2 `--path` install beside a

0 commit comments

Comments
 (0)