From 09fb4a997ecbe6fd7791abeb5a41b2cce8cb3375 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 24 Oct 2026 14:22:54 +0000 Subject: [PATCH 1/4] Start fix for #450 Assisted-by: Claude Code:claude-opus-5-5 From 86c9f23050c648a63ea051ce9ebc9338079b2eee Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 24 Oct 2026 14:34:51 +0000 Subject: [PATCH 2/4] Keep rollback records patched in other scope The agent manifest does not record whether a patch went into the project or a global install. A rollback in the wrong scope found an untouched copy, counted it as rolled back, then deleted the manifest record and its blobs. The copy that was really patched stayed patched and nothing could find it again (#450). When every copy in this run's scope is already original, rollback now looks in the other scope (the project for -g runs, the default global install otherwise). If a copy there still holds the patched bytes, the record and blobs are kept and a patched_in_other_scope warning names the copy and the rollback command for its scope. Exit status is unchanged. If nothing is patched anywhere, the record is dropped as before. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/src/commands/rollback.rs | 103 ++++++++++ .../tests/global_scope_project_state.rs | 185 ++++++++++++++++++ 2 files changed, 288 insertions(+) diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 93b0929dd..b0f3b4c2f 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -289,6 +289,88 @@ fn handled_in_place(results: &[RollbackResult]) -> HashSet<&str> { .collect() } +/// Purls whose every copy this run found is already original (they would +/// leave the manifest as rolled back), restricted to those not failed. +fn all_copies_already_original(results: &[RollbackResult]) -> Vec { + let mut by_key: std::collections::BTreeMap<&str, Vec<&RollbackResult>> = + std::collections::BTreeMap::new(); + for r in results { + by_key.entry(r.package_key.as_str()).or_default().push(r); + } + by_key + .into_iter() + .filter(|(_, copies)| { + copies + .iter() + .all(|r| r.success && all_files_already_original(r)) + }) + .map(|(key, _)| key.to_string()) + .collect() +} + +/// The manifest records no scope (#450), so a copy that is already +/// original in this run's scope says nothing about the copy the patch was +/// applied to: a project apply rolled back with `-g` (or a `-g` apply +/// rolled back from the project) finds an untouched copy here while the +/// patched one sits in the other scope. Of `purls`, 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 { + cwd: common.cwd.clone(), + global: !common.is_global(), + global_prefix: None, + }; + 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; + if 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. +fn patched_in_other_scope_warning(purl: &str, path: &Path, global_run: bool) -> String { + let (scope, command) = if global_run { + ("the project", "socket-patch rollback") + } else { + ("the global install", "socket-patch rollback -g") + }; + format!( + "{purl} is already original in this scope, but its copy in {scope} ({}) is still \ + patched; 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 { @@ -1924,10 +2006,31 @@ pub async fn run(args: RollbackArgs) -> i32 { .is_some_and(|(k, e)| e.covers_purl(k, purl)) }) }; + let this_scope: HashSet<&str> = + results.iter().map(|r| r.package_path.as_str()).collect(); + let elsewhere = if cleanup_allowed { + patched_in_other_scope( + &all_copies_already_original(&results), + &manifest, + &args.common, + &socket_dir, + &this_scope, + ) + .await + } else { + Vec::new() + }; + for (purl, path) in &elsewhere { + run_warnings.push(( + "patched_in_other_scope".into(), + patched_in_other_scope_warning(purl, path, args.common.is_global()), + )); + } let succeeded_purls: HashSet = results .iter() .filter(|r| r.success) .map(|r| r.package_key.clone()) + .filter(|key| !elsewhere.iter().any(|(purl, _)| purl == key)) .collect(); // Bases whose attempted variant(s) failed hold their whole // group in the manifest (narrowed-away siblings included). diff --git a/crates/socket-patch-cli/tests/global_scope_project_state.rs b/crates/socket-patch-cli/tests/global_scope_project_state.rs index 0a918bf8f..169b6f422 100644 --- a/crates/socket-patch-cli/tests/global_scope_project_state.rs +++ b/crates/socket-patch-cli/tests/global_scope_project_state.rs @@ -732,3 +732,188 @@ fn global_vendor_check_is_refused() { |what| project.assert_untouched(what), ); } + +// ═══════════════ 6. agent records patched in the other scope (#450) ═══════════════ + +/// `manifest_project` with its `node_modules/left-pad` copy holding `index`. +fn agent_project(index: &[u8]) -> tempfile::TempDir { + let (tmp, _) = manifest_project(); + write_left_pad(&tmp.path().join("node_modules/left-pad"), index); + tmp +} + +fn manifest_of(root: &Path) -> Value { + serde_json::from_slice(&std::fs::read(root.join(".socket/manifest.json")).unwrap()).unwrap() +} + +/// The record and both of its blobs are still on disk. +fn assert_record_kept(root: &Path, v: &Value, what: &str) { + assert!( + manifest_of(root)["patches"].get(V_PURL).is_some(), + "{what}: the record of the copy patched in the other scope must stay; {v}" + ); + for blob in [git_sha256(ORIG_INDEX), git_sha256(PATCHED_INDEX)] { + assert!( + root.join(".socket/blobs").join(&blob).is_file(), + "{what}: blob {blob} must survive GC; {v}" + ); + } + let warning = v["warnings"] + .as_array() + .expect("warnings") + .iter() + .find(|w| w["code"] == "patched_in_other_scope") + .unwrap_or_else(|| panic!("{what}: expected a patched_in_other_scope warning; {v}")); + assert!( + warning["detail"].as_str().unwrap().contains(V_PURL), + "{what}: the warning names the purl; {v}" + ); +} + +/// Project apply, then `rollback --global-prefix` (#450, direction 1): the +/// global copy is already original, but the project's copy is still +/// patched, so the record and its blobs stay and the run says where the +/// patched copy is. +#[test] +fn global_rollback_keeps_a_record_patched_in_the_project() { + let project = agent_project(PATCHED_INDEX); + let root = project.path(); + let prefix = empty_prefix(); + write_left_pad(&prefix.path().join("left-pad"), ORIG_INDEX); + for dry_run in [true, false] { + let mut args = vec![ + "rollback", + "--json", + "--yes", + "--offline", + "--global-prefix", + prefix.path().to_str().unwrap(), + ]; + if dry_run { + args.push("--dry-run"); + } + let (code, stdout, stderr) = run(root, &args); + assert_eq!(code, 0, "{args:?}: stdout={stdout}\nstderr={stderr}"); + let v = parse(&stdout, &stderr); + assert_record_kept(root, &v, &format!("{args:?}")); + assert!( + !v["events"] + .as_array() + .expect("events") + .iter() + .any(|e| e["action"] == "removed"), + "{args:?}: nothing leaves the manifest; {v}" + ); + } + assert_eq!( + std::fs::read(root.join("node_modules/left-pad/index.js")).unwrap(), + PATCHED_INDEX, + "a global rollback leaves the project's copy alone" + ); + + // The rollback in the right scope then restores it and drops the record. + let (code, stdout, stderr) = run(root, &["rollback", "--json", "--yes", "--offline"]); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + assert_eq!( + std::fs::read(root.join("node_modules/left-pad/index.js")).unwrap(), + ORIG_INDEX + ); + assert!( + manifest_of(root)["patches"].get(V_PURL).is_none(), + "{stdout}" + ); +} + +/// Every copy original in both scopes (the user reinstalled): rollback's +/// end state holds, so a global run still drops the record as before. +#[test] +fn global_rollback_drops_a_record_original_in_both_scopes() { + let project = agent_project(ORIG_INDEX); + let root = project.path(); + let prefix = empty_prefix(); + write_left_pad(&prefix.path().join("left-pad"), ORIG_INDEX); + let (code, stdout, stderr) = run( + root, + &[ + "rollback", + "--json", + "--yes", + "--offline", + "--global-prefix", + prefix.path().to_str().unwrap(), + ], + ); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + let v = parse(&stdout, &stderr); + assert!(manifest_of(root)["patches"].get(V_PURL).is_none(), "{v}"); + assert!( + !v["warnings"] + .as_array() + .is_some_and(|w| w.iter().any(|w| w["code"] == "patched_in_other_scope")), + "{v}" + ); +} + +/// `-g` apply, then a project `rollback` (#450, direction 2): the project's +/// copy is already original, but the default global install (here the +/// root a stub `npm root -g` prints) is still patched, so the record and +/// its blobs stay for `rollback -g`. +#[cfg(unix)] +#[test] +fn project_rollback_keeps_a_record_patched_globally() { + use std::os::unix::fs::PermissionsExt; + + let project = agent_project(ORIG_INDEX); + let root = project.path(); + let global = empty_prefix(); + let global_root = global.path().join("lib/node_modules"); + write_left_pad(&global_root.join("left-pad"), PATCHED_INDEX); + let bin = tempfile::tempdir().unwrap(); + let npm = bin.path().join("npm"); + std::fs::write( + &npm, + format!("#!/bin/sh\necho '{}'\n", global_root.display()), + ) + .unwrap(); + std::fs::set_permissions(&npm, std::fs::Permissions::from_mode(0o755)).unwrap(); + let path = bin.path().to_str().unwrap(); + + let (code, stdout, stderr) = common::run_with_env( + root, + &["rollback", "--json", "--yes", "--offline"], + &[("SOCKET_PATCH_SERVER_URL", PATCH_HOST), ("PATH", path)], + ); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + let v = parse(&stdout, &stderr); + assert_record_kept(root, &v, "project rollback"); + assert!( + !v["events"] + .as_array() + .expect("events") + .iter() + .any(|e| e["action"] == "removed"), + "nothing leaves the manifest; {v}" + ); + assert_eq!( + std::fs::read(global_root.join("left-pad/index.js")).unwrap(), + PATCHED_INDEX, + "a project rollback leaves the global copy alone" + ); + + // `rollback -g` restores the global copy and drops the record. + let (code, stdout, stderr) = common::run_with_env( + root, + &["rollback", "-g", "--json", "--yes", "--offline"], + &[("SOCKET_PATCH_SERVER_URL", PATCH_HOST), ("PATH", path)], + ); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + assert_eq!( + std::fs::read(global_root.join("left-pad/index.js")).unwrap(), + ORIG_INDEX, + "{stdout}" + ); + assert!( + manifest_of(root)["patches"].get(V_PURL).is_none(), + "{stdout}" + ); +} From 558df01782b424c70dcd72b0a86f00897776131a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 24 Oct 2026 14:41:35 +0000 Subject: [PATCH 3/4] Probe the other scope for every dropped record Review follow-up for #450. The first fix only checked the other scope when every copy in this run's scope was already original. Two common shapes still lost the record: - rollback -g where the global install has no copy at all (the purl is not installed in this scope) - a project rollback that restores its own copy while the same manifest's global copy is still patched Now every record about to leave the manifest because it was restored, already original, or not installed here is checked against the other scope. A record kept this way keeps its variant group too. The warning names --cwd when the run used one. The new warning code is documented in CLI_CONTRACT.md. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 3 +- .../socket-patch-cli/src/commands/rollback.rs | 111 ++++++++-------- .../tests/global_scope_project_state.rs | 122 ++++++++++++++++-- 3 files changed, 165 insertions(+), 71 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index e2bc9b3d9..b2d4f02f9 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/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 keeps the record (with its variant group) and its blobs, and this warning names the purl, the copy and the command that restores it (`socket-patch rollback` or `socket-patch rollback -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). | diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index b0f3b4c2f..66f15c112 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -289,33 +289,15 @@ fn handled_in_place(results: &[RollbackResult]) -> HashSet<&str> { .collect() } -/// Purls whose every copy this run found is already original (they would -/// leave the manifest as rolled back), restricted to those not failed. -fn all_copies_already_original(results: &[RollbackResult]) -> Vec { - let mut by_key: std::collections::BTreeMap<&str, Vec<&RollbackResult>> = - std::collections::BTreeMap::new(); - for r in results { - by_key.entry(r.package_key.as_str()).or_default().push(r); - } - by_key - .into_iter() - .filter(|(_, copies)| { - copies - .iter() - .all(|r| r.success && all_files_already_original(r)) - }) - .map(|(key, _)| key.to_string()) - .collect() -} - -/// The manifest records no scope (#450), so a copy that is already -/// original in this run's scope says nothing about the copy the patch was -/// applied to: a project apply rolled back with `-g` (or a `-g` apply -/// rolled back from the project) finds an untouched copy here while the -/// patched one sits in the other scope. Of `purls`, 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. +/// 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, @@ -328,9 +310,9 @@ async fn patched_in_other_scope( } let partitioned = partition_purls(purls, common.ecosystems.as_deref()); let other = CrawlerOptions { - cwd: common.cwd.clone(), 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"); @@ -357,16 +339,21 @@ async fn patched_in_other_scope( patched } -/// `rollback`'s pointer at the scope that still holds a patched copy. -fn patched_in_other_scope_warning(purl: &str, path: &Path, global_run: bool) -> String { - let (scope, command) = if global_run { - ("the project", "socket-patch rollback") +/// `rollback`'s pointer at the scope that still holds a patched copy, +/// 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", "socket-patch rollback".to_string()) } else { - ("the global install", "socket-patch rollback -g") + ("the global install", "socket-patch rollback -g".to_string()) }; + if common.cwd != Path::new(".") { + command.push_str(&format!(" --cwd {}", common.cwd.display())); + } format!( - "{purl} is already original in this scope, but its copy in {scope} ({}) is still \ - patched; its manifest record was kept — run `{command}` to restore it", + "{purl}: its copy in {scope} ({}) is still patched, so its manifest record was \ + kept; run `{command}` to restore it", path.display() ) } @@ -2006,31 +1993,10 @@ pub async fn run(args: RollbackArgs) -> i32 { .is_some_and(|(k, e)| e.covers_purl(k, purl)) }) }; - let this_scope: HashSet<&str> = - results.iter().map(|r| r.package_path.as_str()).collect(); - let elsewhere = if cleanup_allowed { - patched_in_other_scope( - &all_copies_already_original(&results), - &manifest, - &args.common, - &socket_dir, - &this_scope, - ) - .await - } else { - Vec::new() - }; - for (purl, path) in &elsewhere { - run_warnings.push(( - "patched_in_other_scope".into(), - patched_in_other_scope_warning(purl, path, args.common.is_global()), - )); - } let succeeded_purls: HashSet = results .iter() .filter(|r| r.success) .map(|r| r.package_key.clone()) - .filter(|key| !elsewhere.iter().any(|(purl, _)| purl == key)) .collect(); // Bases whose attempted variant(s) failed hold their whole // group in the manifest (narrowed-away siblings included). @@ -2064,6 +2030,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 = 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": {}}` diff --git a/crates/socket-patch-cli/tests/global_scope_project_state.rs b/crates/socket-patch-cli/tests/global_scope_project_state.rs index 169b6f422..e4246fef6 100644 --- a/crates/socket-patch-cli/tests/global_scope_project_state.rs +++ b/crates/socket-patch-cli/tests/global_scope_project_state.rs @@ -854,6 +854,37 @@ fn global_rollback_drops_a_record_original_in_both_scopes() { ); } +/// A default global npm install at a temp root: a stub `npm` on `PATH` +/// answers `npm root -g` with it, holding a `left-pad` copy with `index`. +#[cfg(unix)] +struct StubGlobal { + _tmp: tempfile::TempDir, + bin: tempfile::TempDir, + root: PathBuf, +} + +#[cfg(unix)] +impl StubGlobal { + fn new(index: &[u8]) -> Self { + use std::os::unix::fs::PermissionsExt; + let tmp = empty_prefix(); + let root = tmp.path().join("lib/node_modules"); + write_left_pad(&root.join("left-pad"), index); + let bin = tempfile::tempdir().unwrap(); + let npm = bin.path().join("npm"); + std::fs::write(&npm, format!("#!/bin/sh\necho '{}'\n", root.display())).unwrap(); + std::fs::set_permissions(&npm, std::fs::Permissions::from_mode(0o755)).unwrap(); + Self { + _tmp: tmp, + bin, + root, + } + } + fn path(&self) -> &str { + self.bin.path().to_str().unwrap() + } +} + /// `-g` apply, then a project `rollback` (#450, direction 2): the project's /// copy is already original, but the default global install (here the /// root a stub `npm root -g` prints) is still patched, so the record and @@ -861,22 +892,11 @@ fn global_rollback_drops_a_record_original_in_both_scopes() { #[cfg(unix)] #[test] fn project_rollback_keeps_a_record_patched_globally() { - use std::os::unix::fs::PermissionsExt; - let project = agent_project(ORIG_INDEX); let root = project.path(); - let global = empty_prefix(); - let global_root = global.path().join("lib/node_modules"); - write_left_pad(&global_root.join("left-pad"), PATCHED_INDEX); - let bin = tempfile::tempdir().unwrap(); - let npm = bin.path().join("npm"); - std::fs::write( - &npm, - format!("#!/bin/sh\necho '{}'\n", global_root.display()), - ) - .unwrap(); - std::fs::set_permissions(&npm, std::fs::Permissions::from_mode(0o755)).unwrap(); - let path = bin.path().to_str().unwrap(); + let global = StubGlobal::new(PATCHED_INDEX); + let global_root = global.root.clone(); + let path = global.path(); let (code, stdout, stderr) = common::run_with_env( root, @@ -917,3 +937,77 @@ fn project_rollback_keeps_a_record_patched_globally() { "{stdout}" ); } + +/// Project apply, then `rollback --global-prefix` over a global root with +/// no copy at all (#450, the common shape of direction 1): the purl is +/// not installed in this scope, but the project's copy is still patched, +/// so the record stays. +#[test] +fn global_rollback_keeps_a_record_not_installed_globally() { + let project = agent_project(PATCHED_INDEX); + let root = project.path(); + let prefix = empty_prefix(); + let (code, stdout, stderr) = run( + root, + &[ + "rollback", + "--json", + "--yes", + "--offline", + "--global-prefix", + prefix.path().to_str().unwrap(), + ], + ); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + let v = parse(&stdout, &stderr); + assert_record_kept(root, &v, "rollback --global-prefix (not installed)"); + assert_eq!( + std::fs::read(root.join("node_modules/left-pad/index.js")).unwrap(), + PATCHED_INDEX + ); +} + +/// The same manifest applied in both scopes: a project `rollback` restores +/// the project's copy, but the global copy is still patched, so the record +/// stays for `rollback -g` (#450), with a human warning naming it. +#[cfg(unix)] +#[test] +fn project_rollback_restoring_its_copy_keeps_a_record_patched_globally() { + let project = agent_project(PATCHED_INDEX); + let root = project.path(); + let global = StubGlobal::new(PATCHED_INDEX); + let env = [ + ("SOCKET_PATCH_SERVER_URL", PATCH_HOST), + ("PATH", global.path()), + ]; + + let (code, stdout, stderr) = + common::run_with_env(root, &["rollback", "--json", "--yes", "--offline"], &env); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + let v = parse(&stdout, &stderr); + assert_record_kept(root, &v, "project rollback (both patched)"); + assert_eq!( + std::fs::read(root.join("node_modules/left-pad/index.js")).unwrap(), + ORIG_INDEX, + "the project's copy is restored" + ); + assert_eq!( + std::fs::read(global.root.join("left-pad/index.js")).unwrap(), + PATCHED_INDEX + ); + + // Human mode says where the patched copy is and how to restore it. + write_left_pad(&root.join("node_modules/left-pad"), PATCHED_INDEX); + let (code, stdout, stderr) = + common::run_with_env(root, &["rollback", "--yes", "--offline"], &env); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + assert!( + stderr.contains(&format!( + "Warning: {V_PURL}: its copy in the global install ({}) is still patched, so its \ + manifest record was kept; run `socket-patch rollback -g` to restore it", + global.root.join("left-pad").display() + )), + "{stderr}" + ); + assert!(!stdout.contains("Removed"), "{stdout}"); +} From 71d22fab4be28f176e6cde4de06c15b19ce975a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 24 Oct 2026 15:33:52 +0000 Subject: [PATCH 4/4] Fail closed on unverifiable copies; scope remedy Bugbot follow-up for #450. - A copy in the other scope whose patched file exists but cannot be hashed (unreadable, a directory, an I/O error) now keeps the record. Before, it counted as unpatched, so the record and its blobs could be dropped while the copy might still hold the patched bytes. - The patched_in_other_scope warning now names a rollback of just the kept purl (`socket-patch rollback [-g]`) instead of a bare rollback that would restore every patch in that scope. Assisted-by: Claude Code:claude-opus-5-5 --- crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- .../socket-patch-cli/src/commands/rollback.rs | 22 +++++++++---- .../tests/global_scope_project_state.rs | 33 +++++++++++++++++-- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index b2d4f02f9..8158f4ba8 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1517,7 +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 keeps the record (with its variant group) and its blobs, and this warning names the purl, the copy and the command that restores it (`socket-patch rollback` or `socket-patch rollback -g`, plus `--cwd` when the run used one). Never flips the exit (#450). | +| `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 ` or `socket-patch rollback -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). | diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 66f15c112..ee801ff23 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -329,7 +329,12 @@ async fn patched_in_other_scope( } for (file, info) in &record.files { let v = verify_file_rollback(path, file, info, &blobs).await; - if v.current_hash.as_deref() == Some(info.after_hash.as_str()) { + // 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; } @@ -339,21 +344,24 @@ async fn patched_in_other_scope( patched } -/// `rollback`'s pointer at the scope that still holds a patched copy, -/// naming `--cwd` when the run used one (the record lives in that +/// `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", "socket-patch rollback".to_string()) + ("the project", format!("socket-patch rollback {purl}")) } else { - ("the global install", "socket-patch rollback -g".to_string()) + ( + "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, so its manifest record was \ - kept; run `{command}` to restore it", + "{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() ) } diff --git a/crates/socket-patch-cli/tests/global_scope_project_state.rs b/crates/socket-patch-cli/tests/global_scope_project_state.rs index e4246fef6..90a618253 100644 --- a/crates/socket-patch-cli/tests/global_scope_project_state.rs +++ b/crates/socket-patch-cli/tests/global_scope_project_state.rs @@ -1003,11 +1003,40 @@ fn project_rollback_restoring_its_copy_keeps_a_record_patched_globally() { assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); assert!( stderr.contains(&format!( - "Warning: {V_PURL}: its copy in the global install ({}) is still patched, so its \ - manifest record was kept; run `socket-patch rollback -g` to restore it", + "Warning: {V_PURL}: its copy in the global install ({}) is still patched or could \ + not be verified, so its manifest record was kept; run `socket-patch rollback \ + {V_PURL} -g` to restore it", global.root.join("left-pad").display() )), "{stderr}" ); assert!(!stdout.contains("Removed"), "{stdout}"); } + +/// A copy in the other scope whose patched file exists but cannot be +/// hashed (here a directory where `index.js` should be) may still be +/// patched, so the record stays rather than being stranded (#450). +#[test] +fn global_rollback_keeps_a_record_unverifiable_in_the_project() { + let project = agent_project(ORIG_INDEX); + let root = project.path(); + let index = root.join("node_modules/left-pad/index.js"); + std::fs::remove_file(&index).unwrap(); + std::fs::create_dir(&index).unwrap(); + let prefix = empty_prefix(); + write_left_pad(&prefix.path().join("left-pad"), ORIG_INDEX); + let (code, stdout, stderr) = run( + root, + &[ + "rollback", + "--json", + "--yes", + "--offline", + "--global-prefix", + prefix.path().to_str().unwrap(), + ], + ); + assert_eq!(code, 0, "stdout={stdout}\nstderr={stderr}"); + let v = parse(&stdout, &stderr); + assert_record_kept(root, &v, "rollback --global-prefix (unverifiable)"); +}