From 24d479b805cdecc4c8b31bbf3947729f1e9f45f6 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 12 Aug 2026 16:40:21 -0700 Subject: [PATCH] fix(scan): keep hosted --json envelope schema-consistent across discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scan --json --mode hosted` emitted two incompatible top-level shapes: the classic scan object (scannedPackages/totalPatches/canAccessPaidPatches/ packages) on zero discovery, but a bare `{status, redirect}` envelope once >=1 package was found — dropping every scan-count key and the per-patch enumeration, breaking JSON consumers on the redirect path. Follow the vendored-mode precedent: always emit the classic scan object and NEST the redirect result under a `redirect` key, for both the zero-discovery and >=1-discovery paths. `run_redirect` now takes the classic scan object (`Option`) built in `run` and folds `redirect` (and error status) into it; the zero-package early-return gains a no-op `redirect` block. Human (non-JSON) output is unchanged. Kills sweep findings: hosted-scan-json-schema-flips-with-discovery, hosted-scan-json-omits-enumeration. Co-Authored-By: Claude Opus 4.8 --- .../src/commands/scan/hosted.rs | 156 ++++++++++++++---- .../socket-patch-cli/src/commands/scan/mod.rs | 46 +++++- 2 files changed, 167 insertions(+), 35 deletions(-) diff --git a/crates/socket-patch-cli/src/commands/scan/hosted.rs b/crates/socket-patch-cli/src/commands/scan/hosted.rs index 8f917fa4..b538154d 100644 --- a/crates/socket-patch-cli/src/commands/scan/hosted.rs +++ b/crates/socket-patch-cli/src/commands/scan/hosted.rs @@ -66,19 +66,39 @@ fn parse_purl_simple(purl: &str) -> Option<(String, String, String)> { } /// The hosted-mode JSON error envelope, for bail-outs that return before the -/// result envelope at the bottom of [`run_redirect`] is built. A `--json` -/// consumer must always get parseable stdout — `status`/`error` mirror the -/// success envelope's error fold — never empty output plus an exit code. -fn emit_json_error(message: &str) { - println!( - "{}", - serde_json::to_string_pretty(&serde_json::json!({ - "status": "error", - "error": message, - "redirect": { "mode": "hosted" }, - })) - .unwrap() - ); +/// success envelope at the bottom of [`run_redirect`] is built. When the +/// classic scan object (`scan_result`, threaded in from `run`) is present it +/// is reused so the error envelope carries the SAME top-level scan keys as +/// the success path — folding in `status`/`error` and a minimal `redirect` +/// block — instead of a bare shape that flips the schema. When absent (never +/// in JSON mode today) the bare envelope is emitted. A `--json` consumer must +/// always get parseable stdout — never empty output plus an exit code. +fn emit_json_error(scan_result: Option, message: &str) { + let mut result = scan_result.unwrap_or_else(|| serde_json::json!({ "status": "error" })); + result["status"] = serde_json::json!("error"); + result["error"] = serde_json::json!(message); + if !result.get("redirect").is_some_and(|r| r.is_object()) { + result["redirect"] = serde_json::json!({ "mode": "hosted" }); + } + println!("{}", serde_json::to_string_pretty(&result).unwrap()); +} + +/// Build the hosted `--json` success envelope: the classic scan object +/// (`scan_result`, built by `run` — scannedPackages / totalPatches / +/// canAccessPaidPatches plus the `packages` enumeration) with the redirect +/// summary NESTED under `redirect`, mirroring vendored mode's nested `vendor` +/// block. Extracted so the schema (classic scan keys + nested `redirect`) is +/// unit-testable without a live API. When `scan_result` is absent (never in +/// JSON mode today) a minimal `{status:"success"}` base is used so stdout is +/// still parseable. +fn build_redirect_json_envelope( + scan_result: Option, + redirect: serde_json::Value, +) -> serde_json::Value { + let mut result = scan_result.unwrap_or_else(|| serde_json::json!({ "status": "success" })); + result["status"] = serde_json::json!("success"); + result["redirect"] = redirect; + result } /// `scan --redirect`: resolve hosted-patch references for the selected patches, @@ -91,6 +111,11 @@ pub(super) async fn run_redirect( effective_org_slug: Option<&str>, all_packages_with_patches: &[BatchPackagePatches], can_access_paid_patches: bool, + // The classic scan object `run` builds for the `--json` path (`Some` in + // JSON mode, `None` for human output). The redirect result is NESTED into + // it so the hosted `--json` envelope stays schema-consistent with every + // other scan; `.take()` at each terminal (error or success) folds it in. + mut scan_result: Option, ) -> i32 { use socket_patch_core::manifest::schema::PatchRecord; use socket_patch_core::patch::redirect::{ @@ -114,7 +139,7 @@ pub(super) async fn run_redirect( // stdout is never empty on failure. Err((code, message)) => { if args.common.json { - emit_json_error(&message); + emit_json_error(scan_result.take(), &message); } return code; } @@ -138,7 +163,7 @@ pub(super) async fn run_redirect( let message = format!("failed to resolve patch references: {e}"); eprintln!("{message}"); if args.common.json { - emit_json_error(&message); + emit_json_error(scan_result.take(), &message); } return 1; } @@ -419,7 +444,7 @@ pub(super) async fn run_redirect( let message = format!("failed to write {rel}: {e}"); eprintln!("{message}"); if args.common.json { - emit_json_error(&message); + emit_json_error(scan_result.take(), &message); } return 1; } @@ -459,7 +484,7 @@ pub(super) async fn run_redirect( let message = format!("failed to write .socket/vendor/redirect-state.json: {e}"); eprintln!("{message}"); if args.common.json { - emit_json_error(&message); + emit_json_error(scan_result.take(), &message); } return 1; } @@ -506,20 +531,25 @@ pub(super) async fn run_redirect( warnings.extend(record_warnings.iter().cloned()); warnings.extend(migration_warnings.iter().cloned()); warnings.extend(rush_warnings.iter().cloned()); - let mut result = serde_json::json!({ - "status": "success", - "redirect": { - // Final mode naming: `--redirect` IS hosted mode. Additive - // key so JSON consumers can dispatch on the mode without - // inferring it from which sub-object is present. - "mode": "hosted", - "redirected": confirmed.len(), - "rewrittenFiles": rewritten, - "skipped": skipped, - "warnings": warnings, - "dryRun": args.common.dry_run, - } + // Nest the redirect result under `redirect` inside the classic scan + // object (built by `run`, threaded in via `scan_result`), mirroring + // vendored mode's nested `vendor` block. This keeps the hosted `--json` + // envelope schema-consistent with the zero-discovery and non-hosted + // scan envelopes — same top-level scan keys (scannedPackages, + // totalPatches, canAccessPaidPatches) plus the `packages` enumeration — + // instead of the bare `{status, redirect}` it used to emit. + let redirect = serde_json::json!({ + // Final mode naming: `--redirect` IS hosted mode. Additive key so + // JSON consumers can dispatch on the mode without inferring it from + // which sub-object is present. + "mode": "hosted", + "redirected": confirmed.len(), + "rewrittenFiles": rewritten, + "skipped": skipped, + "warnings": warnings, + "dryRun": args.common.dry_run, }); + let mut result = build_redirect_json_envelope(scan_result.take(), redirect); if let Some(statements) = vex_statements { result["vex"] = serde_json::json!({ "path": args.vex.vex.as_ref().unwrap().display().to_string(), @@ -592,9 +622,73 @@ pub(super) async fn run_redirect( #[cfg(test)] mod tests { - use super::REDIRECT_CANDIDATE_FILES; + use super::{build_redirect_json_envelope, REDIRECT_CANDIDATE_FILES}; use socket_patch_core::constants::npm_family; + /// The classic scan object `run` builds for the `--json` path with ≥1 + /// discovered package (scannedPackages/totalPatches/… + the `packages` + /// enumeration). Mirrors the `serde_json::json!` in `scan::run`. + fn classic_scan_result() -> serde_json::Value { + serde_json::json!({ + "status": "success", + "scannedPackages": 3, + "lockfileOnlyPackages": 0, + "packagesWithPatches": 1, + "totalPatches": 2, + "freePatches": 2, + "paidPatches": 0, + "canAccessPaidPatches": false, + "packages": [ + { "purl": "pkg:npm/minimist@1.2.2", "patches": [ { "uuid": "abc-123" } ] } + ], + "updates": [], + }) + } + + #[test] + fn hosted_json_envelope_nests_redirect_into_classic_scan_object() { + // Regression for hosted-scan-json-schema-flips-with-discovery / + // hosted-scan-json-omits-enumeration: with ≥1 package, the hosted + // `--json` envelope must carry the SAME top-level scan keys as a + // zero-discovery / non-hosted scan (the old bare `{status, redirect}` + // dropped them) AND nest the redirect summary under `redirect`. + let redirect = serde_json::json!({ + "mode": "hosted", + "redirected": 1, + "rewrittenFiles": ["package-lock.json"], + "skipped": [], + "warnings": [], + "dryRun": false, + }); + let envelope = build_redirect_json_envelope(Some(classic_scan_result()), redirect); + + // Classic scan keys survive — the bug was that they did not. + assert_eq!(envelope["status"], "success"); + assert_eq!(envelope["scannedPackages"], 3); + assert_eq!(envelope["packagesWithPatches"], 1); + assert_eq!(envelope["totalPatches"], 2); + assert_eq!(envelope["freePatches"], 2); + assert_eq!(envelope["paidPatches"], 0); + assert_eq!(envelope["canAccessPaidPatches"], false); + assert!(envelope["updates"].is_array()); + + // Per-package / patch-uuid enumeration is present (the omission). + assert!(envelope["packages"].is_array()); + assert_eq!(envelope["packages"][0]["purl"], "pkg:npm/minimist@1.2.2"); + assert_eq!(envelope["packages"][0]["patches"][0]["uuid"], "abc-123"); + + // Redirect result is NESTED, preserving every sub-field, not replacing + // the whole envelope. + let r = &envelope["redirect"]; + assert!(r.is_object()); + assert_eq!(r["mode"], "hosted"); + assert_eq!(r["redirected"], 1); + assert_eq!(r["rewrittenFiles"][0], "package-lock.json"); + assert!(r["skipped"].is_array()); + assert!(r["warnings"].is_array()); + assert_eq!(r["dryRun"], false); + } + #[test] fn redirect_candidates_match_the_shared_npm_family_table() { // Drift guard, both directions, without classifying the non-npm diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index 40da341e..8990729e 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -619,6 +619,19 @@ pub async fn run(mut args: ScanArgs) -> i32 { "packages": [], "updates": [], }); + // Hosted mode: keep the `--json` envelope schema-consistent with + // the ≥1-package path by including a (no-op) nested `redirect` + // block — nothing was discovered, so nothing is redirected. + if hosted { + result["redirect"] = serde_json::json!({ + "mode": "hosted", + "redirected": 0, + "rewrittenFiles": [], + "skipped": [], + "warnings": [], + "dryRun": args.common.dry_run, + }); + } let code = embed_vex_into_json(&args.common, &args.vex, &manifest_path, 0, &mut result).await; println!("{}", serde_json::to_string_pretty(&result).unwrap()); @@ -848,16 +861,23 @@ pub async fn run(mut args: ScanArgs) -> i32 { ) .await; - // Registry-redirect mode is a distinct, self-contained flow (rewrite - // lockfiles → hosted vendored patches). It reuses discovery above, then - // returns — it must NOT fall through to the apply/vendor branches. - if hosted { + // Registry-redirect (hosted) mode is a distinct, self-contained flow + // (rewrite lockfiles → hosted vendored patches). It reuses discovery + // above, then returns — it must NOT fall through to the apply/vendor + // branches. The HUMAN path returns here; the `--json` path returns from + // inside the JSON block below (after building the classic scan object) + // so the redirect result can be NESTED under a `redirect` key — keeping + // the hosted `--json` envelope schema-consistent with the zero-discovery + // and non-hosted paths (mirroring vendored mode's nested `vendor` block) + // rather than replacing the whole envelope with a bare `{status, redirect}`. + if hosted && !args.common.json { return run_redirect( &args, &api_client, effective_org_slug, &all_packages_with_patches, can_access_paid_patches, + None, ) .await; } @@ -904,6 +924,24 @@ pub async fn run(mut args: ScanArgs) -> i32 { } } + // Hosted mode: NEST the redirect result under `redirect` in the classic + // scan object just built above (mirrors vendored mode's nested `vendor` + // block), so the hosted `--json` envelope carries the same top-level + // scan keys and `packages` enumeration as every other scan plus the + // redirect summary. Returns before the apply/vendor/prune branches, + // which are mutually exclusive with hosted mode. + if hosted { + return run_redirect( + &args, + &api_client, + effective_org_slug, + &all_packages_with_patches, + can_access_paid_patches, + Some(result), + ) + .await; + } + // `apply` and `prune` are computed once at the top of run() // (factoring in --sync, which implies both). They're independent // here: a bot can `--apply` without `--prune`, or `--prune`