diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index eec27aed1..9bbe8c1c1 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -554,6 +554,9 @@ pub(super) async fn run_redirect( stage: &mut super::rollout::Stage, // Scan's pre-redirect lockfile discovery (see `rollout::Gate::prior`). prior: Option>, + // `--prune` / `--sync`, gated by the policy exactly as the human arm + // gates it (`patches.enabled: false` writes nothing, the GC included). + prune: bool, ) -> i32 { // Same discovery/selection as agent and vendored mode. let discovered = match discover_selected( @@ -606,7 +609,7 @@ pub(super) async fn run_redirect( run_redirect_selected( &args.common, &args.vex, - args.prune || args.sync, + prune, api_client, &pairs, scan_result, diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index a3d5b08e6..b02e9cfdd 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -489,7 +489,6 @@ async fn discover_selected( // Some queries failed, some succeeded: a `--json` run has no stderr // warning (`warn` is human-only), so each failed package becomes a // run-level `warnings[]` entry — never a silent drop from the envelope. - let fetched = all_search_results.len(); let offers = select_accessible(all_search_results, can_access_paid_patches, policy); if let Some(result) = json_warnings { for (purl, e) in &failures { @@ -503,18 +502,15 @@ async fn discover_selected( } Ok(Discovered { offers, - fetched, failed: failures, }) } -/// [`discover_selected`]'s result: the offers, how many records came back -/// (before the tier filter), and each failed detail query as `(purl, -/// error)` (a failure for a package with no recorded patch makes a capped -/// run's data incomplete). +/// [`discover_selected`]'s result: the offers and each failed detail +/// query as `(purl, error)` (a failure for a package with no recorded +/// patch makes a capped run's data incomplete). struct Discovered { offers: rollout::Offers, - fetched: usize, failed: Vec<(String, String)>, } @@ -672,8 +668,9 @@ fn open_paragraph(opened: &mut bool) { /// arm treats an empty merged set as a fetch failure). The two output /// knobs are human-only: `show_progress` shows the status-line counter on /// stderr, `warn` prints a warning per failed package once the loop is -/// done — only when some query succeeded (when every one failed, the -/// caller's error line carries the cause instead, so nothing repeats). +/// done — only when some query succeeded, even with no records (when +/// every one failed, the caller's error line carries the cause instead, +/// so nothing repeats). async fn fetch_patch_details( api_client: &socket_patch_core::api::client::ApiClient, packages: &[BatchPackagePatches], @@ -715,7 +712,8 @@ async fn fetch_patch_details( } } status.finish(); - if warn && !results.is_empty() { + // Not when every query failed: the caller's error line names it. + if warn && failures.len() < packages.len() { for (purl, e) in &failures { eprintln!("Warning: could not fetch details for {purl}: {e}"); } @@ -2544,6 +2542,7 @@ async fn run_scan( batch_error_count > 0, &mut stage, prior_discovery, + prune, ) .await; } @@ -2843,10 +2842,11 @@ async fn run_scan( // The by-package records every arm selects from, fetched before the // table so its `[UPDATE]` markers are the same UPGRADE rows the - // selection acts on (§5.1). Discovery said these packages HAVE - // patches, so an empty merged set is a fetch failure. - // A failed discovery still prints the table first; its exit code is - // returned below it. + // selection acts on (§5.1). Only `discover_selected`'s own `Err` + // (every query failed) is a fetch failure: queries that succeed with + // no records leave nothing to select, in every arm and in `--json` + // alike (#1062). A failed discovery still prints the table first; its + // exit code is returned below it. let mut discovery_failure: Option = None; let rows: Vec = if downloadable_count == 0 { Vec::new() @@ -2864,13 +2864,6 @@ async fn run_scan( ) .await { - // The agent / vendored / report-only arms need records to show: - // an empty merged set is a fetch failure there. - Ok(discovered) if !hosted && discovered.fetched == 0 => { - eprintln!("{}", render::fetch_details_failed(&discovered.failed)); - discovery_failure = Some(1); - Vec::new() - } Ok(discovered) => { let rows = classified_rows( &mut stage, diff --git a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs index a598a3577..545a778e8 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs @@ -2774,3 +2774,199 @@ async fn scan_agent_json_mismatch_overwrite_reaches_the_apply_block() { b"after\n" ); } + +// --------------------------------------------------------------------------- +// #1062: human / JSON parity when every detail query succeeds but is empty +// --------------------------------------------------------------------------- + +/// Mount a by-package response that succeeds with NO patch records (the +/// batch said the package has one; the detail query found none to offer). +async fn mount_by_package_empty(mock: &MockServer, purl: &str) { + Mock::given(method("GET")) + .and(path(format!( + "/v0/orgs/{ORG_SLUG}/patches/by-package/{}", + encode_purl(purl) + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "patches": [], + "canAccessPaidPatches": false, + }))) + .mount(mock) + .await; +} + +/// #1062: when every detail query succeeds but returns no records, scan +/// has nothing to select. That is a successful run with no applicable +/// patches in every mode, so the human arm must exit like the `--json` +/// arm (0), not report a fetch failure and exit 1. +#[tokio::test] +async fn scan_empty_detail_results_exit_alike_in_human_and_json() { + let purl = "pkg:npm/minimist@1.2.2"; + // `--prune` alone is report-only (no mode); `--dry-run` keeps every + // mode read-only. + let modes: [&[&str]; 4] = [ + &["--mode", "agent"], + &["--mode", "vendored"], + &["--mode", "hosted"], + &["--prune"], + ]; + for mode in modes { + for dry in [false, true] { + let mock = MockServer::start().await; + mount_batch_one(&mock, purl, UUID, "free", &[], false).await; + mount_by_package_empty(&mock, purl).await; + + let mut codes = Vec::new(); + for json in [false, true] { + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2", b"x\n"); + let mut extra: Vec<&str> = mode.to_vec(); + if dry { + extra.push("--dry-run"); + } + if json { + extra.push("--json"); + } + let (code, stdout, stderr) = run_scan_human(tmp.path(), &mock.uri(), &extra); + assert!( + !stderr.contains("could not fetch patch details"), + "{extra:?}: no query failed, so no fetch failure; stderr={stderr}" + ); + if json { + let v: serde_json::Value = + serde_json::from_str(stdout.trim()).expect("valid JSON"); + assert_ne!(v["status"], "error", "{extra:?}: {v}"); + } + codes.push((code, stdout, stderr)); + } + let (human, json) = (&codes[0], &codes[1]); + assert_eq!( + human.0, json.0, + "{mode:?} dry={dry}: human and --json exit alike\n\ + human stdout={}\nhuman stderr={}\njson stdout={}\njson stderr={}", + human.1, human.2, json.1, json.2 + ); + assert_eq!(human.0, 0, "{mode:?} dry={dry}: nothing to do is a success"); + } + } +} + +/// #1062: a human `--dry-run --prune` previews the GC (once) where the +/// `--json` arm previews it (`gc` block). +#[tokio::test] +async fn scan_dry_run_prune_previews_gc_in_human_and_json() { + let purl = "pkg:npm/minimist@1.2.2"; + let stale = "pkg:npm/left-pad@1.3.0"; + // Agent and report-only (no mode; `--prune` below). Vendored mode's + // human GC on early exits is #1127. + for mode in [&["--mode", "agent"][..], &[][..]] { + let mock = MockServer::start().await; + mount_batch_one(&mock, purl, UUID, "free", &[], false).await; + mount_by_package(&mock, purl, UUID, serde_json::json!({})).await; + for json in [false, true] { + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2", b"x\n"); + // A recorded patch for a package that is not installed: the + // GC would prune its entry. + seed_manifest(tmp.path(), &[(stale, OLD_UUID)]); + let manifest = tmp.path().join(".socket/manifest.json"); + let before = std::fs::read(&manifest).unwrap(); + let mut extra: Vec<&str> = mode.to_vec(); + extra.extend(["--prune", "--dry-run", "--yes"]); + if json { + extra.push("--json"); + } + let (code, stdout, stderr) = run_scan_human(tmp.path(), &mock.uri(), &extra); + assert_eq!(code, 0, "{extra:?}: stdout={stdout}; stderr={stderr}"); + if json { + let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON"); + assert!( + v["gc"]["prunableManifestEntries"] + .as_array() + .is_some_and(|a| a.iter().any(|p| p == stale)), + "{extra:?}: {v}" + ); + } else { + assert_eq!( + stdout + .matches("[dry-run] GC would prune 1 manifest entry") + .count(), + 1, + "{extra:?}: the human dry run previews the GC once; stdout={stdout}" + ); + } + assert_eq!( + std::fs::read(&manifest).unwrap(), + before, + "{extra:?}: a dry run writes nothing" + ); + } + } +} + +/// #1062: when some detail queries fail and the rest succeed with no +/// records, the human run still warns per failed package (the `--json` +/// arm adds a `patch_details_failed` warning each) and exits like it. +#[tokio::test] +async fn scan_partial_failure_with_empty_results_still_warns() { + let empty = "pkg:npm/minimist@1.2.2"; + let bad = "pkg:npm/lodash@4.17.20"; + let mock = MockServer::start().await; + let patch = |purl: &str| { + serde_json::json!({ + "uuid": UUID, "purl": purl, "tier": "free", "cveIds": [], + "ghsaIds": [], "severity": "high", "title": "t" + }) + }; + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG_SLUG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "packages": [ + {"purl": empty, "patches": [patch(empty)]}, + {"purl": bad, "patches": [patch(bad)]}, + ], + "canAccessPaidPatches": false, + }))) + .mount(&mock) + .await; + mount_by_package_empty(&mock, empty).await; + Mock::given(method("GET")) + .and(path(format!( + "/v0/orgs/{ORG_SLUG}/patches/by-package/{}", + encode_purl(bad) + ))) + .respond_with(ResponseTemplate::new(500)) + .mount(&mock) + .await; + + let mut codes = Vec::new(); + for json in [false, true] { + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2", b"x\n"); + write_npm_package(tmp.path(), "lodash", "4.17.20", b"x\n"); + let mut extra = vec!["--mode", "agent", "--dry-run"]; + if json { + extra.push("--json"); + } + let (code, stdout, stderr) = run_scan_human(tmp.path(), &mock.uri(), &extra); + if json { + let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON"); + assert!( + v["warnings"] + .as_array() + .is_some_and(|w| w.iter().any(|w| w["code"] == "patch_details_failed")), + "{v}" + ); + } else { + assert!( + stderr.contains(&format!("Warning: could not fetch details for {bad}")), + "the failed package is named; stderr={stderr}" + ); + } + codes.push(code); + } + assert_eq!(codes[0], codes[1], "human and --json exit alike"); +} diff --git a/crates/socket-patch-cli/tests/in_process_scan.rs b/crates/socket-patch-cli/tests/in_process_scan.rs index 2089818f3..52bca9c6a 100644 --- a/crates/socket-patch-cli/tests/in_process_scan.rs +++ b/crates/socket-patch-cli/tests/in_process_scan.rs @@ -930,10 +930,13 @@ async fn scan_non_json_with_patches_prints_table() { let code = run_scrubbed(args).await; // Non-JSON path: discovery → batch query → render table → fetch - // per-package details. We only mount the batch mock, so detail-fetch - // 404s and scan exits 1 ("Error: could not fetch patch details"). That exit is - // deterministic given these mocks. - assert_eq!(code, 1, "missing detail mock → detail fetch fails → exit 1"); + // per-package details. We only mount the batch mock, so the detail + // query 404s, which the client reads as "no records": nothing to + // select, a clean exit 0, exactly like the `--json` arm (#1062). + assert_eq!( + code, 0, + "an empty detail result is no patch to apply, not a failure" + ); // Prove the table-rendering path actually ran against real discovered // data: the batch endpoint was queried with the package, and the path // proceeded to the per-package detail fetch (i.e. it had a row to print). diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/mod.rs b/crates/socket-patch-core/src/patch/redirect/upstream/mod.rs index 944435d03..8c2a0d793 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/mod.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/mod.rs @@ -917,7 +917,10 @@ mod tests { fn bun_lock_remedies_name_the_forced_reinstall() { for file in ["bun.lockb", "bun.lock", "packages/app/bun.lockb"] { let remedy = checkout_remedy(&[file.to_string()]); - assert!(remedy.contains(&format!("`git checkout -- {file}`")), "{remedy}"); + assert!( + remedy.contains(&format!("`git checkout -- {file}`")), + "{remedy}" + ); assert!(remedy.ends_with( ", then run `bun install --force` (a plain `bun install` keeps the patched copy)" ), "{remedy}");