From f20e0fc3384d025a6ea8eda2a078475b31f4dd37 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 11:26:40 -0400 Subject: [PATCH] feat: require validation failure history in review reports (Closes #396) Reviewer runs that observe a validation failure and later rerun to green must report the failure and its investigation; a later pass never erases the earlier failure: - New reviewer_validation_failure_history.py verifier: reports must carry a Validation failure history section naming every observed failure with command, failing test or error, suspected cause, reproduced status, baseline master status, and PR-caused evidence; rerun-pass claims must explain what changed between runs, whether environmental state (such as stale /tmp files) was cleaned, and why the pass is trustworthy; an unexplained transient failure cannot be reported as a bare pass and must use the status 'passed after transient failure investigation' (exported as TRANSIENT_INVESTIGATION_STATUS). - Re-export from review_proofs and wire into build_final_report as a downgrade check. - Review-merge final-report schema gains the Validation failure history template section; contract test locks the markers and an export test locks the re-export. - 12 tests covering the issue's scenarios: failing-then-passing rerun, baseline-equivalent failure, /tmp state contamination, unexplained transient failure, and a report omitting an earlier failure. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 19 ++ reviewer_validation_failure_history.py | 109 +++++++++ .../schemas/review-merge-final-report.md | 25 ++- tests/test_llm_workflow_split.py | 8 + ...est_reviewer_validation_failure_history.py | 207 ++++++++++++++++++ 5 files changed, 367 insertions(+), 1 deletion(-) create mode 100644 reviewer_validation_failure_history.py create mode 100644 tests/test_reviewer_validation_failure_history.py diff --git a/review_proofs.py b/review_proofs.py index ac479d9..fcc7008 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1735,6 +1735,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, if report_text else {"proven": True, "block": False, "reasons": []} ) + validation_failure_history = ( + assess_validation_failure_history_report(report_text) + if report_text + else {"proven": True, "block": False, "reasons": []} + ) if baseline_validation is None and report_text: baseline_validation = assess_reviewer_baseline_validation_proof( report_text=report_text, @@ -1932,6 +1937,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "inventory pagination or worktree fields inconsistent (#293)" ) downgrade_reasons.extend(inventory_worktree.get("reasons", [])) + if not validation_failure_history.get("proven"): + downgrade_reasons.append( + "validation failure history incomplete or omitted (#396)" + ) + downgrade_reasons.extend(validation_failure_history.get("reasons", [])) if not baseline_validation.get("proven"): downgrade_reasons.append( "reviewer baseline validation proof missing or failed (#325)" @@ -5425,6 +5435,15 @@ def assess_inventory_worktree_report(report_text, **kwargs): return _assess(report_text, **kwargs) +def assess_validation_failure_history_report(report_text, **kwargs): + """#396: observed validation failures must be reported with investigation.""" + from reviewer_validation_failure_history import ( + assess_validation_failure_history_report as _assess, + ) + + return _assess(report_text, **kwargs) + + def assess_infra_stop_handoff_report(report_text, **kwargs): """#289: repair handoff purity when infra_stop blocks reviewer capability.""" from reviewer_infra_stop_handoff import assess_infra_stop_handoff_report as _assess diff --git a/reviewer_validation_failure_history.py b/reviewer_validation_failure_history.py new file mode 100644 index 0000000..74bb144 --- /dev/null +++ b/reviewer_validation_failure_history.py @@ -0,0 +1,109 @@ +"""Validation failure history verifier for reviewer reports (#396). + +A reviewer run that observes a validation failure and later reruns to green +must report the failure, its investigation, and why the eventual pass is +trustworthy. A later pass never erases the duty to report the earlier +failure. This verifier fails closed when known failures are omitted, when +failure entries lack the required investigation fields, when a rerun-pass +claim lacks a what-changed explanation, or when an unexplained transient +failure is reported as a bare pass. +""" + +from __future__ import annotations + +import re +from typing import Any + +# Issue #396 criterion 4: required status wording when the cause is unknown. +TRANSIENT_INVESTIGATION_STATUS = "passed after transient failure investigation" + +_HISTORY_SECTION_RE = re.compile( + r"(?:^#+\s*validation failure history\b|validation failure history\s*:)", + re.IGNORECASE | re.MULTILINE, +) +_FAILURE_ENTRY_FIELDS = ( + ("command", re.compile(r"command\s*:", re.IGNORECASE)), + ("failing test or error", re.compile(r"(?:failing test|error)\s*:", re.IGNORECASE)), + ("suspected cause", re.compile(r"suspected cause\s*:", re.IGNORECASE)), + ("reproduced status", re.compile(r"reproduced\s*:", re.IGNORECASE)), + ("baseline master status", re.compile(r"baseline(?:\s+master)?\s*:", re.IGNORECASE)), + ("PR-caused evidence", re.compile(r"(?:pr-caused|not pr-caused|evidence)", re.IGNORECASE)), +) +_RERUN_PASS_RE = re.compile( + r"(?:passed on rerun|rerun[^.\n]*passed|later (?:run|rerun) passed)", + re.IGNORECASE, +) +_WHAT_CHANGED_RE = re.compile(r"what changed", re.IGNORECASE) +_ENV_STATE_RE = re.compile( + r"(?:environmental state|/tmp/|state (?:was )?cleaned|none cleaned)", + re.IGNORECASE, +) +_TRUST_RE = re.compile(r"(?:trustworthy|trust the pass|why the pass)", re.IGNORECASE) +_UNKNOWN_CAUSE_RE = re.compile(r"suspected cause\s*:\s*unknown", re.IGNORECASE) +_BARE_PASSED_STATUS_RE = re.compile( + r"validation status\s*:\s*passed\s*$", + re.IGNORECASE | re.MULTILINE, +) + + +def assess_validation_failure_history_report( + report_text: str, + *, + observed_failures: list[dict] | None = None, +) -> dict[str, Any]: + """Require complete failure-history reporting for observed failures (#396).""" + text = report_text or "" + reasons: list[str] = [] + has_history = bool(_HISTORY_SECTION_RE.search(text)) + + if observed_failures and not has_history: + reasons.append( + "validation failures were observed this session but the report has " + "no Validation failure history section" + ) + + if observed_failures and has_history: + for entry in observed_failures: + failure = str(entry.get("failure") or "").strip() + if failure and failure not in text: + reasons.append( + f"observed validation failure omitted from history: {failure}" + ) + + missing_fields = [ + name for name, pattern in _FAILURE_ENTRY_FIELDS if not pattern.search(text) + ] + for name in missing_fields: + reasons.append(f"failure history entry missing required field: {name}") + + if has_history and _RERUN_PASS_RE.search(text): + if not _WHAT_CHANGED_RE.search(text): + reasons.append( + "rerun-pass claim does not explain what changed between runs" + ) + if not _ENV_STATE_RE.search(text): + reasons.append( + "rerun-pass claim does not state whether environmental state " + "was cleaned" + ) + if not _TRUST_RE.search(text): + reasons.append( + "rerun-pass claim does not explain why the pass is trustworthy" + ) + + if ( + observed_failures + and _UNKNOWN_CAUSE_RE.search(text) + and _BARE_PASSED_STATUS_RE.search(text) + ): + reasons.append( + "unexplained transient failure cannot be reported as a bare pass; " + f"use '{TRANSIENT_INVESTIGATION_STATUS}'" + ) + + return { + "proven": not reasons, + "block": bool(reasons), + "reasons": reasons, + "history_section_present": has_history, + } diff --git a/skills/llm-project-workflow/schemas/review-merge-final-report.md b/skills/llm-project-workflow/schemas/review-merge-final-report.md index 08c094b..a2ca8e3 100644 --- a/skills/llm-project-workflow/schemas/review-merge-final-report.md +++ b/skills/llm-project-workflow/schemas/review-merge-final-report.md @@ -91,4 +91,27 @@ Verifier: `review_proofs.assess_queue_status_report()`. Narrative final report and controller handoff must agree on eligibility class, candidate/reviewed head SHA, mutation state, worktree usage, review decision, -terminal review mutation, merge result, and linked issue status. \ No newline at end of file +terminal review mutation, merge result, and linked issue status. + +### Validation failure history (#396) + +Every validation failure observed during the run must appear here, even if a +later rerun passed. A later pass never erases the earlier failure. + +For each observed failure report: + +- Command: +- Failing test or error: +- Suspected cause: +- Reproduced: +- Baseline master: +- PR-caused evidence: + +If a later rerun passed, also report what changed between runs, whether any +environmental state (for example stale `/tmp` files) was cleaned, and why the +pass is trustworthy. + +If the cause is unknown, the validation status must be +`passed after transient failure investigation` — never a bare `passed`. + +Verifier: `review_proofs.assess_validation_failure_history_report()`. \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 72c5515..29974a3 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -74,6 +74,14 @@ def test_review_merge_final_report_schema_exists(): assert "Candidate head SHA:" in text assert "Terminal review mutation:" in text assert "Workspace mutations" in text + assert "### Validation failure history (#396)" in text + assert "passed after transient failure investigation" in text + + +def test_validation_failure_history_verifier_exported(): + from review_proofs import assess_validation_failure_history_report + + assert callable(assess_validation_failure_history_report) def test_reconcile_landed_workflow_contract(): diff --git a/tests/test_reviewer_validation_failure_history.py b/tests/test_reviewer_validation_failure_history.py new file mode 100644 index 0000000..f5a4483 --- /dev/null +++ b/tests/test_reviewer_validation_failure_history.py @@ -0,0 +1,207 @@ +"""Tests for validation failure history reporting (#396).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from reviewer_validation_failure_history import ( # noqa: E402 + TRANSIENT_INVESTIGATION_STATUS, + assess_validation_failure_history_report, +) + +_OBSERVED = [ + { + "command": "venv/bin/pytest tests/ -q", + "failure": "tests/test_worktrees.py::TestWorktreeStart::test_rejects_untraceable_branches", + } +] + + +def _history_block(**overrides): + lines = { + "heading": "## Validation failure history", + "command": "- Command: venv/bin/pytest tests/ -q in branches/review-pr381", + "failure": ( + "- Failing test: tests/test_worktrees.py::TestWorktreeStart::" + "test_rejects_untraceable_branches" + ), + "cause": "- Suspected cause: stale /tmp/gitea_issue_lock.json from a concurrent session", + "reproduced": "- Reproduced: no (single occurrence, not reproducible in isolation)", + "baseline": "- Baseline master: does not fail on clean baseline", + "evidence": ( + "- PR-caused evidence: not PR-caused; failure touches issue-lock state " + "outside the PR diff" + ), + "rerun": ( + "- Rerun: passed on rerun; what changed between runs: stale " + "/tmp/gitea_issue_lock.json environmental state was cleaned; the pass is " + "trustworthy because the failing test passes in isolation and in the full " + "suite after cleanup" + ), + } + lines.update(overrides) + return "\n".join(v for v in lines.values() if v) + + +class TestOmittedFailures(unittest.TestCase): + def test_report_omitting_earlier_failure_fails(self): + report = "## Final Report\n- Validation: passed (1497 passed, 6 skipped)\n" + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertFalse(result["proven"]) + self.assertTrue( + any("history" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_history_section_missing_observed_test_fails(self): + report = ( + "## Validation failure history\n" + "- Command: pytest tests/test_other.py\n" + "- Failing test: tests/test_other.py::test_something_else\n" + ) + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertFalse(result["proven"]) + self.assertTrue( + any("test_rejects_untraceable_branches" in r for r in result["reasons"]), + result["reasons"], + ) + + def test_no_observed_failures_and_no_claims_passes(self): + report = "## Final Report\n- Validation: passed (1501 passed)\n" + result = assess_validation_failure_history_report(report) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestFailureEntryFields(unittest.TestCase): + def test_full_history_entry_passes(self): + report = _history_block() + "\n\nValidation status: " + TRANSIENT_INVESTIGATION_STATUS + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertTrue(result["proven"], result["reasons"]) + + def test_missing_suspected_cause_fails(self): + report = _history_block(cause="") + "\n\nValidation status: " + TRANSIENT_INVESTIGATION_STATUS + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertFalse(result["proven"]) + self.assertTrue( + any("suspected cause" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_missing_baseline_status_fails(self): + report = _history_block(baseline="") + "\n\nValidation status: " + TRANSIENT_INVESTIGATION_STATUS + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertFalse(result["proven"]) + self.assertTrue( + any("baseline" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_baseline_equivalent_failure_passes(self): + report = ( + _history_block( + baseline="- Baseline master: fails identically on clean baseline worktree", + evidence=( + "- PR-caused evidence: not PR-caused; identical failure signature " + "on baseline master" + ), + rerun="- Rerun: not rerun; baseline-equivalent failure documented", + ) + + "\n\nValidation status: failed (baseline-equivalent, not PR-caused)" + ) + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestRerunPassClaims(unittest.TestCase): + def test_rerun_pass_without_what_changed_fails(self): + report = ( + _history_block(rerun="- Rerun: passed on rerun") + + "\n\nValidation status: " + TRANSIENT_INVESTIGATION_STATUS + ) + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertFalse(result["proven"]) + self.assertTrue( + any("what changed" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_tmp_state_contamination_documented_passes(self): + result = assess_validation_failure_history_report( + _history_block() + "\n\nValidation status: " + TRANSIENT_INVESTIGATION_STATUS, + observed_failures=_OBSERVED, + ) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestStatusVocabulary(unittest.TestCase): + def test_unexplained_transient_with_bare_passed_fails(self): + report = ( + _history_block( + cause="- Suspected cause: unknown", + rerun=( + "- Rerun: passed on rerun; what changed between runs: nothing " + "identified; environmental state: none cleaned; the pass is " + "trustworthy because three consecutive reruns pass" + ), + ) + + "\n\nValidation status: passed" + ) + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertFalse(result["proven"]) + self.assertTrue( + any(TRANSIENT_INVESTIGATION_STATUS in r for r in result["reasons"]), + result["reasons"], + ) + + def test_unexplained_transient_with_investigation_status_passes(self): + report = ( + _history_block( + cause="- Suspected cause: unknown", + rerun=( + "- Rerun: passed on rerun; what changed between runs: nothing " + "identified; environmental state: none cleaned; the pass is " + "trustworthy because three consecutive reruns pass" + ), + ) + + "\n\nValidation status: " + TRANSIENT_INVESTIGATION_STATUS + ) + result = assess_validation_failure_history_report( + report, observed_failures=_OBSERVED + ) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestExports(unittest.TestCase): + def test_review_proofs_reexport(self): + from review_proofs import ( + assess_validation_failure_history_report as exported, + ) + + self.assertTrue(callable(exported)) + + def test_status_constant_value(self): + self.assertEqual( + TRANSIENT_INVESTIGATION_STATUS, + "passed after transient failure investigation", + ) + + +if __name__ == "__main__": + unittest.main()