diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index e2bc9b3d9..8158f4ba8 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, 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 93b0929dd..ee801ff23 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/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 { @@ -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 = 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 0a918bf8f..90a618253 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,311 @@ 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}" + ); +} + +/// 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 +/// its blobs stay for `rollback -g`. +#[cfg(unix)] +#[test] +fn project_rollback_keeps_a_record_patched_globally() { + let project = agent_project(ORIG_INDEX); + let root = project.path(); + 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, + &["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}" + ); +} + +/// 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 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)"); +}