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) <[email protected]>
This commit is contained in:
Claude Opus 4.8
@@ -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
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
@@ -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.
|
||||
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()`.
|
||||
@@ -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():
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user