Fix pnpm global-store transitive deps passing silently (#362) - #829
Merged
Mikola Lysenko (mikolalysenko) merged 5 commits intoOct 5, 2026
Merged
Conversation
Assisted-by: Claude Code:claude-opus-5-5
With pnpm's enableGlobalVirtualStore, a transitive dependency lives only in the machine-wide <store>/v<N>/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
Mikola Lysenko (mikolalysenko)
marked this pull request as ready for review
October 5, 2026 07:08
Collaborator
Author
|
BugBot review Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
Collaborator
Author
|
[burn-down agent] Marked Ready for review at head
Generated by Claude Code |
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPd97gQyyjAeSiTVmLw4p3
Mikola Lysenko (mikolalysenko)
removed
the
Ready for review
Collaborator
Author
|
BugBot review Generated by Claude Code |
…ansitive-refusal # Conflicts: # crates/socket-patch-core/src/crawlers/npm_crawler.rs
Tanmay Singla (Tanmay182003)
approved these changes
Oct 5, 2026
Collaborator
Author
|
bugbot run Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Workspace GVS lookup misses relative cwd
- Canonicalized the nm path in enclosing_pnpm_modules_yaml_sync before walking ancestors, ensuring relative paths like 'node_modules' resolve to absolute paths that can reach the workspace root's .modules.yaml file.
Or push these changes by commenting:
@cursor push f315a468eb
Preview (f315a468eb)
diff --git a/crates/socket-patch-core/src/api/ranking.rs b/crates/socket-patch-core/src/api/ranking.rs
--- a/crates/socket-patch-core/src/api/ranking.rs
+++ b/crates/socket-patch-core/src/api/ranking.rs
@@ -164,8 +164,14 @@
/// classify a recorded patch (ALREADY vs UPGRADE) and to report
/// `updates[]`, on the same records that pick the patch, so selection,
/// classification and reporting cannot disagree.
-pub fn search_result_supersedes(candidate: &PatchSearchResult, recorded: &PatchSearchResult) -> bool {
- key_supersedes(&rank_search_result(candidate), &rank_search_result(recorded))
+pub fn search_result_supersedes(
+ candidate: &PatchSearchResult,
+ recorded: &PatchSearchResult,
+) -> bool {
+ key_supersedes(
+ &rank_search_result(candidate),
+ &rank_search_result(recorded),
+ )
}
fn key_supersedes(c: &RankKey<'_>, a: &RankKey<'_>) -> bool {
@@ -371,12 +377,7 @@
"2020-01-01T00:00:00Z",
&["critical", "high"]
),
- search_multi(
- "z_new_low",
- "free",
- "2026-08-01T00:00:00Z",
- &["low", "low"]
- ),
+ search_multi("z_new_low", "free", "2026-08-01T00:00:00Z", &["low", "low"]),
]),
"a_old_critical"
);
diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs
--- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs
+++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs
@@ -769,6 +769,7 @@
/// The nearest `<ancestor>/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<PathBuf> {
+ let nm = std::fs::canonicalize(nm).ok()?;
nm.parent()?.ancestors().skip(1).find_map(|dir| {
let candidate = dir.join("node_modules").join(PNPM_MODULES_YAML);
std::fs::symlink_metadata(&candidate)
diff --git a/crates/socket-patch-core/src/crawlers/python_crawler.rs b/crates/socket-patch-core/src/crawlers/python_crawler.rs
--- a/crates/socket-patch-core/src/crawlers/python_crawler.rs
+++ b/crates/socket-patch-core/src/crawlers/python_crawler.rs
@@ -590,9 +590,7 @@
let saved = match read_regular_to_string(&cwd.join(".pdm-python")).await {
Ok(text) => text.trim().to_string(),
Err(_) => {
- let text = read_regular_to_string(&cwd.join(".pdm.toml"))
- .await
- .ok()?;
+ let text = read_regular_to_string(&cwd.join(".pdm.toml")).await.ok()?;
let doc = text.parse::<toml_edit::DocumentMut>().ok()?;
doc.get("python")?.get("path")?.as_str()?.trim().to_string()
}
@@ -3215,7 +3213,11 @@
fake_venv(&tmp.path().join("uv-env"), "venv");
let uv_env = env_of(&[(
"UV_PROJECT_ENVIRONMENT",
- tmp.path().join("uv-env").join("venv").to_string_lossy().into_owned(),
+ tmp.path()
+ .join("uv-env")
+ .join("venv")
+ .to_string_lossy()
+ .into_owned(),
)]);
assert_eq!(
find_local_venv_site_packages_with(&project, &uv_env).await,
diff --git a/crates/socket-patch-core/src/formats/cargo/mod.rs b/crates/socket-patch-core/src/formats/cargo/mod.rs
--- a/crates/socket-patch-core/src/formats/cargo/mod.rs
+++ b/crates/socket-patch-core/src/formats/cargo/mod.rs
@@ -34,7 +34,6 @@
use crate::vendor::cargo_tag;
use crate::vendor::lock_inventory::{LockIntegrity, LockfileEntry, SourceKind};
-
// ── entry model ──
/// The `[metadata]` key a v1 lock files `name`+`version`'s checksum under.
@@ -332,7 +331,6 @@
(name, version, source)
}
-
// ── the model ──
/// One `Cargo.lock`, parsed once (see the module docs).
@@ -500,7 +498,13 @@
uuid: &str,
copy_tagged: bool,
) -> CopyClaim<'_> {
- vendored_copy_claim(&self.packages, &self.unused, name, version, uuid, copy_tagged)
+ vendored_copy_claim(
+ &self.packages,
+ &self.unused,
+ name,
+ version,
+ uuid,
+ copy_tagged,
+ )
}
}
-
diff --git a/crates/socket-patch-core/src/formats/composer/mod.rs b/crates/socket-patch-core/src/formats/composer/mod.rs
--- a/crates/socket-patch-core/src/formats/composer/mod.rs
+++ b/crates/socket-patch-core/src/formats/composer/mod.rs
@@ -22,7 +22,6 @@
use crate::vendor::lock_inventory::{http_url, LockIntegrity, LockfileEntry, SourceKind};
use crate::vendor::path::{parse_vendor_path, VendorPathParts};
-
// ── entry model ──
/// One entry of a parsed `composer.lock` (see [`composer_lock_packages`]).
@@ -107,7 +106,6 @@
out
}
-
// ── the model ──
/// One `composer.lock`, read once (see the module docs).
@@ -177,4 +175,3 @@
out
}
}
-
diff --git a/crates/socket-patch-core/src/formats/gem/hosted.rs b/crates/socket-patch-core/src/formats/gem/hosted.rs
--- a/crates/socket-patch-core/src/formats/gem/hosted.rs
+++ b/crates/socket-patch-core/src/formats/gem/hosted.rs
@@ -304,4 +304,3 @@
}
None
}
-
diff --git a/crates/socket-patch-core/src/formats/gem/mod.rs b/crates/socket-patch-core/src/formats/gem/mod.rs
--- a/crates/socket-patch-core/src/formats/gem/mod.rs
+++ b/crates/socket-patch-core/src/formats/gem/mod.rs
@@ -27,7 +27,6 @@
use crate::utils::purl::simple_purl;
use crate::vendor::lock_inventory::{http_url, LockIntegrity, LockfileEntry, SourceKind};
-
/// The Bundler lockfiles, legacy spelling first: `Gemfile.lock` and
/// `gems.locked` (what bundler writes instead when the manifest is
/// `gems.rb`).
@@ -208,7 +207,6 @@
}
}
-
/// Where a rubygems-compatible registry at `base` (no trailing `/`) serves
/// `name`-`version`'s `.gem` — the inventory's resolved URL and ledger
/// recovery's fetch URL. `None` for a non-http(s) base.
diff --git a/crates/socket-patch-core/src/formats/mod.rs b/crates/socket-patch-core/src/formats/mod.rs
--- a/crates/socket-patch-core/src/formats/mod.rs
+++ b/crates/socket-patch-core/src/formats/mod.rs
@@ -26,13 +26,13 @@
//! [`registry()`] is the one table of which project files carry a lock or
//! its wiring, and in which roles.
+pub(crate) mod bun;
pub mod cargo;
pub mod composer;
pub mod gem;
pub(crate) mod maven;
pub(crate) mod nuget;
pub mod pnpm;
-pub(crate) mod bun;
pub mod registry;
pub mod yarn;
@@ -81,7 +81,11 @@
.filter(|l| !l.trim_start().starts_with("//"))
.collect::<Vec<_>>()
.join("\n");
- let used: Vec<&str> = IMPURE.iter().copied().filter(|n| code.contains(n)).collect();
+ let used: Vec<&str> = IMPURE
+ .iter()
+ .copied()
+ .filter(|n| code.contains(n))
+ .collect();
assert!(
used.is_empty(),
"{}: a format model uses {used:?} — models are pure (module docs)",
diff --git a/crates/socket-patch-core/src/formats/pnpm/mod.rs b/crates/socket-patch-core/src/formats/pnpm/mod.rs
--- a/crates/socket-patch-core/src/formats/pnpm/mod.rs
+++ b/crates/socket-patch-core/src/formats/pnpm/mod.rs
@@ -39,7 +39,6 @@
use crate::vendor::lock_inventory::{http_url, LockIntegrity, LockfileEntry};
use crate::vendor::path::parse_vendor_path;
-
// ── entry model ──
/// One `packages:` entry of a pnpm lock, read with the entry grammar
@@ -283,7 +282,10 @@
let value = rest.trim().trim_matches(|c| c == '\'' || c == '"');
let mut parts = value.split('.');
let major = parts.next().and_then(|m| m.parse::<u32>().ok());
- let minor = parts.next().and_then(|m| m.parse::<u32>().ok()).unwrap_or(0);
+ let minor = parts
+ .next()
+ .and_then(|m| m.parse::<u32>().ok())
+ .unwrap_or(0);
Some((major, minor))
})
}
@@ -303,7 +305,8 @@
/// rejects it): a `shrinkwrapVersion` lock (pnpm 1–2) or lockfileVersion
/// 5.0–5.2 (pnpm 3–5). Later locks never get the `--store` note.
pub fn may_need_store_flag(text: &str) -> bool {
- text.lines().any(|line| line.starts_with("shrinkwrapVersion:"))
+ text.lines()
+ .any(|line| line.starts_with("shrinkwrapVersion:"))
|| lock_versions(text).any(|(major, minor)| major == Some(5) && minor <= 2)
}
@@ -491,7 +494,9 @@
if !in_section {
continue;
}
- if let Some(uuid) = lines::parse_key_line(line, 2).and_then(|(key, _, _)| vendored_npm_uuid(key)) {
+ if let Some(uuid) =
+ lines::parse_key_line(line, 2).and_then(|(key, _, _)| vendored_npm_uuid(key))
+ {
out.insert(uuid);
}
}
@@ -508,17 +513,52 @@
fn resolves_reads_every_key_generation_boundary_anchored() {
let lock = |keys: &str| format!("lockfileVersion: '9.0'\n\npackages:\n\n{keys}");
let yes = [
- (" left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n", "left-pad", "1.3.0"),
- (" /left-pad@1.3.0:\n resolution: {}\n", "left-pad", "1.3.0"),
- (" /left-pad/1.3.0:\n resolution: {}\n", "left-pad", "1.3.0"),
- (" 'left-pad@1.3.0(react@18.0.0)':\n dev: false\n", "left-pad", "1.3.0"),
- (" /left-pad/1.3.0_react@18.0.0:\n dev: false\n", "left-pad", "1.3.0"),
- (" '@scope/name@1.0.0':\n dev: false\n", "@scope/name", "1.0.0"),
- (" /@scope/name@1.0.0:\n dev: false\n", "@scope/name", "1.0.0"),
- (" /@scope/name/1.0.0:\n dev: false\n", "@scope/name", "1.0.0"),
+ (
+ " left-pad@1.3.0:\n resolution: {integrity: sha512-x}\n",
+ "left-pad",
+ "1.3.0",
+ ),
+ (
+ " /left-pad@1.3.0:\n resolution: {}\n",
+ "left-pad",
+ "1.3.0",
+ ),
+ (
+ " /left-pad/1.3.0:\n resolution: {}\n",
+ "left-pad",
+ "1.3.0",
+ ),
+ (
+ " 'left-pad@1.3.0(react@18.0.0)':\n dev: false\n",
+ "left-pad",
+ "1.3.0",
+ ),
+ (
+ " /left-pad/1.3.0_react@18.0.0:\n dev: false\n",
+ "left-pad",
+ "1.3.0",
+ ),
+ (
+ " '@scope/name@1.0.0':\n dev: false\n",
+ "@scope/name",
+ "1.0.0",
+ ),
+ (
+ " /@scope/name@1.0.0:\n dev: false\n",
+ "@scope/name",
+ "1.0.0",
+ ),
+ (
+ " /@scope/name/1.0.0:\n dev: false\n",
+ "@scope/name",
+ "1.0.0",
+ ),
];
for (keys, name, version) in yes {
- assert!(PnpmLock::parse(&lock(keys)).resolves(name, version), "{keys}");
+ assert!(
+ PnpmLock::parse(&lock(keys)).resolves(name, version),
+ "{keys}"
+ );
}
let no = [
(" left-pad@1.3.0-beta.1:\n dev: false\n", "left-pad", "1.3.0"),
@@ -534,7 +574,10 @@
),
];
for (keys, name, version) in no {
- assert!(!PnpmLock::parse(&lock(keys)).resolves(name, version), "{keys}");
+ assert!(
+ !PnpmLock::parse(&lock(keys)).resolves(name, version),
+ "{keys}"
+ );
}
// Keys outside `packages:` (importers, overrides) resolve nothing.
let importers = "lockfileVersion: '9.0'\n\nimporters:\n\n left-pad@1.3.0:\n x: y\n";
@@ -557,7 +600,10 @@
let other = "22222222-2222-4222-8222-222222222222";
assert!(!PnpmLock::parse(text).vendored_in_use(other));
let crlf = text.replace('\n', "\r\n");
- assert!(PnpmLock::parse(&crlf).vendored_in_use(UUID), "CRLF reads like LF");
+ assert!(
+ PnpmLock::parse(&crlf).vendored_in_use(UUID),
+ "CRLF reads like LF"
+ );
}
// An overrides declaration alone is not usage.
let overrides = format!(
diff --git a/crates/socket-patch-core/src/formats/registry.rs b/crates/socket-patch-core/src/formats/registry.rs
--- a/crates/socket-patch-core/src/formats/registry.rs
+++ b/crates/socket-patch-core/src/formats/registry.rs
@@ -67,8 +67,12 @@
const REGISTRY: &[FormatFile] = &[
// ── npm family ──
row("package-lock.json", "npm", HOSTED | VENDORED | PROBE | ROOT),
- row("npm-shrinkwrap.json", "npm", HOSTED | VENDORED | PROBE | ROOT),
row(
+ "npm-shrinkwrap.json",
+ "npm",
+ HOSTED | VENDORED | PROBE | ROOT,
+ ),
+ row(
"pnpm-lock.yaml",
"npm",
HOSTED | VENDORED | PROBE | ROOT | PNPM_MARKER,
@@ -118,7 +122,11 @@
row(".cargo/config", "cargo", HOSTED | VENDORED | PROBE),
// ── composer ──
row("composer.json", "composer", VENDORED),
- row("composer.lock", "composer", HOSTED | VENDORED | PROBE | ROOT),
+ row(
+ "composer.lock",
+ "composer",
+ HOSTED | VENDORED | PROBE | ROOT,
+ ),
// ── nuget ──
row("nuget.config", "nuget", HOSTED | PROBE),
row("NuGet.config", "nuget", HOSTED | PROBE),
@@ -213,7 +221,11 @@
paths.dedup();
assert_eq!(before, paths.len(), "duplicate registry path");
for f in REGISTRY.iter().filter(|f| f.has(ROOT)) {
- assert!(!f.path.contains('/'), "{}: a root marker is a basename", f.path);
+ assert!(
+ !f.path.contains('/'),
+ "{}: a root marker is a basename",
+ f.path
+ );
}
}
diff --git a/crates/socket-patch-core/src/formats/yarn/mod.rs b/crates/socket-patch-core/src/formats/yarn/mod.rs
--- a/crates/socket-patch-core/src/formats/yarn/mod.rs
+++ b/crates/socket-patch-core/src/formats/yarn/mod.rs
@@ -64,8 +64,11 @@
#[test]
fn sniff_prefers_berry_and_skips_a_bom() {
- assert_eq!(sniff_grammar("__metadata:\n version: 8\n"), Some(YarnLockGrammar::Berry));
assert_eq!(
+ sniff_grammar("__metadata:\n version: 8\n"),
+ Some(YarnLockGrammar::Berry)
+ );
+ assert_eq!(
sniff_grammar("\u{feff}# yarn lockfile v1\r\n"),
Some(YarnLockGrammar::Classic)
);
diff --git a/crates/socket-patch-core/src/hosted/guidance.rs b/crates/socket-patch-core/src/hosted/guidance.rs
--- a/crates/socket-patch-core/src/hosted/guidance.rs
+++ b/crates/socket-patch-core/src/hosted/guidance.rs
@@ -173,9 +173,7 @@
pub fn npm_lock_url_needles(artifact_url: &str) -> Vec<String> {
let mut needles: Vec<String> =
crate::patch::redirect::artifact_url_spellings(artifact_url).into();
- needles.push(crate::utils::uri::encode_uri_component(
- artifact_url,
- ));
+ needles.push(crate::utils::uri::encode_uri_component(artifact_url));
needles
}
@@ -310,11 +308,7 @@
/// The auto-config variant: `allow-remote=all` was (or, on `--dry-run`,
/// would be) written to the project `.npmrc`, so installs need no flags.
-pub fn npm_allow_remote_configured_detail(
- hosts: &[&str],
- created: bool,
- dry_run: bool,
-) -> String {
+pub fn npm_allow_remote_configured_detail(hosts: &[&str], created: bool, dry_run: bool) -> String {
let how = match (created, dry_run) {
(true, false) => "`allow-remote=all` was written to a new",
(false, false) => "`allow-remote=all` was appended to the existing",
diff --git a/crates/socket-patch-core/src/hosted/memory/discover.rs b/crates/socket-patch-core/src/hosted/memory/discover.rs
--- a/crates/socket-patch-core/src/hosted/memory/discover.rs
+++ b/crates/socket-patch-core/src/hosted/memory/discover.rs
@@ -14,9 +14,7 @@
use crate::api::client::{ApiError, ApiFuture, PatchApi};
use crate::api::ranking::cmp_search_results;
-use crate::api::types::{
- BatchPackagePatches, PackageVendorResult, PatchResponse, SearchResponse,
-};
+use crate::api::types::{BatchPackagePatches, PackageVendorResult, PatchResponse, SearchResponse};
use crate::utils::purl::{normalize_purl, strip_purl_qualifiers};
use super::types::MAX_REFERENCE_BATCH;
diff --git a/crates/socket-patch-core/src/hosted/memory/limits.rs b/crates/socket-patch-core/src/hosted/memory/limits.rs
--- a/crates/socket-patch-core/src/hosted/memory/limits.rs
+++ b/crates/socket-patch-core/src/hosted/memory/limits.rs
@@ -42,12 +42,7 @@
/// as `flag`), then the socket.yml `patches.maxNewPatches`, then
/// unlimited; `maxNewPatchesCap` only tightens it.
pub(crate) fn max_new(&self, file: Option<u32>) -> crate::rollout::MaxNew {
- crate::rollout::resolve_max_new(
- self.max_new_patches,
- None,
- file,
- self.max_new_patches_cap,
- )
+ crate::rollout::resolve_max_new(self.max_new_patches, None, file, self.max_new_patches_cap)
}
}
@@ -79,8 +74,9 @@
let min_severity = match options.min_severity.as_deref() {
None => None,
Some(value) => Some((
- crate::policy::parse_min_severity(value)
- .map_err(|e| EngineError::invalid("invalid_min_severity", format!("minSeverity: {e}")))?,
+ crate::policy::parse_min_severity(value).map_err(|e| {
+ EngineError::invalid("invalid_min_severity", format!("minSeverity: {e}"))
+ })?,
crate::policy::OverrideSource::Flag,
)),
};
diff --git a/crates/socket-patch-core/src/hosted/memory/mod.rs b/crates/socket-patch-core/src/hosted/memory/mod.rs
--- a/crates/socket-patch-core/src/hosted/memory/mod.rs
+++ b/crates/socket-patch-core/src/hosted/memory/mod.rs
@@ -58,17 +58,17 @@
pub use select::{candidate_files, safe_repo_path, select_paths};
pub use types::*;
+use crate::policy::{
+ canon, patch_severity_order, policy_block, FilterReason, FilteredEntry, MemoryPolicyFs,
+ PolicyError, PolicySource, Root, RootFile, SelectionPolicy, PATCHES_DISABLED,
+ POLICY_FILE_NAMES,
+};
use crate::rollout::stage::{
classify, lookup_incomplete, mentioned_uuids, offers_from_results, Offers, RecordedIndex, Row,
- Stage,
- ROLLOUT_DEFERRED,
+ Stage, ROLLOUT_DEFERRED,
};
use discover::Provider;
use stages::{Planned, RewriteRefused, Rewritten, StageOptions};
-use crate::policy::{
- canon, patch_severity_order, policy_block, FilterReason, FilteredEntry, MemoryPolicyFs, PolicyError,
- PolicySource, Root, RootFile, SelectionPolicy, PATCHES_DISABLED, POLICY_FILE_NAMES,
-};
/// `"<crate version>+<git sha or 'unknown'>"`; the sha comes from the
/// `SOCKET_PATCH_GIT_SHA` build-time variable.
@@ -419,11 +419,8 @@
.map(|p| (purl.clone(), p.uuid.clone()))
})
.collect();
- let merged = crate::ledgers::merge_ledger_records_for_updates(
- manifest.as_ref(),
- vendor.as_ref(),
- &pins,
- );
+ let merged =
+ crate::ledgers::merge_ledger_records_for_updates(manifest.as_ref(), vendor.as_ref(), &pins);
RecordedIndex::new(merged.as_deref(), &pins)
}
@@ -450,13 +447,20 @@
// The repo's socket.yml policy, before any root is processed: a file
// that cannot be honored fails the whole session closed.
- let (policy, policy_warnings) =
- match SelectionPolicy::load(&memory_policy_fs(&files, &options.policy_paths), &options.policy_overrides) {
- Ok(loaded) => loaded,
- Err(error) => {
- return Ok(policy_error_output(&error, warnings, files_input, bytes_input));
- }
- };
+ let (policy, policy_warnings) = match SelectionPolicy::load(
+ &memory_policy_fs(&files, &options.policy_paths),
+ &options.policy_overrides,
+ ) {
+ Ok(loaded) => loaded,
+ Err(error) => {
+ return Ok(policy_error_output(
+ &error,
+ warnings,
+ files_input,
+ bytes_input,
+ ));
+ }
+ };
// Path selection chose which files to send by the policy it read; a
// different policy here would judge roots it never fetched.
let read = match policy.source() {
@@ -465,16 +469,27 @@
};
// Selection returns no digest when it bypassed the file, so a digest
// with a bypassed session means the two sides disagree.
- let expected = if options.policy_overrides.bypass { None } else { read.map(|(_, sha)| sha) };
+ let expected = if options.policy_overrides.bypass {
+ None
+ } else {
+ read.map(|(_, sha)| sha)
+ };
if expected != options.policy_sha256.as_deref() {
let error = PolicyError::Invalid {
- file: read.map_or(POLICY_FILE_NAMES[0], |(path, _)| path).to_string(),
+ file: read
+ .map_or(POLICY_FILE_NAMES[0], |(path, _)| path)
+ .to_string(),
key: String::new(),
message: "the policy content differs from the one path selection read: pass \
selectHostedScanPaths' policySha256 and stream the same text"
.to_string(),
};
- return Ok(policy_error_output(&error, warnings, files_input, bytes_input));
+ return Ok(policy_error_output(
+ &error,
+ warnings,
+ files_input,
+ bytes_input,
+ ));
}
for w in policy_warnings {
warnings.push(EngineWarning::new(w.code, w.detail, None));
@@ -722,23 +737,25 @@
// the tree's manifest and vendor ledger, and the hosted pins its
// lockfiles name. ALREADY rows carry the recorded uuid, so a re-scan
// re-confirms a pin instead of swapping it.
- let mut stage = Stage::new(options.max_new(policy.max_new_patches()), None, std::path::Path::new(""));
+ let mut stage = Stage::new(
+ options.max_new(policy.max_new_patches()),
+ None,
+ std::path::Path::new(""),
+ );
// A root whose every lookup failed hides packages that could have been
// NEW: a capped run then admits none anywhere (§5.2).
- stage.incomplete |= states
- .iter()
- .any(|s| s.error.as_ref().is_some_and(|e| e.code == "patch_lookup_failed"));
+ stage.incomplete |= states.iter().any(|s| {
+ s.error
+ .as_ref()
+ .is_some_and(|e| e.code == "patch_lookup_failed")
+ });
let roots_by_path: Vec<String> = states.iter().map(|s| s.root.clone()).collect();
for state in states.iter_mut().filter(|s| s.error.is_none()) {
let Some(project) = state.project.as_ref() else {
continue;
};
let recorded = memory_recorded(project, &state.root, &roots_by_path, &state.offers);
- stage.incomplete |= lookup_incomplete(
- &recorded,
- &state.failed_details,
- batch_failed,
- );
+ stage.incomplete |= lookup_incomplete(&recorded, &state.failed_details, batch_failed);
let mut rows = classify(&state.offers, &recorded, &state.root);
for row in &mut rows {
row.candidate.in_flight = options.in_flight.contains(&row.candidate.base_purl);
@@ -873,8 +890,11 @@
unknown_roots.contains(&row.candidate.project)
|| confirmed.contains(&(row.candidate.project.clone(), row.writer.uuid.clone()))
});
- let deferred_rows: Vec<(crate::rollout::Candidate, u32)> =
- stage.plan.as_ref().map(|p| p.deferred.clone()).unwrap_or_default();
+ let deferred_rows: Vec<(crate::rollout::Candidate, u32)> = stage
+ .plan
+ .as_ref()
+ .map(|p| p.deferred.clone())
+ .unwrap_or_default();
if !deferred_rows.is_empty() {
let root_index: BTreeMap<String, usize> = states
.iter()
@@ -1126,7 +1146,10 @@
let mut by_purl: BTreeMap<String, Vec<(PatchSearchResult, FilterReason)>> = BTreeMap::new();
for (patch, reason) in dropped {
if !chosen.contains(patch.purl.as_str()) {
- by_purl.entry(patch.purl.clone()).or_default().push((patch, reason));
+ by_purl
+ .entry(patch.purl.clone())
+ .or_default()
+ .push((patch, reason));
}
}
for (purl, mut group) in by_purl {
diff --git a/crates/socket-patch-core/src/hosted/memory/roots.rs b/crates/socket-patch-core/src/hosted/memory/roots.rs
--- a/crates/socket-patch-core/src/hosted/memory/roots.rs
+++ b/crates/socket-patch-core/src/hosted/memory/roots.rs
@@ -40,17 +40,23 @@
/// trees, VCS and tool state, and vendored dependencies. Structural, so no
/// policy can negate them. (Test and fixture trees are the socket.yml
/// policy's overridable built-in ignores.)
-pub(crate) const EXCLUDED_ROOT_SEGMENTS: [&str; 5] = ["node_modules", ".git", ".socket", ".yarn", "vendor"];
+pub(crate) const EXCLUDED_ROOT_SEGMENTS: [&str; 5] =
+ ["node_modules", ".git", ".socket", ".yarn", "vendor"];
/// The marker basenames of `root` among `paths` (the files the policy's
/// path filters test for that root).
-pub(crate) fn root_markers<'a>(root: &str, paths: impl IntoIterator<Item = &'a str>) -> Vec<String> {
+pub(crate) fn root_markers<'a>(
+ root: &str,
+ paths: impl IntoIterator<Item = &'a str>,
+) -> Vec<String> {
let mut out: Vec<String> = paths
.into_iter()
.filter_map(|path| {
let (dir, base) = split_path(path);
let marker = marker_ecosystem(base).is_some()
- || UNSUPPORTED_MARKERS.iter().any(|(_, names)| names.contains(&base));
+ || UNSUPPORTED_MARKERS
+ .iter()
+ .any(|(_, names)| names.contains(&base));
(dir == root && marker).then(|| base.to_string())
})
.collect();
@@ -211,7 +217,15 @@
#[test]
fn root_markers_name_every_marker_of_the_root_only() {
assert_eq!(
- root_markers("a", ["a/yarn.lock", "a/package.json", "a/b/yarn.lock", "a/pom.xml"]),
+ root_markers(
+ "a",
+ [
+ "a/yarn.lock",
+ "a/package.json",
+ "a/b/yarn.lock",
+ "a/pom.xml"
+ ]
+ ),
vec!["pom.xml".to_string(), "yarn.lock".to_string()]
);
}
diff --git a/crates/socket-patch-core/src/hosted/memory/select.rs b/crates/socket-patch-core/src/hosted/memory/select.rs
--- a/crates/socket-patch-core/src/hosted/memory/select.rs
+++ b/crates/socket-patch-core/src/hosted/memory/select.rs
@@ -15,8 +15,8 @@
use crate::utils::python_lock::is_python_lock_name;
use crate::policy::{
- MemoryPolicyFs, PolicyOverrides, PolicySource, Root, RootFile, SelectionPolicy, POLICY_FILE_NAMES,
- SOCKET_YML_INVALID,
+ MemoryPolicyFs, PolicyOverrides, PolicySource, Root, RootFile, SelectionPolicy,
+ POLICY_FILE_NAMES, SOCKET_YML_INVALID,
};
use super::roots::{
@@ -185,7 +185,10 @@
/// The listed root policy files with the text the caller fetched first. A
/// listed file with no text (not passed, `missing`, or a symlink) is present
/// without content, so loading it fails closed.
-fn selection_policy_fs(blobs: &BTreeMap<String, bool>, supplied: &[PolicyFileInput]) -> MemoryPolicyFs {
+fn selection_policy_fs(
+ blobs: &BTreeMap<String, bool>,
+ supplied: &[PolicyFileInput],
+) -> MemoryPolicyFs {
let mut fs = MemoryPolicyFs::default();
for name in POLICY_FILE_NAMES {
let Some(&symlink) = blobs.get(name) else {
@@ -211,7 +214,10 @@
options: &SelectOptions,
) -> Result<SelectionPolicy, PolicyErrorInfo> {
let supplied = options.policy_files.as_deref().unwrap_or_default();
- if let Some(bad) = supplied.iter().find(|f| !POLICY_FILE_NAMES.contains(&f.path.as_str())) {
+ if let Some(bad) = supplied
+ .iter()
+ .find(|f| !POLICY_FILE_NAMES.contains(&f.path.as_str()))
+ {
return Err(PolicyErrorInfo {
code: SOCKET_YML_INVALID.to_string(),
detail: format!(
diff --git a/crates/socket-patch-core/src/hosted/memory/types.rs b/crates/socket-patch-core/src/hosted/memory/types.rs
--- a/crates/socket-patch-core/src/hosted/memory/types.rs
+++ b/crates/socket-patch-core/src/hosted/memory/types.rs
@@ -171,7 +171,9 @@
if v == "none" {
Ok(MaxNewPatchesOption(None))
} else {
- Err(E::custom(format!("maxNewPatches must be a number or \"none\", not `{v}`")))
+ Err(E::custom(format!(
+ "maxNewPatches must be a number or \"none\", not `{v}`"
+ )))
}
}
}
diff --git a/crates/socket-patch-core/src/ledgers.rs b/crates/socket-patch-core/src/ledgers.rs
--- a/crates/socket-patch-core/src/ledgers.rs
+++ b/crates/socket-patch-core/src/ledgers.rs
@@ -371,7 +371,6 @@
}
}
-
/// Fold the hosted pins and the vendor ledger's patch records into the
/// manifest view update detection consults. Hosted mode records purl→uuid
/// ONLY in the lockfiles (`hosted_pins`, uuid only; v5 keeps no hosted
diff --git a/crates/socket-patch-core/src/lib.rs b/crates/socket-patch-core/src/lib.rs
--- a/crates/socket-patch-core/src/lib.rs
+++ b/crates/socket-patch-core/src/lib.rs
@@ -15,7 +15,6 @@
pub mod vendor;
pub mod vex;
-
#[cfg(test)]
mod golden;
#[cfg(test)]
diff --git a/crates/socket-patch-core/src/manifest/records.rs b/crates/socket-patch-core/src/manifest/records.rs
--- a/crates/socket-patch-core/src/manifest/records.rs
+++ b/crates/socket-patch-core/src/manifest/records.rs
@@ -32,7 +32,10 @@
/// `patch`. `files` is the (purl-keyed) before/after-hash map the
/// caller built — semantics for what counts as a "patchable file" differ
/// between the get and download flows, so the caller owns that decision.
-pub fn build_patch_record(patch: &PatchResponse, files: HashMap<String, PatchFileInfo>) -> PatchRecord {
+pub fn build_patch_record(
+ patch: &PatchResponse,
+ files: HashMap<String, PatchFileInfo>,
+) -> PatchRecord {
PatchRecord {
uuid: patch.uuid.clone(),
exported_at: patch.published_at.clone(),
diff --git a/crates/socket-patch-core/src/patch/redirect/cargo_lock_equivalence_tests.rs b/crates/socket-patch-core/src/patch/redirect/cargo_lock_equivalence_tests.rs
--- a/crates/socket-patch-core/src/patch/redirect/cargo_lock_equivalence_tests.rs
+++ b/crates/socket-patch-core/src/patch/redirect/cargo_lock_equivalence_tests.rs
@@ -97,7 +97,6 @@
out
}
-
const INDEX: &str = "sparse+https://socket.example/cargo/index/";
fn plan_new(lock: &str, name: &str, version: &str, cksum: &str) -> CargoLockPlan {
@@ -182,7 +181,8 @@
"[root]\nname = \"app\"\nversion = \"0.1.0\"\ndependencies = [\n \"d 1.0.0 ({crates_io})\",\n]\n\n[[package]]\nname = \"d\"\nversion = \"1.0.0\"\nsource = \"{crates_io}\"\n\n[[package]]\nname = \"u\"\nversion = \"2.0.0\"\nsource = \"{crates_io}\"\ndependencies = [\n \"d 1.0.0 ({crates_io})\",\n]\n\n[metadata]\n\"checksum d 1.0.0 ({crates_io})\" = \"cc\"\n\"checksum u 2.0.0 ({crates_io})\" = \"dd\"\n"
);
let sourceless_v1 = "[[package]]\nname = \"s\"\nversion = \"1.0.0\"\n\n[metadata]\n\"checksum s 1.0.0 (registry+x)\" = \"ee\"\n".to_string();
- let source_at_eof = format!("[[package]]\nname = \"e\"\nversion = \"1.0.0\"\nsource = \"{crates_io}\"");
+ let source_at_eof =
+ format!("[[package]]\nname = \"e\"\nversion = \"1.0.0\"\nsource = \"{crates_io}\"");
let bare = "version = 3\n\n[[package]]\nname = \"b\"\nversion = \"1.0.0\"\n\n[[package]]\nname = \"c\"\nversion = \"1.0.0\"\n".to_string();
let mut g = Golden::new(
"cargo_lock_hand_written",
diff --git a/crates/socket-patch-core/src/patch/redirect/golang_equivalence_tests.rs b/crates/socket-patch-core/src/patch/redirect/golang_equivalence_tests.rs
--- a/crates/socket-patch-core/src/patch/redirect/golang_equivalence_tests.rs
+++ b/crates/socket-patch-core/src/patch/redirect/golang_equivalence_tests.rs
@@ -10,7 +10,11 @@
use crate::golden::Golden;
use crate::test_rng::Rng;
-fn run(g: &mut Golden, files: &BTreeMap<String, String>, overrides: &[DepOverride]) -> RewriteResult {
+fn run(
+ g: &mut Golden,
+ files: &BTreeMap<String, String>,
+ overrides: &[DepOverride],
... diff truncated: showing 800 of 3475 linesYou can send follow-ups to the cloud agent here.
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LPd97gQyyjAeSiTVmLw4p3
Collaborator
Author
|
bugbot run Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5513533. Configure here.
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
Collaborator
Author
|
Burn-down agent: labeled Ready for review at
Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
deleted the
agent/fix-pnpm-gvs-transitive-refusal
branch
October 5, 2026 18:16
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
LLM Description written by Claude Code:claude-opus-5-5
Fixes #362
Root cause
#365 fixed the custom
virtualStoreDirhalf of #362. With pnpm's global virtual store (enableGlobalVirtualStore),node_modules/.modules.yamlrecordsvirtualStoreDir: ../../store/v10/links, which is outside the project. The npm crawler deliberately ignores a store outside the importer. So a transitive dependency, which lives only at<store>/v<N>/links/@/<name>/<version>/<hash>/node_modules/<name>, was never found. Since #555 a lockfile-resolved miss counts as a calm "lockfile-only" skip, so agentapplyexited 0 withsuccessand Node kept loading the unpatched copy. A direct dependency is found through itsnode_modules/<dep>link and already got the loud shared-store refusal from #486.Fix
npm_crawler.rs: newreachable_pnpm_global_virtual_store_entries_sync. When.modules.yamlnames a real pnpm global virtual store (<store>/v<N>/linkswith the store'sfiles/beside it), the scan walk (gather_node_modules) and the apply/rollback resolver (find_by_purls) follow only this project's part of it. They start from the importer's links into the store, then follow each entry's dependency links to sibling entries, scoped ones included, with cycles handled. The sharedlinksdir is never listed, so packages other projects put in the store stay invisible..modules.yamlonly at the workspace root. When an importernode_moduleshas package links but no.modules.yaml, the nearest enclosingnode_modules/.modules.yamlnames the store, and the member's own links seed the walk.apply_failed,partialFailure, exit 1) with the "set enableGlobalVirtualStore to false and reinstall, or use hosted / vendored mode" remedy. It is no longer passed off as lockfile-only.shared_store.rs: the GVSlinksdir test is factored intois_pnpm_global_virtual_store_dir, so the crawler and the refusal gate share one definition.docs/ecosystems.md: describes the reachable-entries walk, the workspace-member case, and the transitive refusal.npm/,pypi/andgem/only dispatch to the binary.Test evidence
applyrefuses, not lockfile-only success)tests/apply/pnpm_global_virtual_store.rs::transitive_global_virtual_store_dep_is_refused_not_skippedsuccess)…::other_projects_global_virtual_store_entries_stay_invisible…::workspace_member_transitive_global_virtual_store_dep_is_refusednpm_crawler::tests::test_pnpm_global_virtual_store_reachable_entries_are_walkednpm_crawler::tests::test_pnpm_global_virtual_store_workspace_member_entries_are_walkedReal pnpm check (pnpm 10.28.0, Linux,
enableGlobalVirtualStore: true,is-odd@3.0.1→ transitiveis-number@6.0.0, hand-staged manifest):apply --offline --jsonnow givespartialFailure,apply_failed"Refusing to patch …/store/v10/links/@/is-number/6.0.0//node_modules/is-number: it is in pnpm's global virtual store …", exit 1, and the store copy is unchanged. Onmainthe same run givessuccess,package_not_installed"lockfile-only", exit 0.Local runs on
7f952bd(merged with currentmain):cargo clippy --workspace --all-features -- -D warnings: clean.cargo test --workspace --all-features: everything passes except 12 tests. All 12 rely on chmod-based write refusals that don't fire as root (this sandbox runs as root, CI doesn't), and none of them touch the crawler.e2e_safety_pnpm: needs patches-api access, which this sandbox doesn't have. CI covers it.🤖 Generated with Claude Code
https://claude.ai/code/session_01LPd97gQyyjAeSiTVmLw4p3
Note
Medium Risk
Changes npm install discovery and
applyoutcomes for pnpm GVS layouts; fixes a false-success path but could affect edge cases in workspace and symlink resolution.Overview
Fixes #362: transitive npm packages that exist only in pnpm’s global virtual store (
enableGlobalVirtualStore) were invisible to the crawler, soapplycould exit successfully with a lockfile-only skip while Node still loaded the unpatched store copy.The npm crawler now walks only this project’s reachable GVS entries—seeded from importer
node_moduleslinks, then following each store entry’s dependency links (with workspace support via the root’s.modules.yamlwhen a member has none). Those paths are reported at their real store location so existing shared-store refusal applies to transitive deps too, not just direct links. Other projects’ store packages stay out of scope.is_pnpm_global_virtual_store_diris extracted inshared_store.rsso detection matches between crawl and patch refusal. Docs inecosystems.mdand new apply/crawler tests cover refusal vs lockfile-only, workspace members, and--cwdfrom a member package.Reviewed by Cursor Bugbot for commit 5513533. Configure here.
Generated by Claude Code