Fix rollback dropping records patched in the other scope (#450)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -1190,7 +1190,7 @@ v5.0 replaces v4's per-purl reverts and whole-ledger reverse replay (`revert_rem
 | Hosted leg: pin refused / write failed / contested wiring | `failed` `hosted_restore_refused` / artifact-level `hosted_write_failed` / artifact-level `hosted_wiring_contested`, `details.mode: "hosted"` |
 | Manifest entry dropped | `removed` (`verified` on `--dry-run`) with `details.manifest: true` |
 
-Events carry the patch `uuid` when one is recorded. A package wired through two legs gets one event per leg. Cargo's `.cargo-checksum.json` resync rides the envelope's `sidecars[]`. Rollback's own top-level keys: `hosted: {editedFiles: N}` (distinct files the hosted leg rewrote) and `paths` (the path-glob targets verbatim, `[]` when none); `path` on `manifest_not_found` and `legacyRedirectLedgerRemoved` on the pre-v5-ledger exit. `gc` is the shared GC object (via `summary.bytesFreed` too); it is absent under `--preserve-state`, and absent with a `gc_skipped` warning after a before-blob gate abort or under a corrupt vendor ledger. `warnings[]` (omitted when empty): `reinstall_required`, `hosted_state_not_preservable`, `out_of_scope_copies_restored`, `vendor_state_unreadable`, `cleanup_failed`, `manifest_write_failed`, `gc_skipped`, `legacy_redirect_ledger_kept`, `ownership_not_restored`, `rollback_record_superseded`, `gradle_m2_copy_not_restored`, `cargo_build_cache_stale` (see the apply warnings), `vendor_orphan_still_wired`, the upstream-restore advisories (including `hosted_resolution_orphaned`) and the vendored/hosted leg advisories.
+Events carry the patch `uuid` when one is recorded. A package wired through two legs gets one event per leg. Cargo's `.cargo-checksum.json` resync rides the envelope's `sidecars[]`. Rollback's own top-level keys: `hosted: {editedFiles: N}` (distinct files the hosted leg rewrote) and `paths` (the path-glob targets verbatim, `[]` when none); `path` on `manifest_not_found` and `legacyRedirectLedgerRemoved` on the pre-v5-ledger exit. `gc` is the shared GC object (via `summary.bytesFreed` too); it is absent under `--preserve-state`, and absent with a `gc_skipped` warning after a before-blob gate abort or under a corrupt vendor ledger. `warnings[]` (omitted when empty): `reinstall_required`, `hosted_state_not_preservable`, `out_of_scope_copies_restored`, `patched_in_other_scope`, `vendor_state_unreadable`, `cleanup_failed`, `manifest_write_failed`, `gc_skipped`, `legacy_redirect_ledger_kept`, `ownership_not_restored`, `rollback_record_superseded`, `gradle_m2_copy_not_restored`, `cargo_build_cache_stale` (see the apply warnings), `vendor_orphan_still_wired`, the upstream-restore advisories (including `hosted_resolution_orphaned`) and the vendored/hosted leg advisories.
 
 **Status and exit**: `success` (exit 0) unless something leaves the system still patched — an agent failure, a vendored drift-keep or failure, a hosted refusal, contested wiring under an unscoped run, a corrupt vendor ledger, a failed manifest write — which is `partialFailure` (exit 1). When something failed and nothing was rolled back, previewed, already original or not installed, the run failed as a whole: `status: "error"` with `error: {code: "rollback_failed"}` (exit 1; the `failed` events stay). Not-installed entries never flip the exit (the apply/rollback asymmetry). GC failures never affect the exit. A dry run's would-be refusals stay `failed` events (not `skipped`): a dry run that cannot roll something back exits 1, as before. Every early error (`manifest_not_found`, `manifest_invalid`, `manifest_unreadable`, `patch_not_found`, `ambiguous_target`, `path_glob_no_match`, `hosted_wiring_contested`, `vendor_ledger_missing`, `lock_held` / `lock_io`, the catch-all `rollback_failed`, and the exit-2 usage error `path_glob_invalid`) is a full envelope with empty `events`.
 
@@ -1517,6 +1517,7 @@ command's integration tests check its output.
 | `reinstall_required`      | rollback `warnings[]` | rollback (v5.0): vendored/hosted wiring was unwound, but installed trees keep their patched bytes until the next package-manager install — the stale-install advisory. When the run also emits a Bun advisory (`vendor_bun_reinstall_required` / `redirect_bun_reinstall_required`) the detail and the human note add " (Bun: a plain `bun install` keeps them; run `bun install --force`)". When it emits a PyPI advisory (`vendor_pypi_reinstall_required` / `redirect_pypi_reinstall_required`) they add " (PDM, uv and Pipenv keep a same-version install through a plain sync: run the reinstall the pypi_reinstall_required warning names)". |
 | `hosted_state_not_preservable` | rollback / remove `warnings[]` | rollback `--preserve-state` (v5.0): hosted pins were restored to upstream anyway — the lockfile pins are hosted mode's only record, so there is no local state to preserve; re-run `scan --mode hosted` to re-wire. (`remove --preserve-state` reports the same code in its `warnings[]` — manifest-backed and hosted-only alike — and prints it as a `Note:` on stderr in human mode.) |
 | `out_of_scope_copies_restored` | rollback `warnings[]` | path-scoped rollback (v5.0): a selected patch had installed copies outside the given patterns; ALL copies were restored (patches are per-package). Informational — never flips the exit. |
+| `patched_in_other_scope` | rollback `warnings[]` | the manifest records no scope, so before a rollback drops a record it restored, found already original, or found not installed in its own scope, it checks the other scope (the project at `--cwd` for a `-g` / `--global-prefix` run, the default global install otherwise). A copy there that still holds the patch's afterHash bytes, or has a patched file that exists but cannot be hashed, keeps the record (with its variant group) and its blobs. This warning names the purl, the copy and the rollback of just that purl that restores it (`socket-patch rollback <purl>` or `socket-patch rollback <purl> -g`, plus `--cwd` when the run used one). Never flips the exit (#450). |
 | `vendor_ledger_entry_unwired` | scan `warnings[]` | a vendored entry's dependency left the lockfile (upgraded or removed), so the ledger supplement skipped it; the detail names the purls and points at `scan --prune`, which reverts them (no warning on a pruning non-hosted run). An entry that prune drift-keeps (its lock entries were re-resolved since vendoring, e.g. an npm uninstall re-locked it away) is reported on the prune's `GC: kept` line and keeps being warned about. |
 | `path_scope_excluded_supplements` | scan `warnings[]` | path-scoped scan (v5.0): lockfile-only / vendor-ledger supplement packages have no installed path and were excluded from the scoped scan; the detail carries the count. |
 | `vendor_commit_failed` | top-level error (`vendor`, and the nested vendor envelope of `scan` / `get --mode vendored`); also `failed` (per package) | v5.0 group commit: the run's lockfile / manifest / ledger edits could not be written (the detail names the I/O error). Exit 1; the project's lockfiles and `.socket/vendor/state.json` are left as they were before the run (a partially-applied commit is put back). As for `redirect_symlinked_file_unsupported` (#898), the artifact dirs the run added under `.socket/vendor/` are removed (any that cannot be are named in the detail), each package the run vendored is a `failed` event with this code, not `applied`, its `skipped` advisories from the run are dropped, and the human run prints no "Vendored N packages" count for them and no "Next steps:". When putting a partially-applied commit back fails too, the journal is kept instead, the detail says the next socket-patch command in the project finishes the commit, and the per-package events stand (that commit does complete). |

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

@@ -289,6 +289,83 @@ fn handled_in_place(results: &[RollbackResult]) -> HashSet<&str> {
         .collect()
 }
 
+/// The manifest records no scope (#450), so this run's view of a record
+/// says nothing about the copy the patch may also have been applied to in
+/// the other scope: a project apply rolled back with `-g` (or a `-g` apply
+/// rolled back from the project) finds an untouched copy, or none, here
+/// while the patched one sits in the other scope. Of `purls` (the records
+/// about to leave the manifest), those with a copy in the other scope (the
+/// project at `--cwd` for a global run, the default global install
+/// otherwise) whose files still hold the patch's afterHash content, with
+/// that copy's path. Their records must stay.
+async fn patched_in_other_scope(
+    purls: &[String],
+    manifest: &PatchManifest,
+    common: &GlobalArgs,
+    socket_dir: &Path,
+    this_scope: &HashSet<&str>,
+) -> Vec<(String, PathBuf)> {
+    if purls.is_empty() {
+        return Vec::new();
+    }
+    let partitioned = partition_purls(purls, common.ecosystems.as_deref());
+    let other = CrawlerOptions {
+        global: !common.is_global(),
+        global_prefix: None,
+        ..common.crawler_options()
+    };
+    let found = find_all_packages_for_rollback(&partitioned, &other, true).await;
+    let blobs = socket_dir.join("blobs");
+    let mut patched = Vec::new();
+    for purl in purls {
+        let (Some(record), Some(paths)) = (manifest.patches.get(purl), found.get(purl)) else {
+            continue;
+        };
+        'copies: for path in paths {
+            // Both scopes can resolve to one tree (a `--cwd` inside the
+            // global root): this run already verified that copy.
+            if this_scope.contains(path.to_string_lossy().as_ref()) {
+                continue;
+            }
+            for (file, info) in &record.files {
+                let v = verify_file_rollback(path, file, info, &blobs).await;
+                // A file that is there but cannot be hashed may still hold
+                // the patched bytes: keep the record rather than strand it.
+                let unverifiable = v.current_hash.is_none()
+                    && v.status != VerifyRollbackStatus::AlreadyOriginal
+                    && !v.is_absent();
+                if unverifiable || v.current_hash.as_deref() == Some(info.after_hash.as_str()) {
+                    patched.push((purl.clone(), path.clone()));
+                    break 'copies;
+                }
+            }
+        }
+    }
+    patched
+}
+
+/// `rollback`'s pointer at the scope that still holds a patched copy: a
+/// rollback of just that purl, naming `--cwd` when the run used one (the record lives in that
+/// project's manifest).
+fn patched_in_other_scope_warning(purl: &str, path: &Path, common: &GlobalArgs) -> String {
+    let (scope, mut command) = if common.is_global() {
+        ("the project", format!("socket-patch rollback {purl}"))
+    } else {
+        (
+            "the global install",
+            format!("socket-patch rollback {purl} -g"),
+        )
+    };
+    if common.cwd != Path::new(".") {
+        command.push_str(&format!(" --cwd {}", common.cwd.display()));
+    }
+    format!(
+        "{purl}: its copy in {scope} ({}) is still patched or could not be \
+         verified, so its manifest record was kept; run `{command}` to restore it",
+        path.display()
+    )
+}
+
 /// The wet run's per-package blocks: what was rolled back (naming each
 /// physical copy when a package has several) and what failed.
 fn format_rollback_results(results: &[RollbackResult], cwd: &Path) -> Vec<String> {
@@ -1961,6 +2038,39 @@ pub async fn run(args: RollbackArgs) -> i32 {
                 .cloned()
                 .collect();
             removable.sort();
+            // Records restored (or found original, or not installed) here
+            // whose copy in the other scope is still patched stay, with
+            // their whole variant group (#450).
+            let elsewhere = if cleanup_allowed {
+                let probe: Vec<String> = removable
+                    .iter()
+                    .filter(|purl| {
+                        (succeeded_purls.contains(*purl) || not_installed.contains(purl))
+                            && !vendored_excluded.contains(purl)
+                            && !purl_keys_cover(&vendored_keys, purl)
+                    })
+                    .cloned()
+                    .collect();
+                let this_scope: HashSet<&str> =
+                    results.iter().map(|r| r.package_path.as_str()).collect();
+                patched_in_other_scope(&probe, &manifest, &args.common, &socket_dir, &this_scope)
+                    .await
+            } else {
+                Vec::new()
+            };
+            if !elsewhere.is_empty() {
+                let kept_bases: HashSet<&str> = elsewhere
+                    .iter()
+                    .map(|(purl, _)| strip_purl_qualifiers(purl))
+                    .collect();
+                removable.retain(|purl| !kept_bases.contains(strip_purl_qualifiers(purl)));
+                for (purl, path) in &elsewhere {
+                    run_warnings.push((
+                        "patched_in_other_scope".into(),
+                        patched_in_other_scope_warning(purl, path, &args.common),
+                    ));
+                }
+            }
 
             // The manifest is rewritten only when an entry actually leaves
             // it; an emptied manifest stays on disk as `{"patches": {}}`