Make every --json top-level error a {code, message} object (#704)

crates/socket-patch-cli/src/commands/agent_download.rs

@@ -175,22 +175,24 @@ pub(crate) fn merge_metadata(record: &mut serde_json::Value, meta: serde_json::V
     }
 }
 
-/// Report an error to the caller: a `{status, error}` envelope on
-/// stdout when `json` is true, otherwise a plain `Error: ...` on stderr.
-pub(crate) fn report_error(json: bool, message: impl std::fmt::Display) {
+/// Report an error to the caller: a `{status: "error", error: {code,
+/// message}}` object on stdout when `json` is true, otherwise a plain
+/// `Error: ...` on stderr. Every top-level `get` failure goes through here
+/// (or [`report_lock_failure`]) so the error shape cannot drift.
+pub(crate) fn report_error(json: bool, code: &str, message: impl std::fmt::Display) {
     let message = message.to_string();
     if json {
-        print_json(&serde_json::json!({"status": "error", "error": message}));
+        crate::json_envelope::print_legacy_error(code, &message);
     } else {
         eprintln!("Error: {message}");
     }
 }
 
 /// Report a failed apply-lock acquire in get's legacy error shape — the
-/// `{status: "error", error: "<message>"}` envelope every other hard error
-/// here uses, plus the stable `errorCode` (`lock_held` / `lock_io`) the
-/// other lock sites emit — and return the envelope for the caller's
-/// early-return guard. The message/code mapping is
+/// `{status: "error", error: {code, message}}` object every other hard
+/// error here uses, with the stable code (`lock_held` / `lock_io`) the
+/// other lock sites emit — and return it for the caller's early-return
+/// guard. The message/code mapping is
 /// [`crate::commands::lock_cli::lock_failure`]'s, so the waited clause and
 /// the I/O rendering cannot drift from `apply`'s.
 pub(crate) fn report_lock_failure(
@@ -200,11 +202,7 @@ pub(crate) fn report_lock_failure(
     timeout: Duration,
 ) -> serde_json::Value {
     let (code, message) = lock_failure(err, timeout);
-    let envelope = serde_json::json!({
-        "status": "error",
-        "errorCode": code,
-        "error": message,
-    });
+    let envelope = crate::json_envelope::legacy_error(code, &message);
     if json {
         print_json(&envelope);
     } else {
@@ -1407,7 +1405,8 @@ pub(crate) fn apply_key_covers(key: &str, record: &str) -> bool {
 /// as on every `failed` record); any other failed manifest patch (one this
 /// run did not select) gets its own `failed` record (`uuid_of` looks up
 /// its uuid); a run-level reason rides the envelope's top-level
-/// `errorCode` / `error`. `failed` grows by every record marked or
+/// `error: {code, message}` (status stays `partial_failure`: the downloads
+/// it reports still landed). `failed` grows by every record marked or
 /// appended here. Returns `applied`: how many of the run's recorded
 /// patches apply really patched (or found already patched).
 pub(crate) fn fold_apply_failures(
@@ -1474,8 +1473,10 @@ pub(crate) fn fold_apply_failures(
     let failed = envelope["failed"].as_u64().unwrap_or(0) as usize + added;
     envelope["failed"] = serde_json::json!(failed);
     if let Some((code, error)) = &report.run_error {
-        envelope["errorCode"] = serde_json::json!(code);
-        envelope["error"] = serde_json::json!(error);
+        crate::json_envelope::set_error_keep_status(
+            envelope,
+            crate::json_envelope::EnvelopeError::new(code, error),
+        );
     }
     applied
 }
@@ -1521,8 +1522,11 @@ pub async fn download_and_apply_patches_with(
         // and destroy every tracked patch record.
         Err(e) => {
             let err = format!("Failed to read manifest: {e}");
-            report_error(params.json, &err);
-            return (1, serde_json::json!({"status": "error", "error": err}));
+            report_error(params.json, "manifest_unreadable", &err);
+            return (
+                1,
+                crate::json_envelope::legacy_error("manifest_unreadable", &err),
+            );
         }
     };
 
@@ -1574,8 +1578,11 @@ pub async fn download_and_apply_patches_with(
             // unwind exactly those (a pre-existing record's blobs stay).
             unwind_new_blobs(&blobs_dir, &new_blobs).await;
             let msg = format!("Failed to write manifest: {e}");
-            report_error(params.json, &msg);
-            return (1, serde_json::json!({ "status": "error", "error": msg }));
+            report_error(params.json, "manifest_write_failed", &msg);
+            return (
+                1,
+                crate::json_envelope::legacy_error("manifest_write_failed", &msg),
+            );
         }
     }
     // Every selected patch that is now recorded is owed the nested apply:

crates/socket-patch-cli/src/commands/get.rs

@@ -47,6 +47,7 @@ use crate::commands::vlt_preflight::{
 use crate::ecosystem_dispatch::{
     crawl_all_ecosystems, find_packages_for_rollback, partition_purls,
 };
+use crate::json_envelope::{usage_error, Command as JsonCommand};
 use crate::ui::{print_json, select_one, SelectError};
 
 /// Best-effort ecosystem extractor for a `pkg:<eco>/...` PURL. Used as
@@ -86,7 +87,7 @@ async fn report_fetch_failure(
 ) -> i32 {
     let msg = error.to_string();
     track_patch_fetch_failed(identifier, &msg, fallback_to_proxy, api_token, org_slug).await;
-    report_error(json, msg);
+    report_error(json, "patch_fetch_failed", msg);
     1
 }
 
@@ -622,6 +623,11 @@ fn format_single_save(
 /// `--ghsa`, so a typo fails fast with a readable message instead of a raw
 /// API 400 body. `None` when it is well-formed (or the type is not
 /// shape-checked).
+///
+/// The message never echoes the argument: it reaches stderr, and CodeQL
+/// treats anything that may be a patch uuid as sensitive
+/// (rust/cleartext-logging). The user typed it, so naming the expected
+/// form is enough.
 fn forced_identifier_error(identifier: &str, id_type: IdentifierType) -> Option<String> {
     let (ok, what, form) = match id_type {
         IdentifierType::Uuid => (
@@ -637,7 +643,7 @@ fn forced_identifier_error(identifier: &str, id_type: IdentifierType) -> Option<
         ),
         IdentifierType::Purl | IdentifierType::Package => return None,
     };
-    (!ok).then(|| format!("\"{identifier}\" is not a valid {what} (expected {form})"))
+    (!ok).then(|| format!("The identifier is not a valid {what} (expected {form})"))
 }
 
 /// Select one patch per PURL from available patches.
@@ -736,7 +742,10 @@ pub(crate) fn select_patches(
                         .collect();
                     print_json(&serde_json::json!({
                         "status": "selection_required",
-                        "error": format!("Multiple patches available for {purl}. Re-run with the chosen UUID as the identifier (`socket-patch get <uuid>`) to select one."),
+                        "error": {
+                            "code": "selection_required",
+                            "message": format!("Multiple patches available for {purl}. Re-run with the chosen UUID as the identifier (`socket-patch get <uuid>`) to select one."),
+                        },
                         "purl": purl,
                         "options": options_json,
                     }));
@@ -1026,11 +1035,13 @@ pub async fn run(args: GetArgs) -> i32 {
         .filter(|&&f| f)
         .count();
     if type_flags > 1 {
-        report_error(
+        return usage_error(
+            JsonCommand::Get,
             args.common.json,
+            args.common.dry_run,
+            "invalid_args",
             "Only one of --id, --cve, --ghsa, or --package can be specified",
         );
-        return 2;
     }
     // v5: hosted by default, like scan. `--save-only` (records a manifest
     // entry) and global installs (no project lockfile) mean agent mode.
@@ -1045,20 +1056,27 @@ pub async fn run(args: GetArgs) -> i32 {
     // Global installs have no project lockfile: an explicit hosted or
     // vendored mode would rewire the cwd project, not the global copy.
     if let Some(conflict) = super::global_mode_conflict(&args.common, mode) {
-        report_error(args.common.json, conflict);
-        return 2;
+        return usage_error(
+            JsonCommand::Get,
+            args.common.json,
+            args.common.dry_run,
+            "global_scope_unsupported",
+            &conflict,
+        );
     }
     if args.save_only && mode != super::scan::ScanMode::Agent {
-        report_error(
+        return usage_error(
+            JsonCommand::Get,
             args.common.json,
-            format!(
+            args.common.dry_run,
+            "invalid_args",
+            &format!(
                 "--save-only cannot be used with --mode {}: hosted mode never writes the \
                  manifest, and vendored mode's vendor step IS the persistence (plain \
                  `get --save-only` already records without applying)",
                 mode.cli_name()
             ),
         );
-        return 2;
     }
     // Strict airgap (CLI_CONTRACT.md `--offline`: never contact the
     // network; operations that need remote data fail loudly). Every `get`
@@ -1069,6 +1087,7 @@ pub async fn run(args: GetArgs) -> i32 {
     if args.common.offline {
         report_error(
             args.common.json,
+            "offline_unsupported",
             "Fetching patches needs network access, so `get` cannot run with \
              --offline/SOCKET_OFFLINE (strict airgap)",
         );
@@ -1091,8 +1110,13 @@ pub async fn run(args: GetArgs) -> i32 {
     // a typo reads as a plain message instead of a raw API 400 body.
     if args.id || args.cve || args.ghsa {
         if let Some(err) = forced_identifier_error(&args.identifier, id_type) {
-            report_error(args.common.json, err);
-            return 2;
+            return usage_error(
+                JsonCommand::Get,
+                args.common.json,
+                args.common.dry_run,
+                "identifier_invalid",
+                &err,
+            );
         }
     }
 
@@ -1718,7 +1742,11 @@ async fn agent_dry_run(
     let manifest = match read_manifest(&args.common.resolved_manifest_path()).await {
         Ok(m) => m.unwrap_or_else(PatchManifest::new),
         Err(e) => {
-            report_error(args.common.json, format!("Failed to read manifest: {e}"));
+            report_error(
+                args.common.json,
+                "manifest_unreadable",
+                format!("Failed to read manifest: {e}"),
+            );
             return 1;
         }
     };
@@ -1808,7 +1836,11 @@ async fn save_patch_record(
         // treated as empty would be rewritten below with only this one
         // patch, destroying every tracked record.
         Err(e) => {
-            report_error(args.common.json, format!("Failed to read manifest: {e}"));
+            report_error(
+                args.common.json,
+                "manifest_unreadable",
+                format!("Failed to read manifest: {e}"),
+            );
             return Err(1);
         }
     };
@@ -1825,6 +1857,7 @@ async fn save_patch_record(
     if files.is_empty() {
         report_error(
             args.common.json,
+            "patch_no_applicable_files",
             format!(
                 "Patch {} has no applicable files; nothing to apply",
                 patch.purl
@@ -1850,7 +1883,10 @@ async fn save_patch_record(
                 "found": 1,
                 "downloaded": 0,
                 "applied": 0,
-                "error": "Blob decode or write failed",
+                "error": {
+                    "code": "blob_write_failed",
+                    "message": "Blob decode or write failed",
+                },
                 "patches": [{
                     "purl": patch.purl,
                     "uuid": patch.uuid,
@@ -1873,7 +1909,11 @@ async fn save_patch_record(
     if let Err(e) = write_manifest(manifest_path, &manifest).await {
         // No record points at the blobs just written: unwind exactly those.
         unwind_new_blobs(&blobs_dir, &new_blobs).await;
-        report_error(args.common.json, format!("Failed to write manifest: {e}"));
+        report_error(
+            args.common.json,
+            "manifest_write_failed",
+            format!("Failed to write manifest: {e}"),
+        );
         return Err(1);
     }
     Ok(action)
@@ -2320,8 +2360,10 @@ async fn run_get_vendored(
                     result["vendor"] =
                         serde_json::to_value(&*venv).unwrap_or_else(|_| serde_json::json!({}));
                 }
-                result["status"] = serde_json::json!("error");
-                result["error"] = serde_json::json!({ "code": code, "message": message });
+                crate::json_envelope::set_error(
+                    &mut result,
+                    crate::json_envelope::EnvelopeError::new(code, message),
+                );
                 print_json(&result);
             } else {
                 eprintln!(
@@ -3285,8 +3327,12 @@ mod tests {
             applied: Vec::new(),
         };
         assert_eq!(fold_apply_failures(&mut env, &report, |_| None), 0);
-        assert_eq!(env["errorCode"], "yarn_pnp_unsupported", "{env}");
-        assert_eq!(env["error"], "pnp", "{env}");
+        assert!(env.get("errorCode").is_none(), "{env}");
+        assert_eq!(
+            env["error"],
+            serde_json::json!({"code": "yarn_pnp_unsupported", "message": "pnp"}),
+            "{env}"
+        );
         assert_eq!(env["failed"], 0, "{env}");
     }
 
@@ -4270,15 +4316,15 @@ mod tests {
     fn forced_identifier_shapes() {
         assert_eq!(
             forced_identifier_error("lodash", IdentifierType::Uuid).as_deref(),
-            Some("\"lodash\" is not a valid patch UUID (expected xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)")
+            Some("The identifier is not a valid patch UUID (expected xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx)")
         );
         assert_eq!(
             forced_identifier_error("lodash", IdentifierType::Cve).as_deref(),
-            Some("\"lodash\" is not a valid CVE ID (expected CVE-YYYY-NNNN)")
+            Some("The identifier is not a valid CVE ID (expected CVE-YYYY-NNNN)")
         );
         assert_eq!(
             forced_identifier_error("GHSA-1", IdentifierType::Ghsa).as_deref(),
-            Some("\"GHSA-1\" is not a valid GHSA ID (expected GHSA-xxxx-xxxx-xxxx)")
+            Some("The identifier is not a valid GHSA ID (expected GHSA-xxxx-xxxx-xxxx)")
         );
         assert_eq!(
             forced_identifier_error("a8b05a61-1e2f-4c5f-a65b-93e71deba1ae", IdentifierType::Uuid),

crates/socket-patch-cli/src/commands/remove.rs

@@ -315,11 +315,14 @@ pub async fn run(args: RemoveArgs) -> i32 {
     // `--preserve-state` restores the tree and keeps the state — together
     // they select the do-nothing quadrant.
     if args.preserve_state && args.skip_rollback {
-        eprintln!(
-            "Error: --preserve-state cannot be used with --skip-rollback: the \
-             combination would be a no-op (nothing would change)"
+        return crate::json_envelope::usage_error(
+            Command::Remove,
+            args.common.json,
+            args.common.dry_run,
+            "invalid_args",
+            "--preserve-state cannot be used with --skip-rollback: the \
+             combination would be a no-op (nothing would change)",
         );
-        return 2;
     }
 
     let (telemetry_client, _) =

crates/socket-patch-cli/src/commands/repair.rs

@@ -47,14 +47,13 @@ pub async fn run(args: RepairArgs) -> i32 {
     // --offline implies strict airgap: no network calls. `--download-only`
     // is the inverse (network-only). The two are now mutually exclusive.
     if args.common.offline && args.download_only {
-        let msg = "--offline and --download-only are mutually exclusive";
-        if args.common.json {
-            let env = error_envelope(Command::Repair, args.common.dry_run, "invalid_args", msg);
-            println!("{}", env.to_pretty_json());
-        } else {
-            eprintln!("Error: {msg}");
-        }
-        return 2;
+        return crate::json_envelope::usage_error(
+            Command::Repair,
+            args.common.json,
+            args.common.dry_run,
+            "invalid_args",
+            "--offline and --download-only are mutually exclusive",
+        );
     }
 
     let manifest_path = args.common.resolved_manifest_path();