Skip to content

Commit 61cfb9b

Browse files
Fix npm crawler missing relocated dependency stores (#359, #362) (#365)
* Start fix for #359, #362 Assisted-by: Claude Code:claude-opus-5-5 * Find packages in npm .store and pnpm virtualStoreDir With npm's install-strategy=linked, or a pnpm virtualStoreDir moved away from node_modules/.pnpm, transitive dependencies live only in a store the crawler never walked: it matched stores by fixed name and skipped every other hidden dir. apply, scan and vendor then reported those packages as not installed and left them unpatched. The crawler now walks npm's node_modules/.store (including scoped entries one level down) and the pnpm store that .modules.yaml records, in scan, apply's resolver and the peer-copy fan-out. A recorded store outside the project, such as pnpm's global virtual store shared by other projects, is still left alone. Assisted-by: Claude Code:claude-opus-5-5 * Add real npm/pnpm e2e for relocated stores Covers #359 (npm install-strategy=linked, transitive dep only in node_modules/.store: apply, rollback and vendor) and #362 (pnpm virtualStoreDir at .vstore and node_modules/.custom: apply and rollback), each checking that Node loads the patched copy. Both fail on main with package_not_installed. Assisted-by: Claude Code:claude-opus-5-5 * Document npm .store and pnpm virtualStoreDir walks Assisted-by: Claude Code:claude-opus-5-5 * Keep peer fan-out to stores the copy lives in The fan-out that patches every peer-variant copy looked up a relocated pnpm store from any ancestor directory. For a project nested inside another pnpm project, that could pick the outer project's store and patch copies this project never loads. A relocated store now counts only when the copy being patched sits inside it. Assisted-by: Claude Code:claude-opus-5-5 * Skip the linked-store e2e below npm 9.4 npm 9.0-9.3 ignore install-strategy=linked and install the hoisted tree, so the npm 9.0.0 compatibility cell has no .store to test. Assisted-by: Claude Code:claude-opus-5-5 * Keep the caller's path spelling for relocated pnpm stores On macOS the temp dir sits under the /var -> /private/var link, so the peer-variant fan-out from a direct dep's importer link only matched the relocated store on the canonical chain and reported the copy as /private/var/..., unlike the other store layouts. Containment is now also checked canonically, so the store is kept as the caller spelled it. The regression test reaches its project through a linked ancestor so this is covered on every platform. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HcXxpY7X3MC84EEfA8jws6 * Refuse relocated pnpm stores outside a relative importer With the CLI's default `--cwd .` the importer is the empty relative path, and `strip_prefix("")` accepts every path, so an absolute `virtualStoreDir` such as pnpm's global virtual store passed the in-project check and was crawled (and would be patched in place). The store must now sit strictly below the importer by plain child names: no root, prefix or `..` component. Reported by Cursor Bugbot on #365. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HcXxpY7X3MC84EEfA8jws6 * Normalize separators before mklink in crawler tests The Windows test helper shells out to `cmd /C mklink /J`, which reads a `/` inside a path (`is-odd@3.0.1/node_modules/is-number`) as a switch, so the linked-store and relocated-store tests failed on Windows with "Invalid switch". Rebuild both paths from their components first so every separator is `\`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HcXxpY7X3MC84EEfA8jws6 * Walk absolute in-project pnpm stores from cwd . Old pnpm records virtualStoreDir as an absolute path. With the default --cwd . the importer is the empty path, which is never a prefix of an absolute store, so a store inside the project was skipped and its transitive packages stayed package_not_installed. An absolute store is now also compared with both sides canonicalized, which also covers a project reached through a linked ancestor. A store that resolves outside the project, such as pnpm's global virtual store, is still refused. Reported by Cursor Bugbot on #365. Assisted-by: Claude Code:claude-opus-5-5 * Skip an absolute default pnpm store by its tail pnpm's default store, node_modules/.pnpm, is walked by name. An old or Windows pnpm can record it in .modules.yaml as an absolute path, and from --cwd . (or through a linked ancestor) that spelling missed the lexical default-location check. The relocated-store reader then accepted it too, so every store copy was reported twice. The check now compares the importer-relative tail, so any spelling of the default store, or of node_modules itself, is skipped. Reported by Cursor Bugbot on #365. Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9d718cf commit 61cfb9b

6 files changed

Lines changed: 1105 additions & 2 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,11 @@ limits, and required install commands.
169169
- Transient apply locks are removed on normal command exit; no-op scans and full
170170
reversal avoid leaving unused `.socket/` state. Terminal output, telemetry
171171
timeouts, and update-check handling are more consistent.
172+
- Agent mode finds transitive npm packages in npm's linked store
173+
(`install-strategy=linked`, `node_modules/.store`) and in a relocated pnpm
174+
`virtualStoreDir`, instead of reporting them `package_not_installed` (#359,
175+
#362). A store outside the project, such as pnpm's global virtual store, is
176+
shared with other projects and is still not patched in place.
172177
- npm locks keep their own layout when edited. `scan --mode hosted`,
173178
`scan --mode vendored`, `rollback` and `vendor --revert`
174179
re-serialized `package-lock.json` / `npm-shrinkwrap.json` with LF line

‎crates/socket-patch-cli/tests/e2e_vendor_npm_build.rs‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1273,3 +1273,124 @@ fn npm6_installs_a_vendored_v2_lock_from_its_legacy_mirror() {
12731273
&[VexVia::Apply, VexVia::Vendor],
12741274
);
12751275
}
1276+
1277+
/// The one real package dir npm's linked store holds for `name@version`
1278+
/// (`node_modules/.store/<name>@<version>-<hash>/node_modules/<name>`).
1279+
fn linked_store_copy(proj: &Path, name: &str, version: &str) -> Option<PathBuf> {
1280+
let prefix = format!("{name}@{version}-");
1281+
std::fs::read_dir(proj.join("node_modules/.store"))
1282+
.ok()?
1283+
.flatten()
1284+
.find(|e| e.file_name().to_string_lossy().starts_with(&prefix))
1285+
.map(|e| e.path().join("node_modules").join(name))
1286+
}
1287+
1288+
/// What Node loads for `dep` when `from` requires it.
1289+
fn node_loads(proj: &Path, from: &str, dep: &str) -> String {
1290+
let script = format!(
1291+
"const p=require('path');process.stdout.write(require('fs').readFileSync(\
1292+
require.resolve('{dep}',{{paths:[p.dirname(require.resolve('{from}'))]}}),'utf8'))"
1293+
);
1294+
let out = Command::new("node")
1295+
.args(["-e", &script])
1296+
.current_dir(proj)
1297+
.output()
1298+
.expect("node runs");
1299+
assert!(
1300+
out.status.success(),
1301+
"{}",
1302+
npm_e2e_common::output_text(&out)
1303+
);
1304+
String::from_utf8_lossy(&out.stdout).into_owned()
1305+
}
1306+
1307+
/// #359: with `install-strategy=linked` (npm 9.4+), a transitive package
1308+
/// is a real dir ONLY in `node_modules/.store`. `apply` must patch the
1309+
/// copy Node loads, `rollback` must restore it, and `vendor` must build
1310+
/// its tarball from it; before the fix all three reported
1311+
/// `package_not_installed`.
1312+
#[test]
1313+
fn npm_linked_strategy_transitive_package_is_patched_rolled_back_and_vendored() {
1314+
let suite = "e2e_vendor_npm_build (linked)";
1315+
let Some(major) = npm_major_or_skip(suite) else {
1316+
return;
1317+
};
1318+
// `install-strategy=linked` arrived in npm 9.4.0 (measured: 9.0-9.3
1319+
// ignore it and install the hoisted tree).
1320+
let minor: u32 = npm_e2e_common::npm_version()
1321+
.and_then(|v| v.split('.').nth(1)?.parse().ok())
1322+
.unwrap_or(0);
1323+
if major < 9 || (major == 9 && minor < 4) {
1324+
println!("SKIP {suite}: npm {major}.{minor} has no install-strategy=linked");
1325+
return;
1326+
}
1327+
let tmp = tempfile::tempdir().unwrap();
1328+
let proj = tmp.path().join("proj");
1329+
std::fs::create_dir_all(&proj).unwrap();
1330+
std::fs::write(
1331+
proj.join("package.json"),
1332+
r#"{"name":"linked-capstone","version":"0.0.0","private":true}"#,
1333+
)
1334+
.unwrap();
1335+
std::fs::write(proj.join(".npmrc"), "install-strategy=linked\n").unwrap();
1336+
let cache = tmp.path().join("npm-cache");
1337+
// is-odd@3.0.1 depends on is-number@6.0.0: transitive, so store-only.
1338+
if !npm_e2e_common::install_fixture(suite, &proj, &cache, "is-odd@3.0.1") {
1339+
return;
1340+
}
1341+
let copy = linked_store_copy(&proj, "is-number", "6.0.0")
1342+
.expect("npm's linked strategy put is-number in node_modules/.store");
1343+
assert!(!proj.join("node_modules/is-number").exists());
1344+
let index = copy.join("index.js");
1345+
let orig = std::fs::read(&index).unwrap();
1346+
let patched: Vec<u8> = [MARKER.as_bytes(), orig.as_slice()].concat();
1347+
stage_patch_with_vuln(
1348+
&proj,
1349+
"pkg:npm/is-number@6.0.0",
1350+
"package/index.js",
1351+
&orig,
1352+
&patched,
1353+
"GHSA-link-npm-real",
1354+
);
1355+
// Rollback restores from the before-blob.
1356+
std::fs::write(proj.join(".socket/blobs").join(git_sha256(&orig)), &orig).unwrap();
1357+
let cwd = proj.to_str().unwrap();
1358+
1359+
let (code, stdout, stderr) = run_socket(&proj, &["apply", "--json", "--offline", "--cwd", cwd]);
1360+
assert_eq!(code, 0, "apply failed.\n{stdout}\n{stderr}");
1361+
let env = parse_envelope(&stdout);
1362+
assert_eq!(env["status"], "success", "{env}");
1363+
assert_eq!(env["summary"]["applied"], 1, "{env}");
1364+
assert_eq!(std::fs::read(&index).unwrap(), patched);
1365+
assert!(node_loads(&proj, "is-odd", "is-number").starts_with(MARKER));
1366+
1367+
let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--json", "--cwd", cwd]);
1368+
assert_eq!(code, 0, "rollback failed.\n{stdout}\n{stderr}");
1369+
assert_eq!(std::fs::read(&index).unwrap(), orig);
1370+
1371+
// Rollback drops the record from the manifest; stage it again.
1372+
stage_patch_with_vuln(
1373+
&proj,
1374+
"pkg:npm/is-number@6.0.0",
1375+
"package/index.js",
1376+
&orig,
1377+
&patched,
1378+
"GHSA-link-npm-real",
1379+
);
1380+
let lock_before = std::fs::read(proj.join("package-lock.json")).unwrap();
1381+
let (code, stdout, stderr) =
1382+
run_socket(&proj, &["vendor", "--json", "--offline", "--cwd", cwd]);
1383+
assert_eq!(code, 0, "vendor failed.\n{stdout}\n{stderr}");
1384+
assert_eq!(parse_envelope(&stdout)["status"], "success", "{stdout}");
1385+
assert_ne!(
1386+
std::fs::read(proj.join("package-lock.json")).unwrap(),
1387+
lock_before,
1388+
"vendor must rewire the lock: {stdout}"
1389+
);
1390+
let (code, stdout, stderr) = run_socket(&proj, &["vendor", "--revert", "--json", "--cwd", cwd]);
1391+
assert_eq!(code, 0, "vendor --revert failed.\n{stdout}\n{stderr}");
1392+
assert_eq!(
1393+
std::fs::read(proj.join("package-lock.json")).unwrap(),
1394+
lock_before
1395+
);
1396+
}

