Fix agent apply writing into shared package stores (#332, #361)

CHANGELOG.md

@@ -188,6 +188,15 @@ limits, and required install commands.
   unparseable (hosted) or refused as `vendor_lockfile_version_unsupported`
   (vendored). The lock now keeps its BOM, indent and line endings, and the
   undo is byte-exact (#324).
+- Agent mode no longer patches other projects through a store they share.
+  PDM 2.0–2.12 with `install.cache` and `cache_method = symlink` links
+  `site-packages/<pkg>` into its package cache, and pnpm's global virtual
+  store (`enableGlobalVirtualStore`) links `node_modules/<dep>` into
+  `<store>/links`. `apply` (also `-g`) wrote the patch into that shared
+  directory, so every project using it was patched, and a `rollback` in one
+  project silently unpatched the rest. `apply` and `rollback` now fail on
+  such a package, naming the store and how to get a private copy
+  (#332, #361).
 - `vendor` under `--global` / `--global-prefix` (or `SOCKET_GLOBAL` /
   `SOCKET_GLOBAL_PREFIX`) is now a usage error (exit 2,
   `global_scope_unsupported`), like `scan` and `get` with `--mode vendored`.

crates/socket-patch-core/src/patch/apply.rs

@@ -811,6 +811,21 @@ async fn apply_package_patch_at(
         sidecar: None,
     };
 
+    // A package dir that resolves into a store other projects link to
+    // (PDM's symlink cache, pnpm's global virtual store) is not ours to
+    // patch: the rename below would land in the shared dir and patch every
+    // project using it. Refused in every state, dry run and already-patched
+    // included, so this project never records the shared copy as its patch.
+    if let Some(store) = crate::patch::shared_store::shared_store_of_patch_dirs(
+        pkg_path,
+        files.keys().map(String::as_str),
+    )
+    .await
+    {
+        result.error = Some(store.refusal("patch"));
+        return result;
+    }
+
     // First, verify all files
     for (file_name, file_info) in files_in_order(files) {
         // SECURITY: reject any manifest key that would escape the package dir
@@ -3489,4 +3504,162 @@ mod tests {
             Some("store copy /store/pkg@1.0.0_peer failed to patch: boom")
         );
     }
+
+    /// Lay out one shared store package with `index.js` (original bytes)
+    /// and link it into two projects' install dirs, the way pnpm's global
+    /// virtual store (#361) and PDM's symlink cache (#332) do. Returns
+    /// (root, [project A, project B] package roots, file key, blobs dir,
+    /// files, original, patched). The package root is what the crawlers
+    /// hand apply: the linked `node_modules/<dep>` for npm, but the
+    /// `site-packages` dir for PyPI, whose keys are `<pkg>/<file>`.
+    #[cfg(unix)]
+    fn shared_store_fixture(
+        pdm: bool,
+    ) -> (
+        tempfile::TempDir,
+        [std::path::PathBuf; 2],
+        String,
+        std::path::PathBuf,
+        HashMap<String, PatchFileInfo>,
+        Vec<u8>,
+        Vec<u8>,
+    ) {
+        use crate::patch::shared_store::test_support::{make_pdm_cache_entry, make_pnpm_gvs};
+        let root = tempfile::tempdir().unwrap();
+        let store_pkg = if pdm {
+            make_pdm_cache_entry(root.path())
+        } else {
+            make_pnpm_gvs(root.path())
+        };
+        let original = b"original shared bytes".to_vec();
+        let patched = b"PATCHED shared bytes".to_vec();
+        std::fs::write(store_pkg.join("index.js"), &original).unwrap();
+        let roots = ["a", "b"].map(|p| {
+            let install_dir = if pdm {
+                root.path()
+                    .join(p)
+                    .join(".venv/lib/python3.11/site-packages")
+            } else {
+                root.path().join(p).join("node_modules")
+            };
+            std::fs::create_dir_all(&install_dir).unwrap();
+            let link = install_dir.join(store_pkg.file_name().unwrap());
+            std::os::unix::fs::symlink(&store_pkg, &link).unwrap();
+            if pdm {
+                install_dir
+            } else {
+                link
+            }
+        });
+        let key = if pdm { "urllib3/index.js" } else { "index.js" }.to_string();
+        let blobs = root.path().join("blobs");
+        std::fs::create_dir_all(&blobs).unwrap();
+        let before_hash = compute_git_sha256_from_bytes(&original);
+        let after_hash = compute_git_sha256_from_bytes(&patched);
+        std::fs::write(blobs.join(&before_hash), &original).unwrap();
+        std::fs::write(blobs.join(&after_hash), &patched).unwrap();
+        let mut files = HashMap::new();
+        files.insert(
+            key.clone(),
+            PatchFileInfo {
+                before_hash,
+                after_hash,
+            },
+        );
+        (root, roots, key, blobs, files, original, patched)
+    }
+
+    /// #361 / #332: agent apply must not write through a package directory
+    /// that is a link into a store shared with other projects. It fails
+    /// closed (dry run included), and the other project keeps its bytes.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_apply_refuses_shared_store_package_dir() {
+        for (pdm, purl) in [
+            (false, "pkg:npm/left-pad@1.3.0"),
+            (true, "pkg:pypi/urllib3@1.26.18"),
+        ] {
+            let (_root, [a, b], key, blobs, files, original, _patched) = shared_store_fixture(pdm);
+            let sources = PatchSources::blobs_only(&blobs);
+            for dry_run in [true, false] {
+                let result = apply_package_patch(
+                    purl,
+                    &a,
+                    &files,
+                    &sources,
+                    None,
+                    dry_run,
+                    MismatchPolicy::Warn,
+                )
+                .await;
+                assert!(!result.success, "{purl} dry_run={dry_run}: must refuse");
+                let err = result.error.unwrap_or_default();
+                assert!(
+                    err.contains(crate::patch::shared_store::SHARED_STORE_REFUSAL_MARKER),
+                    "{purl}: {err}"
+                );
+                assert!(result.files_patched.is_empty());
+            }
+            assert_eq!(std::fs::read(b.join(&key)).unwrap(), original, "{purl}");
+        }
+    }
+
+    /// The same store package already patched (by another project, or by
+    /// an apply before this guard existed) is refused too: this project
+    /// does not own the shared copy, so it must not record it as patched.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_apply_refuses_already_patched_shared_store_package_dir() {
+        let (_root, [a, _b], key, blobs, files, _original, patched) = shared_store_fixture(false);
+        std::fs::write(a.join(key), &patched).unwrap();
+        let result = apply_package_patch(
+            "pkg:npm/left-pad@1.3.0",
+            &a,
+            &files,
+            &PatchSources::blobs_only(&blobs),
+            None,
+            false,
+            MismatchPolicy::Warn,
+        )
+        .await;
+        assert!(!result.success);
+    }
+
+    /// A per-project pnpm store reached through a symlink is still patched.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_apply_patches_through_per_project_pnpm_link() {
+        let root = tempfile::tempdir().unwrap();
+        let nm = root.path().join("node_modules");
+        let real = nm.join(".pnpm/left-pad@1.3.0/node_modules/left-pad");
+        std::fs::create_dir_all(&real).unwrap();
+        let original = b"original".to_vec();
+        let patched = b"patched!".to_vec();
+        std::fs::write(real.join("index.js"), &original).unwrap();
+        std::os::unix::fs::symlink(&real, nm.join("left-pad")).unwrap();
+        let blobs = root.path().join("blobs");
+        std::fs::create_dir_all(&blobs).unwrap();
+        let after_hash = compute_git_sha256_from_bytes(&patched);
+        std::fs::write(blobs.join(&after_hash), &patched).unwrap();
+        let mut files = HashMap::new();
+        files.insert(
+            "index.js".to_string(),
+            PatchFileInfo {
+                before_hash: compute_git_sha256_from_bytes(&original),
+                after_hash,
+            },
+        );
+        let result = apply_package_patch(
+            "pkg:npm/left-pad@1.3.0",
+            &nm.join("left-pad"),
+            &files,
+            &PatchSources::blobs_only(&blobs),
+            None,
+            false,
+            MismatchPolicy::Warn,
+        )
+        .await;
+        assert!(result.success, "{:?}", result.error);
+        assert_eq!(std::fs::read(real.join("index.js")).unwrap(), patched);
+    }
 }

