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 12acfeb4..a53283f1 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, 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; @@ -51,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 @@ -230,6 +255,99 @@ 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 +/// 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(), + // 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()) + .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,8 +417,20 @@ pub async fn run(args: RollbackArgs) -> i32 { Err(code) => return code, }; - match rollback_patches_inner(&args, &manifest_path).await { - Ok((success, results, vendored)) => { + match rollback_patches_inner(&args, &manifest_path, Some(&telemetry_client)).await { + 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()) @@ -329,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") ); @@ -421,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, @@ -473,7 +625,13 @@ pub async fn run(args: RollbackArgs) -> i32 { async fn rollback_patches_inner( args: &RollbackArgs, manifest_path: &Path, -) -> Result<(bool, Vec, Vec), String> { + // 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 { let manifest = read_manifest(manifest_path) .await .map_err(|e| e.to_string())? @@ -505,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 @@ -526,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 @@ -644,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, @@ -671,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 @@ -706,12 +893,54 @@ 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"): 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,36 +948,99 @@ 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( + &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(RollbackOutcome { + success: false, + results, + vendored_skipped, + not_installed, + }); } 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)); } - // 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. + // 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( + &abort_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(RollbackOutcome { + success: false, + results, + vendored_skipped, + not_installed, + }); } } @@ -756,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 @@ -829,11 +1129,32 @@ 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). +// 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 +// 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). +// +// 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 @@ -850,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 { @@ -862,7 +1183,13 @@ pub(crate) async fn rollback_patches( }, one_off: false, }; - rollback_patches_inner(&args, manifest_path).await + let outcome = rollback_patches_inner(&args, manifest_path, None).await?; + Ok(( + outcome.success, + outcome.results, + outcome.vendored_skipped, + outcome.not_installed, + )) } #[cfg(test)] @@ -1154,6 +1481,100 @@ 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")); + } + + /// 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(); + 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 @@ -1505,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, @@ -1595,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, @@ -1671,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, @@ -1689,9 +2110,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(); @@ -1700,6 +2148,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(), @@ -1720,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, @@ -1730,10 +2183,158 @@ 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].package_path.is_empty(), + "a gated package is installed, so its path must be reported, got {results:?}" + ); + 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:?}" + ); + } + + /// 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, not_installed) = 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 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 + /// 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, not_installed) = 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!( + 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!( + 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 726fba49..2aff4f1c 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; @@ -77,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( @@ -144,8 +169,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/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}" + ); + } } // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-cli/tests/remove_invariants.rs b/crates/socket-patch-cli/tests/remove_invariants.rs index c23dfa58..d5d05079 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(), @@ -659,16 +694,260 @@ 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 /// 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_network.rs b/crates/socket-patch-cli/tests/remove_network.rs index dc300b45..5c22d684 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}"); @@ -172,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 @@ -190,6 +278,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!( 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 3f4cf1b8..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) @@ -253,13 +275,27 @@ 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. + // 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, @@ -270,31 +306,243 @@ 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!( + !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!( - 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()); + install_manifest_package(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()); + install_manifest_package(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}" ); } // --------------------------------------------------------------------------- -// 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"; @@ -302,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}" + ); } // --------------------------------------------------------------------------- @@ -345,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!( @@ -699,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}" + ); } 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; }