From ab48affa4a208d8641e3ae99bba55c442e223933 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 11 Sep 2026 14:33:02 -0700 Subject: [PATCH] Fix stale comparison images and raw evidence between gate attempts Clear generated comparison evidence before preflight and after partial failures. Keep unrelated files, avoid following region symlinks, and fail the gate when cleanup fails. AI assistance: implemented and validated with OpenAI Codex under maintainer direction. --- crates/comp-verbs/src/build_phase.rs | 43 ++++++++++- .../src/build_phase/integrity_tests.rs | 72 +++++++++++++++++++ docs/COMP-GATE-INTEGRITY.md | 7 +- 3 files changed, 120 insertions(+), 2 deletions(-) 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