diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index ae9598e32..812807f3a 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -914,8 +914,9 @@ fn format_save_summary( /// The summary after a single-uuid save. `what` is `"Patch"` or `"Patch /// record"`. `ends_run` says an unchanged record really ends the run (the -/// agent path skips apply); the vendored path still runs its vendor step, -/// so it must not promise "nothing to update". +/// agent path under `--save-only`); the agent path still re-applies an +/// unchanged record and the vendored path still runs its vendor step, so +/// neither may promise "nothing to update". fn format_single_save( what: &str, action: &PatchAction, @@ -1653,6 +1654,10 @@ struct FetchBatch { /// Selection size after installed-release narrowing. found: usize, skipped: usize, + /// Manifest store only: the `skipped` patches whose same uuid is + /// already recorded — still owed a nested apply, since the installed + /// copy may have been reinstalled since the record was written. + already_recorded: usize, failed: usize, /// Fetched, recordable patches in selection order. fetched: Vec, @@ -1843,6 +1848,7 @@ async fn fetch_selected_patches( let mut batch = FetchBatch { found: selected.len(), skipped: 0, + already_recorded: 0, failed: 0, fetched: Vec::new(), reused: Vec::new(), @@ -1997,6 +2003,7 @@ async fn fetch_selected_patches( "action": "skipped", })); batch.skipped += 1; + batch.already_recorded += 1; continue; } @@ -2432,10 +2439,16 @@ pub async fn download_and_apply_patches_with( return (1, serde_json::json!({ "status": "error", "error": msg })); } } + // Every selected patch that is now recorded is owed the nested apply: + // the fetched ones AND the already-recorded (`skipped`) ones, whose + // installed copy may be pristine again after a reinstall or a failed + // earlier apply (#454). Apply is idempotent on already-patched files, + // so an in-sync re-run stays a no-op on disk. + let to_apply = downloaded + batch.already_recorded; // The lock outlives the manifest write only when a nested apply follows // (it is handed the guard and releases it after its last mutation); // otherwise nothing more is written and it is released here. - let apply_lock = if !params.save_only && downloaded > 0 { + let apply_lock = if !params.save_only && to_apply > 0 { Some(guard) } else { drop(guard); @@ -2476,13 +2489,13 @@ pub async fn download_and_apply_patches_with( .await; } - // An apply step that ran (patches were added, not --save-only) but - // failed is a partial failure too — not just download failures. The + // An apply step that ran (recorded patches selected, not --save-only) + // but failed is a partial failure too — not just download failures. The // `status` field must agree with `exit_code`; reporting `success` // alongside a non-zero exit code misleads JSON consumers (the scan // wrapper recomputes status from the exit code for exactly this // reason, but `get` surfaces this envelope directly). - let apply_failed = !apply_succeeded && downloaded > 0 && !params.save_only; + let apply_failed = !apply_succeeded && to_apply > 0 && !params.save_only; let (status, exit_code) = run_outcome(batch.failed > 0, apply_failed); let mut result_json = serde_json::json!({ "status": status, @@ -2490,7 +2503,7 @@ pub async fn download_and_apply_patches_with( "downloaded": downloaded, "skipped": batch.skipped, "failed": batch.failed, - "applied": if apply_succeeded { downloaded } else { 0 }, + "applied": if apply_succeeded { to_apply } else { 0 }, "updated": updated, "patches": batch.patches_json, }); @@ -3390,9 +3403,12 @@ async fn save_and_apply_patch(args: &GetArgs, client: &ApiClient, patch: &PatchR Err(code) => return code, }; let changed = action != PatchAction::Skipped; - // Carried into the nested apply when one follows (it releases the lock - // after its last mutation), released here otherwise. - let apply_lock = if !args.save_only && changed { + // The record is now in the manifest whatever `action` says, so the + // nested apply follows unless `--save-only`: a same-uuid re-get must + // still reconcile an installed copy that was reinstalled pristine since + // it was recorded (#454). Carried into the nested apply (it releases + // the lock after its last mutation), released here otherwise. + let apply_lock = if !args.save_only { Some(guard) } else { drop(guard); @@ -3427,7 +3443,14 @@ async fn save_and_apply_patch(args: &GetArgs, client: &ApiClient, patch: &PatchR if !quiet { eprintln!( "{}", - format_single_save("Patch", &action, &manifest_path, &patch.purl, true) + format_single_save( + "Patch", + &action, + &manifest_path, + &patch.purl, + // An unchanged record ends the run only when no apply follows. + apply_lock.is_none() + ) ); } @@ -3446,11 +3469,11 @@ async fn save_and_apply_patch(args: &GetArgs, client: &ApiClient, patch: &PatchR .await; } - // The apply step ran (patch added, not --save-only) but failed → + // The apply step ran (not --save-only) but failed → // partial failure. The `status` field must agree with the exit code // returned below; a hardcoded `success` alongside a non-zero exit // misleads JSON consumers. - let apply_failed = !apply_succeeded && changed && !args.save_only; + let apply_failed = !apply_succeeded && !args.save_only; // No "download failed" concept here — a blob failure early-returns // with status `error` above — so only the apply step can degrade us. let (status, exit_code) = run_outcome(false, apply_failed); diff --git a/crates/socket-patch-cli/tests/in_process_agent_reapply.rs b/crates/socket-patch-cli/tests/in_process_agent_reapply.rs new file mode 100644 index 000000000..23a9f0c25 --- /dev/null +++ b/crates/socket-patch-cli/tests/in_process_agent_reapply.rs @@ -0,0 +1,317 @@ +//! Agent-mode `scan` / `get` must reconcile a patch that is already +//! recorded in `.socket/manifest.json` against the installed tree (#454). +//! +//! The first run records and applies the patch. Then the package is +//! "reinstalled" (the pristine file comes back, as after `hatch env +//! create`, `pip install --force-reinstall` or a CI cache miss). The +//! re-run finds the same uuid already in the manifest (`skipped`) and must +//! still run the nested apply. Before the fix it exited 0 with the package +//! unpatched. + +use std::path::Path; + +use serial_test::serial; +use sha2::{Digest, Sha256}; +use socket_patch_cli::commands::get::{run as get_run, GetArgs}; +use socket_patch_cli::commands::scan::{run as scan_run, ScanArgs, ScanMode}; +use wiremock::matchers::{method, path, path_regex}; +use wiremock::{Mock, MockServer, ResponseTemplate}; + +const ORG: &str = "test-org"; +const NAME: &str = "agent-reapply"; +const VERSION: &str = "1.0.0"; +const PURL: &str = "pkg:npm/agent-reapply@1.0.0"; +const UUID: &str = "45445445-4544-4544-8544-454454454454"; +const ORIGINAL: &[u8] = b"module.exports = 'original';\n"; +const PATCHED: &[u8] = b"module.exports = 'patched';\n"; + +fn git_sha256(content: &[u8]) -> String { + let mut hasher = Sha256::new(); + hasher.update(format!("blob {}\0", content.len()).as_bytes()); + hasher.update(content); + hex::encode(hasher.finalize()) +} + +fn common(cwd: &Path, server: &MockServer) -> socket_patch_cli::args::GlobalArgs { + socket_patch_cli::args::GlobalArgs { + cwd: cwd.to_path_buf(), + org: Some(ORG.to_string()), + json: true, + yes: true, + api_token: Some("fake".to_string()), + api_url: Some(server.uri()), + download_mode: "diff".to_string(), + ..socket_patch_cli::args::GlobalArgs::default() + } +} + +fn scan_args(cwd: &Path, server: &MockServer) -> ScanArgs { + ScanArgs { + socket_yml: Default::default(), + paths: Vec::new(), + packages: Vec::new(), + common: common(cwd, server), + batch_size: Some(100), + apply: false, + prune: false, + sync: false, + vendor: false, + mode: None, + all_releases: false, + vex: Default::default(), + rollout: Default::default(), + } +} + +fn agent_get_args(cwd: &Path, server: &MockServer) -> GetArgs { + GetArgs { + common: common(cwd, server), + identifier: UUID.to_string(), + id: true, + cve: false, + ghsa: false, + package: false, + save_only: false, + all_releases: false, + mode: Some(ScanMode::Agent), + } +} + +fn index_js(root: &Path) -> std::path::PathBuf { + root.join("node_modules").join(NAME).join("index.js") +} + +/// Lay down (or re-lay, simulating a reinstall) the pristine package. +fn install(root: &Path) { + std::fs::write( + root.join("package.json"), + r#"{ "name": "agent-reapply-test", "version": "0.0.0" }"#, + ) + .unwrap(); + let pkg = root.join("node_modules").join(NAME); + std::fs::create_dir_all(&pkg).unwrap(); + std::fs::write( + pkg.join("package.json"), + format!(r#"{{ "name": "{NAME}", "version": "{VERSION}" }}"#), + ) + .unwrap(); + std::fs::write(index_js(root), ORIGINAL).unwrap(); +} + +async fn mock_api(server: &MockServer) { + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "packages": [{ + "purl": PURL, + "patches": [{ + "uuid": UUID, "purl": PURL, + "tier": "free", "cveIds": [], "ghsaIds": [], + "severity": "high", "title": "agent re-apply fixture" + }] + }], + "canAccessPaidPatches": false, + }))) + .mount(server) + .await; + Mock::given(method("GET")) + .and(path_regex(format!( + "^/v0/orgs/{ORG}/patches/by-package/.+$" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "patches": [{ + "uuid": UUID, "purl": PURL, + "publishedAt": "2024-01-01T00:00:00Z", + "description": "x", "license": "MIT", "tier": "free", + "vulnerabilities": {} + }], + "canAccessPaidPatches": false, + }))) + .mount(server) + .await; + use base64::Engine; + Mock::given(method("GET")) + .and(path(format!("/v0/orgs/{ORG}/patches/view/{UUID}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "uuid": UUID, + "purl": PURL, + "publishedAt": "2024-01-01T00:00:00Z", + "files": { + "package/index.js": { + "beforeHash": git_sha256(ORIGINAL), + "afterHash": git_sha256(PATCHED), + "blobContent": base64::engine::general_purpose::STANDARD.encode(PATCHED), + } + }, + "vulnerabilities": {}, + "description": "x", "license": "MIT", "tier": "free", + }))) + .mount(server) + .await; +} + +fn recorded_patch_id(root: &Path) -> Option { + let text = std::fs::read_to_string(root.join(".socket/manifest.json")).ok()?; + let manifest: serde_json::Value = serde_json::from_str(&text).ok()?; + manifest["patches"][PURL]["uuid"] + .as_str() + .map(str::to_owned) +} + +async fn scan(args: ScanArgs) -> i32 { + // Same scrub as in_process_scan.rs: an ambient venv would add purls. + std::env::remove_var("VIRTUAL_ENV"); + scan_run(args).await +} + +/// Record + apply once, then reinstall the pristine package. +async fn first_run_then_reinstall(root: &Path, server: &MockServer) { + install(root); + let mut args = scan_args(root, server); + args.mode = Some(ScanMode::Agent); + assert_eq!(scan(args).await, 0, "first agent scan must apply cleanly"); + assert_eq!(std::fs::read(index_js(root)).unwrap(), PATCHED); + assert_eq!(recorded_patch_id(root).as_deref(), Some(UUID)); + install(root); + assert_eq!(std::fs::read(index_js(root)).unwrap(), ORIGINAL); +} + +#[tokio::test] +#[serial] +async fn agent_scan_reapplies_already_recorded_patch_after_reinstall() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + first_run_then_reinstall(tmp.path(), &server).await; + + let mut args = scan_args(tmp.path(), &server); + args.mode = Some(ScanMode::Agent); + assert_eq!(scan(args).await, 0); + assert_eq!( + std::fs::read(index_js(tmp.path())).unwrap(), + PATCHED, + "a re-scan must re-apply the recorded patch to the reinstalled package" + ); + assert_eq!(recorded_patch_id(tmp.path()).as_deref(), Some(UUID)); +} + +#[tokio::test] +#[serial] +async fn scan_sync_reapplies_already_recorded_patch_after_reinstall() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + first_run_then_reinstall(tmp.path(), &server).await; + + let mut args = scan_args(tmp.path(), &server); + args.sync = true; + assert_eq!(scan(args).await, 0); + assert_eq!( + std::fs::read(index_js(tmp.path())).unwrap(), + PATCHED, + "scan --sync must end fully reconciled" + ); +} + +#[tokio::test] +#[serial] +async fn agent_scan_in_sync_rerun_is_a_clean_noop() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + install(tmp.path()); + let mut args = scan_args(tmp.path(), &server); + args.mode = Some(ScanMode::Agent); + assert_eq!(scan(args).await, 0); + let manifest_before = std::fs::read(tmp.path().join(".socket/manifest.json")).unwrap(); + + // Nothing reinstalled: the re-run still succeeds and changes nothing. + let mut args = scan_args(tmp.path(), &server); + args.mode = Some(ScanMode::Agent); + assert_eq!(scan(args).await, 0); + assert_eq!(std::fs::read(index_js(tmp.path())).unwrap(), PATCHED); + assert_eq!( + std::fs::read(tmp.path().join(".socket/manifest.json")).unwrap(), + manifest_before, + "an all-skipped run must not rewrite the manifest" + ); +} + +#[tokio::test] +#[serial] +async fn agent_scan_rerun_fails_when_recorded_patch_cannot_be_applied() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + first_run_then_reinstall(tmp.path(), &server).await; + // A different upstream build: neither the before nor the after hash, + // which `--strict` refuses to overwrite. + let other = b"module.exports = 'other';\n"; + std::fs::write(index_js(tmp.path()), other).unwrap(); + + let mut args = scan_args(tmp.path(), &server); + args.mode = Some(ScanMode::Agent); + args.common.strict = true; + assert_eq!( + scan(args).await, + 1, + "an all-skipped run whose apply fails must not report success" + ); + assert_eq!(std::fs::read(index_js(tmp.path())).unwrap(), other); +} + +#[tokio::test] +#[serial] +async fn get_uuid_agent_reapplies_already_recorded_patch_after_reinstall() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + install(tmp.path()); + assert_eq!(get_run(agent_get_args(tmp.path(), &server)).await, 0); + assert_eq!(std::fs::read(index_js(tmp.path())).unwrap(), PATCHED); + + install(tmp.path()); + assert_eq!(get_run(agent_get_args(tmp.path(), &server)).await, 0); + assert_eq!( + std::fs::read(index_js(tmp.path())).unwrap(), + PATCHED, + "get of an already-recorded patch must re-apply it" + ); +} + +#[tokio::test] +#[serial] +async fn get_purl_agent_reapplies_already_recorded_patch_after_reinstall() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + first_run_then_reinstall(tmp.path(), &server).await; + + let mut args = agent_get_args(tmp.path(), &server); + args.identifier = PURL.to_string(); + args.id = false; + assert_eq!(get_run(args).await, 0); + assert_eq!( + std::fs::read(index_js(tmp.path())).unwrap(), + PATCHED, + "get of an already-recorded patch must re-apply it" + ); +} + +#[tokio::test] +#[serial] +async fn get_save_only_of_recorded_patch_still_does_not_apply() { + let server = MockServer::start().await; + mock_api(&server).await; + let tmp = tempfile::tempdir().unwrap(); + first_run_then_reinstall(tmp.path(), &server).await; + + let mut args = agent_get_args(tmp.path(), &server); + args.save_only = true; + assert_eq!(get_run(args).await, 0); + assert_eq!( + std::fs::read(index_js(tmp.path())).unwrap(), + ORIGINAL, + "--save-only keeps its record-only intent" + ); +} diff --git a/crates/socket-patch-cli/tests/scan/scan_sync_e2e.rs b/crates/socket-patch-cli/tests/scan/scan_sync_e2e.rs index 8dccae2cb..b5065ebef 100644 --- a/crates/socket-patch-cli/tests/scan/scan_sync_e2e.rs +++ b/crates/socket-patch-cli/tests/scan/scan_sync_e2e.rs @@ -386,9 +386,10 @@ async fn scan_apply_with_existing_blob_uses_local_cache() { assert_eq!(v["status"], "success", "envelope={v}"); // The pre-staged manifest already carries this exact UUID, so the patch - // MUST be classified `skipped` (not re-applied / re-added). Nothing in - // the original test verified this — exit 0 alone would also hold if the - // patch were wrongly re-applied. + // MUST be classified `skipped` (not re-downloaded / re-added), yet the + // installed copy is still pristine, so the nested apply must reconcile it + // from the cached blob (#454: a recorded-but-unapplied patch used to be + // left unpatched with exit 0). let apply = v["apply"] .as_object() .unwrap_or_else(|| panic!("scan --apply must emit an apply sub-object; envelope={v}")); @@ -398,8 +399,8 @@ async fn scan_apply_with_existing_blob_uses_local_cache() { "patch must be skipped; apply={apply:?}" ); assert_eq!( - apply["applied"], 0, - "nothing applied on a skip; apply={apply:?}" + apply["applied"], 1, + "the recorded patch is applied to the pristine install; apply={apply:?}" ); assert_eq!(apply["failed"], 0, "apply.failed; apply={apply:?}"); // The defining claim of this test ("skip the blob download / use the cached @@ -424,8 +425,8 @@ async fn scan_apply_with_existing_blob_uses_local_cache() { patches[0] ); - // A skip must NOT touch the file: index.js stays at its original - // ("before") content (the patch was never re-applied). + // The skipped record is still applied: index.js now holds the cached + // blob's ("after") content, with no blob download (asserted above). let on_disk = std::fs::read( tmp.path() .join("node_modules") @@ -434,8 +435,8 @@ async fn scan_apply_with_existing_blob_uses_local_cache() { ) .expect("index.js must exist"); assert_eq!( - on_disk, before, - "skipped patch must leave the file untouched" + on_disk, after, + "a recorded patch must be applied to the pristine install from the cache" ); // The pre-staged cached blob must still be present and unchanged.