‎crates/socket-patch-cli/tests/e2e_vendor_pnpm_build.rs‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1799,3 +1799,85 @@ fn run_legacy_capstone(pm: &str, lock_head: &str) {
17991799
assert!(!proj.join(".socket/vendor").exists());
18001800
eprintln!("REVERT OK ({pm})");
18011801
}
1802+
1803+
/// #362: pnpm's `virtualStoreDir` moves the virtual store, and a
1804+
/// transitive dependency lives only there. Agent-mode `apply` must find
1805+
/// it through `node_modules/.modules.yaml` and patch the copy Node
1806+
/// loads, and `rollback` must restore it: both for a store next to
1807+
/// `node_modules` and for one inside it under a custom hidden name.
1808+
#[test]
1809+
fn pnpm_agent_apply_patches_a_transitive_dep_in_a_relocated_virtual_store() {
1810+
if !has_corepack_pm(PNPM_PRIMARY) {
1811+
println!("SKIP: `corepack {PNPM_PRIMARY}` unavailable");
1812+
return;
1813+
}
1814+
for (setting, store_rel) in [
1815+
(".vstore", ".vstore"),
1816+
("node_modules/.custom", "node_modules/.custom"),
1817+
] {
1818+
let tmp = tempfile::tempdir().unwrap();
1819+
let proj = tmp.path().join("proj");
1820+
std::fs::create_dir_all(&proj).unwrap();
1821+
std::fs::write(
1822+
proj.join("package.json"),
1823+
r#"{"name":"vsd","version":"0.0.0","private":true,"dependencies":{"is-odd":"3.0.1"}}"#,
1824+
)
1825+
.unwrap();
1826+
std::fs::write(
1827+
proj.join("pnpm-workspace.yaml"),
1828+
format!("virtualStoreDir: {setting}\n"),
1829+
)
1830+
.unwrap();
1831+
let store = tmp.path().join("pnpm-store");
1832+
let install = corepack(
1833+
&proj,
1834+
PNPM_PRIMARY,
1835+
&["install", "--store-dir", store.to_str().unwrap()],
1836+
);
1837+
if !install.status.success() {
1838+
assert!(!pnpm_required(), "fixture install failed: {install:?}");
1839+
println!("SKIP: fixture `pnpm install` failed: {install:?}");
1840+
return;
1841+
}
1842+
// is-odd@3.0.1 depends on is-number@6.0.0: transitive, store-only.
1843+
let copy = proj
1844+
.join(store_rel)
1845+
.join("is-number@6.0.0/node_modules/is-number");
1846+
let index = copy.join("index.js");
1847+
let orig = std::fs::read(&index)
1848+
.unwrap_or_else(|e| panic!("{setting}: pnpm put is-number at {}: {e}", copy.display()));
1849+
let patched: Vec<u8> = [MARKER.as_bytes(), orig.as_slice()].concat();
1850+
stage_patch(
1851+
&proj,
1852+
"pkg:npm/is-number@6.0.0",
1853+
"package/index.js",
1854+
&orig,
1855+
&patched,
1856+
);
1857+
std::fs::write(proj.join(".socket/blobs").join(git_sha256(&orig)), &orig).unwrap();
1858+
let cwd = proj.to_str().unwrap();
1859+
1860+
let (code, stdout, stderr) =
1861+
run_socket(&proj, &["apply", "--json", "--offline", "--cwd", cwd]);
1862+
assert_eq!(code, 0, "{setting}: apply failed.\n{stdout}\n{stderr}");
1863+
let env = parse_envelope(&stdout);
1864+
assert_eq!(env["summary"]["applied"], 1, "{setting}: {env}");
1865+
assert_eq!(std::fs::read(&index).unwrap(), patched, "{setting}");
1866+
// Node loads that very copy.
1867+
let script = "const p=require('path');process.stdout.write(require('fs').readFileSync(\
1868+
require.resolve('is-number',{paths:[p.dirname(require.resolve('is-odd'))]}),'utf8'))";
1869+
let out = Command::new("node")
1870+
.args(["-e", script])
1871+
.current_dir(&proj)
1872+
.output()
1873+
.expect("node runs");
1874+
assert!(
1875+
String::from_utf8_lossy(&out.stdout).starts_with(MARKER),
1876+
"{setting}: {out:?}"
1877+
);
1878+
1879+
let (code, stdout, stderr) = run_socket(&proj, &["rollback", "--json", "--cwd", cwd]);
1880+
assert_eq!(code, 0, "{setting}: rollback failed.\n{stdout}\n{stderr}");
1881+
assert_eq!(std::fs::read(&index).unwrap(), orig, "{setting}");
1882+
}
1883+
}
Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
//! `scan` with the default `--cwd .` must not walk a pnpm `virtualStoreDir`
2+
//! outside the project.
3+
//!
4+
//! The default cwd makes the importer the empty relative path, which a
5+
//! bare `strip_prefix` accepts as a prefix of every path: an absolute
6+
//! recorded store (pnpm's global virtual store, `<store-dir>/v10/links`,
7+
//! shared by every project on the machine) then passed the in-project
8+
//! check and was crawled, so `apply` would patch the other projects too
9+
//! (#361). A store inside the project is still walked from the same cwd,
10+
//! whether it is recorded relative or absolute.
11+
12+
use std::path::{Path, PathBuf};
13+
use std::process::Command;
14+
15+
use wiremock::matchers::{method, path};
16+
use wiremock::{Mock, MockServer, ResponseTemplate};
17+
18+
const ORG: &str = "test-org";
19+
20+
fn binary() -> PathBuf {
21+
env!("CARGO_BIN_EXE_socket-patch").into()
22+
}
23+
24+
fn write_pkg(dir: &Path, name: &str) {
25+
std::fs::create_dir_all(dir).unwrap();
26+
std::fs::write(
27+
dir.join("package.json"),
28+
format!(r#"{{ "name": "{name}", "version": "1.0.0" }}"#),
29+
)
30+
.unwrap();
31+
}
32+
33+
/// A project under `tmp/proj` whose `.modules.yaml` records
34+
/// `virtual_store_dir`, plus a pnpm-shaped store entry for `name` at
35+
/// `store`.
36+
fn stage(tmp: &Path, virtual_store_dir: &str, store: &Path, name: &str) -> PathBuf {
37+
let proj = tmp.join("proj");
38+
std::fs::create_dir_all(proj.join("node_modules")).unwrap();
39+
std::fs::write(
40+
proj.join("package.json"),
41+
r#"{ "name": "cwd-root", "version": "0.0.0" }"#,
42+
)
43+
.unwrap();
44+
std::fs::write(
45+
proj.join("node_modules/.modules.yaml"),
46+
serde_json::to_string(&serde_json::json!({
47+
"layoutVersion": 5,
48+
"virtualStoreDir": virtual_store_dir,
49+
}))
50+
.unwrap(),
51+
)
52+
.unwrap();
53+
write_pkg(
54+
&store.join(format!("{name}@1.0.0/node_modules/{name}")),
55+
name,
56+
);
57+
proj
58+
}
59+
60+
/// `scan --json` from `cwd` with no `--cwd` flag, returning the batch
61+
/// request bodies the crawl produced.
62+
async fn scan_bodies(cwd: &Path) -> String {
63+
let server = MockServer::start().await;
64+
Mock::given(method("POST"))
65+
.and(path(format!("/v0/orgs/{ORG}/patches/batch")))
66+
.respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
67+
"packages": [], "canAccessPaidPatches": false,
68+
})))
69+
.mount(&server)
70+
.await;
71+
let mut cmd = Command::new(binary());
72+
cmd.arg("scan").current_dir(cwd);
73+
for (key, _) in std::env::vars_os() {
74+
if key.to_string_lossy().starts_with("SOCKET_")
75+
&& key.to_string_lossy() != "SOCKET_NO_CONFIG"
76+
{
77+
cmd.env_remove(&key);
78+
}
79+
}
80+
cmd.env_remove("VIRTUAL_ENV");
81+
cmd.env("CARGO_HOME", cwd.join(".cargo-home"));
82+
cmd.env("SOCKET_TELEMETRY_DISABLED", "1");
83+
let out = cmd
84+
.args([
85+
"--json",
86+
"-e",
87+
"npm",
88+
"--api-url",
89+
&server.uri(),
90+
"--api-token",
91+
"fake-token-for-test",
92+
"--org",
93+
ORG,
94+
])
95+
.output()
96+
.expect("run socket-patch");
97+
assert!(
98+
out.status.success(),
99+
"stdout={}; stderr={}",
100+
String::from_utf8_lossy(&out.stdout),
101+
String::from_utf8_lossy(&out.stderr)
102+
);
103+
server
104+
.received_requests()
105+
.await
106+
.unwrap_or_default()
107+
.iter()
108+
.filter(|r| r.url.path().ends_with("/patches/batch"))
109+
.map(|r| String::from_utf8_lossy(&r.body).into_owned())
110+
.collect::<Vec<_>>()
111+
.join("\n")
112+
}
113+
114+
#[tokio::test]
115+
async fn default_cwd_scan_skips_an_absolute_store_outside_the_project() {
116+
let tmp = tempfile::tempdir().unwrap();
117+
let global = tmp.path().join("pnpm-store/v10/links");
118+
let proj = stage(
119+
tmp.path(),
120+
&global.display().to_string(),
121+
&global,
122+
"shared-dep",
123+
);
124+
let bodies = scan_bodies(&proj).await;
125+
assert!(!bodies.contains("shared-dep"), "{bodies}");
126+
}
127+
128+
#[tokio::test]
129+
async fn default_cwd_scan_walks_a_store_inside_the_project() {
130+
let tmp = tempfile::tempdir().unwrap();
131+
let store = tmp.path().join("proj/.vstore");
132+
let proj = stage(tmp.path(), "../.vstore", &store, "inproj-dep");
133+
let bodies = scan_bodies(&proj).await;
134+
assert!(bodies.contains("pkg:npm/inproj-dep@1.0.0"), "{bodies}");
135+
}
136+
137+
/// Old pnpm records `virtualStoreDir` as an absolute path. From the
138+
/// default cwd the importer is the empty path, which is no lexical prefix
139+
/// of an absolute store, but a store inside the project is still walked.
140+
#[tokio::test]
141+
async fn default_cwd_scan_walks_an_absolute_store_inside_the_project() {
142+
let tmp = tempfile::tempdir().unwrap();
143+
let store = tmp.path().join("proj/.vstore");
144+
let proj = stage(
145+
tmp.path(),
146+
&store.display().to_string(),
147+
&store,
148+
"absolute-dep",
149+
);
150+
let bodies = scan_bodies(&proj).await;
151+
assert!(bodies.contains("pkg:npm/absolute-dep@1.0.0"), "{bodies}");
152+
}

0 commit comments

Comments
 (0)