Skip to content

Commit ef6b60b

Browse files
committed
Skip nested stores in apply's peer fan-out too
apply's peer-variant fan-out walked up from a patched copy and probed every ancestor node_modules for a store, including a store entry's own node_modules. A peer variant in a nested .pnpm there was patched although scan never reports it. That walk now skips a store entry's node_modules, matching the scan and the resolver. The test oracle for find_by_purls follows the same store-entry rule, and the regression test now also covers .vlt, a .modules.yaml relocated store and the peer fan-out. Review cleanups: the resolver's store listing returns a plain Vec, a misplaced #362 doc comment is back on its own test, and the walk docs point at StoreKind::classify. Refs #1899 Assisted-by: Claude Code:claude-opus-5-5
1 parent 844a112 commit ef6b60b

2 files changed

Lines changed: 70 additions & 32 deletions

File tree

‎crates/socket-patch-core/src/crawlers/npm_crawler.rs‎

Lines changed: 63 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -616,6 +616,24 @@ fn pnpm_shaped_store_layout(name: &str) -> Option<StoreLayout> {
616616
.map(|(_, layout)| *layout)
617617
}
618618

619+
/// Whether `dir` is a named store's entry's own `node_modules`
620+
/// (`<store>/<entry>/node_modules`, `<store>` one of the pnpm-shaped
621+
/// stores, `.vlt` or `.store`), whose children are never stores (see
622+
/// [`StoreKind::classify`]).
623+
fn is_store_entry_node_modules(dir: &Path) -> bool {
624+
dir.file_name() == Some(OsStr::new("node_modules"))
625+
&& dir
626+
.parent()
627+
.and_then(Path::parent)
628+
.and_then(Path::file_name)
629+
.and_then(OsStr::to_str)
630+
.is_some_and(|store| {
631+
pnpm_shaped_store_layout(store).is_some()
632+
|| store == VLT_STORE_NAME
633+
|| store == NPM_LINKED_STORE_NAME
634+
})
635+
}
636+
619637
/// Which package-manager store a `node_modules` child is. The resolver
620638
/// (`find_by_purls`) and the scan (`crawl_all`) both classify a child
621639
/// through [`StoreKind::classify`], so they agree on which children are
@@ -2929,8 +2947,8 @@ impl NpmCrawler {
29292947
if pending.is_empty() {
29302948
return pending;
29312949
}
2932-
// Each dir is tagged `true` when it is a pnpm or vlt store entry's
2933-
// `node_modules`.
2950+
// Each dir is tagged `true` when it is a store entry's
2951+
// `node_modules` (see `StoreKind::classify`).
29342952
let mut level: Vec<(PathBuf, bool)> = vec![(node_modules_path.to_path_buf(), false)];
29352953
let mut visited = HashSet::new();
29362954
// The root's relocated Yarn pnpm-linker store (#859), contributed
@@ -3218,10 +3236,8 @@ impl NpmCrawler {
32183236
) -> Vec<NestedNodeModules> {
32193237
let name_str = entry.name_str.as_str();
32203238
if let Some(kind) = StoreKind::classify(name_str, entry.file_type, store_entry) {
3221-
return Self::resolver_store_entries(nm_path, &entry.name, kind)
3222-
.map(NestedNodeModules::StoreEntries)
3223-
.into_iter()
3224-
.collect();
3239+
let entries = Self::resolver_store_entries(nm_path, &entry.name, kind);
3240+
return vec![NestedNodeModules::StoreEntries(entries)];
32253241
}
32263242
if name_str.starts_with('.') || name_str == "node_modules" {
32273243
return Vec::new();
@@ -3254,7 +3270,7 @@ impl NpmCrawler {
32543270
}
32553271

32563272
/// The entries of the store `kind` that `nm_path`'s child `name` is,
3257-
/// for the resolver to probe; `None` when there are none.
3273+
/// for the resolver to probe.
32583274
///
32593275
/// Under the isolated linkers a store is the ONLY physical home of
32603276
/// transitive dependencies: the importer's `node_modules` holds links
@@ -3271,13 +3287,9 @@ impl NpmCrawler {
32713287
/// relocated by `virtualStoreDir` (found through `.modules.yaml`, it may
32723288
/// sit outside this `node_modules`) and pnpm <=3's hidden
32733289
/// `.<registry-host>` store all have this property.
3274-
fn resolver_store_entries(
3275-
nm_path: &Path,
3276-
name: &OsStr,
3277-
kind: StoreKind,
3278-
) -> Option<Vec<StoreEntry>> {
3290+
fn resolver_store_entries(nm_path: &Path, name: &OsStr, kind: StoreKind) -> Vec<StoreEntry> {
32793291
let store = nm_path.join(name);
3280-
let entries = match kind {
3292+
match kind {
32813293
StoreKind::PnpmShaped(layout) => StoreEntry::from_dirs(
32823294
Self::list_pnpm_shaped_store_entries_sync(&store, layout, false, false),
32833295
),
@@ -3294,8 +3306,7 @@ impl NpmCrawler {
32943306
StoreKind::LegacyPnpm => {
32953307
StoreEntry::pnpm(Self::collect_nested_store_entries_sync(&store))
32963308
}
3297-
};
3298-
(!entries.is_empty()).then_some(entries)
3309+
}
32993310
}
33003311

33013312
/// Whether a virtual-store entry can still hold a pending target.
@@ -3567,14 +3578,14 @@ impl NpmCrawler {
35673578
/// Gather one `node_modules` directory's scan into [`ScanEvent`]s, in
35683579
/// the exact order the sequential walk visits it: the directory's own
35693580
/// entries (each package followed by its nested `node_modules`), then
3570-
/// the deferred pnpm and vlt store(s). `listing` is the directory's
3571-
/// listing when the caller already read it. `store_entry` selects the
3572-
/// one policy bit distinguishing an importer/package tree from a pnpm
3573-
/// or vlt store entry's `node_modules`:
3581+
/// the deferred store(s). `listing` is the directory's listing when the
3582+
/// caller already read it. `store_entry` selects the one policy bit
3583+
/// distinguishing an importer/package tree from a store entry's
3584+
/// `node_modules`:
35743585
/// - importer trees accept both directories and symlinks (pnpm and vlt
35753586
/// link direct deps; `npm link` targets) but never traverse a symlink,
3576-
/// and a `.pnpm` (or pnpm <=3 `.<registry-host>`) or `.vlt` child is
3577-
/// the virtual store, walked after the loop;
3587+
/// and a store child (see [`StoreKind::classify`]) is walked after
3588+
/// the loop;
35783589
/// - a store entry accepts REAL directories only — a symlinked (or, on
35793590
/// Windows, junctioned) entry there is the package's dependency
35803591
/// pointing at a sibling store entry, inventoried via that entry, and
@@ -4512,7 +4523,9 @@ async fn find_store_peer_variant_copies_reusing(
45124523
stores.push((StoreLayout::NpmLinked, dir.to_path_buf()));
45134524
}
45144525
}
4515-
Some("node_modules") => {
4526+
// A store entry's own `node_modules` never holds a store
4527+
// (see `StoreKind::classify`), so only other ones are probed.
4528+
Some("node_modules") if !is_store_entry_node_modules(dir) => {
45164529
let others = [
45174530
(VLT_STORE_NAME, StoreLayout::Vlt),
45184531
(NPM_LINKED_STORE_NAME, StoreLayout::NpmLinked),
@@ -6679,16 +6692,13 @@ mod tests {
66796692
assert_eq!(found[&scoped_purl][0].namespace.as_deref(), Some("@s"));
66806693
}
66816694

6682-
/// #362: pnpm's `virtualStoreDir` moves the virtual store, and
6683-
/// `node_modules/.modules.yaml` records where (relative to
6684-
/// `node_modules`: JSON on pnpm 10+, YAML before). A relocated store
6685-
/// inside the project is walked like `.pnpm`, wherever it sits.
66866695
/// #1899: a store entry's own `node_modules` holds packages and links
66876696
/// to sibling entries, never a nested store. Scan always skipped a
66886697
/// store-named child there as a hidden entry, but the resolver walked
66896698
/// it as a store, so `apply` could patch a copy `scan` never reported.
66906699
/// Both now classify through `StoreKind::classify` and agree: such a
6691-
/// copy is neither reported nor resolved, for every store name.
6700+
/// copy is neither reported, resolved nor reached by the apply peer
6701+
/// fan-out, for every store name.
66926702
#[tokio::test]
66936703
async fn test_store_named_child_of_a_store_entry_is_not_a_store() {
66946704
for (store_name, nested_entry) in [
@@ -6697,6 +6707,9 @@ mod tests {
66976707
(".deno", "hidden@1.0.0/node_modules/hidden"),
66986708
(".store", "hidden@1.0.0-abcdef/node_modules/hidden"),
66996709
(".registry.npmjs.org", "hidden/1.0.0/node_modules/hidden"),
6710+
(".vlt", "··hidden@1.0.0/node_modules/hidden"),
6711+
// `.modules.yaml` names a relocated store, `.vs` here.
6712+
(".vs", "hidden@1.0.0/node_modules/hidden"),
67006713
] {
67016714
let tmp = tempfile::tempdir().unwrap();
67026715
let root: PathBuf = tmp.path().components().collect();
@@ -6707,6 +6720,19 @@ mod tests {
67076720
link_dir(&outer, &nm.join("outer"));
67086721
let hidden = entry_nm.join(store_name).join(nested_entry);
67096722
write_pkg(&hidden, "hidden", "1.0.0");
6723+
if store_name == ".vs" {
6724+
std::fs::write(
6725+
entry_nm.join(".modules.yaml"),
6726+
"layoutVersion: 5\nvirtualStoreDir: '.vs'\n",
6727+
)
6728+
.unwrap();
6729+
}
6730+
// A peer variant of `outer` in the nested store: the apply
6731+
// fan-out must not reach it either.
6732+
let nested_twin = entry_nm.join(".pnpm/outer@1.0.0(react@18.2.0)/node_modules/outer");
6733+
if store_name == ".pnpm" {
6734+
write_pkg(&nested_twin, "outer", "1.0.0");
6735+
}
67106736

67116737
let scanned = scan_paths(&root).await;
67126738
assert!(
@@ -6730,9 +6756,19 @@ mod tests {
67306756
found.is_empty(),
67316757
"{store_name}: resolver walked a nested store: {found:?}"
67326758
);
6759+
assert!(
6760+
!find_store_peer_variant_copies(&outer)
6761+
.await
6762+
.contains(&nested_twin),
6763+
"{store_name}: peer fan-out walked a nested store"
6764+
);
67336765
}
67346766
}
67356767

6768+
/// #362: pnpm's `virtualStoreDir` moves the virtual store, and
6769+
/// `node_modules/.modules.yaml` records where (relative to
6770+
/// `node_modules`: JSON on pnpm 10+, YAML before). A relocated store
6771+
/// inside the project is walked like `.pnpm`, wherever it sits.
67366772
#[tokio::test]
67376773
async fn test_pnpm_relocated_virtual_store_dir_is_walked() {
67386774
for (modules_yaml, store_rel) in [

‎crates/socket-patch-core/src/crawlers/npm_crawler/oracle.rs‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ impl LegacyNpmCrawler {
267267
.map(|t| t.dir_key.as_str())
268268
.collect();
269269
let filter = filter_store_entries.then_some(&unmatched_names);
270-
Self::collect_nested_node_modules(&nm_path, filter, &mut queue).await;
270+
Self::collect_nested_node_modules(&nm_path, store_entry, filter, &mut queue).await;
271271
}
272272
// Only the targets with zero copies remain "pending" for pass 2.
273273
pending.retain(|t| !result.contains_key(&t.purl));
@@ -337,9 +337,11 @@ impl LegacyNpmCrawler {
337337
/// store (see below); `pending_names` — `Some(the still-unresolved
338338
/// targets' full package names)` — bounds which store entries get
339339
/// enqueued, while `None` (the pass-2 fallback of `find_by_purls`)
340-
/// enqueues every store entry.
340+
/// enqueues every store entry. Inside a store entry's own
341+
/// `node_modules` (`store_entry`) no child is a store (#1899).
341342
async fn collect_nested_node_modules(
342343
nm_path: &Path,
344+
store_entry: bool,
343345
pending_names: Option<&HashSet<&str>>,
344346
queue: &mut VecDeque<(PathBuf, bool)>,
345347
) {
@@ -358,7 +360,7 @@ impl LegacyNpmCrawler {
358360
// BFS order guarantees a root-linked install has already been
359361
// probed (and removed from `pending`) before these are
360362
// dequeued, so a package is never resolved twice.
361-
if name_str == ".pnpm" {
363+
if !store_entry && name_str == ".pnpm" {
362364
let Some(file_type) = crate::utils::fs::entry_file_type(&entry).await else {
363365
continue;
364366
};
@@ -377,7 +379,7 @@ impl LegacyNpmCrawler {
377379
// vlt's store has the same transitive-only-home property: every
378380
// package lives at `.vlt/<DepID>/node_modules/<name>` and the
379381
// importer holds only links into it.
380-
if name_str == VLT_STORE_NAME {
382+
if !store_entry && name_str == VLT_STORE_NAME {
381383
let Some(file_type) = crate::utils::fs::entry_file_type(&entry).await else {
382384
continue;
383385
};
@@ -394,7 +396,7 @@ impl LegacyNpmCrawler {
394396
// Must run before the generic hidden-entry skip below, which
395397
// would otherwise swallow it — leaving every transitive-only
396398
// install unpatchable on those layouts.
397-
if is_legacy_pnpm_store_dir_name(&name_str) {
399+
if !store_entry && is_legacy_pnpm_store_dir_name(&name_str) {
398400
let Some(file_type) = crate::utils::fs::entry_file_type(&entry).await else {
399401
continue;
400402
};

0 commit comments

Comments
 (0)