diff --git a/crates/comp-verbs/src/build_phase.rs b/crates/comp-verbs/src/build_phase.rs index acb6b1086..27b6c9381 100644 --- a/crates/comp-verbs/src/build_phase.rs +++ b/crates/comp-verbs/src/build_phase.rs @@ -1113,7 +1113,48 @@ fn unavailable_report(io: &Io, out_dir: &str, gate: &Gate, phase: &str) -> Resul let report = json!({ "interpretation": format!("{phase}-gate"), "measurementsAvailable": false, "regions": [], "gate": { "ok": false, "reasons": gate.reasons, "advisories": gate.advisories, "unscopedReasons": gate.reasons } }); - atomic_report(&abs(io, &format!("{out_dir}/report.json")), &report) + // Invalidate the report before touching images; also clean partial writes on failure. + // Attempt cleanup even when the report itself cannot be replaced. + let report_write = atomic_report(&abs(io, &format!("{out_dir}/report.json")), &report); + let cleanup = clear_comparison_artifacts(&abs(io, out_dir)); + report_write.and(cleanup) +} + +fn clear_comparison_artifacts(out_dir: &Path) -> Result<(), String> { + // Only remove generated files. Never recursively delete a caller's output directory + // or follow a regions symlink into another directory. Unexpected directories at + // file paths remain obstructions: the subsequent writer must still fail closed. + fn remove_file(path: &Path) -> std::io::Result<()> { + match std::fs::symlink_metadata(path) { + Ok(meta) if !meta.is_dir() => std::fs::remove_file(path), + Ok(_) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(e), + } + } + let mut errors = Vec::new(); + for name in ["raw-report.json", "side-by-side.png", "heatmap.png"] { + if let Err(e) = remove_file(&out_dir.join(name)) { errors.push(format!("{name}: {e}")); } + } + fn clear_regions(path: &Path, errors: &mut Vec) -> std::io::Result<()> { + let meta = match std::fs::symlink_metadata(path) { + Ok(meta) => meta, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; + if meta.file_type().is_symlink() { return std::fs::remove_file(path); } + if meta.is_dir() { + for entry in std::fs::read_dir(path)? { + let child = entry?.path(); + if let Err(e) = clear_regions(&child, errors) { errors.push(format!("{}: {e}", child.display())); } + } + } else if path.extension().and_then(|ext| ext.to_str()) == Some("png") { + std::fs::remove_file(path)?; + } + Ok(()) + } + if let Err(e) = clear_regions(&out_dir.join("regions"), &mut errors) { errors.push(format!("regions: {e}")); } + if errors.is_empty() { Ok(()) } else { Err(format!("cannot clear comparison artifacts: {}", errors.join("; "))) } } #[allow(clippy::too_many_arguments)] diff --git a/crates/comp-verbs/src/build_phase/integrity_tests.rs b/crates/comp-verbs/src/build_phase/integrity_tests.rs index 1c3b9ef23..13bcef665 100644 --- a/crates/comp-verbs/src/build_phase/integrity_tests.rs +++ b/crates/comp-verbs/src/build_phase/integrity_tests.rs @@ -329,6 +329,7 @@ fn failed_evidence_writes_cannot_publish_success() { &no_organic_scan, ); assert!(!g.ok, "write failure must block: {blocked_file}"); + assert_no_current_measurements(&ws); let report: Value = serde_json::from_slice(&std::fs::read(ws.path.join("diff/report.json")).unwrap()) .unwrap(); @@ -433,6 +434,7 @@ fn responsive_failures_replace_previous_success_evidence() { } let bad = gate_responsive(&ws.io(), &mut state, RESPONSIVE_MIN, "diff"); assert!(!bad.ok); + assert_no_current_measurements(&ws); let report: Value = serde_json::from_slice(&std::fs::read(ws.path.join("diff/report.json")).unwrap()) .unwrap(); @@ -442,3 +444,73 @@ fn responsive_failures_replace_previous_success_evidence() { assert_eq!(report["gate"]["reasons"], json!(bad.reasons)); } } + +fn assert_no_current_measurements(ws: &Workspace) { + for file in ["raw-report.json", "side-by-side.png", "heatmap.png", "regions/button.png", "regions/retired.png", "regions/nested/retired.png"] { + assert!(!ws.path.join("diff").join(file).is_file(), "stale evidence: {file}"); + } +} + +#[test] +fn failed_preflight_clears_complete_evidence_for_both_gates() { + for responsive in [false, true] { + let (ws, mut state) = simple_hero_workspace(); + let image = std::fs::read(ws.path.join("comp.png")).unwrap(); + ws.write(".impeccable/review/desktop.png", &image); + ws.write(".impeccable/review/mobile.png", &image); + let run = |state: &mut Value| if responsive { + gate_responsive(&ws.io(), state, RESPONSIVE_MIN, "diff") + } else { + gate_hero(&ws.io(), state, "comp.png", HERO_MIN, "diff", Some("index.html"), &no_organic_scan) + }; + assert!(run(&mut state).ok); + ws.write("diff/regions/retired.png", &image); + ws.write("diff/regions/nested/retired.png", &image); + ws.write("diff/notes.txt", b"keep unrelated files"); + std::fs::remove_file(ws.path.join("comp.png")).unwrap(); + assert!(!run(&mut state).ok); + assert_no_current_measurements(&ws); + assert_eq!(std::fs::read(ws.path.join("diff/notes.txt")).unwrap(), b"keep unrelated files"); + } +} + +#[test] +fn successful_repeat_removes_retired_region_crops() { + let (ws, mut state) = simple_hero_workspace(); + ws.write("diff/regions/retired.png", b"old crop"); + let gate = gate_hero(&ws.io(), &mut state, "comp.png", HERO_MIN, "diff", Some("index.html"), &no_organic_scan); + assert!(gate.ok, "{:?}", gate.reasons); + assert!(!ws.path.join("diff/regions/retired.png").exists()); + assert!(ws.path.join("diff/regions/button.png").is_file()); +} + +#[cfg(unix)] +#[test] +fn artifact_cleanup_does_not_follow_region_directory_symlinks() { + let (ws, mut state) = simple_hero_workspace(); + ws.write("elsewhere/keep.png", b"unrelated image"); + std::fs::create_dir_all(ws.path.join("diff")).unwrap(); + std::os::unix::fs::symlink(ws.path.join("elsewhere"), ws.path.join("diff/regions")).unwrap(); + let gate = gate_hero(&ws.io(), &mut state, "comp.png", HERO_MIN, "diff", Some("index.html"), &no_organic_scan); + assert!(gate.ok, "{:?}", gate.reasons); + assert_eq!(std::fs::read(ws.path.join("elsewhere/keep.png")).unwrap(), b"unrelated image"); + assert!(!ws.path.join("elsewhere/button.png").exists()); + assert!(ws.path.join("diff/regions/button.png").is_file()); +} + +#[cfg(unix)] +#[test] +fn artifact_cleanup_failure_blocks_the_gate() { + use std::os::unix::fs::PermissionsExt; + let (ws, mut state) = simple_hero_workspace(); + ws.write("diff/regions/retired.png", b"stale crop"); + let dir = ws.path.join("diff/regions"); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o555)).unwrap(); + let gate = gate_hero(&ws.io(), &mut state, "comp.png", HERO_MIN, "diff", Some("index.html"), &no_organic_scan); + std::fs::set_permissions(&dir, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(!gate.ok); + assert!(gate.reasons.iter().any(|r| r.contains("cannot clear comparison artifacts")), "{:?}", gate.reasons); + let report: Value = serde_json::from_slice(&std::fs::read(ws.path.join("diff/report.json")).unwrap()).unwrap(); + assert_eq!(report["measurementsAvailable"], false); + assert_eq!(report["gate"]["ok"], false); +} diff --git a/docs/COMP-GATE-INTEGRITY.md b/docs/COMP-GATE-INTEGRITY.md index 46be43e6c..0c2e3bdd6 100644 --- a/docs/COMP-GATE-INTEGRITY.md +++ b/docs/COMP-GATE-INTEGRITY.md @@ -35,7 +35,12 @@ gate reasons. It does not present the preceding capture as current evidence. Repair crops include only regions with current blockers; shared readings retain all affected region IDs. Frame-wide blockers keep their whole-frame evidence. Both hero and responsive reports are marked incomplete before preflight and -image writes, then committed atomically only after the evidence files succeed. Write failures block the gate +image writes, then committed atomically only after the evidence files succeed. +Each attempt clears the previous raw report, whole-frame PNGs, and region PNGs; +failed attempts also clear any partial evidence they wrote. Removed regions cannot +leave old crops in a successful comparison. Cleanup is limited to generated files, +does not follow region-directory symlinks, and blocks the gate if it fails. +Write failures block the gate and, when the report location is writable, publish an unavailable-evidence report. Stall feedback follows repeated blocking reasons. It never chooses an asset to