crates/socket-patch-core/src/patch/mod.rs

@@ -8,4 +8,5 @@ pub mod package;
 pub(crate) mod path_safety;
 pub mod redirect;
 pub mod rollback;
+pub mod shared_store;
 pub mod sidecars;

crates/socket-patch-core/src/patch/rollback.rs

@@ -437,6 +437,21 @@ async fn rollback_package_patch_at(
         .files_verified
         .iter()
         .all(|v| v.status == VerifyRollbackStatus::AlreadyOriginal);
+    // Restoring bytes into a package dir shared with other projects (PDM's
+    // symlink cache, pnpm's global virtual store) would silently unpatch
+    // them. Refused whenever a write would happen, dry run included; an
+    // already-original shared copy needs no write and passes.
+    if !all_original {
+        if let Some(store) = crate::patch::shared_store::shared_store_of_patch_dirs(
+            pkg_path,
+            files.keys().map(String::as_str),
+        )
+        .await
+        {
+            result.error = Some(store.refusal("roll back"));
+            return result;
+        }
+    }
     if all_original || dry_run {
         result.success = true;
         return result;
@@ -2505,4 +2520,66 @@ mod tests {
             "an already-original primary must still heal a patched twin"
         );
     }
+
+    /// #361 / #332: rollback in one project must not restore the original
+    /// bytes into a package directory shared with other projects (that
+    /// would silently unpatch them). It fails closed, dry run included,
+    /// and leaves the shared bytes as they are.
+    #[cfg(unix)]
+    #[tokio::test]
+    async fn test_rollback_refuses_shared_store_package_dir() {
+        use crate::patch::shared_store::test_support::{make_pdm_cache_entry, make_pnpm_gvs};
+        for (pdm, purl) in [
+            (false, "pkg:npm/left-pad@1.3.0"),
+            (true, "pkg:pypi/urllib3@1.26.18"),
+        ] {
+            let root = tempfile::tempdir().unwrap();
+            let store_pkg = if pdm {
+                make_pdm_cache_entry(root.path())
+            } else {
+                make_pnpm_gvs(root.path())
+            };
+            let original = b"original shared bytes".to_vec();
+            let patched = b"PATCHED shared bytes".to_vec();
+            std::fs::write(store_pkg.join("index.js"), &patched).unwrap();
+            let install_dir = root.path().join("a").join("install");
+            std::fs::create_dir_all(&install_dir).unwrap();
+            let link = install_dir.join(store_pkg.file_name().unwrap());
+            std::os::unix::fs::symlink(&store_pkg, &link).unwrap();
+            // What the crawlers hand rollback: the linked package dir for
+            // npm, but `site-packages` (keys `<pkg>/<file>`) for PyPI.
+            let (pkg_root, key) = if pdm {
+                (install_dir.clone(), "urllib3/index.js")
+            } else {
+                (link.clone(), "index.js")
+            };
+            let blobs = root.path().join("blobs");
+            std::fs::create_dir_all(&blobs).unwrap();
+            let before_hash = compute_git_sha256_from_bytes(&original);
+            std::fs::write(blobs.join(&before_hash), &original).unwrap();
+            let mut files = HashMap::new();
+            files.insert(
+                key.to_string(),
+                PatchFileInfo {
+                    before_hash,
+                    after_hash: compute_git_sha256_from_bytes(&patched),
+                },
+            );
+            for dry_run in [true, false] {
+                let result = rollback_package_patch(purl, &pkg_root, &files, &blobs, dry_run).await;
+                assert!(!result.success, "{purl} dry_run={dry_run}: must refuse");
+                let err = result.error.unwrap_or_default();
+                assert!(
+                    err.contains(crate::patch::shared_store::SHARED_STORE_REFUSAL_MARKER),
+                    "{purl}: {err}"
+                );
+            }
+            assert_eq!(std::fs::read(store_pkg.join("index.js")).unwrap(), patched);
+
+            // Already original: nothing to write, so nothing to refuse.
+            std::fs::write(store_pkg.join("index.js"), &original).unwrap();
+            let result = rollback_package_patch(purl, &pkg_root, &files, &blobs, false).await;
+            assert!(result.success, "{purl}: {:?}", result.error);
+        }
+    }
 }