From 7c2eec75c7a470229beab36736a2de29b362ba8c Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 16:32:59 +0000 Subject: [PATCH 1/3] Add human/JSON scan parity tests for empty results A scan whose detail queries all succeed with no records exits 1 in human output but 0 with --json, and a human --dry-run --prune never previews the GC the --json arm previews. Pin both forks (#1062). Also applies rustfmt to a test in redirect/upstream that main left unformatted. Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/covgap_commands_scan_mod.rs | 126 ++++++++++++++++++ .../src/patch/redirect/upstream/mod.rs | 5 +- 2 files changed, 130 insertions(+), 1 deletion(-) 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..702e3cc5a 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,129 @@ 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 the `--json` arm +/// previews (`gc` block), instead of skipping it. +#[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"; + for mode in [&["--mode", "agent"][..], &["--prune"][..]] { + 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!( + stdout.contains("[dry-run] GC would prune 1 manifest entry"), + "{extra:?}: the human dry run previews the GC; stdout={stdout}" + ); + } + assert_eq!( + std::fs::read(&manifest).unwrap(), + before, + "{extra:?}: a dry run writes nothing" + ); + } + } +} 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}"); From 5af7a1e3a7a0cfaef58abec22e28ffb617eff943 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 16:43:44 +0000 Subject: [PATCH 2/3] Make scan exit alike in human and JSON output A scan whose detail queries all succeeded but returned no records exited 1 in human output ("could not fetch patch details") and 0 with --json, so CI and a developer saw different results for the same API state. Human --dry-run --prune also never previewed the GC that the --json arm reports, and hosted --json passed the raw --prune flag where the human arm used the policy-gated one. Only a real fetch failure (every query errored) now fails either arm; the human dry run previews the GC; hosted mode gets the gated prune in both arms (#1062). Assisted-by: Claude Code:claude-opus-5-5 --- .../src/commands/scan/hosted.rs | 5 ++- .../socket-patch-cli/src/commands/scan/mod.rs | 40 ++++++++++--------- .../tests/covgap_commands_scan_mod.rs | 20 +++++++--- .../socket-patch-cli/tests/in_process_scan.rs | 11 +++-- 4 files changed, 47 insertions(+), 29 deletions(-) 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..b08427794 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)>, } @@ -2544,6 +2540,7 @@ async fn run_scan( batch_error_count > 0, &mut stage, prior_discovery, + prune, ) .await; } @@ -2843,10 +2840,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 +2862,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, @@ -3228,6 +3219,19 @@ async fn run_scan( } } print_rollout_human(&stage, true, silent); + // `finish_human` previews the agent / report-only GC; vendored + // runs its own GC, so its dry run previews it here, as the + // `--json` arm reports it in `gc`. + if prune && vendor { + gc::run_human_gc( + &args.common, + &manifest_path, + &socket_dir, + &scanned_purls, + &vendored_purls, + ) + .await; + } return finish_human(0).await; } 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 702e3cc5a..c5a73dd35 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs @@ -2852,13 +2852,18 @@ async fn scan_empty_detail_results_exit_alike_in_human_and_json() { } } -/// #1062: a human `--dry-run --prune` previews the GC the `--json` arm -/// previews (`gc` block), instead of skipping it. +/// #1062: a human `--dry-run --prune` previews the GC (once) in every +/// mode the `--json` arm previews it (`gc` block); vendored skipped it. #[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"; - for mode in [&["--mode", "agent"][..], &["--prune"][..]] { + // Agent, vendored, and report-only (no mode; `--prune` below). + for mode in [ + &["--mode", "agent"][..], + &["--mode", "vendored"][..], + &[][..], + ] { let mock = MockServer::start().await; mount_batch_one(&mock, purl, UUID, "free", &[], false).await; mount_by_package(&mock, purl, UUID, serde_json::json!({})).await; @@ -2887,9 +2892,12 @@ async fn scan_dry_run_prune_previews_gc_in_human_and_json() { "{extra:?}: {v}" ); } else { - assert!( - stdout.contains("[dry-run] GC would prune 1 manifest entry"), - "{extra:?}: the human dry run previews the GC; stdout={stdout}" + 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!( 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). From 42853e30ea1c13593d3e00a8f0ced5e3eab5857e Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 17:10:36 +0000 Subject: [PATCH 3/3] Warn on partial detail failures with no records When some detail queries failed and the rest returned no records, the human scan printed nothing about the failures (the warnings were gated on a non-empty result, a case that used to end in the removed fetch error) while --json still reported patch_details_failed. Warn whenever at least one query succeeded. Vendored mode's human --prune GC on early exits is left to #1127 (PR #1338), which fixes it in finish_human for every early exit. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/src/commands/scan/mod.rs | 21 ++--- .../tests/covgap_commands_scan_mod.rs | 78 +++++++++++++++++-- 2 files changed, 75 insertions(+), 24 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index b08427794..b02e9cfdd 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -668,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], @@ -711,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}"); } @@ -3219,19 +3221,6 @@ async fn run_scan( } } print_rollout_human(&stage, true, silent); - // `finish_human` previews the agent / report-only GC; vendored - // runs its own GC, so its dry run previews it here, as the - // `--json` arm reports it in `gc`. - if prune && vendor { - gc::run_human_gc( - &args.common, - &manifest_path, - &socket_dir, - &scanned_purls, - &vendored_purls, - ) - .await; - } return finish_human(0).await; } 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 c5a73dd35..545a778e8 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs @@ -2852,18 +2852,15 @@ async fn scan_empty_detail_results_exit_alike_in_human_and_json() { } } -/// #1062: a human `--dry-run --prune` previews the GC (once) in every -/// mode the `--json` arm previews it (`gc` block); vendored skipped it. +/// #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, vendored, and report-only (no mode; `--prune` below). - for mode in [ - &["--mode", "agent"][..], - &["--mode", "vendored"][..], - &[][..], - ] { + // 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; @@ -2908,3 +2905,68 @@ async fn scan_dry_run_prune_previews_gc_in_human_and_json() { } } } + +/// #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"); +}