Fix store-copy fold dropping copy writes (#756, #772)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -1126,6 +1126,8 @@ Every `--json` invocation emits a single JSON object that follows the **unified
 }
 ```
 
+`files[].path` is the manifest file key (`package/index.js`). A pnpm or vlt package installed as more than one peer-variant store copy is patched (and rolled back) in every copy; a file of a copy other than the one the package resolved to is listed under that copy's on-disk path (`…/.pnpm/foo@1.0.0_react@18.3.1/node_modules/foo/index.js`). So a run that wrote only such a copy reports `applied` with that file, not `already_patched`, and rollback's `filesRolledBack` / `filesVerified` carry the same paths (it counts the package in `rolledBack`, not `alreadyOriginal`).
+
 `details` is intentionally schemaless — different subcommands attach different keys. Consumers MUST treat unknown keys as best-effort metadata and must not break on absence.
 
 ### `PatchAction` vocabulary

crates/socket-patch-cli/src/commands/vex_consumed.rs

@@ -715,8 +715,11 @@ mod tests {
             None,
         )
         .await;
-        assert_eq!(installed_again, installed);
-        let (paths, calls) = tracked_npm_hosted(&common, &installed_again).await;
+        // Since #605 the name-keyed resolver probes bundled trees itself, so
+        // it already returns the aliases and the nested store's peers. Feed
+        // the earlier, alias-free set to keep exercising alias expansion;
+        // the resolver's own set is checked against the same result below.
+        let (paths, calls) = tracked_npm_hosted(&common, &installed).await;
         assert_eq!(calls.len(), 1);
         let mut inputs = calls[0].clone();
         inputs.sort();
@@ -738,6 +741,9 @@ mod tests {
                 .len(),
             paths.len()
         );
+        let (mut resolved, _) = tracked_npm_hosted(&common, &installed_again).await;
+        resolved.sort();
+        assert_eq!(resolved, expected, "the resolver's own copy set");
     }
 
     #[cfg(unix)]
@@ -768,14 +774,19 @@ mod tests {
             None,
         )
         .await;
-        assert!(installed.is_empty(), "{installed:?}");
-        let (mut paths, calls) = tracked_npm_hosted(&common, &installed).await;
+        // Since #605 the name-keyed resolver reaches the alias and its
+        // sibling peers on its own. An alias-only set (what an alias-blind
+        // resolver returns) must still expand to the same copies.
+        let (mut paths, calls) = tracked_npm_hosted(&common, &HashMap::new()).await;
         assert_eq!(calls, vec![vec![alias.clone()]]);
         let mut expected = peers;
         expected.push(alias);
         paths.sort();
         expected.sort();
         assert_eq!(paths, expected);
+        let (mut resolved, _) = tracked_npm_hosted(&common, &installed).await;
+        resolved.sort();
+        assert_eq!(resolved, expected, "the resolver's own copy set");
     }
 
     #[cfg(unix)]

crates/socket-patch-cli/tests/apply/in_process_npm_multicopy.rs

@@ -398,6 +398,121 @@ fn apply_and_rollback_reach_both_transitive_only_vlt_store_copies() {
     assert_vlt_copies([&primary, &twin], false, "after rollback");
 }
 
+/// pnpm twin: one store entry per peer combination
+/// (`.pnpm/dupvuln@1.0.0_react@18.2.0/` and `…_react@18.3.1/`), the
+/// importer linking the first. Returns `(root, primary index.js, twin
+/// index.js)`; the before and after blobs are both staged.
+#[cfg(unix)]
+fn build_pnpm_peer_variant_tree(tmp: &Path) -> (PathBuf, PathBuf, PathBuf) {
+    let name = "dupvuln";
+    let purl = "pkg:npm/dupvuln@1.0.0";
+    let original = b"module.exports = function(){ return 'VULNERABLE'; };\n";
+    let mut patched = original.to_vec();
+    patched.extend_from_slice(b"// SOCKET-PATCHED-MULTICOPY\n");
+    std::fs::write(
+        tmp.join("package.json"),
+        r#"{ "name": "pnpm-root", "version": "0.0.0" }"#,
+    )
+    .unwrap();
+    let store = tmp.join("node_modules").join(".pnpm");
+    let entry = |id: &str| store.join(id).join("node_modules").join(name);
+    let primary = write_copy(
+        &entry("dupvuln@1.0.0_react@18.2.0"),
+        name,
+        "1.0.0",
+        original,
+    );
+    let twin = write_copy(
+        &entry("dupvuln@1.0.0_react@18.3.1"),
+        name,
+        "1.0.0",
+        original,
+    );
+    std::os::unix::fs::symlink(
+        ".pnpm/dupvuln@1.0.0_react@18.2.0/node_modules/dupvuln",
+        tmp.join("node_modules").join(name),
+    )
+    .unwrap();
+    stage_manifest_and_blob(
+        tmp,
+        purl,
+        &git_sha256(original),
+        &git_sha256(&patched),
+        &patched,
+    );
+    std::fs::write(
+        tmp.join(".socket").join("blobs").join(git_sha256(original)),
+        original,
+    )
+    .unwrap();
+    (tmp.to_path_buf(), primary, twin)
+}
+
+/// The `dupvuln` events of an envelope, as `(action, errorCode)` pairs.
+fn dupvuln_events(v: &serde_json::Value) -> Vec<(String, String)> {
+    v["events"]
+        .as_array()
+        .expect("events array")
+        .iter()
+        .filter(|e| e["purl"] == "pkg:npm/dupvuln@1.0.0")
+        .map(|e| {
+            (
+                e["action"].as_str().unwrap_or_default().to_string(),
+                e["errorCode"].as_str().unwrap_or_default().to_string(),
+            )
+        })
+        .collect()
+}
+
+/// #756: with the primary already patched and only a store twin
+/// unpatched, apply writes the twin and must report the package as
+/// applied (it used to say `already_patched` / `applied: 0`). Rollback is
+/// the mirror: with the primary already original and only the twin
+/// patched, it restores the twin and must count it as rolled back (it used
+/// to say `rolledBack: 0` / `alreadyOriginal: 1`). Covered for both pnpm
+/// and vlt store layouts, which share the fan-out.
+#[cfg(unix)]
+#[test]
+fn apply_and_rollback_report_a_write_to_only_a_store_twin() {
+    for layout in ["pnpm", "vlt"] {
+        let tmp = tempfile::tempdir().unwrap();
+        let (root, primary, twin) = if layout == "pnpm" {
+            build_pnpm_peer_variant_tree(tmp.path())
+        } else {
+            build_vlt_peer_variant_tree(tmp.path(), true)
+        };
+        let original = std::fs::read(&twin).unwrap();
+
+        let (code, v) = run_apply(&root);
+        assert_eq!(code, 0, "{layout}: first apply; envelope={v}");
+        assert_vlt_copies([&primary, &twin], true, "after first apply");
+
+        // A re-created twin: upstream bytes back in the twin only.
+        std::fs::remove_file(&twin).unwrap();
+        std::fs::write(&twin, &original).unwrap();
+        let (code, v) = run_apply(&root);
+        assert_eq!(code, 0, "{layout}: heal apply; envelope={v}");
+        assert_vlt_copies([&primary, &twin], true, "after heal apply");
+        assert_eq!(v["summary"]["applied"], 1, "{layout}: envelope={v}");
+        assert_eq!(v["summary"]["skipped"], 0, "{layout}: envelope={v}");
+        assert_eq!(
+            dupvuln_events(&v),
+            vec![("applied".to_string(), String::new())],
+            "{layout}: envelope={v}"
+        );
+
+        // Upstream bytes back in the primary only: rollback restores the
+        // twin and must say so.
+        std::fs::remove_file(&primary).unwrap();
+        std::fs::write(&primary, &original).unwrap();
+        let (code, v) = run_rollback(&root);
+        assert_eq!(code, 0, "{layout}: rollback; envelope={v}");
+        assert_vlt_copies([&primary, &twin], false, "after rollback");
+        assert_eq!(v["rolledBack"], 1, "{layout}: envelope={v}");
+        assert_eq!(v["alreadyOriginal"], 0, "{layout}: envelope={v}");
+    }
+}
+
 /// #601: a copy bundled inside ANOTHER package's vlt or pnpm store entry
 /// (`.vlt/~npm~bundler@1.0.0/node_modules/bundler/node_modules/dupvuln`)
 /// is what that package loads, so apply must patch it even when the same

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

@@ -725,10 +725,11 @@ async fn chown_blocking(
 /// the primary is AlreadyPatched, which is precisely the state a
 /// single-copy apply left behind (patched primary, vulnerable twin). A
 /// failed copy fails the whole result: claiming the CVE fixed while a
-/// physical copy remains unpatched is the fail-open this closes. The
-/// primary's per-file records are what the returned `ApplyResult`
-/// carries (the envelope shape is unchanged); copies contribute only
-/// success/error state.
+/// physical copy remains unpatched is the fail-open this closes. Each
+/// copy's per-file records are merged into the returned `ApplyResult`
+/// with the file qualified by the copy's path, so a write to a twin alone
+/// still reports the package as applied (see
+/// [`store_copies`](crate::patch::store_copies)).
 pub async fn apply_package_patch(
     package_key: &str,
     pkg_path: &Path,
@@ -738,50 +739,46 @@ pub async fn apply_package_patch(
     dry_run: bool,
     policy: MismatchPolicy,
 ) -> ApplyResult {
-    let mut result =
-        apply_package_patch_at(package_key, pkg_path, files, sources, uuid, dry_run, policy).await;
-    // Only npm purls can name pnpm or vlt store copies; everything else skips the
-    // (already cheap) discovery outright.
-    if result.success && package_key.starts_with("pkg:npm/") {
-        for copy in crate::crawlers::npm_crawler::find_store_peer_variant_copies(pkg_path).await {
-            let copy_result =
-                apply_package_patch_at(package_key, &copy, files, sources, uuid, dry_run, policy)
-                    .await;
-            fold_copy_result(&mut result, &copy, copy_result);
-        }
-    }
-    result
+    crate::patch::store_copies::fan_out(package_key, pkg_path, |path| async move {
+        apply_package_patch_at(package_key, &path, files, sources, uuid, dry_run, policy).await
+    })
+    .await
 }
 
-/// Merge one pnpm or vlt store copy's result into the primary's. A failed
-/// copy fails the whole result with a `store copy <path> failed to patch: …`
-/// note. A copy that patched fine but could not put file ownership back
-/// (`success: true, error: Some("<path>: patched, but ownership could not
-/// be restored…")`) keeps `success` and appends that advisory verbatim (it
-/// already names the copy's full file path), so the CLI's
-/// `ownership_not_restored` warning sees every copy. Only the ownership
-/// advisory is carried: the `--force` all-skipped note describes the copy
-/// alone and would mislead on a primary that actually patched.
-fn fold_copy_result(result: &mut ApplyResult, copy: &Path, copy_result: ApplyResult) {
-    let note = if copy_result.success {
-        match copy_result.error {
-            Some(advisory) if advisory.contains(OWNERSHIP_NOT_RESTORED_MARKER) => advisory,
-            _ => return,
+impl crate::patch::store_copies::CopyFold for ApplyResult {
+    const VERB: &'static str = "patch";
+
+    fn success(&self) -> bool {
+        self.success
+    }
+
+    fn mark_failed(&mut self) {
+        self.success = false;
+    }
+
+    fn error_mut(&mut self) -> &mut Option<String> {
+        &mut self.error
+    }
+
+    fn extend_files(&mut self, copy: &mut Self, qualify: &dyn Fn(&str) -> String) {
+        for mut verified in copy.files_verified.drain(..) {
+            // A successful copy's NotFound is a `--force` skip: like its
+            // all-skipped note, it describes that copy alone and must not
+            // turn an already-patched primary into a no-op "applied".
+            if copy.success && verified.status == VerifyStatus::NotFound {
+                continue;
+            }
+            verified.file = qualify(&verified.file);
+            self.files_verified.push(verified);
         }
-    } else {
-        result.success = false;
-        format!(
-            "store copy {} failed to patch: {}",
-            copy.display(),
-            copy_result
-                .error
-                .unwrap_or_else(|| "unknown error".to_string())
-        )
-    };
-    result.error = Some(match result.error.take() {
-        Some(prev) => format!("{prev}; {note}"),
-        None => note,
-    });
+        for file in copy.files_patched.drain(..) {
+            let qualified = qualify(&file);
+            if let Some(via) = copy.applied_via.remove(&file) {
+                self.applied_via.insert(qualified.clone(), via);
+            }
+            self.files_patched.push(qualified);
+        }
+    }
 }
 
 /// The substring every ownership advisory carries (see
@@ -3442,7 +3439,7 @@ mod tests {
     /// verbatim (it already names the copy's file path); a copy's `--force`
     /// all-skipped note is NOT carried (it describes the copy alone).
     #[test]
-    fn fold_copy_result_carries_ownership_advisories_and_failures() {
+    fn store_copy_fold_carries_ownership_advisories_and_failures() {
         let copy = Path::new("/store/pkg@1.0.0_peer");
         let clean = || ApplyResult {
             package_key: "pkg:npm/a@1.0.0".to_string(),
@@ -3456,14 +3453,14 @@ mod tests {
         };
 
         let mut primary = clean();
-        fold_copy_result(&mut primary, copy, clean());
+        crate::patch::store_copies::fold(&mut primary, copy, clean());
         assert!(primary.success && primary.error.is_none());
 
         let advisory = format!(
             "/store/pkg@1.0.0_peer/index.js: patched, but {OWNERSHIP_NOT_RESTORED_MARKER} to uid 1 gid 2: EPERM"
         );
         let mut primary = clean();
-        fold_copy_result(
+        crate::patch::store_copies::fold(
             &mut primary,
             copy,
             ApplyResult {
@@ -3475,7 +3472,7 @@ mod tests {
         assert_eq!(primary.error.as_deref(), Some(advisory.as_str()));
 
         let mut primary = clean();
-        fold_copy_result(
+        crate::patch::store_copies::fold(
             &mut primary,
             copy,
             ApplyResult {
@@ -3489,7 +3486,7 @@ mod tests {
         );
 
         let mut primary = clean();
-        fold_copy_result(
+        crate::patch::store_copies::fold(
             &mut primary,
             copy,
             ApplyResult {

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

@@ -10,3 +10,4 @@ pub mod redirect;
 pub mod rollback;
 pub mod shared_store;
 pub mod sidecars;
+pub(crate) mod store_copies;