From 502fef0f3ca3455219596634eb789057f3cf2a69 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 18 Aug 2026 19:59:28 -0400 Subject: [PATCH 1/5] fix(rollback): surface missing-blob aborts in the JSON envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When rollback aborted because beforeHash blobs were missing (offline) or undownloadable (online 404), the --json envelope was contentless and self-contradictory: exit 1 with failed:0, empty results/warnings, and no diagnostic on any stream — while human mode explained the failure fine. Confirmed on every apply leg of the 2026-08-18 pnpm matrix. - Both pre-flight abort paths now synthesize one failed RollbackResult per gated package: purl, the missing blob hash, the offline-gate or per-hash download reason, and the `socket-patch repair` hint; counters follow apply's per-package semantics (failed:N, exit 1). - The abort error string is built by cannot_rollback_error(), shared with the engine's first-blocking-file emission so the two shapes cannot drift. - The "No SOCKET_API_TOKEN set" advisory printed twice per invocation (one per internal ApiClient construction); the client is now threaded through, printing once. The envelope's snake_case partial_failure status is deliberately kept: CLI_CONTRACT.md pins rollback as a pre-v3.0 legacy shape whose snake_case status is the documented consumer discriminator, and a status-string change is rated MAJOR. Co-Authored-By: Claude Fable 5 --- .../socket-patch-cli/src/commands/rollback.rs | 252 +++++++++++++++++- .../tests/cli_rollback_silent.rs | 10 +- .../tests/rollback_invariants.rs | 175 +++++++++++- .../socket-patch-core/src/patch/rollback.rs | 11 +- 4 files changed, 419 insertions(+), 29 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 12acfeb4..8c6fe443 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -1,12 +1,13 @@ use clap::Args; use socket_patch_core::api::blob_fetcher::{fetch_blobs_by_hash, format_fetch_result}; -use socket_patch_core::api::client::get_api_client_with_overrides; +use socket_patch_core::api::client::{get_api_client_with_overrides, ApiClient}; use socket_patch_core::crawlers::CrawlerOptions; use socket_patch_core::manifest::operations::{get_before_hash_blobs, read_manifest}; use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord}; use socket_patch_core::patch::apply::select_installed_variants; use socket_patch_core::patch::rollback::{ - rollback_package_patch, RollbackResult, VerifyRollbackStatus, + cannot_rollback_error, rollback_package_patch, RollbackResult, VerifyRollbackResult, + VerifyRollbackStatus, }; use socket_patch_core::telemetry::{track_patch_rollback_failed, track_patch_rolled_back}; use socket_patch_core::utils::purl::strip_purl_qualifiers; @@ -230,6 +231,83 @@ fn result_to_json(result: &RollbackResult) -> serde_json::Value { }) } +/// Per-package failure results for the pre-flight before-blob abort. +/// +/// The abort fires before the rollback loop produces any per-package +/// results, so without these the `--json` envelope claimed `failed: 0` +/// with empty `results[]` on an exit-1 run — contentless and +/// self-contradictory, and `--json` mutes the stderr explanation the +/// human path gets. One failed result per affected package keeps the +/// `failed` counter meaning "packages that failed" (the same per-package +/// semantics as a mid-run failure) and names each missing blob hash plus +/// the `socket-patch repair` remedy in machine-readable form, using the +/// engine's own `missing_blob` verify vocabulary. `reason_for` renders +/// the per-hash diagnostic (offline gate vs. download failure). +fn missing_blob_abort_results( + gate_manifest: &PatchManifest, + missing_blobs: &HashSet, + all_packages: &HashMap, + reason_for: impl Fn(&str) -> String, +) -> Vec { + // The manifest map is a HashMap — sort so the envelope is deterministic. + let mut purls: Vec<&String> = gate_manifest.patches.keys().collect(); + purls.sort(); + let mut results = Vec::new(); + for purl in purls { + let patch = &gate_manifest.patches[purl]; + let mut files: Vec<(&String, &PatchFileInfo)> = patch + .files + .iter() + .filter(|(_, info)| { + // Empty beforeHash is the created-by-patch sentinel: no + // blob backs it, so it can never be "missing". + !info.before_hash.is_empty() && missing_blobs.contains(&info.before_hash) + }) + .collect(); + if files.is_empty() { + continue; + } + files.sort_by(|a, b| a.0.cmp(b.0)); + let files_verified: Vec = files + .iter() + .map(|(file, info)| VerifyRollbackResult { + file: (*file).clone(), + status: VerifyRollbackStatus::MissingBlob, + message: Some(reason_for(&info.before_hash)), + current_hash: None, + expected_hash: None, + target_hash: Some(info.before_hash.clone()), + }) + .collect(); + // The engine's own first-blocking-file error constructor, so this + // synthesized abort is byte-identical to a mid-run missing-blob + // failure. + let first = &files_verified[0]; + let error = cannot_rollback_error( + &first.file, + first + .message + .as_deref() + .expect("message is set for every synthesized entry above"), + ); + results.push(RollbackResult { + package_key: purl.clone(), + // In-scope purls the crawler could not resolve are still gated + // fail-closed but have no installed path to report. + package_path: all_packages + .get(purl) + .map(|p| p.display().to_string()) + .unwrap_or_default(), + success: false, + files_verified, + files_rolled_back: Vec::new(), + error: Some(error), + sidecar: None, + }); + } + results +} + pub async fn run(args: RollbackArgs) -> i32 { apply_env_toggles(&args.common); @@ -299,7 +377,7 @@ pub async fn run(args: RollbackArgs) -> i32 { Err(code) => return code, }; - match rollback_patches_inner(&args, &manifest_path).await { + match rollback_patches_inner(&args, &manifest_path, Some(&telemetry_client)).await { Ok((success, results, vendored)) => { let rolled_back_count = results .iter() @@ -473,6 +551,12 @@ pub async fn run(args: RollbackArgs) -> i32 { async fn rollback_patches_inner( args: &RollbackArgs, manifest_path: &Path, + // The client `run()` already built. Constructing one per phase printed + // the core client's "No SOCKET_API_TOKEN set" notice once per + // construction — twice in a single rollback. `None` (the `remove` + // delegation path) builds one on demand, only when the blob download + // below actually fires. + api_client: Option<&ApiClient>, ) -> Result<(bool, Vec, Vec), String> { let manifest = read_manifest(manifest_path) .await @@ -710,8 +794,9 @@ async fn rollback_patches_inner( if !missing_blobs.is_empty() { if args.common.offline { // Errors print even under --silent ("errors only", never - // "nothing"): this bail is the run's ONLY diagnostic — the JSON - // envelope carries a contentless partial_failure. + // "nothing"): in human mode this bail is the run's only + // stderr diagnostic; `--json` mutes it and instead carries + // the synthesized per-package failures below. if !args.common.json { eprintln!( "Error: {} blob(s) are missing and --offline mode is enabled.", @@ -719,15 +804,31 @@ async fn rollback_patches_inner( ); eprintln!("Run \"socket-patch repair\" to download missing blobs."); } - return Ok((false, Vec::new(), vendored_skipped)); + let results = + missing_blob_abort_results(&gate_manifest, &missing_blobs, &all_packages, |hash| { + format!( + "Before blob not found: {hash} and --offline prevents fetching. \ + Run \"socket-patch repair\" to download missing blobs." + ) + }); + return Ok((false, results, vendored_skipped)); } if !args.common.silent && !args.common.json { println!("Downloading {} missing blob(s)...", missing_blobs.len()); } - let (client, _) = get_api_client_with_overrides(args.common.api_client_overrides()).await; - let fetch_result = fetch_blobs_by_hash(&missing_blobs, &blobs_path, &client, None).await; + let built_client; + let client = match api_client { + Some(c) => c, + None => { + built_client = get_api_client_with_overrides(args.common.api_client_overrides()) + .await + .0; + &built_client + } + }; + let fetch_result = fetch_blobs_by_hash(&missing_blobs, &blobs_path, client, None).await; if !args.common.silent && !args.common.json { println!("{}", format_fetch_result(&fetch_result)); @@ -741,14 +842,38 @@ async fn rollback_patches_inner( let still_missing = get_missing_before_blobs(&gate_manifest, &blobs_path).await; if !still_missing.is_empty() { // Errors print even under --silent — same contract as the - // offline bail above. + // offline bail above (and same `--json` carrier). if !args.common.json { eprintln!( "{} blob(s) could not be downloaded. Cannot rollback.", still_missing.len() ); } - return Ok((false, Vec::new(), vendored_skipped)); + // Per-hash download outcomes; a hash the fetch never reported + // on still fails closed with the generic reason. + let download_errors: HashMap<&str, &str> = fetch_result + .results + .iter() + .filter(|r| !r.success) + .map(|r| { + ( + r.hash.as_str(), + r.error.as_deref().unwrap_or("unknown error"), + ) + }) + .collect(); + let results = + missing_blob_abort_results(&gate_manifest, &still_missing, &all_packages, |hash| { + let why = download_errors + .get(hash) + .copied() + .unwrap_or("download failed"); + format!( + "Before blob could not be downloaded: {hash} - {why}. \ + Run \"socket-patch repair\" to download missing blobs." + ) + }); + return Ok((false, results, vendored_skipped)); } } @@ -862,7 +987,7 @@ pub(crate) async fn rollback_patches( }, one_off: false, }; - rollback_patches_inner(&args, manifest_path).await + rollback_patches_inner(&args, manifest_path, None).await } #[cfg(test)] @@ -1154,6 +1279,97 @@ mod tests { ); } + /// The pre-flight bail must map each missing blob back to its package: + /// one failed result per affected package (that's what the envelope's + /// `failed` counter counts), files carrying the engine's `missing_blob` + /// status + the missing hash, packages whose blobs are all present left + /// untouched, and created-by-patch sentinels (empty beforeHash) never + /// counted — they are backed by no blob. + #[test] + fn missing_blob_abort_results_map_hashes_to_packages() { + let mut patches = HashMap::new(); + patches.insert( + "pkg:npm/foo@1.0.0".to_string(), + record_with_file("uuid-foo", "a.js", "missing_a"), + ); + patches.insert( + "pkg:npm/bar@1.0.0".to_string(), + record_with_file("uuid-bar", "b.js", "present_b"), + ); + patches.insert( + "pkg:npm/baz@1.0.0".to_string(), + record_with_file("uuid-baz", "c.js", ""), + ); + let gate = PatchManifest { + patches, + setup: None, + }; + let missing: HashSet = ["missing_a".to_string(), "".to_string()] + .into_iter() + .collect(); + let mut all_packages = HashMap::new(); + all_packages.insert("pkg:npm/foo@1.0.0".to_string(), PathBuf::from("/tmp/foo")); + + let results = + missing_blob_abort_results(&gate, &missing, &all_packages, |h| format!("gone: {h}")); + + assert_eq!( + results.len(), + 1, + "only the package referencing a genuinely missing blob fails, got {results:?}" + ); + let r = &results[0]; + assert_eq!(r.package_key, "pkg:npm/foo@1.0.0"); + assert_eq!(r.package_path, "/tmp/foo"); + assert!(!r.success); + assert!(r.files_rolled_back.is_empty()); + assert_eq!( + r.error.as_deref(), + Some("Cannot rollback: a.js - gone: missing_a"), + "error mirrors the engine's first-blocking-file shape" + ); + assert_eq!(r.files_verified.len(), 1); + let f = &r.files_verified[0]; + assert_eq!(f.file, "a.js"); + assert_eq!(f.status, VerifyRollbackStatus::MissingBlob); + assert_eq!(f.target_hash.as_deref(), Some("missing_a")); + assert_eq!(f.message.as_deref(), Some("gone: missing_a")); + } + + /// Undiscovered packages (crawler found no installed copy — their blobs + /// still gate the run fail-closed) synthesize with an empty path, and + /// multiple affected packages come out purl-sorted so the envelope is + /// deterministic across the manifest HashMap's iteration order. + #[test] + fn missing_blob_abort_results_sorted_and_pathless_when_undiscovered() { + let mut patches = HashMap::new(); + patches.insert( + "pkg:npm/zeta@1.0.0".to_string(), + record_with_file("uuid-zeta", "z.js", "missing_z"), + ); + patches.insert( + "pkg:npm/alpha@1.0.0".to_string(), + record_with_file("uuid-alpha", "a.js", "missing_a"), + ); + let gate = PatchManifest { + patches, + setup: None, + }; + let missing: HashSet = ["missing_a".to_string(), "missing_z".to_string()] + .into_iter() + .collect(); + + let results = + missing_blob_abort_results(&gate, &missing, &HashMap::new(), |h| h.to_string()); + + let keys: Vec<&str> = results.iter().map(|r| r.package_key.as_str()).collect(); + assert_eq!(keys, ["pkg:npm/alpha@1.0.0", "pkg:npm/zeta@1.0.0"]); + assert!( + results.iter().all(|r| r.package_path.is_empty()), + "no discovered install path to report, got {results:?}" + ); + } + /// Cargo now patches in place (vendored or registry cache) and rolls back /// by restoring from before-blobs — exactly like npm/pypi. So a cargo PURL /// must NOT be excluded by the before-blob gate: a missing cargo before-blob @@ -1730,10 +1946,22 @@ mod tests { ) .await .expect("rollback must not error"); - assert!(results.is_empty()); assert!( !success, "an in-scope missing before-blob must still abort the offline run" ); + // The abort synthesizes the per-package failure the JSON envelope + // reports (`failed` would otherwise claim 0 on this exit-1 path). + assert_eq!(results.len(), 1, "got {results:?}"); + assert_eq!(results[0].package_key, "pkg:npm/foo@1.0.0"); + assert!(!results[0].success); + assert!( + results[0] + .files_verified + .iter() + .any(|f| f.status == VerifyRollbackStatus::MissingBlob + && f.target_hash.as_deref() == Some("npm_before_hash")), + "the missing blob must be named, got {results:?}" + ); } } diff --git a/crates/socket-patch-cli/tests/cli_rollback_silent.rs b/crates/socket-patch-cli/tests/cli_rollback_silent.rs index 726fba49..4dd96491 100644 --- a/crates/socket-patch-cli/tests/cli_rollback_silent.rs +++ b/crates/socket-patch-cli/tests/cli_rollback_silent.rs @@ -18,8 +18,9 @@ //! (all three modes), `apply` (`--silent`/`--check` mutes), and `remove`. //! //! Stderr assertions ignore the "No SOCKET_API_TOKEN set" client warning: -//! it's printed unconditionally by `get_api_client_with_overrides` in core -//! for every command and is out of scope for `rollback`'s `--silent` gating. +//! it's printed by `get_api_client_with_overrides` in core for every ONLINE +//! command (offline runs suppress it) and is out of scope for `rollback`'s +//! `--silent` gating. use std::path::{Path, PathBuf}; use std::process::Command; @@ -144,8 +145,9 @@ fn rollback_silent_unknown_identifier_keeps_error_output() { } /// `rollback --silent --offline` with a missing before-blob (the offline -/// bail) must still print the error. This path returns a contentless -/// partial_failure — the eprintln IS the only diagnostic. +/// bail) must still print the error. In human mode the eprintln IS the +/// only diagnostic — the bail's synthesized per-package failure records +/// surface only in the `--json` envelope. #[test] fn rollback_silent_offline_missing_blob_keeps_error_output() { let tmp = tempfile::tempdir().unwrap(); diff --git a/crates/socket-patch-cli/tests/rollback_invariants.rs b/crates/socket-patch-cli/tests/rollback_invariants.rs index 3f4cf1b8..33b6f155 100644 --- a/crates/socket-patch-cli/tests/rollback_invariants.rs +++ b/crates/socket-patch-cli/tests/rollback_invariants.rs @@ -253,11 +253,20 @@ fn rollback_unknown_identifier_emits_error() { ); } +/// The beforeHash pinned in `MANIFEST_JSON` (deliberately not staged on +/// disk by the missing-blob tests). +const MISSING_BEFORE_HASH: &str = + "0000000000000000000000000000000000000000000000000000000000000000"; + #[test] fn rollback_offline_with_missing_before_blob_partial_failure() { // Manifest has a patch whose beforeHash is NOT on disk; --offline - // means we won't fetch. Rollback should fail out before touching - // anything. + // means we won't fetch. Rollback must fail out before touching + // anything — and the JSON envelope must SAY so. The bail fires before + // the rollback loop produces any per-package results, so the failures + // are synthesized: before the fix the envelope claimed `failed: 0` + // with empty `results[]` on an exit-1 run, and `--json` mutes the + // stderr explanation, leaving machine consumers zero diagnostic. let tmp = tempfile::tempdir().expect("tempdir"); make_socket_dir(tmp.path()); let (code, stdout) = run(tmp.path(), &["--json", "--offline"]); @@ -270,20 +279,162 @@ fn rollback_offline_with_missing_before_blob_partial_failure() { assert_eq!(v["rolledBack"], 0); assert_eq!(v["alreadyOriginal"], 0); assert_eq!(v["dryRun"], false, "not a dry-run"); - // Known design gap (see memory `apply-invariants-test-hardened`): the - // offline missing-blob bail returns a *contentless* partial_failure — it - // aborts after discovery but before the rollback loop produces any - // per-package results, so `failed` stays 0 and `results` is empty even - // though the run did not succeed. Pin that exact shape so the bail can't - // silently morph into either a real failure count or a spurious success. + // The gated package is counted as failed — same per-package counter + // semantics as a mid-run failure. A `failed: 0` partial_failure is + // self-contradictory. assert_eq!( - v["failed"], 0, - "contentless bail records no per-package failure" + v["failed"], 1, + "the blob-gated package must be counted as failed; stdout=\n{stdout}" + ); + let results = v["results"].as_array().expect("results array"); + assert_eq!( + results.len(), + 1, + "the bail must synthesize one failed result per gated package; stdout=\n{stdout}" ); + let entry = &results[0]; + assert_eq!(entry["purl"], "pkg:npm/__rollback_test__@1.0.0"); + assert_eq!(entry["success"], false); assert_eq!( - v["results"].as_array().expect("results array").len(), + entry["filesRolledBack"] + .as_array() + .expect("filesRolledBack array") + .len(), 0, - "offline bail must abort before producing any per-package results" + "nothing was restored on the bail" + ); + // The error names the remedy; the per-file record names the blob. + let err = entry["error"].as_str().expect("error message string"); + assert!( + err.contains("socket-patch repair"), + "error must carry the repair remedy; got: {err}" + ); + let verified = entry["filesVerified"] + .as_array() + .expect("filesVerified array"); + let file = verified + .iter() + .find(|f| f["file"] == "package/index.js") + .unwrap_or_else(|| panic!("gated file must appear in filesVerified; stdout=\n{stdout}")); + assert_eq!( + file["status"], "missing_blob", + "the engine's missing_blob vocabulary; stdout=\n{stdout}" + ); + assert_eq!( + file["targetHash"], MISSING_BEFORE_HASH, + "the missing blob hash must be machine-readable" + ); + let msg = file["message"].as_str().expect("message string"); + assert!( + msg.contains(MISSING_BEFORE_HASH), + "message must name the missing hash; got: {msg}" + ); + assert!( + msg.contains("--offline") && msg.contains("socket-patch repair"), + "message must name the offline gate and the repair remedy; got: {msg}" + ); +} + +/// Human-mode parity for the offline missing-blob bail: stderr explains +/// the gate (count + repair hint) and the stdout summary names the failed +/// package — the same information the JSON envelope now carries. +#[test] +fn rollback_offline_missing_blob_human_names_package_and_remedy() { + let tmp = tempfile::tempdir().expect("tempdir"); + make_socket_dir(tmp.path()); + let out = rollback_cmd(tmp.path()) + .args(["--offline"]) + .output() + .expect("run socket-patch"); + assert_eq!( + out.status.code(), + Some(1), + "offline + missing blob must exit 1" + ); + let stderr = String::from_utf8_lossy(&out.stderr); + assert!( + stderr.contains("blob(s) are missing") && stderr.contains("--offline"), + "stderr must explain the offline gate; stderr=\n{stderr}" + ); + assert!( + stderr.contains("socket-patch repair"), + "stderr must carry the repair remedy; stderr=\n{stderr}" + ); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("Failed to rollback:") + && stdout.contains("pkg:npm/__rollback_test__@1.0.0"), + "the human summary must name the failed package; stdout=\n{stdout}" + ); +} + +/// Online counterpart: the blob download fires (against an unroutable +/// localhost port, so the failure is instant and nothing leaves the +/// machine), the still-missing re-check aborts, and the envelope carries +/// the same synthesized per-package failures with the download reason. +/// +/// Also pins the client-notice dedupe: building an API client per internal +/// phase (telemetry client in `run()`, then a second one for this download) +/// printed the core "No SOCKET_API_TOKEN set" notice twice in a single +/// rollback invocation. +#[test] +fn rollback_undownloadable_blob_envelope_names_blob_and_remedy() { + let tmp = tempfile::tempdir().expect("tempdir"); + make_socket_dir(tmp.path()); + let out = rollback_cmd(tmp.path()) + .env("SOCKET_TELEMETRY_DISABLED", "1") + .args([ + "--json", + "--api-url", + "http://127.0.0.1:1/", + "--proxy-url", + "http://127.0.0.1:1/", + ]) + .output() + .expect("run socket-patch"); + let stdout = String::from_utf8_lossy(&out.stdout).to_string(); + let stderr = String::from_utf8_lossy(&out.stderr).to_string(); + assert_eq!( + out.status.code(), + Some(1), + "undownloadable blob must exit 1; stdout=\n{stdout}\nstderr=\n{stderr}" + ); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert_eq!(v["status"], "partial_failure"); + assert_eq!(v["failed"], 1, "stdout=\n{stdout}"); + let results = v["results"].as_array().expect("results array"); + assert_eq!(results.len(), 1, "stdout=\n{stdout}"); + let entry = &results[0]; + assert_eq!(entry["purl"], "pkg:npm/__rollback_test__@1.0.0"); + assert_eq!(entry["success"], false); + let err = entry["error"].as_str().expect("error message string"); + assert!( + err.contains("socket-patch repair"), + "error must carry the repair remedy; got: {err}" + ); + let verified = entry["filesVerified"] + .as_array() + .expect("filesVerified array"); + let file = verified + .iter() + .find(|f| f["file"] == "package/index.js") + .unwrap_or_else(|| panic!("gated file must appear in filesVerified; stdout=\n{stdout}")); + assert_eq!(file["status"], "missing_blob"); + assert_eq!(file["targetHash"], MISSING_BEFORE_HASH); + let msg = file["message"].as_str().expect("message string"); + assert!( + msg.contains("could not be downloaded") && msg.contains(MISSING_BEFORE_HASH), + "message must name the download failure and the hash; got: {msg}" + ); + + // The env is scrubbed (no SOCKET_API_TOKEN) and socket-cli config is + // vetoed by the workspace-pinned SOCKET_NO_CONFIG=1, so every client + // build prints the no-token notice — it must appear exactly once per + // invocation, not once per internal phase. + let notes = stderr.matches("No SOCKET_API_TOKEN set").count(); + assert_eq!( + notes, 1, + "the no-token notice must print exactly once per run; stderr=\n{stderr}" ); } diff --git a/crates/socket-patch-core/src/patch/rollback.rs b/crates/socket-patch-core/src/patch/rollback.rs index 8f94dfb0..ec5ac9aa 100644 --- a/crates/socket-patch-core/src/patch/rollback.rs +++ b/crates/socket-patch-core/src/patch/rollback.rs @@ -266,6 +266,15 @@ pub async fn verify_file_rollback( } } +/// The first-blocking-file error line recorded on [`RollbackResult::error`] +/// when a file cannot be rolled back. One constructor so the CLI's +/// pre-flight missing-blob abort (which synthesizes per-package results +/// before this engine runs) emits byte-identical errors — the string +/// reaches users through both stderr and the `--json` envelope. +pub fn cannot_rollback_error(file: &str, why: &str) -> String { + format!("Cannot rollback: {file} - {why}") +} + /// Verify and rollback patches for a single package. /// /// For each file in `files`, this function: @@ -301,7 +310,7 @@ pub async fn rollback_package_patch( .message .clone() .unwrap_or_else(|| format!("{:?}", verify_result.status)); - result.error = Some(format!("Cannot rollback: {} - {}", verify_result.file, msg)); + result.error = Some(cannot_rollback_error(&verify_result.file, &msg)); result.files_verified.push(verify_result); return result; } From d872103a9c09619e52dbbc1855253b6a3a5df71f Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 18 Aug 2026 21:23:54 -0400 Subject: [PATCH 2/5] fix(rollback): gate blobs only for installed targets; surface not-installed entries as markers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The before-blob gate ran before the installed-package match, so a manifest entry whose package was never applied and is not on disk hard-failed the run ('Cannot rollback: ... Before blob not found', path:"") when its blob was missing — even though there was nothing to roll back. Found on the 2026-08-18 pnpm legacy matrix; ecosystem-independent. - The gate now covers only installed rollback targets whose files the engine would genuinely read the blob for (already-original/deleted/ drifted files no longer gate or trigger downloads). - Not-installed in-scope entries surface as additive marker records in results[]: {"purl", "path": null, "skipped": "package_not_installed"} — never failed, never counted in rolledBack/failed, never flip the exit code. Not-installed-only runs keep the pinned exit-0 "succeeds quietly" contract: rollback's job is to make the tree unpatched, and a not-installed package already satisfies that end state (deliberately asymmetric with apply's exit-1-on-unmatched; rationale documented on RollbackOutcome and in the tests). - remove's delegation keeps per-package semantics; fixtures in the remove/silent suites now install the fake package so the blob gate still fires for a genuinely installed patched package, preserving each test's documented intent. Co-Authored-By: Claude Fable 5 --- .../socket-patch-cli/src/commands/rollback.rs | 443 ++++++++++++++++-- .../tests/cli_rollback_silent.rs | 26 +- .../tests/remove_invariants.rs | 41 ++ .../tests/remove_rollback_api_overrides.rs | 24 +- .../tests/rollback_invariants.rs | 332 ++++++++++++- 5 files changed, 796 insertions(+), 70 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index 8c6fe443..aabd42fa 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -6,8 +6,8 @@ use socket_patch_core::manifest::operations::{get_before_hash_blobs, read_manife use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest, PatchRecord}; use socket_patch_core::patch::apply::select_installed_variants; use socket_patch_core::patch::rollback::{ - cannot_rollback_error, rollback_package_patch, RollbackResult, VerifyRollbackResult, - VerifyRollbackStatus, + cannot_rollback_error, rollback_package_patch, verify_file_rollback, RollbackResult, + VerifyRollbackResult, VerifyRollbackStatus, }; use socket_patch_core::telemetry::{track_patch_rollback_failed, track_patch_rolled_back}; use socket_patch_core::utils::purl::strip_purl_qualifiers; @@ -52,6 +52,30 @@ struct PatchToRollback { patch: PatchRecord, } +/// Everything one rollback pass learned. +/// +/// `success` means "no attempted rollback failed" — per-package semantics +/// only. Entries whose package is not installed are NOT failures and never +/// flip it: apply and rollback are deliberately asymmetric here. Apply's job +/// is "make the tree patched", so an unmatched purl means the job was NOT +/// done (apply's all-unmatched run exits 1 / `partialFailure`); rollback's +/// job is "make the tree unpatched", and a not-installed package already +/// satisfies that end state — so even a run whose in-scope targets ALL turn +/// out not-installed exits 0 / `success`. Do not "fix" this into symmetry: +/// `remove` also rides on it (it drops long-uninstalled entries from the +/// manifest via its "No packages found to rollback" path). +struct RollbackOutcome { + /// No attempted rollback failed (per-package; see above). + success: bool, + results: Vec, + /// Vendor-owned purls excluded from in-place rollback (benign). + vendored_skipped: Vec, + /// In-scope manifest entries with no installed package on disk — + /// apply's `unmatched` twin (`package_not_installed`). Never in the + /// before-blob plan, never a failed result. Sorted for determinism. + not_installed: Vec, +} + // ── local-redirect rollback helpers (go only) ──────────────────────────────── // Local go rolls back by dropping the project-local redirect (go's `replace` // directive) + the patched copy — no in-place restore, no before-blob. Cargo @@ -231,6 +255,21 @@ fn result_to_json(result: &RollbackResult) -> serde_json::Value { }) } +/// Skipped marker appended to `results[]` for an in-scope manifest entry +/// with no installed package — apply's `package_not_installed` Skipped +/// event, rollback-side. Deliberately NOT a result record: no `success`, +/// no `error`, `path` null (there is no installed tree to name), and it +/// never counts toward `rolledBack`/`failed` or flips the status — +/// rollback exits 0 even when ALL in-scope targets land here (see +/// `RollbackOutcome` for the apply/rollback asymmetry). +fn skipped_not_installed_json(purl: &str) -> serde_json::Value { + serde_json::json!({ + "purl": purl, + "path": null, + "skipped": "package_not_installed", + }) +} + /// Per-package failure results for the pre-flight before-blob abort. /// /// The abort fires before the rollback loop produces any per-package @@ -292,8 +331,9 @@ fn missing_blob_abort_results( ); results.push(RollbackResult { package_key: purl.clone(), - // In-scope purls the crawler could not resolve are still gated - // fail-closed but have no installed path to report. + // The gate feeds only attempted (crawler-discovered) targets + // here, so a path is always present; the empty-string fallback + // is defensive against that invariant breaking upstream. package_path: all_packages .get(purl) .map(|p| p.display().to_string()) @@ -378,7 +418,19 @@ pub async fn run(args: RollbackArgs) -> i32 { }; match rollback_patches_inner(&args, &manifest_path, Some(&telemetry_client)).await { - Ok((success, results, vendored)) => { + Ok(RollbackOutcome { + success: rollback_success, + results, + vendored_skipped: vendored, + not_installed, + }) => { + // Not-installed entries never flip the exit code — not even + // when ALL in-scope targets land there. Rollback's job is + // "make the tree unpatched", and a not-installed package + // already satisfies that end state, so the run is a success + // (exit 0); apply's all-unmatched `partialFailure` deliberately + // does NOT mirror over. See `RollbackOutcome`. + let success = rollback_success; let rolled_back_count = results .iter() .filter(|r| r.success && !r.files_rolled_back.is_empty()) @@ -407,7 +459,16 @@ pub async fn run(args: RollbackArgs) -> i32 { // Vendor-owned purls excluded from in-place rollback // (benign — `remove` or `vendor --revert` undo them). "vendored": vendored, - "results": results.iter().map(result_to_json).collect::>(), + // Real result records first, then one skipped marker + // per in-scope entry with no installed package — + // apply's `package_not_installed` Skipped event, + // rollback-side. Markers never count toward + // `rolledBack`/`failed` and never flip the status. + "results": results + .iter() + .map(result_to_json) + .chain(not_installed.iter().map(|p| skipped_not_installed_json(p))) + .collect::>(), })) .expect("serializing an in-memory JSON value cannot fail") ); @@ -499,6 +560,19 @@ pub async fn run(args: RollbackArgs) -> i32 { } } + // Apply's unmatched warning, rollback-side — informational only + // (the run still exits 0; see `RollbackOutcome`), so --silent + // mutes it like every other non-error notice. + if !args.common.json && !args.common.silent && !not_installed.is_empty() { + eprintln!( + "\nWarning: {} manifest patch(es) had no matching installed package:", + not_installed.len() + ); + for purl in ¬_installed { + eprintln!(" - {purl}"); + } + } + if success { track_patch_rolled_back( rolled_back_count, @@ -557,7 +631,7 @@ async fn rollback_patches_inner( // delegation path) builds one on demand, only when the blob download // below actually fires. api_client: Option<&ApiClient>, -) -> Result<(bool, Vec, Vec), String> { +) -> Result { let manifest = read_manifest(manifest_path) .await .map_err(|e| e.to_string())? @@ -589,7 +663,12 @@ async fn rollback_patches_inner( if !args.common.silent && !args.common.json { println!("No patches found in manifest"); } - return Ok((true, Vec::new(), Vec::new())); + return Ok(RollbackOutcome { + success: true, + results: Vec::new(), + vendored_skipped: Vec::new(), + not_installed: Vec::new(), + }); } // Vendor-owned purls are excluded from in-place rollback: their patch @@ -610,7 +689,12 @@ async fn rollback_patches_inner( if patches_to_rollback.is_empty() { // Everything targeted is vendor-owned: a benign skip, not an error // (and not `not_found` — the identifier did match). - return Ok((true, Vec::new(), vendored_skipped)); + return Ok(RollbackOutcome { + success: true, + results: Vec::new(), + vendored_skipped, + not_installed: Vec::new(), + }); } // Create filtered manifest (a synthetic rollback-target subset, never @@ -728,26 +812,32 @@ async fn rollback_patches_inner( } // Check for missing beforeHash blobs — AFTER discovery and variant - // narrowing, so a broad manifest's sibling variants that resolved to - // the same installed package but were narrowed away (they describe a - // distribution that is not on disk) don't gate the run: an - // unfetchable sibling before-blob used to abort the WHOLE rollback - // (`--offline`: wholesale; online: on any download failure) even - // though that variant was never going to be attempted. In-scope - // purls the crawler could NOT resolve keep the fail-closed gate - // (their blobs are still fetched up front). Local-redirect PURLs - // (local-mode go) are excluded as before: their rollback just drops - // the project-local redirect + copy and reads no blobs, so a missing - // before-blob must not block an offline redirect rollback. + // narrowing, so the gate covers ONLY the packages this run will + // actually attempt to restore in place: + // + // * Narrowed-away sibling variants (they describe a distribution + // that is not on disk) don't gate: an unfetchable sibling + // before-blob used to abort the WHOLE rollback even though that + // variant was never going to be attempted. + // * In-scope purls the crawler could NOT resolve (package not + // installed) don't gate either: there is nothing on disk to + // restore, so no before-blob is ever read for them. They used to + // be gated "fail-closed", which hard-failed the run (exit 1, + // `Cannot rollback: ... Before blob not found`, `path: ""`) over + // an entry that had nothing to roll back — the same entry apply + // reports as a benign `package_not_installed` skip. They surface + // via `not_installed` below instead. + // * Local-redirect PURLs (local-mode go) are excluded as before: + // their rollback just drops the project-local redirect + copy and + // reads no blobs, so a missing before-blob must not block an + // offline redirect rollback. let attempted_purls: HashSet<&str> = rollback_targets.iter().map(|(p, _)| p.as_str()).collect(); let gate_manifest = exclude_local_redirects( &PatchManifest { patches: scoped_manifest .patches .iter() - .filter(|(purl, _)| { - attempted_purls.contains(purl.as_str()) || !all_packages.contains_key(*purl) - }) + .filter(|(purl, _)| attempted_purls.contains(purl.as_str())) .map(|(k, v)| (k.clone(), v.clone())) .collect(), setup: None, @@ -755,6 +845,19 @@ async fn rollback_patches_inner( &args.common, ); + // Apply's `unmatched` twin: in-scope manifest entries the crawler found + // no installed package for. Undiscovered local redirects are NOT + // not-installed — their rollback runs from the manifest alone (the + // fallback loop below). Sorted so every consumer sees a deterministic + // order across the manifest HashMap's iteration order. + let mut not_installed: Vec = scoped_manifest + .patches + .keys() + .filter(|purl| !all_packages.contains_key(*purl) && !undiscovered_redirects.contains(*purl)) + .cloned() + .collect(); + not_installed.sort(); + // `--dry-run`: verification needs real blob content for an accurate // preview, but the preview must not leave new files in the committable // `.socket/blobs` (a wet run's sweep would have removed them) — so stage @@ -790,8 +893,49 @@ async fn rollback_patches_inner( None }; - let missing_blobs = get_missing_before_blobs(&gate_manifest, &blobs_path).await; + // Of the absent blobs, keep only those an installed file would actually + // READ: the engine restores from a before-blob only when the on-disk + // file exists and is not already at its original bytes — + // `verify_file_rollback` reports `MissingBlob` exactly then (and checks + // `AlreadyOriginal` BEFORE probing the blob). An absent blob for an + // already-original, deleted, or locally-drifted file is never read, so + // it must not abort the run or trigger a download; the rollback loop's + // own per-file verification still reports those states honestly + // (already_original / not_found / hash_mismatch). + let absent_blobs = get_missing_before_blobs(&gate_manifest, &blobs_path).await; + let mut missing_blobs: HashSet = HashSet::new(); + let mut blob_gated_purls: HashSet = HashSet::new(); + if !absent_blobs.is_empty() { + for (purl, patch) in &gate_manifest.patches { + let pkg_path = all_packages + .get(purl) + .expect("gate manifest holds only attempted targets, which the crawler discovered"); + for (file, info) in &patch.files { + if info.before_hash.is_empty() || !absent_blobs.contains(&info.before_hash) { + continue; + } + let v = verify_file_rollback(pkg_path, file, info, &blobs_path).await; + if v.status == VerifyRollbackStatus::MissingBlob { + missing_blobs.insert(info.before_hash.clone()); + blob_gated_purls.insert(purl.clone()); + } + } + } + } if !missing_blobs.is_empty() { + // Only the packages that genuinely need a missing blob enter the + // synthesized abort envelope — a gated sibling file that happens to + // share a needed hash rides along, but a package none of whose + // absent blobs are needed never fails here. + let abort_manifest = PatchManifest { + patches: gate_manifest + .patches + .iter() + .filter(|(purl, _)| blob_gated_purls.contains(purl.as_str())) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(), + setup: None, + }; if args.common.offline { // Errors print even under --silent ("errors only", never // "nothing"): in human mode this bail is the run's only @@ -804,14 +948,23 @@ async fn rollback_patches_inner( ); eprintln!("Run \"socket-patch repair\" to download missing blobs."); } - let results = - missing_blob_abort_results(&gate_manifest, &missing_blobs, &all_packages, |hash| { + let results = missing_blob_abort_results( + &abort_manifest, + &missing_blobs, + &all_packages, + |hash| { format!( "Before blob not found: {hash} and --offline prevents fetching. \ Run \"socket-patch repair\" to download missing blobs." ) - }); - return Ok((false, results, vendored_skipped)); + }, + ); + return Ok(RollbackOutcome { + success: false, + results, + vendored_skipped, + not_installed, + }); } if !args.common.silent && !args.common.json { @@ -834,12 +987,17 @@ async fn rollback_patches_inner( println!("{}", format_fetch_result(&fetch_result)); } - // Re-check against `gate_manifest` (NOT `filtered_manifest`): the - // download only targeted blobs from the local-go-excluded gate, so - // local-go before-hashes must stay excluded here too. Re-checking - // the full filtered manifest would re-introduce those never-needed - // blobs and spuriously abort a mixed local-go rollback. - let still_missing = get_missing_before_blobs(&gate_manifest, &blobs_path).await; + // Re-check ONLY the needed-missing set the download targeted (built + // from the local-go-excluded, installed-only gate above) — never the + // full filtered manifest, which would re-introduce never-needed + // blobs (local-go, not-installed, already-original) and spuriously + // abort the run over a blob nothing will read. + let mut still_missing: HashSet = HashSet::new(); + for hash in &missing_blobs { + if tokio::fs::metadata(blobs_path.join(hash)).await.is_err() { + still_missing.insert(hash.clone()); + } + } if !still_missing.is_empty() { // Errors print even under --silent — same contract as the // offline bail above (and same `--json` carrier). @@ -862,8 +1020,11 @@ async fn rollback_patches_inner( ) }) .collect(); - let results = - missing_blob_abort_results(&gate_manifest, &still_missing, &all_packages, |hash| { + let results = missing_blob_abort_results( + &abort_manifest, + &still_missing, + &all_packages, + |hash| { let why = download_errors .get(hash) .copied() @@ -872,8 +1033,14 @@ async fn rollback_patches_inner( "Before blob could not be downloaded: {hash} - {why}. \ Run \"socket-patch repair\" to download missing blobs." ) - }); - return Ok((false, results, vendored_skipped)); + }, + ); + return Ok(RollbackOutcome { + success: false, + results, + vendored_skipped, + not_installed, + }); } } @@ -881,7 +1048,15 @@ async fn rollback_patches_inner( if !args.common.silent && !args.common.json { println!("No packages found that match patches to rollback"); } - return Ok((true, Vec::new(), vendored_skipped)); + // `success: true` — per-package semantics for the `remove` + // delegation. The CLI boundary layers apply's "nothing matched at + // all" exit-1 on top via `not_installed`. + return Ok(RollbackOutcome { + success: true, + results: Vec::new(), + vendored_skipped, + not_installed, + }); } // Rollback patches @@ -954,12 +1129,24 @@ async fn rollback_patches_inner( results.push(result); } - Ok((!has_errors, results, vendored_skipped)) + Ok(RollbackOutcome { + success: !has_errors, + results, + vendored_skipped, + not_installed, + }) } // Export for use by remove command. The third tuple element lists // vendor-owned purls that were excluded from in-place rollback (benign). // +// The returned `bool` is `RollbackOutcome::success` — per-package semantics +// only. Manifest entries whose package is not installed are NOT failures +// here (there is nothing on disk to restore), so `remove` proceeds to drop +// them from the manifest; the CLI `rollback` boundary's apply-mirroring +// "none matched → exit 1" rule deliberately does NOT apply to this +// delegation (it would wedge `remove` for packages long uninstalled). +// // Takes the caller's `GlobalArgs` as the base (only the per-call fields are // overridden): the nested missing-blob download builds its API client from // `api_client_overrides()`, so flag-passed `--api-url` / `--api-token` / @@ -987,7 +1174,8 @@ pub(crate) async fn rollback_patches( }, one_off: false, }; - rollback_patches_inner(&args, manifest_path, None).await + let outcome = rollback_patches_inner(&args, manifest_path, None).await?; + Ok((outcome.success, outcome.results, outcome.vendored_skipped)) } #[cfg(test)] @@ -1336,10 +1524,13 @@ mod tests { assert_eq!(f.message.as_deref(), Some("gone: missing_a")); } - /// Undiscovered packages (crawler found no installed copy — their blobs - /// still gate the run fail-closed) synthesize with an empty path, and - /// multiple affected packages come out purl-sorted so the envelope is - /// deterministic across the manifest HashMap's iteration order. + /// Helper-level determinism + tolerance pin: multiple affected packages + /// come out purl-sorted (stable envelope across the manifest HashMap's + /// iteration order), and a purl absent from `all_packages` degrades to + /// an empty path rather than panicking. Production can no longer feed + /// an undiscovered purl here — since the gate reorder, only attempted + /// (crawler-discovered) targets enter the blob plan, so `path` is + /// always populated in real envelopes; the tolerance is defensive. #[test] fn missing_blob_abort_results_sorted_and_pathless_when_undiscovered() { let mut patches = HashMap::new(); @@ -1905,9 +2096,36 @@ mod tests { ); } - /// The scoped gate still protects in-scope patches: with no - /// `--ecosystems` filter, a missing before-blob for an in-scope npm patch - /// must abort the offline run exactly as before. + /// Write a fake installed npm package so the crawler discovers it and + /// the before-blob gate has an attempted target to protect. `content` + /// is the installed `index.js` bytes (whose hash decides whether the + /// engine would actually need the before-blob). + async fn install_fake_npm_package(root: &Path, name: &str, version: &str, content: &[u8]) { + tokio::fs::write( + root.join("package.json"), + r#"{ "name": "gate-test-root", "version": "0.0.0" }"#, + ) + .await + .unwrap(); + let pkg_dir = root.join("node_modules").join(name); + tokio::fs::create_dir_all(&pkg_dir).await.unwrap(); + tokio::fs::write( + pkg_dir.join("package.json"), + format!(r#"{{ "name": "{name}", "version": "{version}" }}"#), + ) + .await + .unwrap(); + tokio::fs::write(pkg_dir.join("index.js"), content) + .await + .unwrap(); + } + + /// The scoped gate still protects in-scope INSTALLED patches: with no + /// `--ecosystems` filter, a missing before-blob for an installed npm + /// package whose file genuinely needs restoring must abort the offline + /// run exactly as before. (The package is installed here — since the + /// gate reorder a not-installed entry never enters the blob plan; see + /// `not_installed_entry_never_enters_blob_plan` below.) #[tokio::test] async fn before_blob_gate_still_blocks_in_scope_missing_blob() { let tmp = tempfile::tempdir().unwrap(); @@ -1916,6 +2134,11 @@ mod tests { let blobs = socket.join("blobs"); tokio::fs::create_dir_all(&blobs).await.unwrap(); + // Installed, with bytes matching NEITHER beforeHash nor afterHash: + // the file exists and is not already original, so the engine would + // read the before-blob — the gate must fail closed on its absence. + install_fake_npm_package(root, "foo", "1.0.0", b"patched-ish content\n").await; + let mut patches = HashMap::new(); patches.insert( "pkg:npm/foo@1.0.0".to_string(), @@ -1955,6 +2178,10 @@ mod tests { assert_eq!(results.len(), 1, "got {results:?}"); assert_eq!(results[0].package_key, "pkg:npm/foo@1.0.0"); assert!(!results[0].success); + assert!( + !results[0].package_path.is_empty(), + "a gated package is installed, so its path must be reported, got {results:?}" + ); assert!( results[0] .files_verified @@ -1964,4 +2191,124 @@ mod tests { "the missing blob must be named, got {results:?}" ); } + + /// Regression (rollback ordering): a manifest entry whose package is + /// NOT installed must never enter the before-blob plan. Before the gate + /// reorder, its missing before-blob hard-failed the whole offline run + /// (exit 1, `Cannot rollback: ... Before blob not found`, `path: ""`) + /// even though there was nothing on disk to roll back. Through the + /// `remove`-facing delegation this is a benign no-op: success with zero + /// results, exactly as when the blob IS present — so `remove` can drop + /// the entry of a long-uninstalled package either way. + #[tokio::test] + async fn not_installed_entry_never_enters_blob_plan() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let socket = root.join(".socket"); + tokio::fs::create_dir_all(&socket).await.unwrap(); + // No node_modules at all — the package is not installed, and the + // blobs dir does not even exist. + + let mut patches = HashMap::new(); + patches.insert( + "pkg:npm/foo@1.0.0".to_string(), + record_with_file("uuid-npm", "package/index.js", "npm_before_hash"), + ); + let manifest = PatchManifest { + patches, + setup: None, + }; + let manifest_path = socket.join("manifest.json"); + tokio::fs::write(&manifest_path, serde_json::to_string(&manifest).unwrap()) + .await + .unwrap(); + + // `--offline` proves no download is attempted for the unneeded blob. + let common = crate::args::GlobalArgs { + cwd: root.to_path_buf(), + offline: true, + ..crate::args::GlobalArgs::default() + }; + let (success, results, vendored_skipped) = rollback_patches( + &common, + &manifest_path, + None, + false, // dry_run + true, // silent + None, + ) + .await + .expect("rollback must not error"); + assert!( + success, + "a not-installed entry's missing before-blob must not fail the run" + ); + assert!( + results.is_empty(), + "nothing installed, nothing attempted, got {results:?}" + ); + assert!(vendored_skipped.is_empty()); + } + + /// The needed-blob narrowing: an INSTALLED package whose file is already + /// at its original bytes needs no before-blob (the engine checks + /// `AlreadyOriginal` before probing the blob), so a missing — e.g. + /// GC'd — blob must not abort the offline run. The rollback proceeds + /// and reports the no-op honestly. + #[tokio::test] + async fn missing_blob_for_already_original_file_does_not_gate() { + use socket_patch_core::hash::git_sha256::compute_git_sha256_from_bytes; + + let original = b"original content\n"; + let before_hash = compute_git_sha256_from_bytes(original); + + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path(); + let socket = root.join(".socket"); + tokio::fs::create_dir_all(&socket).await.unwrap(); + + // Installed at the BEFORE bytes — rollback is a no-op for it. + install_fake_npm_package(root, "foo", "1.0.0", original).await; + + let mut patches = HashMap::new(); + patches.insert( + "pkg:npm/foo@1.0.0".to_string(), + record_with_file("uuid-npm", "package/index.js", &before_hash), + ); + let manifest = PatchManifest { + patches, + setup: None, + }; + let manifest_path = socket.join("manifest.json"); + tokio::fs::write(&manifest_path, serde_json::to_string(&manifest).unwrap()) + .await + .unwrap(); + // The before-blob is deliberately absent (e.g. garbage-collected). + + let common = crate::args::GlobalArgs { + cwd: root.to_path_buf(), + offline: true, + ..crate::args::GlobalArgs::default() + }; + let (success, results, _vendored_skipped) = rollback_patches( + &common, + &manifest_path, + None, + false, // dry_run + true, // silent + None, + ) + .await + .expect("rollback must not error"); + assert!( + success, + "a blob nothing will read must not gate the run, got {results:?}" + ); + assert_eq!(results.len(), 1, "got {results:?}"); + assert!(results[0].success); + assert!( + all_files_already_original(&results[0]), + "the no-op must be reported as already original, got {results:?}" + ); + } } diff --git a/crates/socket-patch-cli/tests/cli_rollback_silent.rs b/crates/socket-patch-cli/tests/cli_rollback_silent.rs index 4dd96491..2aff4f1c 100644 --- a/crates/socket-patch-cli/tests/cli_rollback_silent.rs +++ b/crates/socket-patch-cli/tests/cli_rollback_silent.rs @@ -78,8 +78,32 @@ fn git_sha256(content: &[u8]) -> String { hex::encode(hasher.finalize()) } -/// Manifest with one npm patch whose before-blob is NOT staged. +/// Manifest with one npm patch whose before-blob is NOT staged — plus the +/// package INSTALLED under `node_modules/` with its file off the original +/// bytes. Installation matters: the before-blob gate covers only installed +/// packages whose files genuinely need their original bytes back. A +/// manifest entry with no installed package is a benign +/// `package_not_installed` skip (nothing on disk to restore), which would +/// never reach the missing-blob/undownloadable-blob error paths these +/// tests pin. fn write_missing_blob_manifest(root: &Path) { + std::fs::write( + root.join("package.json"), + r#"{ "name": "rb-silent-root", "version": "0.0.0" }"#, + ) + .unwrap(); + let pkg_dir = root.join("node_modules/__rb_silent__"); + std::fs::create_dir_all(&pkg_dir).unwrap(); + std::fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "__rb_silent__", "version": "1.0.0" }"#, + ) + .unwrap(); + // Any content that matches neither the (unsatisfiable all-zeros) + // beforeHash nor the afterHash: the file needs restoring, so the + // absent before-blob genuinely gates the rollback. + std::fs::write(pkg_dir.join("index.js"), b"patched-ish content\n").unwrap(); + let socket = root.join(".socket"); std::fs::create_dir_all(&socket).unwrap(); std::fs::write( diff --git a/crates/socket-patch-cli/tests/remove_invariants.rs b/crates/socket-patch-cli/tests/remove_invariants.rs index c23dfa58..b3c18251 100644 --- a/crates/socket-patch-cli/tests/remove_invariants.rs +++ b/crates/socket-patch-cli/tests/remove_invariants.rs @@ -50,6 +50,32 @@ fn make_socket_dir(root: &Path) -> PathBuf { socket } +/// Install `__remove_test_a__` under `node_modules/` with `a.js` matching +/// neither the (unsatisfiable all-zeros) beforeHash nor the afterHash: the +/// file genuinely needs its original bytes back, so an absent before-blob +/// blocks the internal rollback. +/// +/// The rollback-failure tests need this because the before-blob gate covers +/// only INSTALLED packages whose files need restoring: a manifest entry with +/// no installed package is a benign `package_not_installed` skip (nothing on +/// disk to restore), which `remove` correctly proceeds past — it would never +/// reach the `rollback_failed` paths those tests pin. +fn install_remove_test_a(root: &Path) { + std::fs::write( + root.join("package.json"), + r#"{ "name": "remove-invariants-root", "version": "0.0.0" }"#, + ) + .expect("write root package.json"); + let pkg_dir = root.join("node_modules/__remove_test_a__"); + std::fs::create_dir_all(&pkg_dir).expect("create package dir"); + std::fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "__remove_test_a__", "version": "1.0.0" }"#, + ) + .expect("write package.json"); + std::fs::write(pkg_dir.join("a.js"), b"patched-ish content\n").expect("write a.js"); +} + /// All spawns go through `common::run_with_env`, which scrubs the ambient /// `SOCKET_*` environment: an inherited SOCKET_DRY_RUN=true silently turns /// every wet remove below into a no-op preview, and an inherited @@ -258,11 +284,20 @@ fn remove_event_has_required_envelope_fields() { /// `--offline` is what keeps this hermetic: without it, rollback fetches the /// missing before-blob from the live proxy (`GET /patch/blob/`) /// and the test only passes because that request 404s. +/// +/// The package must be INSTALLED (with its file off the original bytes) for +/// the missing before-blob to fail the rollback: since the before-blob gate +/// reorder, a manifest entry with no installed package is a benign +/// `package_not_installed` skip — nothing on disk to restore — and `remove` +/// correctly proceeds to drop it (its "No packages found to rollback (not +/// installed)" path). Only an installed, patched package with an absent +/// before-blob still fails closed. #[test] fn remove_without_skip_rollback_fails_closed_and_keeps_manifest() { let tmp = tempfile::tempdir().expect("tempdir"); let socket = make_socket_dir(tmp.path()); let before = std::fs::read(socket.join("manifest.json")).expect("read before"); + install_remove_test_a(tmp.path()); let (code, stdout, _stderr) = common::run_with_env( tmp.path(), @@ -665,10 +700,16 @@ fn remove_dry_run_previews_blob_sweep_without_deleting() { /// would have deleted. Offline keeps this hermetic: the preview reports the /// missing-blob failure (accurate — a wet offline run fails the same way) /// without inventing directories. +/// +/// The package is installed (see `install_remove_test_a`) so the missing +/// before-blob genuinely fails the rollback preview — and the preview walks +/// its full path, including the throwaway dry-run blob stage the litter +/// check below covers. #[test] fn remove_dry_run_with_rollback_does_not_create_blobs_dir() { let tmp = tempfile::tempdir().unwrap(); let socket = make_socket_dir(tmp.path()); + install_remove_test_a(tmp.path()); assert!(!socket.join("blobs").exists(), "precondition: no blobs dir"); let (code, stdout, _stderr) = common::run_with_env( diff --git a/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs b/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs index b1090242..7433b474 100644 --- a/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs +++ b/crates/socket-patch-cli/tests/remove_rollback_api_overrides.rs @@ -57,6 +57,7 @@ const SOCKET_ENV_VARS: &[&str] = &[ "SOCKET_TELEMETRY_DISABLED", "SOCKET_ONE_OFF", "SOCKET_SKIP_ROLLBACK", + "SOCKET_NO_TRUST_LOCKFILE_CONFIG", ]; /// Drift guard: the scrub must cover every env var `GlobalArgs` binds — the @@ -148,6 +149,8 @@ fn dead_port() -> u16 { fn remove_rollback_downloads_missing_blob_via_flag_overrides() { let before = b"original-content\n"; let before_hash = git_sha256(before); + let after = b"patched-content\n"; + let after_hash = git_sha256(after); let (port, seen_paths) = spawn_blob_server(before_hash.clone(), before.to_vec()); let dead = dead_port(); @@ -155,6 +158,25 @@ fn remove_rollback_downloads_missing_blob_via_flag_overrides() { let tmp = tempfile::tempdir().expect("tempdir"); let socket = tmp.path().join(".socket"); std::fs::create_dir_all(socket.join("blobs")).unwrap(); + // The package must be INSTALLED, at its patched (afterHash) state: + // since the before-blob gate reorder, only installed packages whose + // files genuinely need their original bytes back enter the blob plan — + // a manifest-only entry is a benign `package_not_installed` skip that + // never downloads anything, and the restore below must succeed for + // `remove` to exit 0. + std::fs::write( + tmp.path().join("package.json"), + r#"{ "name": "ovr-root", "version": "0.0.0" }"#, + ) + .unwrap(); + let pkg_dir = tmp.path().join("node_modules/__ovr_test__"); + std::fs::create_dir_all(&pkg_dir).unwrap(); + std::fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "__ovr_test__", "version": "1.0.0" }"#, + ) + .unwrap(); + std::fs::write(pkg_dir.join("index.js"), after).unwrap(); // The before-blob is deliberately ABSENT from .socket/blobs: the // rollback gate must download it through the flag-configured client. let manifest = format!( @@ -166,7 +188,7 @@ fn remove_rollback_downloads_missing_blob_via_flag_overrides() { "files": {{ "package/index.js": {{ "beforeHash": "{before_hash}", - "afterHash": "1111111111111111111111111111111111111111111111111111111111111111" + "afterHash": "{after_hash}" }} }}, "vulnerabilities": {{}}, diff --git a/crates/socket-patch-cli/tests/rollback_invariants.rs b/crates/socket-patch-cli/tests/rollback_invariants.rs index 33b6f155..830f5c03 100644 --- a/crates/socket-patch-cli/tests/rollback_invariants.rs +++ b/crates/socket-patch-cli/tests/rollback_invariants.rs @@ -75,6 +75,28 @@ fn make_socket_dir(root: &Path) -> PathBuf { socket } +/// Install the `MANIFEST_JSON` package (`__rollback_test__@1.0.0`) as a fake +/// npm package so the crawler discovers it. The installed `index.js` matches +/// NEITHER manifest hash — the file exists and is not already original, so +/// the engine genuinely needs the before-blob and the pre-flight gate must +/// protect it. Since the gate reorder, only installed packages enter the +/// before-blob plan: a manifest-only fixture no longer exercises the gate. +fn install_manifest_package(root: &Path) { + std::fs::write( + root.join("package.json"), + r#"{ "name": "rollback-invariants-root", "version": "0.0.0" }"#, + ) + .expect("write root package.json"); + let pkg_dir = root.join("node_modules/__rollback_test__"); + std::fs::create_dir_all(&pkg_dir).expect("create package dir"); + std::fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "__rollback_test__", "version": "1.0.0" }"#, + ) + .expect("write package.json"); + std::fs::write(pkg_dir.join("index.js"), b"installed-but-drifted\n").expect("write index.js"); +} + fn run(cwd: &Path, args: &[&str]) -> (i32, String) { let out = rollback_cmd(cwd) .args(args) @@ -260,15 +282,20 @@ const MISSING_BEFORE_HASH: &str = #[test] fn rollback_offline_with_missing_before_blob_partial_failure() { - // Manifest has a patch whose beforeHash is NOT on disk; --offline + // The package is INSTALLED (drifted bytes, so the before-blob is + // genuinely needed) but its beforeHash blob is NOT on disk; --offline // means we won't fetch. Rollback must fail out before touching // anything — and the JSON envelope must SAY so. The bail fires before // the rollback loop produces any per-package results, so the failures // are synthesized: before the fix the envelope claimed `failed: 0` // with empty `results[]` on an exit-1 run, and `--json` mutes the // stderr explanation, leaving machine consumers zero diagnostic. + // (Installing the package matters since the gate reorder: an entry + // with no installed package never enters the blob plan — see + // `rollback_only_not_installed_entry_is_never_blob_gated`.) let tmp = tempfile::tempdir().expect("tempdir"); make_socket_dir(tmp.path()); + install_manifest_package(tmp.path()); let (code, stdout) = run(tmp.path(), &["--json", "--offline"]); assert_eq!( code, 1, @@ -295,6 +322,10 @@ fn rollback_offline_with_missing_before_blob_partial_failure() { let entry = &results[0]; assert_eq!(entry["purl"], "pkg:npm/__rollback_test__@1.0.0"); assert_eq!(entry["success"], false); + assert!( + !entry["path"].as_str().expect("path string").is_empty(), + "a blob-gated package is installed, so its path must be reported; stdout=\n{stdout}" + ); assert_eq!( entry["filesRolledBack"] .as_array() @@ -342,6 +373,7 @@ fn rollback_offline_with_missing_before_blob_partial_failure() { fn rollback_offline_missing_blob_human_names_package_and_remedy() { let tmp = tempfile::tempdir().expect("tempdir"); make_socket_dir(tmp.path()); + install_manifest_package(tmp.path()); let out = rollback_cmd(tmp.path()) .args(["--offline"]) .output() @@ -381,6 +413,7 @@ fn rollback_offline_missing_blob_human_names_package_and_remedy() { fn rollback_undownloadable_blob_envelope_names_blob_and_remedy() { let tmp = tempfile::tempdir().expect("tempdir"); make_socket_dir(tmp.path()); + install_manifest_package(tmp.path()); let out = rollback_cmd(tmp.path()) .env("SOCKET_TELEMETRY_DISABLED", "1") .args([ @@ -439,13 +472,77 @@ fn rollback_undownloadable_blob_envelope_names_blob_and_remedy() { } // --------------------------------------------------------------------------- -// No-package-installed happy path +// Not-installed manifest entries (apply-mirrored contract) // --------------------------------------------------------------------------- +/// Shared assertions for the "manifest holds ONLY a not-installed entry" +/// envelope, blob staged or not — the two must be indistinguishable, because +/// a not-installed entry never enters the before-blob plan at all. +/// +/// CONTRACT (deliberately ASYMMETRIC with `apply`): a manifest whose +/// in-scope entries ALL lack an installed package exits 0 with status +/// `success`. Apply's job is "make the tree patched", so its all-unmatched +/// run is a `partialFailure` — the job was NOT done; rollback's job is +/// "make the tree unpatched", and a not-installed package already +/// satisfies that end state. Each such entry is surfaced as one skipped +/// marker appended to `results[]` — `path` null, `skipped: +/// "package_not_installed"`, no `success`/`error` keys — NEVER as a failed +/// result: `failed` stays 0 and no result ever carries `path: ""`. There +/// is no top-level `notInstalled` key. +fn assert_only_not_installed_envelope(code: i32, stdout: &str) { + assert_eq!( + code, 0, + "all-not-installed succeeds quietly (the tree is already unpatched; \ + see the asymmetry contract above); stdout=\n{stdout}" + ); + let v: serde_json::Value = serde_json::from_str(stdout).expect("valid JSON"); + assert_eq!(v["status"], "success", "stdout=\n{stdout}"); + assert_eq!(v["rolledBack"], 0); + assert_eq!(v["alreadyOriginal"], 0); + assert_eq!( + v["failed"], 0, + "nothing was attempted, so nothing failed — a not-installed entry \ + is a skip, not a failure; stdout=\n{stdout}" + ); + assert!( + v.get("notInstalled").is_none(), + "the top-level notInstalled key was dropped in favor of per-entry \ + skipped markers in results[]; stdout=\n{stdout}" + ); + let results = v["results"].as_array().expect("results array"); + assert_eq!( + results.len(), + 1, + "exactly one skipped marker for the one not-installed entry; \ + stdout=\n{stdout}" + ); + let marker = &results[0]; + assert_eq!(marker["purl"], "pkg:npm/__rollback_test__@1.0.0"); + assert!( + marker["path"].is_null(), + "no installed tree to name — path must be null, never \"\"; \ + stdout=\n{stdout}" + ); + assert_eq!( + marker["skipped"], "package_not_installed", + "stdout=\n{stdout}" + ); + assert!( + marker.get("success").is_none() && marker.get("error").is_none(), + "a skipped marker is not a result record; stdout=\n{stdout}" + ); + assert!( + !stdout.contains("missing_blob") && !stdout.contains("Before blob not found"), + "a not-installed entry must never surface a blob problem; stdout=\n{stdout}" + ); +} + #[test] fn rollback_with_no_installed_packages_succeeds_quietly() { - // beforeHash blob is on disk, no installed packages match — rollback - // succeeds with zero results. + // beforeHash blob IS on disk, no installed packages match. Nothing to + // roll back → exit 0: rollback's goal ("tree unpatched") is already + // met, unlike apply's all-unmatched partialFailure (see the asymmetry + // contract on `assert_only_not_installed_envelope`). let tmp = tempfile::tempdir().expect("tempdir"); let socket = make_socket_dir(tmp.path()); let before_hash = "0000000000000000000000000000000000000000000000000000000000000000"; @@ -453,16 +550,193 @@ fn rollback_with_no_installed_packages_succeeds_quietly() { std::fs::create_dir_all(&blobs).unwrap(); std::fs::write(blobs.join(before_hash), b"original content").unwrap(); - let (code, stdout) = run(tmp.path(), &["--json"]); + let (code, stdout) = run(tmp.path(), &["--json", "--offline"]); + assert_only_not_installed_envelope(code, &stdout); +} + +/// Regression (rollback ordering, pnpm matrix legs): the SAME manifest with +/// the before-blob MISSING must produce the identical envelope — the entry +/// has no installed package, so its blob is never planned, probed, or +/// fetched. Before the gate reorder this run hard-failed with exit 1, +/// `failed: 1`, and a synthesized `Cannot rollback: ... Before blob not +/// found` result carrying `path: ""` — a blob error for a package with +/// nothing on disk to roll back. +#[test] +fn rollback_only_not_installed_entry_is_never_blob_gated() { + let tmp = tempfile::tempdir().expect("tempdir"); + make_socket_dir(tmp.path()); + // No node_modules, no .socket/blobs — nothing installed, no blobs. + + let (code, stdout) = run(tmp.path(), &["--json", "--offline"]); + assert_only_not_installed_envelope(code, &stdout); +} + +/// Online twin: a not-installed entry must not trigger a blob download +/// either. Both endpoints are pinned to an unroutable localhost port — if +/// the gate still planned this blob, the fetch would fail and the envelope +/// would carry a `could not be downloaded` failure. Instead the run never +/// fetches and reports the quiet not-installed success envelope. +#[test] +fn rollback_not_installed_entry_triggers_no_blob_download() { + let tmp = tempfile::tempdir().expect("tempdir"); + make_socket_dir(tmp.path()); + + let out = rollback_cmd(tmp.path()) + .env("SOCKET_TELEMETRY_DISABLED", "1") + .args([ + "--json", + "--api-url", + "http://127.0.0.1:1/", + "--proxy-url", + "http://127.0.0.1:1/", + ]) + .output() + .expect("run socket-patch"); + let stdout = String::from_utf8_lossy(&out.stdout).to_string(); + let stderr = String::from_utf8_lossy(&out.stderr).to_string(); + assert_only_not_installed_envelope(out.status.code().unwrap_or(-1), &stdout); + assert!( + !stdout.contains("could not be downloaded") && !stderr.contains("could not be downloaded"), + "no download may be attempted for a not-installed entry's blob; \ + stdout=\n{stdout}\nstderr=\n{stderr}" + ); +} + +/// Mixed manifest: one entry INSTALLED with its before-blob missing (must +/// still fail with the pinned missing-blob abort envelope), one entry NOT +/// installed (must surface as a skipped marker in `results[]`, never as a +/// failed result, and never with `path: ""`). The failure comes solely +/// from the installed package; the not-installed entry rides along as a +/// skip. +#[test] +fn rollback_mixed_installed_gated_and_not_installed_entries() { + let tmp = tempfile::tempdir().expect("tempdir"); + let socket = tmp.path().join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + // Two patches: `installed-target` (on disk below) and + // `__ghost__` (nothing on disk). Both name missing before-blobs. + std::fs::write( + socket.join("manifest.json"), + r#"{ "patches": { + "pkg:npm/installed-target@1.0.0": { + "uuid": "66666666-6666-4666-8666-666666666666", + "exportedAt": "2024-01-01T00:00:00Z", + "files": { "package/index.js": { + "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", + "afterHash": "1111111111111111111111111111111111111111111111111111111111111111" + }}, + "vulnerabilities": {}, "description": "x", + "license": "MIT", "tier": "free" + }, + "pkg:npm/__ghost__@2.0.0": { + "uuid": "77777777-7777-4777-8777-777777777777", + "exportedAt": "2024-01-01T00:00:00Z", + "files": { "package/index.js": { + "beforeHash": "2222222222222222222222222222222222222222222222222222222222222222", + "afterHash": "3333333333333333333333333333333333333333333333333333333333333333" + }}, + "vulnerabilities": {}, "description": "x", + "license": "MIT", "tier": "free" + } + }}"#, + ) + .unwrap(); + // Install ONLY `installed-target`, with drifted bytes so the engine + // genuinely needs the (absent) before-blob. + std::fs::write( + tmp.path().join("package.json"), + r#"{ "name": "mixed-root", "version": "0.0.0" }"#, + ) + .unwrap(); + let pkg_dir = tmp.path().join("node_modules/installed-target"); + std::fs::create_dir_all(&pkg_dir).unwrap(); + std::fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "installed-target", "version": "1.0.0" }"#, + ) + .unwrap(); + std::fs::write(pkg_dir.join("index.js"), b"patched-ish\n").unwrap(); + + let (code, stdout) = run(tmp.path(), &["--json", "--offline"]); assert_eq!( - code, 0, - "no installed packages must exit 0; stdout=\n{stdout}" + code, 1, + "the installed package's missing blob must fail the run; stdout=\n{stdout}" ); let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); - assert_eq!(v["status"], "success"); - assert_eq!(v["rolledBack"], 0); - assert_eq!(v["alreadyOriginal"], 0); - assert_eq!(v["failed"], 0); + assert_eq!(v["status"], "partial_failure"); + assert_eq!( + v["failed"], 1, + "only the installed, blob-gated package counts as failed; stdout=\n{stdout}" + ); + let results = v["results"].as_array().expect("results array"); + assert_eq!( + results.len(), + 2, + "one failed result for the installed package plus one skipped \ + marker for the ghost — never a synthesized ghost failure; \ + stdout=\n{stdout}" + ); + let entry = &results[0]; + assert_eq!(entry["purl"], "pkg:npm/installed-target@1.0.0"); + assert_eq!(entry["success"], false); + assert!( + !entry["path"].as_str().expect("path string").is_empty(), + "the gated package is installed — path must be reported; stdout=\n{stdout}" + ); + // The pinned missing-blob abort envelope survives for the installed + // package: engine vocabulary + repair remedy. + let err = entry["error"].as_str().expect("error message string"); + assert!( + err.contains("Cannot rollback") && err.contains("socket-patch repair"), + "pinned abort error shape; got: {err}" + ); + let verified = entry["filesVerified"] + .as_array() + .expect("filesVerified array"); + assert!( + verified + .iter() + .any(|f| f["status"] == "missing_blob" && f["targetHash"] == MISSING_BEFORE_HASH), + "the missing blob must be named with the engine's vocabulary; stdout=\n{stdout}" + ); + // The ghost entry is a skipped marker appended after the real results + // — not a failure — and its (equally missing) before-blob must appear + // nowhere in the failure output. + let marker = &results[1]; + assert_eq!( + marker["purl"], "pkg:npm/__ghost__@2.0.0", + "stdout=\n{stdout}" + ); + assert!( + marker["path"].is_null(), + "no installed tree to name — path must be null, never \"\"; \ + stdout=\n{stdout}" + ); + assert_eq!( + marker["skipped"], "package_not_installed", + "stdout=\n{stdout}" + ); + assert!( + marker.get("success").is_none() && marker.get("error").is_none(), + "a skipped marker is not a result record; stdout=\n{stdout}" + ); + assert!( + v.get("notInstalled").is_none(), + "the top-level notInstalled key was dropped in favor of per-entry \ + skipped markers; stdout=\n{stdout}" + ); + assert!( + results + .iter() + .filter(|r| r.get("skipped").is_none()) + .all(|r| !r["path"].as_str().unwrap_or("").is_empty()), + "no result record may carry an empty path; stdout=\n{stdout}" + ); + assert!( + !stdout.contains("2222222222222222222222222222222222222222222222222222222222222222"), + "the not-installed entry's blob hash must never surface as a \ + failure; stdout=\n{stdout}" + ); } // --------------------------------------------------------------------------- @@ -496,6 +770,12 @@ fn rollback_json_shape_has_documented_keys() { ] { assert!(keys.contains(key), "rollback JSON missing key: {key}"); } + // Not-installed entries surface as per-entry `skipped` markers inside + // `results[]` — there is deliberately NO top-level `notInstalled` key. + assert!( + v.get("notInstalled").is_none(), + "notInstalled was dropped in favor of skipped markers in results[]" + ); // `warnings` is documented as ALWAYS present (empty array when nothing // fired) so consumers can index `.warnings[]` without null-checking. assert!( @@ -850,20 +1130,32 @@ fn rollback_honors_manifest_path_override() { .output() .expect("run socket-patch"); let stdout = String::from_utf8_lossy(&out.stdout).to_string(); + let v: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap(); + // There is NO default `.socket/manifest.json` here, so reaching the + // not-installed envelope (rather than the "Manifest not found" error) + // can only mean the override path was honored. The manifest's only + // entry has no installed package, so the run succeeds quietly (the + // tree is already unpatched): exit 0, success, failed 0, entry + // surfaced as a skipped marker in results[]. + assert!(v["error"].is_null(), "no error expected; stdout={stdout}"); + assert_eq!(v["status"], "success", "stdout={stdout}"); assert_eq!( out.status.code(), Some(0), - "manifest-path override must load + succeed; stdout={stdout}; stderr={}", + "all-not-installed succeeds quietly; stdout={stdout}; stderr={}", String::from_utf8_lossy(&out.stderr) ); - let v: serde_json::Value = serde_json::from_str(stdout.trim()).unwrap(); - // There is NO default `.socket/manifest.json` here, so a "success" status - // can only mean the override path was honored — had it been ignored, the - // command would have hit the no-manifest error path instead. - assert_eq!(v["status"], "success", "stdout={stdout}"); - assert!(v["error"].is_null(), "no error expected; stdout={stdout}"); - // No installed packages match, so the run is a clean zero-work success. assert_eq!(v["rolledBack"], 0); - assert_eq!(v["failed"], 0); + assert_eq!(v["failed"], 0, "not-installed is a skip, not a failure"); assert_eq!(v["alreadyOriginal"], 0); + let results = v["results"].as_array().expect("results array"); + assert_eq!( + results.len(), + 1, + "the override manifest's entry must be the one reported; stdout={stdout}" + ); + assert_eq!( + results[0]["skipped"], "package_not_installed", + "stdout={stdout}" + ); } From a4cf6a1ccd491d7d1cfafdd518a80443ace8e297 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 18 Aug 2026 21:31:45 -0400 Subject: [PATCH 3/5] test(rollback): pin the not-installed marker contract in the global no-op suite global_packages_e2e's no-op helper asserted results[] empty; the marker records from the previous commit now legitimately appear there. Assert the marker shape (path null, skipped=package_not_installed, no success/error keys) instead of emptiness. Co-Authored-By: Claude Fable 5 --- .../tests/global_packages_e2e.rs | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/crates/socket-patch-cli/tests/global_packages_e2e.rs b/crates/socket-patch-cli/tests/global_packages_e2e.rs index 8ddabe13..eaed0f01 100644 --- a/crates/socket-patch-cli/tests/global_packages_e2e.rs +++ b/crates/socket-patch-cli/tests/global_packages_e2e.rs @@ -190,6 +190,11 @@ fn assert_apply_applied(stdout: &str, purl: &str) { /// Parse `stdout` as the `rollback` JSON envelope and assert the exact /// "nothing to roll back" success outcome (no patches were applied, so /// none can be reverted, but the run is clean — not a failure). +/// +/// A manifest entry with no matching installed package surfaces as an +/// additive marker record in `results[]` — `{purl, path: null, skipped: +/// "package_not_installed"}`, no `success`/`error` keys — and never +/// counts toward `rolledBack`/`failed` or flips the status. fn assert_rollback_noop(stdout: &str) { let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("rollback --global must emit valid JSON"); @@ -201,14 +206,17 @@ fn assert_rollback_noop(stdout: &str) { assert_eq!(v["alreadyOriginal"], 0, "envelope={v}"); assert_eq!(v["failed"], 0, "envelope={v}"); assert_eq!(v["dryRun"], false, "envelope={v}"); - assert_eq!( - v["results"] - .as_array() - .expect("results must be an array") - .len(), - 0, - "no package was patched, so results must be empty; envelope={v}" - ); + for r in v["results"].as_array().expect("results must be an array") { + assert_eq!( + r["skipped"], "package_not_installed", + "a no-op rollback may carry only not-installed markers; envelope={v}" + ); + assert!(r["path"].is_null(), "marker path must be null; envelope={v}"); + assert!( + r.get("success").is_none() && r.get("error").is_none(), + "markers carry no success/error keys; envelope={v}" + ); + } } // --------------------------------------------------------------------------- From 08a58f4a0f5e9478bec1d5dcdf43e2ba4cb13017 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 18 Aug 2026 21:39:55 -0400 Subject: [PATCH 4/5] test(remove): install the fixture package so the blob-gate network tests keep their teeth remove_network's fixtures were manifest-only; under the corrected gate (blobs planned only for installed rollback targets) there was nothing to roll back, so the online test observed no fetch and the offline test succeeded. Installing the package at the patched bytes makes the nested rollback genuinely need the beforeHash blob, preserving both tests' documented intent (online fetches exactly that blob; --offline fails closed without touching the network). Co-Authored-By: Claude Fable 5 --- .../socket-patch-cli/tests/remove_network.rs | 32 ++++++++++++++++--- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/crates/socket-patch-cli/tests/remove_network.rs b/crates/socket-patch-cli/tests/remove_network.rs index dc300b45..42f1d555 100644 --- a/crates/socket-patch-cli/tests/remove_network.rs +++ b/crates/socket-patch-cli/tests/remove_network.rs @@ -41,6 +41,27 @@ fn git_sha256(content: &[u8]) -> String { hex::encode(hasher.finalize()) } +/// Install the fixture package PATCHED (file at `after` bytes) so the +/// nested rollback genuinely needs the beforeHash blob. Rollback's blob +/// gate covers only installed rollback targets — a manifest-only fixture +/// has nothing to roll back, never plans a download, and would give the +/// online/offline fetch assertions below nothing to observe. +fn install_patched_package(root: &Path, after: &[u8]) { + std::fs::write( + root.join("package.json"), + r#"{"name":"remove-network-fixture","version":"0.0.0"}"#, + ) + .expect("write root package.json"); + let pkg = root.join("node_modules").join("remove-network-test"); + std::fs::create_dir_all(&pkg).expect("create package dir"); + std::fs::write( + pkg.join("package.json"), + r#"{"name":"remove-network-test","version":"1.0.0"}"#, + ) + .expect("write pkg package.json"); + std::fs::write(pkg.join("index.js"), after).expect("write patched index.js"); +} + fn write_manifest(socket: &Path, before_hash: &str, after_hash: &str) { std::fs::create_dir_all(socket).expect("create .socket"); let body = format!( @@ -127,10 +148,11 @@ fn run_remove(cwd: &Path, api_url: &str, extra: &[&str]) -> (i32, String) { ) } -/// Online sanity: a missing beforeHash blob is fetched, rollback finds no -/// installed package (nothing to restore → success), and the entry is -/// removed. Establishes that the mock can satisfy the download, which is -/// what gives the `--offline` regression test (below) its teeth. +/// Online sanity: with the fixture package installed at the PATCHED +/// bytes, the missing beforeHash blob is fetched, rollback restores the +/// original file, and the entry is removed. Establishes that the mock +/// can satisfy the download, which is what gives the `--offline` +/// regression test (below) its teeth. #[tokio::test] async fn remove_online_downloads_missing_before_blob_then_removes() { let before = b"before\n"; @@ -144,6 +166,7 @@ async fn remove_online_downloads_missing_before_blob_then_removes() { let tmp = tempfile::tempdir().expect("tempdir"); let socket = tmp.path().join(".socket"); write_manifest(&socket, &before_hash, &after_hash); + install_patched_package(tmp.path(), after); let (code, stdout) = run_remove(tmp.path(), &mock.uri(), &[]); assert_eq!(code, 0, "online remove must succeed; stdout=\n{stdout}"); @@ -190,6 +213,7 @@ async fn remove_offline_does_not_fetch_and_keeps_entry() { let tmp = tempfile::tempdir().expect("tempdir"); let socket = tmp.path().join(".socket"); write_manifest(&socket, &before_hash, &after_hash); + install_patched_package(tmp.path(), after); let (code, stdout) = run_remove(tmp.path(), &mock.uri(), &["--offline"]); assert_eq!( From 858442f49c91a010a298c7a1be3537c0c9d174e1 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Tue, 18 Aug 2026 23:02:28 -0400 Subject: [PATCH 5/5] fix(remove): keep revert data and warn when dropping an entry whose rollback was skipped not-installed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up: a package that is installed but missed by the crawler (layout gaps have precedent) reads as a benign not-installed skip; the remove delegation then dropped the manifest entry AND swept its beforeHash blobs — permanently destroying revert data while patched bytes remained on disk, with no machine-visible signal. remove now records an additive Skipped/rollback_not_installed envelope event (naming the purl, details.beforeBlobsRetained, plus a stderr warning) for each dropped entry whose nested rollback was skipped as not-installed, and pins those entries' beforeHash blobs into the cleanup sweep's keep set so a later repair / rollback --one-off can still restore. Standalone rollback's not-installed-exit-0 contract and --skip-rollback semantics are unchanged (both pinned). RED-verified: pre-fix, the new test observed the before-blob swept (blobsRemoved:2, no event). Co-Authored-By: Claude Fable 5 --- .../socket-patch-cli/src/commands/remove.rs | 115 ++++++++- .../socket-patch-cli/src/commands/rollback.rs | 44 +++- .../tests/remove_invariants.rs | 238 ++++++++++++++++++ .../socket-patch-cli/tests/remove_network.rs | 65 +++++ 4 files changed, 447 insertions(+), 15 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/remove.rs b/crates/socket-patch-cli/src/commands/remove.rs index 96cd7de8..1f6710cc 100644 --- a/crates/socket-patch-cli/src/commands/remove.rs +++ b/crates/socket-patch-cli/src/commands/remove.rs @@ -2,7 +2,7 @@ use clap::Args; use socket_patch_core::api::client::get_api_client_with_overrides; use socket_patch_core::manifest::cleanup_blobs::{cleanup_unused_blobs, format_cleanup_result}; use socket_patch_core::manifest::operations::{read_manifest, write_manifest}; -use socket_patch_core::manifest::schema::PatchManifest; +use socket_patch_core::manifest::schema::{PatchFileInfo, PatchManifest}; use socket_patch_core::telemetry::{track_patch_remove_failed, track_patch_removed}; use socket_patch_core::utils::purl::purl_matches_identifier; use socket_patch_core::vendor::{load_state, save_state, VendorEntry, VendorState}; @@ -272,6 +272,16 @@ pub async fn run(args: RemoveArgs) -> i32 { // First, rollback the patch if not skipped let mut rollback_count = 0; + // In-scope manifest entries the nested rollback SKIPPED because the + // crawler found no installed package (`RollbackOutcome::not_installed`, + // sorted). These were NOT reverted — and "not installed" can also mean + // "installed but missed by the crawler" (layout gaps are a documented + // reality), leaving patched bytes on disk. The removal below still + // drops them from the manifest (the long-uninstalled contract), but + // their beforeHash blobs are kept out of the cleanup sweep and a + // warning event rides the envelope. Empty under `--skip-rollback` + // (no rollback ran, so nothing is known — semantics unchanged). + let mut rollback_not_installed: Vec = Vec::new(); if !args.skip_rollback { if !args.common.json && !args.common.silent { println!("Rolling back patch before removal..."); @@ -286,7 +296,8 @@ pub async fn run(args: RemoveArgs) -> i32 { ) .await { - Ok((success, results, _vendored_skipped)) => { + Ok((success, results, _vendored_skipped, not_installed)) => { + rollback_not_installed = not_installed; if !success { track_patch_remove_failed( "Rollback failed during patch removal", @@ -471,7 +482,7 @@ pub async fn run(args: RemoveArgs) -> i32 { remove_patch_from_manifest(&args.identifier, &manifest_path).await }; match removal { - Ok((removed, manifest)) => { + Ok((removed, updated_manifest)) => { if removed.is_empty() { emit_not_found( args.common.json, @@ -500,11 +511,71 @@ pub async fn run(args: RemoveArgs) -> i32 { } } + // FAIL-CLOSED (crawler-miss guard): dropped entries whose nested + // rollback was skipped as not-installed were never actually + // reverted, and the miss may be a crawler layout gap with the + // patched bytes still on disk. Sweeping their beforeHash blobs + // would permanently destroy the only local revert data, so they + // are pinned into the sweep's keep set; a warning event + stderr + // line surface each one. Entries genuinely rolled back (or + // already original) appear in `results`, never here. + let retained_not_installed: Vec<&str> = rollback_not_installed + .iter() + .map(String::as_str) + .filter(|p| removed.iter().any(|r| r == p)) + .collect(); + if !args.common.json && !args.common.silent && !retained_not_installed.is_empty() { + eprintln!( + "\nWarning: {} removed patch(es) had no matching installed package, so \ + their rollback was skipped (a crawler miss would look the same); their \ + revert data (beforeHash blobs) was kept in .socket/blobs:", + retained_not_installed.len() + ); + for purl in &retained_not_installed { + eprintln!(" - {purl}"); + } + } + // Clean up unused blobs (previewed, not deleted, on --dry-run). + // The reference manifest is the post-removal manifest PLUS one + // synthetic keep record per retained entry above: + // `cleanup_unused_blobs` keeps only afterHash blobs (beforeHash + // blobs are normally re-downloadable on demand), so each pinned + // before-hash is listed in an afterHash slot. Scoped to REVERT + // data only — the retained entries' real afterHash blobs stay + // sweepable like any other orphan. + let mut cleanup_reference = updated_manifest.clone(); + for purl in &retained_not_installed { + let Some(record) = manifest.patches.get(*purl) else { + continue; + }; + let pinned: std::collections::HashMap = record + .files + .iter() + .filter(|(_, info)| !info.before_hash.is_empty()) + .map(|(file, info)| { + ( + file.clone(), + PatchFileInfo { + before_hash: String::new(), + after_hash: info.before_hash.clone(), + }, + ) + }) + .collect(); + if pinned.is_empty() { + continue; // every file was created-by-patch: no revert blobs + } + let mut keep_record = record.clone(); + keep_record.files = pinned; + cleanup_reference + .patches + .insert((*purl).to_string(), keep_record); + } let blobs_path = socket_dir.join("blobs"); let mut blobs_removed = 0; if let Ok(cleanup_result) = - cleanup_unused_blobs(&manifest, &blobs_path, args.common.dry_run).await + cleanup_unused_blobs(&cleanup_reference, &blobs_path, args.common.dry_run).await { blobs_removed = cleanup_result.blobs_removed; if !args.common.json && !args.common.silent && cleanup_result.blobs_removed > 0 { @@ -527,8 +598,40 @@ pub async fn run(args: RemoveArgs) -> i32 { } else { PatchAction::Removed }; - // Chronological: the vendor revert ran before the rollback - // and the manifest mutation. Reverted events bypass + // The crawler-miss warnings first (the rollback skip is the + // earliest outcome chronologically). Recorded — they bump + // `summary.skipped` like the vendor retained/warning events + // — and additive: runs with every target genuinely rolled + // back (or already original) emit none, leaving existing + // consumers byte-identical output. + for purl in &retained_not_installed { + let mut kept: Vec = manifest + .patches + .get(*purl) + .map(|record| { + record + .files + .values() + .filter(|info| !info.before_hash.is_empty()) + .map(|info| info.before_hash.clone()) + .collect() + }) + .unwrap_or_default(); + kept.sort(); + kept.dedup(); + env.record( + PatchEvent::new(PatchAction::Skipped, (*purl).to_string()) + .with_reason( + "rollback_not_installed", + "rollback skipped: no installed package found (a crawler \ + miss would look the same); beforeHash blobs kept in \ + .socket/blobs so a later rollback/repair can still restore", + ) + .with_details(serde_json::json!({ "beforeBlobsRetained": kept })), + ); + } + // Chronological: the vendor revert ran before the manifest + // mutation. Reverted events bypass // `record` so `summary.removed` stays equal to the number // of manifest entries deleted (same rule as the blob-sweep // carrier below); retained/warning Skipped events bump diff --git a/crates/socket-patch-cli/src/commands/rollback.rs b/crates/socket-patch-cli/src/commands/rollback.rs index aabd42fa..a53283f1 100644 --- a/crates/socket-patch-cli/src/commands/rollback.rs +++ b/crates/socket-patch-cli/src/commands/rollback.rs @@ -1138,7 +1138,9 @@ async fn rollback_patches_inner( } // Export for use by remove command. The third tuple element lists -// vendor-owned purls that were excluded from in-place rollback (benign). +// vendor-owned purls that were excluded from in-place rollback (benign); +// the fourth is `RollbackOutcome::not_installed` — in-scope manifest +// entries the crawler found no installed package for. // // The returned `bool` is `RollbackOutcome::success` — per-package semantics // only. Manifest entries whose package is not installed are NOT failures @@ -1147,6 +1149,13 @@ async fn rollback_patches_inner( // "none matched → exit 1" rule deliberately does NOT apply to this // delegation (it would wedge `remove` for packages long uninstalled). // +// The `not_installed` element exists because that drop is IRREVERSIBLE in a +// way a genuine rollback is not: "not installed" can also mean "installed +// but missed by the crawler" (layout gaps are a documented reality), in +// which case the patched bytes are still on disk. `remove` uses the list to +// warn and to keep those entries' beforeHash blobs out of its cleanup +// sweep, so the revert data survives a crawler miss. +// // Takes the caller's `GlobalArgs` as the base (only the per-call fields are // overridden): the nested missing-blob download builds its API client from // `api_client_overrides()`, so flag-passed `--api-url` / `--api-token` / @@ -1162,7 +1171,7 @@ pub(crate) async fn rollback_patches( dry_run: bool, silent: bool, ecosystems: Option>, -) -> Result<(bool, Vec, Vec), String> { +) -> Result<(bool, Vec, Vec, Vec), String> { let args = RollbackArgs { identifier: identifier.map(String::from), common: crate::args::GlobalArgs { @@ -1175,7 +1184,12 @@ pub(crate) async fn rollback_patches( one_off: false, }; let outcome = rollback_patches_inner(&args, manifest_path, None).await?; - Ok((outcome.success, outcome.results, outcome.vendored_skipped)) + Ok(( + outcome.success, + outcome.results, + outcome.vendored_skipped, + outcome.not_installed, + )) } #[cfg(test)] @@ -1912,7 +1926,7 @@ mod tests { offline: true, ..crate::args::GlobalArgs::default() }; - let (success, results, _vendored) = rollback_patches( + let (success, results, _vendored, _not_installed) = rollback_patches( &common, &manifest_path, None, @@ -2002,7 +2016,7 @@ mod tests { offline: true, ..crate::args::GlobalArgs::default() }; - let (success, results, _vendored) = rollback_patches( + let (success, results, _vendored, _not_installed) = rollback_patches( &common, &manifest_path, None, @@ -2078,7 +2092,7 @@ mod tests { offline: true, ..crate::args::GlobalArgs::default() }; - let (success, results, _vendored_skipped) = rollback_patches( + let (success, results, _vendored_skipped, _not_installed) = rollback_patches( &common, &manifest_path, None, @@ -2159,7 +2173,7 @@ mod tests { offline: true, ..crate::args::GlobalArgs::default() }; - let (success, results, _vendored_skipped) = rollback_patches( + let (success, results, _vendored_skipped, _not_installed) = rollback_patches( &common, &manifest_path, None, @@ -2229,7 +2243,7 @@ mod tests { offline: true, ..crate::args::GlobalArgs::default() }; - let (success, results, vendored_skipped) = rollback_patches( + let (success, results, vendored_skipped, not_installed) = rollback_patches( &common, &manifest_path, None, @@ -2248,6 +2262,14 @@ mod tests { "nothing installed, nothing attempted, got {results:?}" ); assert!(vendored_skipped.is_empty()); + // The skip is not silent to the delegation: `remove` needs to know + // this entry was never actually reverted (a crawler miss looks the + // same) so it can keep the before-blobs and warn. + assert_eq!( + not_installed, + vec!["pkg:npm/foo@1.0.0".to_string()], + "the delegation must surface the not-installed entry" + ); } /// The needed-blob narrowing: an INSTALLED package whose file is already @@ -2290,7 +2312,7 @@ mod tests { offline: true, ..crate::args::GlobalArgs::default() }; - let (success, results, _vendored_skipped) = rollback_patches( + let (success, results, _vendored_skipped, not_installed) = rollback_patches( &common, &manifest_path, None, @@ -2304,6 +2326,10 @@ mod tests { success, "a blob nothing will read must not gate the run, got {results:?}" ); + assert!( + not_installed.is_empty(), + "an installed already-original package is not a crawler miss" + ); assert_eq!(results.len(), 1, "got {results:?}"); assert!(results[0].success); assert!( diff --git a/crates/socket-patch-cli/tests/remove_invariants.rs b/crates/socket-patch-cli/tests/remove_invariants.rs index b3c18251..d5d05079 100644 --- a/crates/socket-patch-cli/tests/remove_invariants.rs +++ b/crates/socket-patch-cli/tests/remove_invariants.rs @@ -694,6 +694,244 @@ fn remove_dry_run_previews_blob_sweep_without_deleting() { assert_eq!(manifest["patches"].as_object().unwrap().len(), 2); } +// --------------------------------------------------------------------------- +// Crawler-miss safety: a dropped entry whose rollback was skipped as +// not-installed must keep its beforeHash blobs (the only local revert data) +// --------------------------------------------------------------------------- + +/// beforeHash values distinct per entry (unlike TWO_PATCH_MANIFEST's shared +/// all-zeros), so the sweep assertions can attribute each blob to one entry. +const NI_BEFORE_A: &str = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const NI_AFTER_A: &str = "1111111111111111111111111111111111111111111111111111111111111111"; +const NI_BEFORE_B: &str = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; +const NI_AFTER_B: &str = "2222222222222222222222222222222222222222222222222222222222222222"; + +fn make_distinct_blob_socket_dir(root: &Path) -> PathBuf { + let manifest = format!( + r#"{{ + "patches": {{ + "pkg:npm/__remove_test_a__@1.0.0": {{ + "uuid": "11111111-1111-4111-8111-111111111111", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {{ + "package/a.js": {{ "beforeHash": "{NI_BEFORE_A}", "afterHash": "{NI_AFTER_A}" }} + }}, + "vulnerabilities": {{}}, + "description": "synthetic remove test patch A", + "license": "MIT", + "tier": "free" + }}, + "pkg:npm/__remove_test_b__@2.0.0": {{ + "uuid": "22222222-2222-4222-8222-222222222222", + "exportedAt": "2024-01-02T00:00:00Z", + "files": {{ + "package/b.js": {{ "beforeHash": "{NI_BEFORE_B}", "afterHash": "{NI_AFTER_B}" }} + }}, + "vulnerabilities": {{}}, + "description": "synthetic remove test patch B", + "license": "MIT", + "tier": "free" + }} + }} +}}"# + ); + let socket = root.join(".socket"); + std::fs::create_dir_all(&socket).expect("create .socket"); + std::fs::write(socket.join("manifest.json"), manifest).expect("write manifest"); + socket +} + +/// The purls of events carrying `errorCode: rollback_not_installed`. +fn not_installed_event_purls(v: &serde_json::Value) -> Vec { + v["events"] + .as_array() + .map(|events| { + events + .iter() + .filter(|e| e["errorCode"] == "rollback_not_installed") + .filter_map(|e| e["purl"].as_str().map(str::to_string)) + .collect() + }) + .unwrap_or_default() +} + +/// THE crawler-miss guard: entry A is in the manifest but NOT installed +/// (nothing under `node_modules/`), so the nested rollback skips it as +/// `not_installed` — nothing was actually reverted. A crawler layout gap +/// looks exactly the same, with the patched bytes still on disk. `remove` +/// still drops the manifest entry (the documented long-uninstalled +/// contract), but it must (a) surface a machine-visible warning event and +/// (b) keep A's beforeHash blob out of the sweep — destroying it would +/// permanently lose the only local revert data. +/// +/// `--offline` keeps this hermetic AND proves no download is needed: a +/// not-installed entry never enters the before-blob plan. +#[test] +fn remove_not_installed_keeps_before_blob_and_warns() { + let tmp = tempfile::tempdir().expect("tempdir"); + let socket = make_distinct_blob_socket_dir(tmp.path()); + let blobs = socket.join("blobs"); + std::fs::create_dir_all(&blobs).expect("create blobs dir"); + std::fs::write(blobs.join(NI_BEFORE_A), b"a-before").expect("stage A before blob"); + std::fs::write(blobs.join(NI_AFTER_A), b"a-after").expect("stage A after blob"); + std::fs::write(blobs.join(NI_AFTER_B), b"b-after").expect("stage B after blob"); + + let (code, stdout, _stderr) = common::run_with_env( + tmp.path(), + &[ + "remove", + "pkg:npm/__remove_test_a__@1.0.0", + "--json", + "--yes", + "--offline", + ], + &[], + ); + assert_eq!( + code, 0, + "removing a not-installed entry still succeeds; stdout=\n{stdout}" + ); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + + // The entry is dropped (long-uninstalled contract unchanged)... + let manifest = read_manifest(&socket); + let patches = manifest["patches"].as_object().expect("patches object"); + assert!(!patches.contains_key("pkg:npm/__remove_test_a__@1.0.0")); + assert!(patches.contains_key("pkg:npm/__remove_test_b__@2.0.0")); + + // (a) ...with a machine-visible warning naming the purl whose rollback + // was skipped as not-installed... + assert_eq!( + not_installed_event_purls(&v), + vec!["pkg:npm/__remove_test_a__@1.0.0"], + "expected a rollback_not_installed warning event; envelope={v}" + ); + + // (b) ...and A's beforeHash blob SURVIVES the sweep: it is the only + // local revert data for bytes that may still be patched on disk. + assert!( + blobs.join(NI_BEFORE_A).exists(), + "the not-installed entry's beforeHash blob must be excluded from \ + the cleanup sweep; envelope={v}" + ); + // The keep-set addition is scoped to REVERT data: A's afterHash blob is + // unreferenced patched bytes and is swept as before, and B's referenced + // afterHash blob survives as before. + assert!( + !blobs.join(NI_AFTER_A).exists(), + "A's orphaned afterHash blob is still swept" + ); + assert!( + blobs.join(NI_AFTER_B).exists(), + "B's referenced afterHash blob must remain" + ); +} + +/// Control for the guard above: an entry whose files are INSTALLED and +/// already at their original bytes really is reverted state — rollback +/// reports it `already_original`, so its beforeHash blob is swept exactly +/// as before and no `rollback_not_installed` warning fires. Guards the +/// fail-closed keep-set from degrading into keep-everything. +#[test] +fn remove_already_original_sweeps_before_blob_without_warning() { + let original = b"original bytes\n"; + let before_hash = common::git_sha256(original); + + let tmp = tempfile::tempdir().expect("tempdir"); + let socket = tmp.path().join(".socket"); + std::fs::create_dir_all(&socket).expect("create .socket"); + let manifest = format!( + r#"{{ + "patches": {{ + "pkg:npm/__remove_test_a__@1.0.0": {{ + "uuid": "11111111-1111-4111-8111-111111111111", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {{ + "package/a.js": {{ "beforeHash": "{before_hash}", "afterHash": "{NI_AFTER_A}" }} + }}, + "vulnerabilities": {{}}, + "description": "synthetic remove test patch A", + "license": "MIT", + "tier": "free" + }} + }} +}}"# + ); + std::fs::write(socket.join("manifest.json"), manifest).expect("write manifest"); + let blobs = socket.join("blobs"); + std::fs::create_dir_all(&blobs).expect("create blobs dir"); + std::fs::write(blobs.join(&before_hash), original).expect("stage before blob"); + + // Installed at the BEFORE bytes: rollback verifies already-original. + std::fs::write( + tmp.path().join("package.json"), + r#"{ "name": "remove-invariants-root", "version": "0.0.0" }"#, + ) + .expect("write root package.json"); + let pkg_dir = tmp.path().join("node_modules/__remove_test_a__"); + std::fs::create_dir_all(&pkg_dir).expect("create package dir"); + std::fs::write( + pkg_dir.join("package.json"), + r#"{ "name": "__remove_test_a__", "version": "1.0.0" }"#, + ) + .expect("write package.json"); + std::fs::write(pkg_dir.join("a.js"), original).expect("write a.js"); + + let (code, stdout, _stderr) = common::run_with_env( + tmp.path(), + &[ + "remove", + "pkg:npm/__remove_test_a__@1.0.0", + "--json", + "--yes", + "--offline", + ], + &[], + ); + assert_eq!(code, 0, "stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert!( + not_installed_event_purls(&v).is_empty(), + "an already-original entry is not a crawler miss — no warning; envelope={v}" + ); + assert!( + !blobs.join(&before_hash).exists(), + "an already-original entry's beforeHash blob is swept as before" + ); + assert!( + !read_manifest(&socket)["patches"] + .as_object() + .expect("patches object") + .contains_key("pkg:npm/__remove_test_a__@1.0.0"), + "the entry is removed" + ); +} + +/// `--skip-rollback` semantics unchanged: no rollback runs, so there is no +/// not-installed outcome to react to — the sweep and the (absent) warning +/// behave exactly as before the crawler-miss guard. +#[test] +fn remove_skip_rollback_sweep_semantics_unchanged() { + let tmp = tempfile::tempdir().expect("tempdir"); + let socket = make_distinct_blob_socket_dir(tmp.path()); + let blobs = socket.join("blobs"); + std::fs::create_dir_all(&blobs).expect("create blobs dir"); + std::fs::write(blobs.join(NI_BEFORE_A), b"a-before").expect("stage A before blob"); + + let (code, stdout) = run_remove(tmp.path(), "pkg:npm/__remove_test_a__@1.0.0", &[]); + assert_eq!(code, 0, "stdout=\n{stdout}"); + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert!( + not_installed_event_purls(&v).is_empty(), + "--skip-rollback runs no rollback, so no not-installed warning; envelope={v}" + ); + assert!( + !blobs.join(NI_BEFORE_A).exists(), + "--skip-rollback ('don't touch my tree') keeps the pre-guard sweep \ + behavior: the dropped entry's beforeHash blob is swept" + ); +} + /// The full-path preview (no --skip-rollback) must not create `.socket/blobs` /// either: rollback's preview previously `create_dir_all`'d it (and, online, /// downloaded before-blobs into it) — leaving new files a wet remove's sweep diff --git a/crates/socket-patch-cli/tests/remove_network.rs b/crates/socket-patch-cli/tests/remove_network.rs index 42f1d555..5c22d684 100644 --- a/crates/socket-patch-cli/tests/remove_network.rs +++ b/crates/socket-patch-cli/tests/remove_network.rs @@ -195,6 +195,71 @@ async fn remove_online_downloads_missing_before_blob_then_removes() { ); } +/// Crawler-miss guard, network side: with the package NOT installed the +/// nested rollback skips the entry as `not_installed` — nothing to restore, +/// so nothing to download. `remove` (online, mock armed) must (a) contact +/// the network for NOTHING, (b) still drop the manifest entry, (c) keep the +/// locally-cached beforeHash blob out of the cleanup sweep (it is the only +/// local revert data if the miss was a crawler layout gap rather than a +/// real uninstall), and (d) surface a machine-visible +/// `rollback_not_installed` warning event naming the purl. +#[tokio::test] +async fn remove_not_installed_keeps_blob_and_never_fetches() { + let before = b"before\n"; + let after = b"after\n"; + let before_hash = git_sha256(before); + let after_hash = git_sha256(after); + + let mock = MockServer::start().await; + mount_before_blob(&mock, before, &before_hash).await; + + let tmp = tempfile::tempdir().expect("tempdir"); + let socket = tmp.path().join(".socket"); + write_manifest(&socket, &before_hash, &after_hash); + // Deliberately NOT installed — no node_modules at all. + let blobs = socket.join("blobs"); + std::fs::create_dir_all(&blobs).expect("create blobs dir"); + std::fs::write(blobs.join(&before_hash), before).expect("stage before blob"); + + let (code, stdout) = run_remove(tmp.path(), &mock.uri(), &[]); + assert_eq!( + code, 0, + "removing a not-installed entry succeeds; stdout=\n{stdout}" + ); + assert!( + !manifest_has_entry(&socket), + "the manifest entry is dropped; stdout=\n{stdout}" + ); + + let v: serde_json::Value = serde_json::from_str(&stdout).expect("valid JSON"); + assert!( + v["events"] + .as_array() + .expect("events array") + .iter() + .any(|e| e["errorCode"] == "rollback_not_installed" && e["purl"] == PURL), + "expected a rollback_not_installed warning event; envelope={v}" + ); + assert!( + blobs.join(&before_hash).exists(), + "the not-installed entry's beforeHash blob must survive the sweep; \ + stdout=\n{stdout}" + ); + + // Nothing to restore → nothing to fetch: the armed mock saw no traffic. + let reqs = mock + .received_requests() + .await + .expect("wiremock request recording must be enabled"); + assert!( + reqs.is_empty(), + "a not-installed entry needs no blob download; observed requests={:?}", + reqs.iter() + .map(|r| (r.method.to_string(), r.url.path().to_string())) + .collect::>() + ); +} + /// `--offline` must NOT contact the network: with the beforeHash blob /// missing, rollback cannot proceed, so `remove --offline` aborts and /// leaves the manifest entry in place. The mock IS armed to serve the