diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index dff38f2c8..3eceb7249 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1363,7 +1363,7 @@ rely on these keys. ], // ----- failure path (only on action=failed) ----- - "errorCode": "vendor_bun_workspace_unsupported", // additive; today only the vendored-mode Bun preflight refusals (+ vendor_state_unreadable) + "errorCode": "vendor_bun_workspace_unsupported", // additive; the vendored-mode Bun preflight refusals (+ vendor_state_unreadable) and agent-mode apply failures (apply_failed, package_not_installed) "error": "could not fetch details" } ``` @@ -1398,8 +1398,25 @@ and `vendor_state_unreadable` when the preflight cannot read `.socket/vendor/state.json`) that `get --mode vendored` and `scan --mode vendored` (`download.patches[]`) emit before any download; see "get --mode and installed narrowing" → -Vendored → Bun vendored preflight. Every other `failed` record carries only -`error`. The dry-run preview's `would_refuse` records carry the same pair. +Vendored → Bun vendored preflight. Every other download-phase `failed` +record carries only `error`. The dry-run preview's `would_refuse` records +carry the same pair. + +Agent-mode apply failures (#424): when the nested apply that follows the +download (`get` / `scan --mode agent`, not `--save-only`) fails a patch, +that patch's record becomes `action: "failed"` with the same `errorCode` / +`error` pair the standalone `apply --json` reports — `apply_failed` (the +apply error text, e.g. `Permission denied (os error 13)`) or +`package_not_installed` (no installed copy, and the project's lockfiles +do not resolve it either). The record keeps `purl` and `uuid`, drops the +metadata like every `failed` record, and stays saved in the manifest (only +the apply failed). A failing manifest patch the run did not select (the +nested apply covers the whole `--ecosystems`-scoped manifest) is appended +as its own `failed` record. `failed` counts these records beside the +download failures, and `applied` counts only the patches that did apply. +A failure no single patch explains (an unreadable manifest, the yarn PnP +refusal, unavailable patch sources) sets top-level `errorCode` / `error` +on the same object (`apply` in `scan`'s envelope). `vulnerabilities[]` is always sorted by `id` so consumer diffs and test snapshots are stable. `severity` at the top level is the max diff --git a/crates/socket-patch-cli/src/commands/apply.rs b/crates/socket-patch-cli/src/commands/apply.rs index cfd788a2a..8c4d1649a 100644 --- a/crates/socket-patch-cli/src/commands/apply.rs +++ b/crates/socket-patch-cli/src/commands/apply.rs @@ -802,13 +802,16 @@ fn manifest_targets_npm(manifest: &PatchManifest) -> bool { /// package-manager layout gate below: scan cannot discover PnP packages so /// it never writes a manifest, and the loud `yarn_pnp_unsupported` refusal /// must still be reachable without one. +/// The yarn-berry PnP refusal's envelope error text. +const YARN_PNP_UNSUPPORTED: &str = "yarn-berry Plug'n'Play layout is not supported by socket-patch (packages live inside .yarn/cache zips). Use `yarn patch ` instead."; + fn refuse_yarn_pnp(args: &ApplyArgs) -> i32 { if args.common.json { let mut env = Envelope::new(Command::Apply); env.dry_run = args.common.dry_run; env.mark_error(EnvelopeError::new( "yarn_pnp_unsupported", - "yarn-berry Plug'n'Play layout is not supported by socket-patch (packages live inside .yarn/cache zips). Use `yarn patch ` instead.", + YARN_PNP_UNSUPPORTED, )); println!("{}", env.to_pretty_json()); } else { @@ -964,7 +967,85 @@ pub async fn run(args: ApplyArgs) -> i32 { Err(code) => return code, }; - run_locked(args, manifest_path, &client, lock).await + run_locked(args, manifest_path, &client, lock).await.code +} + +/// One patch the nested apply failed: the manifest purl, a stable code +/// (`apply_failed`, `package_not_installed`) and the error text — the same +/// `errorCode` / `error` pair the standalone `apply --json` reports. +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct ApplyFailure { + pub purl: String, + pub code: String, + pub error: String, +} + +/// What a run of [`run_locked`] reports back. `apply` itself only needs +/// `code`; `get` / `scan --mode agent` fold the rest into their own +/// envelope, because the nested apply never prints JSON (#424). +#[derive(Debug, Default)] +pub(crate) struct ApplyRunReport { + /// The process exit code. + pub code: i32, + /// Per-patch failures. + pub failures: Vec, + /// A failure not tied to one patch (unreadable manifest, the yarn PnP + /// refusal, unavailable patch sources, a failed embedded VEX), as + /// `(errorCode, error)`. Set only when `code != 0` and `failures` + /// alone would not explain it. + pub run_error: Option<(String, String)>, + /// The package keys apply patched or found already patched, so a + /// failed run's caller can count exactly what applied. Filled only + /// when `code != 0`. + pub applied: Vec, +} + +impl ApplyRunReport { + fn run_failure(code: i32, error_code: &str, error: impl Into) -> Self { + Self { + code, + failures: Vec::new(), + run_error: Some((error_code.to_string(), error.into())), + applied: Vec::new(), + } + } +} + +/// The per-patch failures of a failed apply loop: every failed result (one +/// per package, the first error wins). With none, what failed the run is +/// the in-scope manifest purls with no installed package that the +/// project's lockfiles do not resolve either; beside a failed result those +/// are only the "no matching installed package" warning, never a failure. +fn collect_apply_failures( + results: &[ApplyResult], + unmatched: &[String], + lockfile_only: &HashSet, +) -> Vec { + let mut failures: Vec = Vec::new(); + for r in results.iter().filter(|r| !r.success) { + if failures.iter().any(|f| f.purl == r.package_key) { + continue; + } + failures.push(ApplyFailure { + purl: r.package_key.clone(), + code: "apply_failed".to_string(), + error: r + .error + .clone() + .unwrap_or_else(|| "unknown error".to_string()), + }); + } + if !failures.is_empty() { + return failures; + } + for purl in unresolved_purls(unmatched, lockfile_only) { + failures.push(ApplyFailure { + purl, + code: "package_not_installed".to_string(), + error: "No installed package matches this PURL".to_string(), + }); + } + failures } /// The locked half of `apply`: everything from the manifest read on — the @@ -976,13 +1057,14 @@ pub async fn run(args: ApplyArgs) -> i32 { /// re-acquire would contend) and the nested apply never builds a second /// client. `lock` is released explicitly once every mutation is done /// (output and a possibly slow telemetry POST must not keep a sibling -/// waiting), otherwise on return. +/// waiting), otherwise on return. The returned [`ApplyRunReport`] carries +/// the exit code plus what failed, for a nested caller's envelope. pub(crate) async fn run_locked( args: ApplyArgs, manifest_path: PathBuf, client: &ApiClient, lock: LockGuard, -) -> i32 { +) -> ApplyRunReport { let api_token = client.api_token().cloned(); let org_slug = client.org_slug().cloned(); @@ -995,11 +1077,14 @@ pub(crate) async fn run_locked( Ok(Some(m)) => m, Ok(None) => { lock.release(); - return report_apply_failure(&args, "Invalid manifest", &api_token, &org_slug).await; + let code = report_apply_failure(&args, "Invalid manifest", &api_token, &org_slug).await; + return ApplyRunReport::run_failure(code, "apply_failed", "Invalid manifest"); } Err(e) => { lock.release(); - return report_apply_failure(&args, &e.to_string(), &api_token, &org_slug).await; + let error = e.to_string(); + let code = report_apply_failure(&args, &error, &api_token, &org_slug).await; + return ApplyRunReport::run_failure(code, "apply_failed", error); } }; @@ -1017,7 +1102,11 @@ pub(crate) async fn run_locked( match detect_npm_pkg_manager(&args.common.cwd) { NpmPkgManager::YarnBerryPnP => { if eco_in_local_scope(&args.common, Ecosystem::Npm) && manifest_targets_npm(&manifest) { - return refuse_yarn_pnp(&args); + return ApplyRunReport::run_failure( + refuse_yarn_pnp(&args), + "yarn_pnp_unsupported", + YARN_PNP_UNSUPPORTED, + ); } } NpmPkgManager::Pnpm => { @@ -1320,14 +1409,50 @@ pub(crate) async fn run_locked( // A requested-but-failed VEX flips an otherwise-successful // apply to a non-zero exit (fail-the-command contract). if success && !vex_failed { - 0 + return ApplyRunReport::default(); + } + let failures = if success { + Vec::new() + } else { + collect_apply_failures(&results, &unmatched, &lockfile_only) + }; + let run_error = if let Some(Err(e)) = &vex_result { + Some((e.code.to_string(), e.message.clone())) + } else if failures.is_empty() { + // Nothing per-patch explains the failure: the run-level + // reason (sources unavailable) or a generic one. + Some( + run_warnings + .iter() + .find(|w| is_stage_failure_code(&w.code)) + .map(|w| (w.code.clone(), w.detail.clone())) + .unwrap_or_else(|| { + ( + "apply_failed".to_string(), + "One or more patches failed to apply".to_string(), + ) + }), + ) } else { - 1 + None + }; + // Vendor-owned results are skips, not applies. + let applied = results + .iter() + .filter(|r| r.success && r.package_path != VENDOR_OWNED_MARKER) + .map(|r| r.package_key.clone()) + .collect(); + ApplyRunReport { + code: 1, + failures, + run_error, + applied, } } Err(e) => { lock.release(); - report_apply_failure(&args, &e, &api_token, &org_slug).await + let code = report_apply_failure(&args, &e, &api_token, &org_slug).await; + ApplyRunReport::run_failure(code, "apply_failed", e) } } } @@ -4087,4 +4212,69 @@ mod tests { } assert_eq!(result.unwrap(), None); } + + // --- collect_apply_failures (#424) ------------------------------------- + + fn failed_result(purl: &str, error: Option<&str>) -> ApplyResult { + ApplyResult { + package_key: purl.to_string(), + package_path: "/tmp/node_modules/x".to_string(), + success: false, + files_verified: Vec::new(), + files_patched: Vec::new(), + applied_via: HashMap::new(), + error: error.map(str::to_string), + sidecar: None, + } + } + + #[test] + fn collect_apply_failures_reports_each_failed_package_once() { + let results = vec![ + failed_result("pkg:npm/a@1.0.0", Some("Permission denied (os error 13)")), + failed_result("pkg:npm/a@1.0.0", Some("second copy")), + failed_result("pkg:npm/b@1.0.0", None), + sample_applied(VerifyStatus::Ready), + ]; + let failures = collect_apply_failures(&results, &[], &HashSet::new()); + assert_eq!( + failures, + vec![ + ApplyFailure { + purl: "pkg:npm/a@1.0.0".to_string(), + code: "apply_failed".to_string(), + error: "Permission denied (os error 13)".to_string(), + }, + ApplyFailure { + purl: "pkg:npm/b@1.0.0".to_string(), + code: "apply_failed".to_string(), + error: "unknown error".to_string(), + }, + ] + ); + } + + #[test] + fn collect_apply_failures_names_unresolved_purls_only_when_nothing_else_failed() { + let unmatched = vec![ + "pkg:npm/gone@1.0.0".to_string(), + "pkg:npm/opt@1.0.0".to_string(), + ]; + let lockfile_only = HashSet::from(["pkg:npm/opt@1.0.0".to_string()]); + let failures = collect_apply_failures(&[], &unmatched, &lockfile_only); + assert_eq!( + failures, + vec![ApplyFailure { + purl: "pkg:npm/gone@1.0.0".to_string(), + code: "package_not_installed".to_string(), + error: "No installed package matches this PURL".to_string(), + }], + "a lockfile-resolved purl never fails the run (#403)" + ); + // Beside a real failure, an uninstalled patch is only a warning. + let results = vec![failed_result("pkg:npm/a@1.0.0", Some("boom"))]; + let failures = collect_apply_failures(&results, &unmatched, &lockfile_only); + assert_eq!(failures.len(), 1, "{failures:?}"); + assert_eq!(failures[0].purl, "pkg:npm/a@1.0.0"); + } } diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index ecb1954bb..a41f95802 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -31,6 +31,7 @@ use std::sync::LazyLock; use std::time::Duration; use crate::args::{apply_env_toggles, GlobalArgs}; +use crate::commands::apply::ApplyRunReport; use crate::commands::bun_preflight::{ bun_vendor_preflight, bun_vendor_preflight_with_ledger, BunVendorRefusal, }; @@ -2326,18 +2327,20 @@ fn nested_apply_args_from_params( /// on the caller's `client`, under the apply `lock` the caller took for /// its manifest write — one lock window for download → manifest write → /// apply (a same-process re-acquire would contend), released by apply once -/// its last mutation is done. Returns whether apply exited 0. Callers print -/// their own "Applying patches..." line. `json` is the caller's flag: a -/// JSON caller gets no human error lines, from this function or from the -/// nested apply (`common` itself is never JSON). The read-only `--check` -/// redirect verifier stays off and embedded VEX is opt-in on the top-level -/// command only, never on this internal invocation. +/// its last mutation is done. Returns apply's report: its exit code and +/// what failed, for the caller's envelope (see [`fold_apply_failures`]). +/// Callers print their own "Applying patches..." line. `json` is the +/// caller's flag: a JSON caller gets no human error lines, from this +/// function or from the nested apply (`common` itself is never JSON). The +/// read-only `--check` redirect verifier stays off and embedded VEX is +/// opt-in on the top-level command only, never on this internal +/// invocation. async fn run_nested_apply( common: GlobalArgs, json: bool, client: &ApiClient, lock: LockGuard, -) -> bool { +) -> ApplyRunReport { let manifest_path = common.resolved_manifest_path(); let apply_args = super::apply::ApplyArgs { common, @@ -2346,13 +2349,101 @@ async fn run_nested_apply( vex: Default::default(), nested: Some(super::apply::NestedApply { caller_json: json }), }; - let code = super::apply::run_locked(apply_args, manifest_path, client, lock).await; + let report = super::apply::run_locked(apply_args, manifest_path, client, lock).await; // An error, so exempt from --silent ("errors only": a failing exit must // say why); JSON runs carry the failure in the envelope instead. - if code != 0 && !json { + if report.code != 0 && !json { eprintln!("{APPLY_FAILED}"); } - code == 0 + report +} + +/// Whether apply's package key `key` covers the patch record purl +/// `record`: the same purl, or `key` is the unqualified base of a +/// qualified record (apply keys a release-variant base by its base purl). +/// A qualified key never covers a sibling variant. +fn apply_key_covers(key: &str, record: &str) -> bool { + let (key, record) = (normalize_purl(key), normalize_purl(record)); + key == record || (!key.contains(['?', '#']) && record.split(['?', '#']).next() == Some(&*key)) +} + +/// Fold a failed nested apply into a `get` / `scan --mode agent` JSON +/// envelope, so `--json` says what the human run prints (#424). Each +/// `patches[]` record the apply failed becomes the `failed` record shape +/// (`purl`, `uuid`, `action: "failed"`, `errorCode`, `error`; no metadata, +/// 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 +/// appended here. Returns `applied`: how many of the run's recorded +/// patches apply really patched (or found already patched). +fn fold_apply_failures( + envelope: &mut serde_json::Value, + report: &ApplyRunReport, + uuid_of: impl Fn(&str) -> Option, +) -> usize { + let Some(patches) = envelope["patches"].as_array_mut() else { + return 0; + }; + let selected = patches.len(); + let mut marked = 0usize; + for failure in &report.failures { + let mut hit = false; + for rec in patches.iter_mut().take(selected) { + let purl = rec["purl"].as_str().unwrap_or_default(); + if !apply_key_covers(&failure.purl, purl) { + continue; + } + hit = true; + if rec["action"].as_str() != Some("failed") { + *rec = serde_json::json!({ + "purl": rec["purl"], + "uuid": rec["uuid"], + "action": "failed", + "errorCode": failure.code, + "error": failure.error, + }); + marked += 1; + } + } + let appended = patches[selected..].iter().any(|r| { + normalize_purl(r["purl"].as_str().unwrap_or_default()) == normalize_purl(&failure.purl) + }); + if !hit && !appended { + let mut rec = serde_json::json!({ + "purl": failure.purl, + "action": "failed", + "errorCode": failure.code, + "error": failure.error, + }); + if let Some(uuid) = uuid_of(&failure.purl) { + rec["uuid"] = serde_json::json!(uuid); + } + patches.push(rec); + } + } + // Recorded patches (added / updated, or the plain already-recorded + // skip) that apply reports as patched. + let applied = patches[..selected] + .iter() + .filter(|r| match r["action"].as_str() { + Some("added" | "updated") => true, + Some("skipped") => r.get("errorCode").is_none(), + _ => false, + }) + .filter(|r| { + let purl = r["purl"].as_str().unwrap_or_default(); + report.applied.iter().any(|k| apply_key_covers(k, purl)) + }) + .count(); + let added = marked + (patches.len() - selected); + 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); + } + applied } /// Download the selected patches into `.socket/` (manifest records + @@ -2488,20 +2579,23 @@ pub async fn download_and_apply_patches_with( } // Auto-apply unless --save-only (the lock decision above). - let mut apply_succeeded = false; + let mut apply_report: Option = None; if let Some(lock) = apply_lock { if !quiet { eprintln!(); eprintln!("Applying patches..."); } - apply_succeeded = run_nested_apply( - nested_apply_args_from_params(params, run, &manifest_path), - params.json, - run.api_client, - lock, - ) - .await; + apply_report = Some( + run_nested_apply( + nested_apply_args_from_params(params, run, &manifest_path), + params.json, + run.api_client, + lock, + ) + .await, + ); } + let apply_succeeded = apply_report.as_ref().is_some_and(|r| r.code == 0); // An apply step that ran (recorded patches selected, not --save-only) // but failed is a partial failure too — not just download failures. The @@ -2521,6 +2615,13 @@ pub async fn download_and_apply_patches_with( "updated": updated, "patches": batch.patches_json, }); + // A failed apply: name what failed, and count only what applied. + if let Some(report) = apply_report.as_ref().filter(|r| r.code != 0) { + let applied = fold_apply_failures(&mut result_json, report, |purl| { + manifest.patches.get(purl).map(|r| r.uuid.clone()) + }); + result_json["applied"] = serde_json::json!(applied); + } // Surface release-narrowing fallbacks (uninstalled package / no // matching variant) so JSON consumers can see why all variants were // kept. Omitted entirely when narrowing was clean. @@ -3474,20 +3575,23 @@ async fn save_and_apply_patch(args: &GetArgs, client: &ApiClient, patch: &PatchR ); } - let mut apply_succeeded = false; + let mut apply_report: Option = None; if let Some(lock) = apply_lock { if !quiet { eprintln!(); eprintln!("Applying patches..."); } - apply_succeeded = run_nested_apply( - nested_apply_args(&args.common, &manifest_path, quiet), - args.common.json, - client, - lock, - ) - .await; + apply_report = Some( + run_nested_apply( + nested_apply_args(&args.common, &manifest_path, quiet), + args.common.json, + client, + lock, + ) + .await, + ); } + let apply_succeeded = apply_report.as_ref().is_some_and(|r| r.code == 0); // The apply step ran (not --save-only) but failed → // partial failure. The `status` field must agree with the exit code @@ -3519,6 +3623,18 @@ async fn save_and_apply_patch(args: &GetArgs, client: &ApiClient, patch: &PatchR "applied": if apply_succeeded { 1 } else { 0 }, "patches": [patch_record], }); + // A failed apply names what failed (#424); `failed` appears only + // then, so a clean run's envelope is unchanged. + if let Some(report) = apply_report.as_ref().filter(|r| r.code != 0) { + // The manifest names the uuid of any other failing record. + let recorded = Box::pin(read_manifest(&manifest_path)).await.ok().flatten(); + result_json["failed"] = serde_json::json!(0); + let applied = fold_apply_failures(&mut result_json, report, |purl| { + let record = recorded.as_ref()?.patches.get(purl)?; + Some(record.uuid.clone()) + }); + result_json["applied"] = serde_json::json!(applied); + } // Same contract as `download_and_apply_patches_with`: omitted when clean. if !warnings.is_empty() { result_json["warnings"] = serde_json::json!(warnings); @@ -4630,6 +4746,188 @@ mod tests { } } + // --- fold_apply_failures (#424) --------------------------------------- + + fn failure(purl: &str, code: &str, error: &str) -> crate::commands::apply::ApplyFailure { + crate::commands::apply::ApplyFailure { + purl: purl.to_string(), + code: code.to_string(), + error: error.to_string(), + } + } + + fn report_with( + failures: Vec, + applied: &[&str], + ) -> ApplyRunReport { + ApplyRunReport { + code: 1, + failures, + run_error: None, + applied: applied.iter().map(|p| p.to_string()).collect(), + } + } + + #[test] + fn fold_apply_failures_marks_the_failed_record_and_drops_metadata() { + let mut env = serde_json::json!({ + "failed": 0, + "patches": [ + {"purl": "pkg:npm/a@1.0.0", "uuid": "ua", "action": "added", "license": "MIT"}, + {"purl": "pkg:npm/b@1.0.0", "uuid": "ub", "action": "updated", "oldUuid": "o"}, + ], + }); + let report = report_with( + vec![failure("pkg:npm/a@1.0.0", "apply_failed", "denied")], + &["pkg:npm/b@1.0.0"], + ); + assert_eq!( + fold_apply_failures(&mut env, &report, |_| None), + 1, + "b applied" + ); + assert_eq!( + env["patches"][0], + serde_json::json!({ + "purl": "pkg:npm/a@1.0.0", "uuid": "ua", "action": "failed", + "errorCode": "apply_failed", "error": "denied", + }) + ); + assert_eq!(env["patches"][1]["action"], "updated", "{env}"); + assert_eq!(env["failed"], 1, "{env}"); + assert!(env.get("errorCode").is_none(), "{env}"); + } + + #[test] + fn fold_apply_failures_matches_percent_encoded_and_base_purl_keys() { + let mut env = serde_json::json!({ + "failed": 0, + "patches": [ + {"purl": "pkg:npm/%40scope/a@1.0.0", "uuid": "u1", "action": "added"}, + {"purl": "pkg:pypi/six@1.16.0?artifact_id=w", "uuid": "u2", "action": "added"}, + ], + }); + // An unqualified (base) key covers its qualified release variants. + let report = report_with( + vec![ + failure("pkg:npm/@scope/a@1.0.0", "apply_failed", "x"), + failure("pkg:pypi/six@1.16.0", "package_not_installed", "y"), + ], + &[], + ); + assert_eq!(fold_apply_failures(&mut env, &report, |_| None), 0); + assert_eq!(env["patches"][0]["action"], "failed", "{env}"); + assert_eq!(env["patches"][1]["action"], "failed", "{env}"); + assert_eq!(env["patches"][1]["errorCode"], "package_not_installed"); + assert_eq!(env["patches"].as_array().unwrap().len(), 2, "{env}"); + assert_eq!(env["failed"], 2, "{env}"); + } + + #[test] + fn fold_apply_failures_never_blames_a_sibling_variant() { + // A qualified failure that matches no selected record must not be + // pinned on a selected sibling variant that applied: it gets its + // own record, and the sibling stays applied. + let mut env = serde_json::json!({ + "failed": 0, + "patches": [ + {"purl": "pkg:pypi/six@1.16.0?artifact_id=w", "uuid": "u1", "action": "added"}, + ], + }); + let report = report_with( + vec![failure( + "pkg:pypi/six@1.16.0?artifact_id=s", + "apply_failed", + "boom", + )], + &["pkg:pypi/six@1.16.0?artifact_id=w"], + ); + let uuid_of = |p: &str| p.ends_with("=s").then(|| "u0".to_string()); + assert_eq!(fold_apply_failures(&mut env, &report, uuid_of), 1); + assert_eq!(env["patches"][0]["action"], "added", "{env}"); + assert_eq!( + env["patches"][1]["purl"], + "pkg:pypi/six@1.16.0?artifact_id=s" + ); + assert_eq!(env["patches"][1]["uuid"], "u0", "{env}"); + assert_eq!(env["patches"][1]["action"], "failed", "{env}"); + assert_eq!(env["failed"], 1, "{env}"); + } + + #[test] + fn fold_apply_failures_counts_only_patches_apply_reported_applied() { + // `c` is selected and recorded but apply never patched it (not + // installed: only a warning beside `a`'s real failure), so it must + // not count as applied. + let mut env = serde_json::json!({ + "failed": 0, + "patches": [ + {"purl": "pkg:npm/a@1.0.0", "uuid": "ua", "action": "added"}, + {"purl": "pkg:npm/b@1.0.0", "uuid": "ub", "action": "skipped"}, + {"purl": "pkg:npm/c@1.0.0", "uuid": "uc", "action": "added"}, + {"purl": "pkg:npm/d@1.0.0", "uuid": "ud", "action": "skipped", + "errorCode": "package_not_installed"}, + ], + }); + let report = report_with( + vec![failure("pkg:npm/a@1.0.0", "apply_failed", "x")], + &["pkg:npm/b@1.0.0", "pkg:npm/d@1.0.0"], + ); + assert_eq!( + fold_apply_failures(&mut env, &report, |_| None), + 1, + "only the already-recorded b applied: {env}" + ); + assert_eq!(env["patches"][2]["action"], "added", "{env}"); + assert_eq!(env["failed"], 1, "{env}"); + } + + #[test] + fn fold_apply_failures_appends_an_unselected_manifest_failure() { + // The nested apply covers the whole (ecosystem-scoped) manifest: a + // failing record this run did not select still gets named, without + // costing this run's own patch its `applied` count. + let mut env = serde_json::json!({ + "failed": 1, + "patches": [ + {"purl": "pkg:npm/a@1.0.0", "uuid": "ua", "action": "added"}, + ], + }); + let report = report_with( + vec![failure("pkg:npm/old@2.0.0", "apply_failed", "z")], + &["pkg:npm/a@1.0.0"], + ); + let uuid_of = |p: &str| (p == "pkg:npm/old@2.0.0").then(|| "uo".to_string()); + assert_eq!(fold_apply_failures(&mut env, &report, uuid_of), 1); + assert_eq!(env["patches"][0]["action"], "added", "{env}"); + assert_eq!( + env["patches"][1], + serde_json::json!({ + "purl": "pkg:npm/old@2.0.0", "uuid": "uo", "action": "failed", + "errorCode": "apply_failed", "error": "z", + }) + ); + assert_eq!(env["failed"], 2, "download failures stay counted: {env}"); + } + + #[test] + fn fold_apply_failures_carries_a_run_level_error() { + let mut env = serde_json::json!({ + "failed": 0, + "patches": [{"purl": "pkg:npm/a@1.0.0", "uuid": "ua", "action": "added"}], + }); + let report = ApplyRunReport { + code: 1, + failures: Vec::new(), + run_error: Some(("yarn_pnp_unsupported".to_string(), "pnp".to_string())), + 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_eq!(env["failed"], 0, "{env}"); + } + // --- write_blob_entry ------------------------------------------------ // Blob hashes come straight from the API response and are used as // filesystem path components (`blobs_dir.join(hash)`). A hostile or diff --git a/crates/socket-patch-cli/tests/covgap_commands_get.rs b/crates/socket-patch-cli/tests/covgap_commands_get.rs index 4eb66b75c..c98f057a7 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_get.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_get.rs @@ -3125,3 +3125,148 @@ async fn get_vendored_dry_run_human_prints_the_vlt_would_refuse_line() { assert_eq!(hosted::read(root, "vlt-lock.json"), lock); assert!(!root.join(".socket").exists()); } + +// =========================================================================== +// Nested apply failures in the JSON envelope (#424) +// =========================================================================== + +/// The npm project fixture, except `node_modules/covgap-pkg` is a symlink +/// to a first-party `packages/covgap-pkg` (a workspace member that shares +/// the patched name@version). Apply refuses to patch it (it lies outside +/// every `node_modules` tree), a deterministic apply failure that needs no +/// read-only filesystem. +#[cfg(unix)] +fn write_first_party_link_project(root: &Path) { + write_project(root); + let installed = root.join("node_modules").join(NAME); + let member = root.join("packages").join(NAME); + std::fs::create_dir_all(member.parent().unwrap()).unwrap(); + std::fs::rename(&installed, &member).unwrap(); + std::os::unix::fs::symlink(Path::new("../packages").join(NAME), &installed).unwrap(); +} + +/// The agent engine with the nested apply ON, over a project whose +/// installed copy apply refuses to patch (a first-party link): the +/// apply step fails, and the envelope must say so per patch — the same +/// `{action: "failed", errorCode, error}` a standalone `apply --json` +/// reports — with `failed` counting it and nothing counted as applied. +#[cfg(unix)] +#[tokio::test] +#[serial] +async fn engine_nested_apply_failure_reaches_the_json_envelope() { + let server = MockServer::start().await; + mount_real_view(&server, UUID, PURL).await; + + let tmp = tempfile::tempdir().unwrap(); + write_first_party_link_project(tmp.path()); + let mut params = engine_params(tmp.path()); + params.save_only = false; + let selected = vec![search_result(UUID, PURL)]; + let (code, json) = download_and_apply_patches(&selected, ¶ms, &server.uri()).await; + + assert_eq!(code, 1, "json={json}"); + assert_eq!(json["status"], "partial_failure", "json={json}"); + assert_eq!(json["downloaded"], 1, "the download itself worked: {json}"); + assert_eq!( + json["failed"], 1, + "the apply failure must be counted: {json}" + ); + assert_eq!(json["applied"], 0, "json={json}"); + let rec = &json["patches"][0]; + assert_eq!(rec["purl"], PURL, "json={json}"); + assert_eq!(rec["uuid"], UUID, "json={json}"); + assert_eq!(rec["action"], "failed", "json={json}"); + assert_eq!(rec["errorCode"], "apply_failed", "json={json}"); + assert!( + rec["error"].as_str().is_some_and(|e| !e.is_empty()), + "the apply error text must reach the envelope: {json}" + ); + // The record stays saved: only the apply degraded. + assert_eq!(manifest_json(tmp.path())["patches"][PURL]["uuid"], UUID); + assert_eq!( + std::fs::read(tmp.path().join("packages").join(NAME).join("index.js")).unwrap(), + BEFORE_BYTES, + "a failed apply leaves the first-party bytes alone" + ); +} + +/// Nested apply over a project with nothing installed: apply fails because +/// the recorded patch has no installed package. The envelope names that +/// patch and the reason instead of reporting it as a clean `added`. +#[tokio::test] +#[serial] +async fn engine_nested_apply_not_installed_reaches_the_json_envelope() { + let server = MockServer::start().await; + mount_view_files(&server, UUID, PURL, good_files()).await; + + let tmp = tempfile::tempdir().unwrap(); + let mut params = engine_params(tmp.path()); + params.save_only = false; + let selected = vec![search_result(UUID, PURL)]; + let (code, json) = download_and_apply_patches(&selected, ¶ms, &server.uri()).await; + + assert_eq!(code, 1, "json={json}"); + assert_eq!(json["status"], "partial_failure", "json={json}"); + assert_eq!(json["failed"], 1, "json={json}"); + assert_eq!(json["applied"], 0, "json={json}"); + let rec = &json["patches"][0]; + assert_eq!(rec["action"], "failed", "json={json}"); + assert_eq!(rec["errorCode"], "package_not_installed", "json={json}"); + assert!( + rec["error"].as_str().is_some_and(|e| !e.is_empty()), + "json={json}" + ); +} + +/// A successful nested apply still reports a clean envelope: the patch +/// keeps its `added` action and counts as applied. +#[tokio::test] +#[serial] +async fn engine_nested_apply_success_keeps_added_and_counts_applied() { + let server = MockServer::start().await; + mount_real_view(&server, UUID, PURL).await; + + let tmp = tempfile::tempdir().unwrap(); + write_project(tmp.path()); + let mut params = engine_params(tmp.path()); + params.save_only = false; + let selected = vec![search_result(UUID, PURL)]; + let (code, json) = download_and_apply_patches(&selected, ¶ms, &server.uri()).await; + + assert_eq!(code, 0, "json={json}"); + assert_eq!(json["status"], "success", "json={json}"); + assert_eq!(json["failed"], 0, "json={json}"); + assert_eq!(json["applied"], 1, "json={json}"); + assert_eq!(json["patches"][0]["action"], "added", "json={json}"); + assert!(json["patches"][0].get("errorCode").is_none(), "json={json}"); + assert_eq!( + std::fs::read(tmp.path().join("node_modules").join(NAME).join("index.js")).unwrap(), + AFTER_BYTES, + ); +} + +/// `get --json` (the single-patch path) with the nested apply ON +/// over a first-party link: one JSON document whose patch record carries +/// the apply failure, and `failed: 1`. +#[cfg(unix)] +#[tokio::test] +async fn get_uuid_json_nested_apply_failure_names_the_patch() { + let server = MockServer::start().await; + mount_real_view(&server, UUID, PURL).await; + + let tmp = tempfile::tempdir().unwrap(); + write_first_party_link_project(tmp.path()); + let (code, stdout, stderr) = run_get_bin(tmp.path(), &server.uri(), &[UUID, "--json"]); + assert_eq!(code, 1, "stdout={stdout}\nstderr={stderr}"); + let json = parse_single_json_doc(&stdout); + assert_eq!(json["status"], "partial_failure", "json={json}"); + assert_eq!(json["failed"], 1, "json={json}"); + assert_eq!(json["applied"], 0, "json={json}"); + let rec = &json["patches"][0]; + assert_eq!(rec["action"], "failed", "json={json}"); + assert_eq!(rec["errorCode"], "apply_failed", "json={json}"); + assert!( + rec["error"].as_str().is_some_and(|e| !e.is_empty()), + "json={json}" + ); +} 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 7d940fe31..8900fbbbd 100644 --- a/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs +++ b/crates/socket-patch-cli/tests/covgap_commands_scan_mod.rs @@ -2654,3 +2654,45 @@ async fn scan_vendored_ignores_a_degraded_pre_v5_vlt_ledger_edit() { ); } } + +/// `scan --mode agent --json` whose nested apply fails (the installed copy +/// is a symlink to a first-party `packages/` directory, which apply refuses +/// to patch): the `apply` block must carry +/// the per-patch failure — `action: "failed"`, `errorCode`, `error` — and +/// count it in `failed`, not report the patch as a clean `added` (#424). +#[cfg(unix)] +#[tokio::test] +async fn scan_agent_json_nested_apply_failure_reaches_the_apply_block() { + let mock = MockServer::start().await; + let purl = "pkg:npm/apply-fails@1.0.0"; + mount_one_patch_api(&mock, purl, b"before\n").await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "apply-fails", "1.0.0", b"before\n"); + let member = tmp.path().join("packages/apply-fails"); + std::fs::create_dir_all(member.parent().unwrap()).unwrap(); + std::fs::rename(tmp.path().join("node_modules/apply-fails"), &member).unwrap(); + std::os::unix::fs::symlink( + "../packages/apply-fails", + tmp.path().join("node_modules/apply-fails"), + ) + .unwrap(); + + let (code, stdout, stderr) = run_scan_agent(tmp.path(), &mock.uri(), &["--json"]); + assert_eq!(code, 1, "stdout={stdout}\nstderr={stderr}"); + let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("one JSON envelope"); + assert_eq!(v["status"], "partial_failure", "{v}"); + let apply = &v["apply"]; + assert_eq!(apply["failed"], 1, "the apply failure must be counted: {v}"); + assert_eq!(apply["applied"], 0, "{v}"); + let rec = &apply["patches"][0]; + assert_eq!(rec["purl"], purl, "{v}"); + assert_eq!(rec["action"], "failed", "{v}"); + assert_eq!(rec["errorCode"], "apply_failed", "{v}"); + assert!( + rec["error"].as_str().is_some_and(|e| !e.is_empty()), + "the apply error text must reach the envelope: {v}" + ); + assert_eq!(std::fs::read(member.join("index.js")).unwrap(), b"before\n",); +} diff --git a/crates/socket-patch-cli/tests/docker_e2e_composer.rs b/crates/socket-patch-cli/tests/docker_e2e_composer.rs index c050905be..023669bb4 100644 --- a/crates/socket-patch-cli/tests/docker_e2e_composer.rs +++ b/crates/socket-patch-cli/tests/docker_e2e_composer.rs @@ -77,7 +77,9 @@ fn plain_sha256(content: &[u8]) -> String { /// This asserts on the *real structured output* of the run, not just a /// substring marker: /// - scan's JSON shows the monolog patch was discovered AND synced -/// (`"action": "added"`). NOTE: scan's process exit code is +/// (recorded in `.socket/manifest.json`; its `scan.json` record is +/// `added`, or `failed` with the error when scan's own in-place +/// apply fails, #424). NOTE: scan's process exit code is /// deliberately NOT gated — with a transitive dep that has no patch, /// scan reports `"status": "partial_failure"` / exit 1 even though /// the monolog patch is found and synced. Gating exit==0 would fail a @@ -98,8 +100,11 @@ fn verify_snippet() -> &'static str { # --- scan: must have discovered and synced the monolog patch --- grep -qF 'pkg:composer/monolog/monolog@3.5.0' /tmp/scan.json || { echo "FAIL: scan json missing monolog purl" >&2; cat /tmp/scan.json >&2; exit 1; } -grep -qF '"action": "added"' /tmp/scan.json || { - echo "FAIL: scan did not sync (add) the patch" >&2; cat /tmp/scan.json >&2; exit 1; } +# Synced = recorded in the manifest. The patch record in scan.json may say +# `added` or, when scan's own in-place apply step fails on this fixture, +# `failed` with the apply error (#424); either way the record must be saved. +grep -qF '"pkg:composer/monolog/monolog@3.5.0"' .socket/manifest.json || { + echo "FAIL: scan did not sync (record) the patch" >&2; cat /tmp/scan.json .socket/manifest.json >&2; exit 1; } # --- apply: must exit 0 and report a real applied+verified patch --- if [ "${APPLY_EXIT:-1}" != "0" ]; then diff --git a/crates/socket-patch-cli/tests/docker_e2e_gem.rs b/crates/socket-patch-cli/tests/docker_e2e_gem.rs index 039183d84..3109c8125 100644 --- a/crates/socket-patch-cli/tests/docker_e2e_gem.rs +++ b/crates/socket-patch-cli/tests/docker_e2e_gem.rs @@ -129,7 +129,9 @@ fn upstream_before_hash() -> String { /// This asserts on the *real structured output* of the run, not just a /// substring marker: /// - scan's JSON shows the colorize patch was discovered AND synced -/// (`"action": "added"`). NOTE: scan's process exit code is +/// (recorded in `.socket/manifest.json`; its `scan.json` record is +/// `added`, or `failed` with the error when scan's own in-place +/// apply fails, #424). NOTE: scan's process exit code is /// deliberately NOT gated — a non-zero scan exit from an unrelated /// transitive package without a patch must not fail a pipeline whose /// target patch was found and synced. @@ -144,8 +146,11 @@ fn verify_snippet() -> &'static str { # --- scan: must have discovered and synced the colorize patch --- grep -qF 'pkg:gem/colorize@1.1.0' /tmp/scan.json || { echo "FAIL: scan json missing colorize purl" >&2; cat /tmp/scan.json >&2; exit 1; } -grep -qF '"action": "added"' /tmp/scan.json || { - echo "FAIL: scan did not sync (add) the patch" >&2; cat /tmp/scan.json >&2; exit 1; } +# Synced = recorded in the manifest. The patch record in scan.json may say +# `added` or, when scan's own in-place apply step fails on this fixture, +# `failed` with the apply error (#424); either way the record must be saved. +grep -qF '"pkg:gem/colorize@1.1.0"' .socket/manifest.json || { + echo "FAIL: scan did not sync (record) the patch" >&2; cat /tmp/scan.json .socket/manifest.json >&2; exit 1; } # --- apply: must exit 0 and report a real applied+verified patch --- if [ "${APPLY_EXIT:-1}" != "0" ]; then diff --git a/crates/socket-patch-core/src/utils/digest.rs b/crates/socket-patch-core/src/utils/digest.rs index 630adefa1..105225e5e 100644 --- a/crates/socket-patch-core/src/utils/digest.rs +++ b/crates/socket-patch-core/src/utils/digest.rs @@ -135,6 +135,9 @@ mod tests { /// when you move it onto the helpers above; the test fails on a stale /// entry as well as on a new inline copy. const PENDING_INLINE_DIGESTS: &[&str] = &[ + "crawlers/gradle_cache.rs", + "patch/jvm_jar.rs", + "patch/sidecars/maven.rs", "utils/group_commit.rs", "vendor/jvm/mod.rs", "vendor/maven_repo.rs",