Skip to content

fix(scan): keep hosted --json envelope schema-consistent across discovery #163

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Mikola Lysenko (mikolalysenko) wants to merge 1 commit into
base: main
Choose a base branch
from
+167 −35
Open

fix(scan): keep hosted --json envelope schema-consistent across discovery #163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 125 additions & 31 deletions crates/socket-patch-cli/src/commands/scan/hosted.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<serde_json::Value>, 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<serde_json::Value>,
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,
Expand All @@ -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<serde_json::Value>,
) -> i32 {
use socket_patch_core::manifest::schema::PatchRecord;
use socket_patch_core::patch::redirect::{
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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
Expand Down
46 changes: 42 additions & 4 deletions crates/socket-patch-cli/src/commands/scan/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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`
Expand Down
Loading