From bd30a2eb5af94bd48116e2bbe50f589eb74ad4b9 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 06:25:58 +0000 Subject: [PATCH 1/4] Start fix for #362 Assisted-by: Claude Code:claude-opus-5-5 From 0c5b583b340bd368cd676f6e8322ae9f26af00de Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 06:49:34 +0000 Subject: [PATCH 2/4] Refuse pnpm global-store transitive deps With pnpm's enableGlobalVirtualStore, a transitive dependency lives only in the machine-wide /v/links dir. The npm crawler never walked that dir, so agent apply treated the package as lockfile-only, exited 0 with "success", and Node kept loading the unpatched copy. The crawler now follows this project's links into the global store, and each entry's dependency links to sibling entries, so every copy the project loads is found at its real path. Apply then refuses it as shared, the same way it already refuses a direct dependency, and names the remedy. Packages other projects put in the store stay invisible. Fixes #362 Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/tests/apply/main.rs | 1 + .../tests/apply/pnpm_global_virtual_store.rs | 173 ++++++++++++++ .../src/crawlers/npm_crawler.rs | 223 +++++++++++++++++- .../src/patch/shared_store.rs | 19 +- docs/ecosystems.md | 16 +- 5 files changed, 419 insertions(+), 13 deletions(-) create mode 100644 crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs diff --git a/crates/socket-patch-cli/tests/apply/main.rs b/crates/socket-patch-cli/tests/apply/main.rs index f6eca4d02..20dc853e8 100644 --- a/crates/socket-patch-cli/tests/apply/main.rs +++ b/crates/socket-patch-cli/tests/apply/main.rs @@ -20,3 +20,4 @@ mod in_process_gem_multicopy; mod in_process_npm_multicopy; mod in_process_variant_apply_failure; mod lockfile_only_skip; +mod pnpm_global_virtual_store; diff --git a/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs b/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs new file mode 100644 index 000000000..87ac1f876 --- /dev/null +++ b/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs @@ -0,0 +1,173 @@ +//! `apply` on a transitive dependency that lives only in pnpm's global +//! virtual store (`enableGlobalVirtualStore`, #362). +//! +//! The store (`/v/links`) is shared by every project on the +//! machine, so agent mode must refuse to patch it, as it already does for +//! a direct dependency linked into it (#486). A transitive dependency has +//! no importer link, so the crawler never found it. Because the project +//! lock resolves it, the miss was then taken for a calm lockfile-only +//! skip: `apply` exited 0 with `success` while Node kept loading the +//! unpatched copy. + +use std::path::{Path, PathBuf}; + +use serde_json::{json, Value}; + +use crate::common; + +use common::{git_sha256, parse_json_envelope, run_with_env}; + +const BEFORE: &[u8] = b"module.exports = 'pristine';\n"; +const AFTER: &[u8] = b"module.exports = 'patched';\n"; + +fn link_dir(target: &Path, link: &Path) { + #[cfg(unix)] + std::os::unix::fs::symlink(target, link).unwrap(); + #[cfg(windows)] + { + let link: PathBuf = link.components().collect(); + let target: PathBuf = target.components().collect(); + let status = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(&link) + .arg(&target) + .status() + .unwrap(); + assert!(status.success(), "mklink /J failed"); + } +} + +fn write_pkg(dir: &Path, name: &str, version: &str) { + std::fs::create_dir_all(dir).unwrap(); + std::fs::write( + dir.join("package.json"), + format!(r#"{{ "name": "{name}", "version": "{version}" }}"#), + ) + .unwrap(); + std::fs::write(dir.join("index.js"), BEFORE).unwrap(); +} + +/// The layout pnpm 10.12+ writes with `enableGlobalVirtualStore: true`: +/// `proj/node_modules/is-odd` links to the store entry of `is-odd`, whose +/// own `node_modules` links `is-number` to a sibling entry. Another +/// project's `left-pad` sits in the same store. Returns the project dir, +/// the store's `is-number` and `left-pad` dirs. +fn stage(tmp: &Path) -> (PathBuf, PathBuf, PathBuf) { + let v10 = tmp.join("store").join("v10"); + std::fs::create_dir_all(v10.join("files")).unwrap(); + let links = v10.join("links").join("@"); + let odd_nm = links.join("is-odd/3.0.1/aaa111/node_modules"); + write_pkg(&odd_nm.join("is-odd"), "is-odd", "3.0.1"); + let number = links.join("is-number/6.0.0/bbb222/node_modules/is-number"); + write_pkg(&number, "is-number", "6.0.0"); + link_dir(&number, &odd_nm.join("is-number")); + let left_pad = links.join("left-pad/1.3.0/ccc333/node_modules/left-pad"); + write_pkg(&left_pad, "left-pad", "1.3.0"); + + let proj = tmp.join("proj"); + let nm = proj.join("node_modules"); + std::fs::create_dir_all(&nm).unwrap(); + std::fs::write( + proj.join("package.json"), + r#"{ "name": "p", "version": "0.0.0", "private": true, "dependencies": { "is-odd": "3.0.1" } }"#, + ) + .unwrap(); + std::fs::write( + proj.join("pnpm-lock.yaml"), + "lockfileVersion: '9.0'\n\nimporters:\n\n .:\n dependencies:\n is-odd:\n \ + specifier: 3.0.1\n version: 3.0.1\n\npackages:\n\n is-number@6.0.0:\n \ + resolution: {integrity: sha512-AAAA}\n\n is-odd@3.0.1:\n resolution: {integrity: \ + sha512-BBBB}\n\nsnapshots:\n\n is-number@6.0.0: {}\n\n is-odd@3.0.1:\n \ + dependencies:\n is-number: 6.0.0\n", + ) + .unwrap(); + std::fs::write( + nm.join(".modules.yaml"), + "layoutVersion: 5\nnodeLinker: isolated\nvirtualStoreDir: ../../store/v10/links\n", + ) + .unwrap(); + link_dir(&odd_nm.join("is-odd"), &nm.join("is-odd")); + (proj, number, left_pad) +} + +fn write_manifest(root: &Path, purl: &str) { + let socket = root.join(".socket"); + std::fs::create_dir_all(socket.join("blobs")).unwrap(); + std::fs::write(socket.join("blobs").join(git_sha256(AFTER)), AFTER).unwrap(); + std::fs::write( + socket.join("manifest.json"), + serde_json::to_vec_pretty(&json!({ "patches": { purl: { + "uuid": "36236236-0000-4000-8000-000000000001", + "exportedAt": "2024-01-01T00:00:00Z", + "files": { "package/index.js": { + "beforeHash": git_sha256(BEFORE), + "afterHash": git_sha256(AFTER), + }}, + "vulnerabilities": {}, + "description": "pnpm global virtual store fixture", + "license": "MIT", + "tier": "free" + }}})) + .unwrap(), + ) + .unwrap(); +} + +fn event<'a>(v: &'a Value, purl: &str) -> &'a Value { + v["events"] + .as_array() + .expect("events array") + .iter() + .find(|e| e["purl"] == purl) + .unwrap_or_else(|| panic!("no event for {purl}: {v}")) +} + +/// #362: the transitive `is-number` is refused as shared, like a direct +/// dependency would be, and the shared copy stays untouched. It is not +/// reported as a lockfile-only skip. +#[test] +fn transitive_global_virtual_store_dep_is_refused_not_skipped() { + let tmp = tempfile::tempdir().unwrap(); + let (proj, number, _) = stage(tmp.path()); + write_manifest(&proj, "pkg:npm/is-number@6.0.0"); + + let (code, stdout, stderr) = run_with_env( + &proj, + &["apply", "--offline", "--json"], + &[("SOCKET_TELEMETRY_DISABLED", "1")], + ); + let v = parse_json_envelope(&stdout); + assert_ne!( + code, 0, + "a shared-store refusal fails the run; {v}\n{stderr}" + ); + assert_eq!(v["status"], "partialFailure", "{v}"); + let ev = event(&v, "pkg:npm/is-number@6.0.0"); + assert_ne!(ev["errorCode"], "package_not_installed", "{ev}"); + assert!( + ev.to_string().contains("shared by other projects") + && ev.to_string().contains("enableGlobalVirtualStore"), + "the refusal names the shared store and its remedy; {ev}" + ); + assert_eq!(std::fs::read(number.join("index.js")).unwrap(), BEFORE); +} + +/// The walk covers only what this project reaches: a package another +/// project installed into the same store is still "not installed" here. +#[test] +fn other_projects_global_virtual_store_entries_stay_invisible() { + let tmp = tempfile::tempdir().unwrap(); + let (proj, _, left_pad) = stage(tmp.path()); + write_manifest(&proj, "pkg:npm/left-pad@1.3.0"); + + let (code, stdout, stderr) = run_with_env( + &proj, + &["apply", "--offline", "--json"], + &[("SOCKET_TELEMETRY_DISABLED", "1")], + ); + let v = parse_json_envelope(&stdout); + assert_ne!(code, 0, "{v}\n{stderr}"); + let ev = event(&v, "pkg:npm/left-pad@1.3.0"); + assert_eq!(ev["errorCode"], "package_not_installed", "{ev}"); + assert_eq!(std::fs::read(left_pad.join("index.js")).unwrap(), BEFORE); +} diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs index c72e4ca45..887f9fc53 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs @@ -596,6 +596,114 @@ fn relocated_pnpm_virtual_store_sync(nm: &Path) -> Option { Some(dir) } +/// The entries of pnpm's global virtual store (`enableGlobalVirtualStore`) +/// that the importer holding `nm` actually loads (#362). +/// +/// `.modules.yaml` then records `virtualStoreDir` as `/v/links`, +/// outside the project, which [`relocated_pnpm_virtual_store_sync`] +/// rightly refuses to walk: the dir is shared by every project on the +/// machine, so listing it would report other projects' packages. But a +/// transitive dependency lives ONLY there, at +/// `links/////node_modules/`, so +/// skipping the store makes it read as "not installed", which apply takes +/// for a calm lockfile-only skip while Node loads the unpatched copy. +/// +/// So walk just this project's part of the store: start from the +/// importer's links into it (its direct deps), then follow each entry's +/// dependency links to sibling entries. Every entry found is returned +/// (its `node_modules`, and the `(name, version)` its path advertises), +/// so the walks report each copy at its real path in the store, where +/// apply and rollback refuse it as shared (see +/// [`crate::patch::shared_store`]) instead of passing it by. +fn reachable_pnpm_global_virtual_store_entries_sync(nm: &Path) -> Vec { + let Some(links) = pnpm_global_virtual_store_of_sync(nm) else { + return Vec::new(); + }; + let mut seen: HashSet = HashSet::new(); + let mut entries: Vec = Vec::new(); + let mut queue: VecDeque = gvs_link_targets_sync(nm, &links).into(); + while let Some(entry_nm) = queue.pop_front() { + if !seen.insert(entry_nm.clone()) { + continue; + } + queue.extend(gvs_link_targets_sync(&entry_nm, &links)); + entries.push(StoreEntry { + advertised: gvs_entry_advertised(&entry_nm, &links), + node_modules: entry_nm, + }); + } + entries +} + +/// The real `links` dir of pnpm's global virtual store when `nm`'s +/// `.modules.yaml` records one as its `virtualStoreDir`; `None` for the +/// default store, a store inside the project, or any other outside dir. +fn pnpm_global_virtual_store_of_sync(nm: &Path) -> Option { + let text = crate::utils::fs::read_regular_to_string_sync(&nm.join(PNPM_MODULES_YAML)).ok()?; + let recorded = parse_modules_yaml_virtual_store_dir(&text)?; + let links = std::fs::canonicalize(nm.join(recorded)).ok()?; + crate::patch::shared_store::is_pnpm_global_virtual_store_dir(&links).then_some(links) +} + +/// The global-virtual-store entries `nm`'s package links point into: for +/// each link (scoped ones under `@scope/`), the `node_modules` of the +/// entry holding its real target. Real dirs (an entry's own package) and +/// links resolving anywhere else are skipped. +fn gvs_link_targets_sync(nm: &Path, links: &Path) -> Vec { + let mut targets = Vec::new(); + for entry in list_dir_sync(nm).entries { + let Some(file_type) = entry.file_type else { + continue; + }; + if entry.name_str.starts_with('@') && file_type.is_dir() && !file_type.is_symlink() { + for scoped in list_dir_sync(&nm.join(&entry.name)).entries { + if scoped.file_type.is_some_and(|ft| ft.is_symlink()) { + let path = nm.join(&entry.name).join(&scoped.name); + targets.extend(gvs_entry_node_modules(&path, links)); + } + } + } else if file_type.is_symlink() && !entry.name_str.starts_with('.') { + targets.extend(gvs_entry_node_modules(&nm.join(&entry.name), links)); + } + } + targets +} + +/// The `links/////node_modules` dir that +/// `link`'s real target sits in, when it is inside `links`. +fn gvs_entry_node_modules(link: &Path, links: &Path) -> Option { + let real = std::fs::canonicalize(link).ok()?; + let below = real.strip_prefix(links).ok()?; + let parts: Vec<_> = below.components().take(5).collect(); + let [scope, name, version, hash, nm] = parts.as_slice() else { + return None; + }; + if nm.as_os_str() != "node_modules" { + return None; + } + let entry_nm = links + .join(scope) + .join(name) + .join(version) + .join(hash) + .join(nm); + is_dir_sync(&entry_nm).then_some(entry_nm) +} + +/// `(name, version)` a global-virtual-store entry's path advertises: +/// `links/@///…` or `links/@scope///…`. +fn gvs_entry_advertised(entry_nm: &Path, links: &Path) -> Option<(String, String)> { + let below = entry_nm.strip_prefix(links).ok()?; + let mut parts = below.components().map(|c| c.as_os_str().to_str()); + let (scope, name, version) = (parts.next()??, parts.next()??, parts.next()??); + let full_name = if scope == "@" { + name.to_string() + } else { + format!("{scope}/{name}") + }; + Some((full_name, version.to_string())) +} + /// `(name, version)` a `.vlt/` entry name advertises: the vlt store /// decoder over the lossless name, `None` for git/file/remote/workspace /// ids and for anything undecodable (which stays probeable). The pnpm @@ -1490,7 +1598,8 @@ impl NpmCrawler { return Vec::new(); } let Some(store) = relocated_pnpm_virtual_store_sync(nm_path) else { - return Vec::new(); + let entries = reachable_pnpm_global_virtual_store_entries_sync(nm_path); + return vec![NestedNodeModules::StoreEntries(entries)]; }; let entries = Self::list_pnpm_store_entries_sync(&store, false) .into_iter() @@ -1793,6 +1902,7 @@ impl NpmCrawler { let mut vlt_store: Option = None; let mut npm_store: Option = None; let mut relocated_pnpm_store: Option = None; + let mut global_store_entries: Vec = Vec::new(); let mut legacy_stores: Vec = Vec::new(); let mut children: Vec<(PathBuf, String, FileType)> = Vec::new(); // A store entry's links, kept only to find a link to the entry's @@ -1843,6 +1953,10 @@ impl NpmCrawler { if !store_entry && name_str == PNPM_MODULES_YAML { if entry.file_type.is_some_and(|ft| ft.is_file()) { relocated_pnpm_store = relocated_pnpm_virtual_store_sync(node_modules_path); + if relocated_pnpm_store.is_none() { + global_store_entries = + reachable_pnpm_global_virtual_store_entries_sync(node_modules_path); + } } continue; } @@ -1915,6 +2029,18 @@ impl NpmCrawler { let entries = Self::list_pnpm_store_entries_sync(&store_path, true); events.extend(Self::gather_store_entries(entries)); } + if !global_store_entries.is_empty() { + let entries = global_store_entries + .into_iter() + .map(|e| StoreEntryDir { + name: e.node_modules.display().to_string(), + advertised: e.advertised, + node_modules: e.node_modules, + listing: None, + }) + .collect(); + events.extend(Self::gather_store_entries(entries)); + } if let Some(store_path) = vlt_store { let entries = Self::vlt_store_entry_dirs(&store_path); events.extend(Self::gather_store_entries(entries)); @@ -4471,6 +4597,101 @@ mod tests { assert!(scan_paths(&root).await.is_empty()); } + /// #362: pnpm's global virtual store (`enableGlobalVirtualStore`) + /// records `virtualStoreDir` as `/v/links`, outside the + /// project. A transitive dependency lives only there, so the walks + /// follow this project's links into the store (and each entry's + /// dependency links on to sibling entries) and report every copy at + /// its real store path, where apply refuses it as shared. Entries no + /// link of this project reaches (another project's packages) stay + /// invisible. + #[tokio::test] + async fn test_pnpm_global_virtual_store_reachable_entries_are_walked() { + let tmp = tempfile::tempdir().unwrap(); + let base: PathBuf = std::fs::canonicalize(tmp.path()) + .unwrap() + .components() + .collect(); + let v10 = base.join("store").join("v10"); + std::fs::create_dir_all(v10.join("files")).unwrap(); + let links = v10.join("links"); + let odd_nm = links.join("@/is-odd/3.0.1/aaa/node_modules"); + write_pkg(&odd_nm.join("is-odd"), "is-odd", "3.0.1"); + let number_nm = links.join("@/is-number/6.0.0/bbb/node_modules"); + let number = number_nm.join("is-number"); + write_pkg(&number, "is-number", "6.0.0"); + link_dir(&number, &odd_nm.join("is-number")); + // A scoped transitive dep, linked back to is-odd (a cycle). + let frame = links.join("@babel/code-frame/7.0.0/ccc/node_modules/@babel/code-frame"); + write_pkg(&frame, "@babel/code-frame", "7.0.0"); + std::fs::create_dir_all(number_nm.join("@babel")).unwrap(); + link_dir(&frame, &number_nm.join("@babel/code-frame")); + let frame_nm = links.join("@babel/code-frame/7.0.0/ccc/node_modules"); + link_dir(&odd_nm.join("is-odd"), &frame_nm.join("is-odd")); + // Another project's package in the same store. + let other = links.join("@/left-pad/1.3.0/ddd/node_modules/left-pad"); + write_pkg(&other, "left-pad", "1.3.0"); + + let root = base.join("proj"); + let nm = root.join("node_modules"); + std::fs::create_dir_all(nm.join(".pnpm/node_modules")).unwrap(); + std::fs::write( + nm.join(".modules.yaml"), + "{\"layoutVersion\": 5, \"virtualStoreDir\": \"../../store/v10/links\"}", + ) + .unwrap(); + link_dir(&odd_nm.join("is-odd"), &nm.join("is-odd")); + + let scanned = scan_paths(&root).await; + let purls: Vec<&str> = scanned.iter().map(|(p, _)| p.as_str()).collect(); + assert!( + scanned.contains(&("pkg:npm/is-number@6.0.0".to_string(), number.clone())), + "{scanned:?}" + ); + assert!( + scanned.contains(&("pkg:npm/@babel/code-frame@7.0.0".to_string(), frame.clone())), + "{scanned:?}" + ); + assert_eq!( + purls + .iter() + .filter(|p| **p == "pkg:npm/is-odd@3.0.1") + .count(), + 1, + "{scanned:?}" + ); + assert!(!purls.contains(&"pkg:npm/left-pad@1.3.0"), "{scanned:?}"); + + let found = NpmCrawler::new() + .find_by_purls( + &nm, + &[ + "pkg:npm/is-number@6.0.0".to_string(), + "pkg:npm/@babel/code-frame@7.0.0".to_string(), + "pkg:npm/left-pad@1.3.0".to_string(), + ], + ) + .await + .unwrap(); + let paths = |purl: &str| { + found + .get(purl) + .map(|c| c.iter().map(|p| p.path.clone()).collect::>()) + }; + assert_eq!(paths("pkg:npm/is-number@6.0.0"), Some(vec![number])); + assert_eq!(paths("pkg:npm/@babel/code-frame@7.0.0"), Some(vec![frame])); + assert_eq!(paths("pkg:npm/left-pad@1.3.0"), None); + + // Without the store's `files/` beside `links` it is not pnpm's + // global virtual store, so the outside dir is not walked at all. + std::fs::remove_dir(v10.join("files")).unwrap(); + let scanned = scan_paths(&root).await; + assert!( + !scanned.iter().any(|(p, _)| p == "pkg:npm/is-number@6.0.0"), + "{scanned:?}" + ); + } + /// Old pnpm records `virtualStoreDir` as an absolute path. A store /// inside the project must still be walked when the project is /// reached through a different spelling than the recorded one (a diff --git a/crates/socket-patch-core/src/patch/shared_store.rs b/crates/socket-patch-core/src/patch/shared_store.rs index 4c8eea314..cfec94f7f 100644 --- a/crates/socket-patch-core/src/patch/shared_store.rs +++ b/crates/socket-patch-core/src/patch/shared_store.rs @@ -126,13 +126,9 @@ fn shared_store_of_blocking(pkg_path: &Path) -> Option { continue; }; let parent = dir.parent(); - let parent_name = parent.and_then(|p| p.file_name()).and_then(|n| n.to_str()); // pnpm: /v/links, with the store's `files/` beside it. - if name == "links" - && parent_name.is_some_and(is_pnpm_store_version_dir) - && parent.is_some_and(|p| p.join("files").is_dir()) - { + if is_pnpm_global_virtual_store_dir(dir) { return Some(SharedStore { kind: SharedStoreKind::PnpmGlobalVirtualStore, real_path: real.clone(), @@ -160,6 +156,19 @@ fn shared_store_of_blocking(pkg_path: &Path) -> Option { None } +/// Whether `dir` is the `links` directory of pnpm's global virtual store: +/// `/v/links`, with the store's `files/` content directory +/// beside it. Callers pass a real (canonical) path. +pub(crate) fn is_pnpm_global_virtual_store_dir(dir: &Path) -> bool { + let parent = dir.parent(); + dir.file_name().is_some_and(|n| n == "links") + && parent + .and_then(|p| p.file_name()) + .and_then(|n| n.to_str()) + .is_some_and(is_pnpm_store_version_dir) + && parent.is_some_and(|p| p.join("files").is_dir()) +} + /// `v3`, `v10`, `v11`, …: the layout-version directory of a pnpm store. fn is_pnpm_store_version_dir(name: &str) -> bool { name.strip_prefix('v') diff --git a/docs/ecosystems.md b/docs/ecosystems.md index 1beb7b91d..ebd61119e 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -152,13 +152,15 @@ moved it to, as recorded in `node_modules/.modules.yaml`), vlt's `node_modules/.vlt`, Bun's isolated-linker store `node_modules/.bun`, Deno's isolated `nodeModulesDir` store `node_modules/.deno`, and `node_modules/.store`, written by npm's `install-strategy=linked` and by -Yarn 4's pnpm linker (where each entry's `package/` dir is the copy). A recorded virtual store outside the project, -notably pnpm's global virtual store (`enableGlobalVirtualStore`, under the -pnpm store directory), is not walked: other projects on the machine load -the same files, so patching it in place would patch them as well. -For the same reason, agent-mode `apply` and `rollback` fail on a direct -dependency whose `node_modules/` link resolves into that store -(`/v/links`) instead of writing through it. PDM's symlink install +Yarn 4's pnpm linker (where each entry's `package/` dir is the copy). A recorded virtual store outside the project is not +listed: other projects on the machine load the same files, so patching it +in place would patch them as well. For pnpm's global virtual store +(`enableGlobalVirtualStore`, `/v/links` under the pnpm store +directory), only the entries this project reaches are walked: its +`node_modules/` links into the store, and each entry's dependency +links on to other entries. Agent-mode `apply` and `rollback` then fail on +those copies, direct and transitive alike, instead of writing through +them, and never report a transitive one as "not installed". PDM's symlink install cache gets the same treatment: with `install.cache` and `cache_method = symlink` (PDM 2.0–2.12), `site-packages/` links into `/packages//lib`, and that package is refused too. The error From e4f35dd9741102de3d0989a2e2e49a6e859afe6f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 13:38:19 +0000 Subject: [PATCH 3/4] Walk the global virtual store from workspace members pnpm writes .modules.yaml only at a workspace root, so a member's node_modules (just its own links into the global virtual store) never started the store walk, and a member's transitive dependency still read as lockfile-only success. When an importer node_modules has links but no .modules.yaml, the nearest enclosing record now names the store, and the member's own links seed the walk. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LPd97gQyyjAeSiTVmLw4p3 --- .../tests/apply/pnpm_global_virtual_store.rs | 45 +++++++ .../src/crawlers/npm_crawler.rs | 126 +++++++++++++++++- docs/ecosystems.md | 4 +- 3 files changed, 172 insertions(+), 3 deletions(-) diff --git a/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs b/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs index 87ac1f876..e48a5c583 100644 --- a/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs +++ b/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs @@ -171,3 +171,48 @@ fn other_projects_global_virtual_store_entries_stay_invisible() { assert_eq!(ev["errorCode"], "package_not_installed", "{ev}"); assert_eq!(std::fs::read(left_pad.join("index.js")).unwrap(), BEFORE); } + +/// A pnpm workspace (#362, review on #829): pnpm writes `.modules.yaml` +/// only at the workspace root, and a member's `node_modules` holds just +/// its own links into the store. The member's transitive `is-number` is +/// refused as shared too, not reported as a lockfile-only skip. +#[test] +fn workspace_member_transitive_global_virtual_store_dep_is_refused() { + let tmp = tempfile::tempdir().unwrap(); + let (proj, number, _) = stage(tmp.path()); + // Move the direct link from the root to member `packages/a`. + let root_nm = proj.join("node_modules"); + let odd = std::fs::canonicalize(root_nm.join("is-odd")).unwrap(); + #[cfg(unix)] + std::fs::remove_file(root_nm.join("is-odd")).unwrap(); + #[cfg(windows)] + std::fs::remove_dir(root_nm.join("is-odd")).unwrap(); + std::fs::create_dir_all(root_nm.join(".pnpm").join("node_modules")).unwrap(); + std::fs::write( + proj.join("pnpm-workspace.yaml"), + "packages:\n - packages/*\n", + ) + .unwrap(); + let member = proj.join("packages").join("a"); + let member_nm = member.join("node_modules"); + std::fs::create_dir_all(&member_nm).unwrap(); + std::fs::write( + member.join("package.json"), + r#"{ "name": "a", "version": "0.0.0", "dependencies": { "is-odd": "3.0.1" } }"#, + ) + .unwrap(); + link_dir(&odd, &member_nm.join("is-odd")); + write_manifest(&proj, "pkg:npm/is-number@6.0.0"); + + let (code, stdout, stderr) = run_with_env( + &proj, + &["apply", "--offline", "--json"], + &[("SOCKET_TELEMETRY_DISABLED", "1")], + ); + let v = parse_json_envelope(&stdout); + assert_ne!(code, 0, "{v}\n{stderr}"); + let ev = event(&v, "pkg:npm/is-number@6.0.0"); + assert_ne!(ev["errorCode"], "package_not_installed", "{ev}"); + assert!(ev.to_string().contains("enableGlobalVirtualStore"), "{ev}"); + assert_eq!(std::fs::read(number.join("index.js")).unwrap(), BEFORE); +} diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs index 887f9fc53..72835beda 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs @@ -638,13 +638,55 @@ fn reachable_pnpm_global_virtual_store_entries_sync(nm: &Path) -> Vec Option { - let text = crate::utils::fs::read_regular_to_string_sync(&nm.join(PNPM_MODULES_YAML)).ok()?; + let own = nm.join(PNPM_MODULES_YAML); + let modules_yaml = if std::fs::symlink_metadata(&own).is_ok() { + own + } else { + enclosing_pnpm_modules_yaml_sync(nm)? + }; + let text = crate::utils::fs::read_regular_to_string_sync(&modules_yaml).ok()?; let recorded = parse_modules_yaml_virtual_store_dir(&text)?; - let links = std::fs::canonicalize(nm.join(recorded)).ok()?; + let links = std::fs::canonicalize(modules_yaml.parent()?.join(recorded)).ok()?; crate::patch::shared_store::is_pnpm_global_virtual_store_dir(&links).then_some(links) } +/// The nearest `/node_modules/.modules.yaml` above the importer +/// holding `nm` (a pnpm workspace root's record, for a member). +fn enclosing_pnpm_modules_yaml_sync(nm: &Path) -> Option { + nm.parent()?.ancestors().skip(1).find_map(|dir| { + let candidate = dir.join("node_modules").join(PNPM_MODULES_YAML); + std::fs::symlink_metadata(&candidate) + .is_ok_and(|m| m.is_file()) + .then_some(candidate) + }) +} + +/// Whether an importer `node_modules` listing without a `.modules.yaml` +/// may still be a pnpm workspace member linked into the global virtual +/// store (see [`pnpm_global_virtual_store_of_sync`]): it holds a package +/// link or a scope dir. Keeps the enclosing-record lookup off plain +/// npm/yarn trees, whose nested `node_modules` hold real dirs only. +fn may_be_gvs_workspace_member(listing: &Listing) -> bool { + !listing + .entries + .iter() + .any(|e| e.name_str == PNPM_MODULES_YAML) + && listing.entries.iter().any(|e| { + !e.name_str.starts_with('.') + && e.file_type.is_some_and(|ft| { + ft.is_symlink() || (ft.is_dir() && e.name_str.starts_with('@')) + }) + }) +} + /// The global-virtual-store entries `nm`'s package links point into: for /// each link (scoped ones under `@scope/`), the `node_modules` of the /// entry holding its real target. Real dirs (an entry's own package) and @@ -1502,10 +1544,17 @@ impl NpmCrawler { .then_some((index, pkg_path)) }) .collect(); + let gvs_member = !store_entry && may_be_gvs_workspace_member(&listing); let mut nested = Self::collect_nested_node_modules(&nm_path, listing); if store_entry { nested.extend(own_package_nested_node_modules_sync(&nm_path)); } + if gvs_member { + let entries = reachable_pnpm_global_virtual_store_entries_sync(&nm_path); + if !entries.is_empty() { + nested.push(NestedNodeModules::StoreEntries(entries)); + } + } ResolverVisit { store_entry, matched, @@ -1898,6 +1947,7 @@ impl NpmCrawler { store_entry: bool, ) -> Vec { let listing = listing.unwrap_or_else(|| list_dir_sync(node_modules_path)); + let gvs_member = !store_entry && may_be_gvs_workspace_member(&listing); let mut pnpm_shaped_stores: Vec<(PathBuf, StoreLayout)> = Vec::new(); let mut vlt_store: Option = None; let mut npm_store: Option = None; @@ -2008,6 +2058,10 @@ impl NpmCrawler { .flatten() .collect(); events.extend(Self::gather_own_packages(node_modules_path, own_packages)); + if gvs_member { + global_store_entries = + reachable_pnpm_global_virtual_store_entries_sync(node_modules_path); + } for (store_path, layout) in pnpm_shaped_stores { let entries = Self::list_pnpm_shaped_store_entries_sync(&store_path, layout, true); @@ -4692,6 +4746,74 @@ mod tests { ); } + /// #362 in a workspace: pnpm writes `.modules.yaml` only at the + /// workspace root, so a member's `node_modules` (holding just its + /// own links into the global virtual store) has none. The member's + /// transitive deps must still be found through the root's record, + /// seeded from the member's own links only. + #[tokio::test] + async fn test_pnpm_global_virtual_store_workspace_member_entries_are_walked() { + let tmp = tempfile::tempdir().unwrap(); + let base: PathBuf = std::fs::canonicalize(tmp.path()) + .unwrap() + .components() + .collect(); + let v11 = base.join("store").join("v11"); + std::fs::create_dir_all(v11.join("files")).unwrap(); + let links = v11.join("links"); + let odd_nm = links.join("@/is-odd/3.0.1/aaa/node_modules"); + write_pkg(&odd_nm.join("is-odd"), "is-odd", "3.0.1"); + let number = links.join("@/is-number/6.0.0/bbb/node_modules/is-number"); + write_pkg(&number, "is-number", "6.0.0"); + link_dir(&number, &odd_nm.join("is-number")); + // Another project's package in the same store. + let other = links.join("@/left-pad/1.3.0/ddd/node_modules/left-pad"); + write_pkg(&other, "left-pad", "1.3.0"); + + let root = base.join("proj"); + let root_nm = root.join("node_modules"); + std::fs::create_dir_all(root_nm.join(".pnpm/node_modules")).unwrap(); + std::fs::write( + root_nm.join(".modules.yaml"), + "{\"layoutVersion\": 5, \"virtualStoreDir\": \"../../store/v11/links\"}", + ) + .unwrap(); + // The root's hoisted links live under `.pnpm/node_modules`. + link_dir(&other, &root_nm.join(".pnpm/node_modules/left-pad")); + std::fs::write(root.join("package.json"), "{\"name\": \"root\"}").unwrap(); + let member_nm = root.join("packages/a/node_modules"); + std::fs::create_dir_all(&member_nm).unwrap(); + link_dir(&odd_nm.join("is-odd"), &member_nm.join("is-odd")); + + let scanned = scan_paths(&root).await; + assert!( + scanned.contains(&("pkg:npm/is-number@6.0.0".to_string(), number.clone())), + "{scanned:?}" + ); + assert!( + !scanned.iter().any(|(p, _)| p == "pkg:npm/left-pad@1.3.0"), + "{scanned:?}" + ); + + let found = NpmCrawler::new() + .find_by_purls( + &member_nm, + &[ + "pkg:npm/is-number@6.0.0".to_string(), + "pkg:npm/left-pad@1.3.0".to_string(), + ], + ) + .await + .unwrap(); + let paths = |purl: &str| { + found + .get(purl) + .map(|c| c.iter().map(|p| p.path.clone()).collect::>()) + }; + assert_eq!(paths("pkg:npm/is-number@6.0.0"), Some(vec![number])); + assert_eq!(paths("pkg:npm/left-pad@1.3.0"), None); + } + /// Old pnpm records `virtualStoreDir` as an absolute path. A store /// inside the project must still be walked when the project is /// reached through a different spelling than the recorded one (a diff --git a/docs/ecosystems.md b/docs/ecosystems.md index ebd61119e..43a372698 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -158,7 +158,9 @@ in place would patch them as well. For pnpm's global virtual store (`enableGlobalVirtualStore`, `/v/links` under the pnpm store directory), only the entries this project reaches are walked: its `node_modules/` links into the store, and each entry's dependency -links on to other entries. Agent-mode `apply` and `rollback` then fail on +links on to other entries. A workspace member's `node_modules` has no +`.modules.yaml` of its own (pnpm writes it only at the workspace root), so +the root's record is used, and the member's own links seed the walk. Agent-mode `apply` and `rollback` then fail on those copies, direct and transitive alike, instead of writing through them, and never report a transitive one as "not installed". PDM's symlink install cache gets the same treatment: with `install.cache` and From 5513533a5988f07ee4259d3b0a00b2e61dd24ba7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 5 Oct 2026 15:43:06 +0000 Subject: [PATCH 4/4] Resolve the importer before the workspace-root lookup Under the default --cwd . a workspace member's walked node_modules is the relative path ./node_modules, whose lexical ancestors stop at ".", so the root's .modules.yaml was never found and a member's transitive global-store dep again read as a lockfile-only skip. The importer is now resolved to its real path before walking up. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01LPd97gQyyjAeSiTVmLw4p3 --- .../tests/apply/pnpm_global_virtual_store.rs | 43 +++++++++++++++++++ .../src/crawlers/npm_crawler.rs | 14 +++++- 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs b/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs index e48a5c583..1c84dd21c 100644 --- a/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs +++ b/crates/socket-patch-cli/tests/apply/pnpm_global_virtual_store.rs @@ -216,3 +216,46 @@ fn workspace_member_transitive_global_virtual_store_dep_is_refused() { assert!(ev.to_string().contains("enableGlobalVirtualStore"), "{ev}"); assert_eq!(std::fs::read(number.join("index.js")).unwrap(), BEFORE); } + +/// The same workspace member, with `apply` run from the member's own +/// dir under the default `--cwd .`: the walked `node_modules` is then a +/// relative path whose lexical ancestors never reach the workspace root, +/// so the root's `.modules.yaml` must be found from the real path. +#[test] +fn workspace_member_refusal_holds_when_run_from_the_member() { + let tmp = tempfile::tempdir().unwrap(); + let (proj, number, _) = stage(tmp.path()); + let root_nm = proj.join("node_modules"); + let odd = std::fs::canonicalize(root_nm.join("is-odd")).unwrap(); + #[cfg(unix)] + std::fs::remove_file(root_nm.join("is-odd")).unwrap(); + #[cfg(windows)] + std::fs::remove_dir(root_nm.join("is-odd")).unwrap(); + std::fs::write( + proj.join("pnpm-workspace.yaml"), + "packages:\n - packages/*\n", + ) + .unwrap(); + let member = proj.join("packages").join("a"); + let member_nm = member.join("node_modules"); + std::fs::create_dir_all(&member_nm).unwrap(); + std::fs::write( + member.join("package.json"), + r#"{ "name": "a", "version": "0.0.0", "dependencies": { "is-odd": "3.0.1" } }"#, + ) + .unwrap(); + link_dir(&odd, &member_nm.join("is-odd")); + write_manifest(&member, "pkg:npm/is-number@6.0.0"); + + let (code, stdout, stderr) = run_with_env( + &member, + &["apply", "--offline", "--json", "--cwd", "."], + &[("SOCKET_TELEMETRY_DISABLED", "1")], + ); + let v = parse_json_envelope(&stdout); + assert_ne!(code, 0, "{v}\n{stderr}"); + let ev = event(&v, "pkg:npm/is-number@6.0.0"); + assert_ne!(ev["errorCode"], "package_not_installed", "{ev}"); + assert!(ev.to_string().contains("enableGlobalVirtualStore"), "{ev}"); + assert_eq!(std::fs::read(number.join("index.js")).unwrap(), BEFORE); +} diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs index cd5881226..443ad9f14 100644 --- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs +++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs @@ -767,9 +767,19 @@ fn pnpm_global_virtual_store_of_sync(nm: &Path) -> Option { } /// The nearest `/node_modules/.modules.yaml` above the importer -/// holding `nm` (a pnpm workspace root's record, for a member). +/// holding `nm` (a pnpm workspace root's record, for a member). The +/// importer is resolved to its real path first: under the default +/// `--cwd .` `nm` is the relative `node_modules`, whose lexical +/// ancestors never reach the workspace root. fn enclosing_pnpm_modules_yaml_sync(nm: &Path) -> Option { - nm.parent()?.ancestors().skip(1).find_map(|dir| { + let parent = nm.parent()?; + let parent = if parent.as_os_str().is_empty() { + Path::new(".") + } else { + parent + }; + let importer = std::fs::canonicalize(parent).ok()?; + importer.ancestors().skip(1).find_map(|dir| { let candidate = dir.join("node_modules").join(PNPM_MODULES_YAML); std::fs::symlink_metadata(&candidate) .is_ok_and(|m| m.is_file())