diff --git a/crates/comp-verbs/src/build_phase.rs b/crates/comp-verbs/src/build_phase.rs index 27b6c9381..b830b5bdd 100644 --- a/crates/comp-verbs/src/build_phase.rs +++ b/crates/comp-verbs/src/build_phase.rs @@ -1110,16 +1110,67 @@ fn atomic_report(path: &Path, report: &Value) -> Result<(), String> { } fn unavailable_report(io: &Io, out_dir: &str, gate: &Gate, phase: &str) -> Result<(), String> { - let report = json!({ "interpretation": format!("{phase}-gate"), "measurementsAvailable": false, + let mut report = json!({ "interpretation": format!("{phase}-gate"), "measurementsAvailable": false, "regions": [], "gate": { "ok": false, "reasons": gate.reasons, "advisories": gate.advisories, "unscopedReasons": gate.reasons } }); // 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)); + if let Err(error) = &cleanup { + // A read-only regions directory may prevent unlinking its children even + // when its parent permits moving the directory. Preserve those bytes as + // explicitly invalid evidence instead of leaving them at current paths. + report["artifactCleanup"] = json!({"status":"failed", "error":error, + "invalidArtifacts":["raw-report.json", "side-by-side.png", "heatmap.png", "regions/"]}); + match quarantine_comparison_artifacts(&abs(io, out_dir)) { + Ok(record) => report["artifactCleanup"]["quarantine"] = record, + Err(e) => report["artifactCleanup"]["quarantineError"] = json!(e), + } + // If the filesystem also refuses quarantine, the report explicitly marks + // every potentially remaining artifact invalid. The gate still fails. + atomic_report(&abs(io, &format!("{out_dir}/report.json")), &report)?; + } report_write.and(cleanup) } +fn quarantine_comparison_artifacts(out_dir: &Path) -> Result { + static NEXT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); + let quarantine = loop { + let candidate = out_dir.join(format!("invalid-comparison-{}-{}", std::process::id(), NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed))); + match std::fs::create_dir(&candidate) { + Ok(()) => break candidate, + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(e.to_string()), + } + }; + let mut errors = Vec::new(); + let mut artifacts = Map::new(); + let prefix = quarantine.file_name().unwrap().to_string_lossy(); + for name in ["raw-report.json", "side-by-side.png", "heatmap.png", "regions"] { + let path = out_dir.join(name); + // Keep the same parent: moving a read-only directory into another parent + // can require write permission on that directory (to change its `..`). + let target = out_dir.join(format!("{prefix}-{name}")); + let move_artifact = (|| -> std::io::Result<()> { + match std::fs::symlink_metadata(&target) { + Ok(_) => return Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "quarantine target exists")), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => (), + Err(e) => return Err(e), + } + std::fs::rename(&path, &target) + })(); + match move_artifact { + Ok(()) => { artifacts.insert(name.into(), json!(target.to_string_lossy().replace('\\', "/"))); }, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => (), + Err(e) => errors.push(format!("{name}: {e}")), + } + } + let record = json!({"invalid":true, "artifacts":artifacts, "errors":errors}); + atomic_report(&quarantine.join("manifest.json"), &record)?; + Ok(record) +} + 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 diff --git a/crates/comp-verbs/src/build_phase/integrity_tests.rs b/crates/comp-verbs/src/build_phase/integrity_tests.rs index 13bcef665..81640825e 100644 --- a/crates/comp-verbs/src/build_phase/integrity_tests.rs +++ b/crates/comp-verbs/src/build_phase/integrity_tests.rs @@ -507,10 +507,15 @@ fn artifact_cleanup_failure_blocks_the_gate() { 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(); + if dir.exists() { 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); + assert_no_current_measurements(&ws); + let quarantine = ws.path.join(report["artifactCleanup"]["quarantine"]["artifacts"]["regions"].as_str().expect("cleanup failure must identify quarantined evidence")); + assert!(quarantine.join("retired.png").is_file()); + assert_eq!(report["artifactCleanup"]["quarantine"]["errors"], json!([])); + std::fs::set_permissions(quarantine, std::fs::Permissions::from_mode(0o755)).unwrap(); } diff --git a/docs/COMP-GATE-INTEGRITY.md b/docs/COMP-GATE-INTEGRITY.md index 0c2e3bdd6..e1ae11678 100644 --- a/docs/COMP-GATE-INTEGRITY.md +++ b/docs/COMP-GATE-INTEGRITY.md @@ -40,6 +40,13 @@ 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. +When removal fails, the remaining generated set is renamed beside its current paths under a unique +`invalid-comparison-*` prefix. `artifactCleanup` and a quarantine manifest record +the moved paths. Keeping the same parent avoids requiring write access to the +read-only directory itself. +If filesystem permissions also prevent quarantine, that field records the error +and explicitly lists the invalid artifact paths; no evidence is certified and the +gate remains closed. Write failures block the gate and, when the report location is writable, publish an unavailable-evidence report.