Skip to content

Commit 84b3690

Browse files
committed
Keep alias links out of rollback's copy list
The link pass lived in the resolver rollback also restores from, so every rollback did extra filesystem work it then threw away. It now runs only for scan and rollback path targets, in its own lookup. A member's alias link can also point at a pnpm peer variant the crawl skipped once it found the root's copy, so store peer variants now count when matching links. A failed link enumeration falls back to the plain copies instead of aborting the run. Assisted-by: Claude Code:claude-opus-5-5
1 parent 88c91ff commit 84b3690

4 files changed

Lines changed: 137 additions & 60 deletions

File tree

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1591,10 +1591,11 @@ pub async fn run(args: RollbackArgs) -> i32 {
15911591
global: args.common.global,
15921592
global_prefix: args.common.global_prefix.clone(),
15931593
};
1594-
let discovered = find_all_packages_for_rollback(
1594+
let discovered = crate::ecosystem_dispatch::find_package_paths_for_path_scope(
15951595
&partitioned,
15961596
&crawler_options,
15971597
args.common.silent || args.common.json,
1598+
None,
15981599
)
15991600
.await;
16001601
// One single-pattern scope per raw pattern, compiled once (not per

‎crates/socket-patch-cli/src/commands/scan/mod.rs‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,7 @@ use std::path::{Path, PathBuf};
3333
use crate::args::{apply_env_toggles, GlobalArgs};
3434
use crate::commands::vex::{generate_vex_from_manifest_path, VexEmbedArgs};
3535
use crate::ecosystem_dispatch::{
36-
crawl_ecosystems, crawl_ecosystems_with_npm, find_all_packages_for_rollback_reusing,
37-
partition_purls,
36+
crawl_ecosystems, crawl_ecosystems_with_npm, find_package_paths_for_path_scope, partition_purls,
3837
};
3938
use crate::json_envelope::{
4039
usage_error, Command as JsonCommand, Envelope, EnvelopeError, PatchAction, PatchEvent,
@@ -2181,7 +2180,7 @@ async fn run_scan(
21812180
unresolved.dedup();
21822181
if !unresolved.is_empty() {
21832182
let partitioned = partition_purls(&unresolved, args.common.ecosystems.as_deref());
2184-
let copies = find_all_packages_for_rollback_reusing(
2183+
let copies = find_package_paths_for_path_scope(
21852184
&partitioned,
21862185
&crawler_options,
21872186
true,

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

Lines changed: 67 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -634,33 +634,49 @@ pub async fn find_all_packages_for_rollback(
634634
}
635635

636636
/// [`find_all_packages_for_rollback`], taking the npm `node_modules` roots
637-
/// from `prior` as [`find_packages_for_rollback_reusing`] does. Used by
638-
/// `scan`'s path scoping, which must see the same copies `rollback`'s path
639-
/// targets do.
640-
///
641-
/// An npm package's copies here also include each link in a `node_modules`
642-
/// root that resolves to one of them ([`add_npm_links_to_copies`]): path
643-
/// scoping asks which install paths reach a package, and pnpm's isolated
644-
/// linker or npm's `install-strategy=linked` reach a workspace member's
645-
/// ALIAS dependency only through such a link (#1266).
637+
/// from `prior` as [`find_packages_for_rollback_reusing`] does.
646638
pub async fn find_all_packages_for_rollback_reusing(
647639
partitioned: &HashMap<Ecosystem, Vec<String>>,
648640
options: &CrawlerOptions,
649641
silent: bool,
650642
prior: Option<&NpmCrawlSnapshot>,
651643
) -> HashMap<String, Vec<PathBuf>> {
652-
let prior_roots = prior
644+
let npm_roots = prior
653645
.filter(|p| p.taken_with(options))
654-
.map(|p| p.roots.clone());
655-
let npm_roots = match prior_roots {
656-
Some(roots) => Some(roots),
657-
None if partitioned
658-
.get(&Ecosystem::Npm)
659-
.is_some_and(|p| !p.is_empty()) =>
660-
{
661-
NpmCrawler.get_node_modules_paths(options).await.ok()
662-
}
663-
None => None,
646+
.map(|p| p.roots.as_slice());
647+
dispatch_find(partitioned, options, silent, merge_qualified, npm_roots).await
648+
}
649+
650+
/// 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) that resolves to one of them or to one
654+
/// of their store peer variants.
655+
///
656+
/// The crawler records a plain-name link at its own path
657+
/// (`packages/a/node_modules/ms`), but never an alias link
658+
/// (`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). The member's link can resolve to a peer variant the crawl
664+
/// skipped once it found the root's copy, so the variants count too.
665+
/// The result is for path matching only: it can name one physical copy
666+
/// more than once.
667+
pub async fn find_package_paths_for_path_scope(
668+
partitioned: &HashMap<Ecosystem, Vec<String>>,
669+
options: &CrawlerOptions,
670+
silent: bool,
671+
prior: Option<&NpmCrawlSnapshot>,
672+
) -> HashMap<String, Vec<PathBuf>> {
673+
let has_npm = partitioned
674+
.get(&Ecosystem::Npm)
675+
.is_some_and(|purls| !purls.is_empty());
676+
let npm_roots = match prior.and_then(|p| p.roots_for(options)) {
677+
Some(roots) if has_npm => Some(roots.to_vec()),
678+
None if has_npm => NpmCrawler.get_node_modules_paths(options).await.ok(),
679+
_ => None,
664680
};
665681
let mut found = dispatch_find(
666682
partitioned,
@@ -670,40 +686,43 @@ pub async fn find_all_packages_for_rollback_reusing(
670686
npm_roots.as_deref(),
671687
)
672688
.await;
673-
if let Some(roots) = npm_roots {
674-
found = tokio::task::spawn_blocking(move || {
675-
add_npm_links_to_copies(&mut found, &roots);
676-
found
677-
})
678-
.await
679-
.expect("npm link enumeration does not panic");
680-
}
681-
found
682-
}
683-
684-
/// Add to each npm purl's `copies` every package link directly in one of
685-
/// `roots` (scoped ones one level down) that resolves to one of its
686-
/// copies. The crawler records a plain-name link at its own path
687-
/// (`packages/a/node_modules/ms`), but never an alias link
688-
/// (`packages/a/node_modules/lp -> …/.pnpm/left-pad@1.3.0/node_modules/left-pad`):
689-
/// links are not physical copies, and apply and VEX must see each physical
690-
/// copy once. Path scoping instead needs every path that installs the
691-
/// package, so the alias link is added here (#1266).
692-
fn add_npm_links_to_copies(copies: &mut HashMap<String, Vec<PathBuf>>, roots: &[PathBuf]) {
689+
let Some(roots) = npm_roots else {
690+
return found;
691+
};
693692
let mut by_target: HashMap<PathBuf, Vec<String>> = HashMap::new();
694-
for (purl, paths) in copies.iter() {
695-
if !purl.starts_with("pkg:npm/") {
693+
for (purl, paths) in &found {
694+
if Ecosystem::from_purl(purl) != Some(Ecosystem::Npm) {
696695
continue;
697696
}
698-
for path in paths {
699-
if let Ok(target) = std::fs::canonicalize(path) {
697+
for path in with_store_peer_variant_copies(paths.clone()).await {
698+
if let Ok(target) = tokio::fs::canonicalize(&path).await {
700699
by_target.entry(target).or_default().push(purl.clone());
701700
}
702701
}
703702
}
704703
if by_target.is_empty() {
705-
return;
704+
return found;
706705
}
706+
// A failed enumeration only loses the extra links; the copies stand.
707+
let links = tokio::task::spawn_blocking(move || npm_links_to(&by_target, &roots))
708+
.await
709+
.unwrap_or_default();
710+
for (purl, link) in links {
711+
let paths = found.entry(purl).or_default();
712+
if !paths.contains(&link) {
713+
paths.push(link);
714+
}
715+
}
716+
found
717+
}
718+
719+
/// Every package link directly in one of `roots` (scoped ones one level
720+
/// down) that resolves to a key of `by_target`, paired with each purl
721+
/// that key belongs to.
722+
fn npm_links_to(
723+
by_target: &HashMap<PathBuf, Vec<String>>,
724+
roots: &[PathBuf],
725+
) -> Vec<(String, PathBuf)> {
707726
let entries = |dir: &Path| -> Vec<(String, PathBuf, std::fs::FileType)> {
708727
let Ok(read) = std::fs::read_dir(dir) else {
709728
return Vec::new();
@@ -716,6 +735,7 @@ fn add_npm_links_to_copies(copies: &mut HashMap<String, Vec<PathBuf>>, roots: &[
716735
})
717736
.collect()
718737
};
738+
let mut out = Vec::new();
719739
for root in roots {
720740
let mut links = Vec::new();
721741
for (name, path, file_type) in entries(root) {
@@ -735,13 +755,11 @@ fn add_npm_links_to_copies(copies: &mut HashMap<String, Vec<PathBuf>>, roots: &[
735755
continue;
736756
};
737757
for purl in by_target.get(&target).into_iter().flatten() {
738-
let paths = copies.get_mut(purl).expect("keyed from copies");
739-
if !paths.contains(&link) {
740-
paths.push(link.clone());
741-
}
758+
out.push((purl.clone(), link.clone()));
742759
}
743760
}
744761
}
762+
out
745763
}
746764

747765
/// Qualified-aware PURL resolution for rollback, vendor, repair and

‎crates/socket-patch-cli/tests/scan/scan_paths_e2e.rs‎

Lines changed: 66 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -982,14 +982,15 @@ async fn assert_member_alias_link_in_scope(store: &str, lp_entry: &str, lp_dir:
982982
"scan {pattern} must not select member b's is-number; body: {}",
983983
bodies[0]
984984
);
985+
if pattern == "packages/a" {
986+
assert!(
987+
bodies[0].contains("pkg:npm/ms@2.1.2"),
988+
"scan packages/a must still select the plain-name link; body: {}",
989+
bodies[0]
990+
);
991+
assert_eq!(v["scannedPackages"], 2, "scan packages/a: {v}");
992+
}
985993
}
986-
let (bodies, v) = scoped_agent_scan(root, "packages/a").await;
987-
assert!(
988-
bodies[0].contains("pkg:npm/ms@2.1.2"),
989-
"scan packages/a must still select the plain-name link; body: {}",
990-
bodies[0]
991-
);
992-
assert_eq!(v["scannedPackages"], 2, "scan packages/a: {v}");
993994
}
994995

995996
/// #1266, pnpm's default isolated linker.
@@ -1007,3 +1008,61 @@ async fn paths_scope_selects_npm_linked_member_alias_link() {
10071008
assert_member_alias_link_in_scope(".store", "left-pad@1.3.0-0a1b2c3d", "left-pad").await;
10081009
assert_member_alias_link_in_scope(".store", "lp@1.3.0-0a1b2c3d", "lp").await;
10091010
}
1011+
1012+
/// #1266: the root's own `left-pad` link resolves to one pnpm peer
1013+
/// variant, member a's alias `lp` to another. The crawl stops at the
1014+
/// root's copy and never lists member a's variant, yet member a loads it,
1015+
/// so scoping to member a must still select left-pad.
1016+
#[cfg(unix)]
1017+
#[tokio::test]
1018+
async fn paths_scope_selects_member_alias_link_to_a_peer_variant() {
1019+
let tmp = tempfile::tempdir().unwrap();
1020+
let root = tmp.path();
1021+
std::fs::write(
1022+
root.join("package.json"),
1023+
r#"{ "name": "root", "version": "1.0.0", "private": true, "dependencies": { "left-pad": "1.3.0" } }"#,
1024+
)
1025+
.unwrap();
1026+
std::fs::write(
1027+
root.join("pnpm-workspace.yaml"),
1028+
"packages: ['packages/*']\n",
1029+
)
1030+
.unwrap();
1031+
for variant in ["left-pad@1.3.0_react@17.0.2", "left-pad@1.3.0_react@18.2.0"] {
1032+
write_npm_package_at(
1033+
root,
1034+
&format!("node_modules/.pnpm/{variant}"),
1035+
"left-pad",
1036+
"1.3.0",
1037+
);
1038+
}
1039+
std::os::unix::fs::symlink(
1040+
".pnpm/left-pad@1.3.0_react@17.0.2/node_modules/left-pad",
1041+
root.join("node_modules/left-pad"),
1042+
)
1043+
.unwrap();
1044+
let member_dir = root.join("packages/a");
1045+
std::fs::create_dir_all(member_dir.join("node_modules")).unwrap();
1046+
std::fs::write(
1047+
member_dir.join("package.json"),
1048+
r#"{ "name": "a", "version": "1.0.0", "dependencies": { "lp": "npm:left-pad@1.3.0" } }"#,
1049+
)
1050+
.unwrap();
1051+
std::os::unix::fs::symlink(
1052+
"../../../node_modules/.pnpm/left-pad@1.3.0_react@18.2.0/node_modules/left-pad",
1053+
member_dir.join("node_modules/lp"),
1054+
)
1055+
.unwrap();
1056+
1057+
let (bodies, v) = scoped_agent_scan(root, "packages/a").await;
1058+
assert_eq!(
1059+
bodies.len(),
1060+
1,
1061+
"scan packages/a must query the batch API once; envelope {v}"
1062+
);
1063+
assert!(
1064+
bodies[0].contains("pkg:npm/left-pad@1.3.0"),
1065+
"scan packages/a must select the peer variant member a's alias loads; body: {}",
1066+
bodies[0]
1067+
);
1068+
}

0 commit comments

Comments
 (0)