diff --git a/final_report_validator.py b/final_report_validator.py index 8b43d63..fac6a78 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -855,6 +855,27 @@ def _rule_reviewer_premerge_baseline_proof(report_text: str) -> list[dict[str, s ) +def _rule_reviewer_post_merge_validation(report_text: str) -> list[dict[str, str]]: + """Block active approval on an already-merged/closed PR, and post-merge moot + validation claimed without merged-state + merge-commit proof (#529).""" + from post_merge_validation import assess_post_merge_validation + + result = assess_post_merge_validation(report_text) + if not result.get("block"): + return [] + return _findings_from_reasons( + "reviewer.post_merge_validation", + result.get("reasons") or [], + field="Validation status", + severity="block", + safe_next_action=result.get("safe_next_action") + or ( + "record post-merge moot validation instead of an active approval; " + "cite PR state merged/closed and the merge commit SHA" + ), + ) + + def _rule_reviewer_validation_status_vocabulary( report_text: str, *, @@ -1519,6 +1540,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_linked_issue, _rule_reviewer_baseline_on_failure, _rule_reviewer_premerge_baseline_proof, + _rule_reviewer_post_merge_validation, _rule_reviewer_validation_status_vocabulary, _rule_reviewer_main_checkout_baseline, _rule_reviewer_main_checkout_path, diff --git a/issue_workflow_labels.py b/issue_workflow_labels.py index 1e9c491..779834c 100644 --- a/issue_workflow_labels.py +++ b/issue_workflow_labels.py @@ -42,12 +42,38 @@ STATUS_LABEL_SPECS: tuple[LabelSpec, ...] = ( LabelSpec("status:wontfix", "000000", "Issue will not be fixed"), ) +# Durable validation-outcome labels (#529): the four canonical distinctions a +# reviewer report can carry. These are orthogonal to the single active status:* +# label, so they use their own prefix and are not subject to the one-status +# invariant. +VALIDATION_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("validation:clean-pass", "0e8a16", "Validation was a clean pass"), + LabelSpec( + "validation:baseline-accepted", + "fbca04", + "Validation passed with a baseline-proven unrelated failure", + ), + LabelSpec( + "validation:blocked", + "b60205", + "Validation blocked by an unresolved failure", + ), + LabelSpec( + "validation:post-merge-moot", + "5319e7", + "Validation is post-merge moot (PR already merged/closed before review)", + ), +) + CANONICAL_LABEL_SPECS: tuple[LabelSpec, ...] = ( - TYPE_LABEL_SPECS + STATUS_LABEL_SPECS + TYPE_LABEL_SPECS + STATUS_LABEL_SPECS + VALIDATION_LABEL_SPECS ) TYPE_LABELS: frozenset[str] = frozenset(spec.name for spec in TYPE_LABEL_SPECS) STATUS_LABELS: frozenset[str] = frozenset(spec.name for spec in STATUS_LABEL_SPECS) +VALIDATION_LABELS: frozenset[str] = frozenset( + spec.name for spec in VALIDATION_LABEL_SPECS +) CANONICAL_LABELS: frozenset[str] = frozenset( spec.name for spec in CANONICAL_LABEL_SPECS ) diff --git a/post_merge_validation.py b/post_merge_validation.py new file mode 100644 index 0000000..90f36a4 --- /dev/null +++ b/post_merge_validation.py @@ -0,0 +1,222 @@ +"""Canonical post-merge (moot) validation outcome and wording gate (#529). + +When a PR is already merged or closed *before* a reviewer submits their +verdict, an ordinary "approve" is misleading: the review never gated the +merge, so a normal active approval overstates controller confidence. The +sanctioned outcome for that case is a **post-merge moot validation** — the +reviewer records what validation found, but classifies it as moot rather than +an active approval. + +This module defines the four canonical validation distinctions #529 requires +and enforces the post-merge case: + +- ``CLEAN_PASS_OUTCOME`` — clean validation pass on an open, reviewable PR. +- ``BASELINE_ACCEPTED_OUTCOME`` — validation pass with a baseline-proven + unrelated failure (proof handled by :mod:`premerge_baseline_proof`). +- ``BLOCKED_OUTCOME`` — validation blocked by an unresolved failure. +- ``POST_MERGE_MOOT_OUTCOME`` — the PR was already merged/closed before review + submission; validation is recorded as post-merge moot, not an active + approval. + +The gate blocks two mistakes: + +1. Claiming an *active approval* on a PR that was already merged/closed before + review submission (must be recorded as post-merge moot validation instead). +2. Claiming post-merge moot validation without proof the PR is actually + merged/closed (a merged-state field plus a merge commit SHA). + +Each outcome maps to a durable ``validation:*`` process-state label so PRs and +issues carry the distinction (#529 acceptance criterion). +""" + +from __future__ import annotations + +import re +from typing import Any + +CLEAN_PASS_OUTCOME = "clean validation pass" +BASELINE_ACCEPTED_OUTCOME = "validation pass with baseline-proven unrelated failure" +BLOCKED_OUTCOME = "validation blocked by unresolved failure" +POST_MERGE_MOOT_OUTCOME = "post-merge moot validation" + +# Canonical validation status label a reviewer writes in the report body for the +# post-merge case; kept in sync with validation_status_vocabulary. +POST_MERGE_MOOT_STATUS = "post-merge moot validation" + +# Durable process-state labels (registered in issue_workflow_labels). +LABEL_CLEAN_PASS = "validation:clean-pass" +LABEL_BASELINE_ACCEPTED = "validation:baseline-accepted" +LABEL_BLOCKED = "validation:blocked" +LABEL_POST_MERGE_MOOT = "validation:post-merge-moot" + +_OUTCOME_TO_LABEL: dict[str, str] = { + CLEAN_PASS_OUTCOME: LABEL_CLEAN_PASS, + BASELINE_ACCEPTED_OUTCOME: LABEL_BASELINE_ACCEPTED, + BLOCKED_OUTCOME: LABEL_BLOCKED, + POST_MERGE_MOOT_OUTCOME: LABEL_POST_MERGE_MOOT, +} + +# The PR was already merged/closed before the review was submitted. +_MERGED_BEFORE_REVIEW_RE = re.compile( + r"(?:already[- ]merged" + r"|merged\s+before\s+review" + r"|closed\s+before\s+review" + r"|pr\s+(?:is|was)\s+(?:already\s+)?(?:merged|closed)" + r"|pr\s+state\s*[:=]\s*(?:merged|closed)" + r"|merged\s*/\s*closed)", + re.IGNORECASE, +) +# An active approval verdict is being claimed. +_ACTIVE_APPROVAL_RE = re.compile( + r"(?:review\s+decision\s*[:=]\s*approve" + r"|submitted\s+['\"]?approve['\"]?" + r"|active\s+approval" + r"|approving\s+the\s+pr" + r"|posting\s+an?\s+approval)", + re.IGNORECASE, +) +# The sanctioned post-merge moot validation wording. +_POST_MERGE_MOOT_RE = re.compile( + r"post[- ]merge\s+(?:moot\s+)?validation" + r"|post[- ]merge\s+moot" + r"|moot\s+validation", + re.IGNORECASE, +) +# Proof the PR is genuinely merged/closed. +_MERGED_STATE_PROOF_RE = re.compile( + r"(?:pr\s+state\s*[:=]\s*(?:merged|closed)" + r"|merged_at\s*[:=]\s*\S" + r"|closed_at\s*[:=]\s*\S" + r"|state\s*[:=]\s*(?:merged|closed))", + re.IGNORECASE, +) +_MERGE_COMMIT_SHA_RE = re.compile( + r"merge\s+commit(?:\s+sha)?\s*[:=]\s*[0-9a-f]{7,40}", + re.IGNORECASE, +) + + +POST_MERGE_VALIDATION_TEMPLATE = ( + "Validation status: post-merge moot validation\n" + "PR state: merged\n" + "Merge commit sha: <40-hex merge commit>\n" + "Validation finding: \n" + "Note: PR merged/closed before review submission; recorded as post-merge " + "moot validation, not an active approval." +) + + +def process_state_label(outcome: str) -> str | None: + """Return the durable ``validation:*`` label for a canonical outcome.""" + return _OUTCOME_TO_LABEL.get(outcome) + + +def assess_post_merge_validation( + report_text: str, + *, + pr_merged_or_closed: bool | None = None, +) -> dict[str, Any]: + """Assess post-merge moot validation wording and proof (#529). + + Args: + report_text: The reviewer final report text. + pr_merged_or_closed: Optional structured signal that the PR is already + merged/closed. When ``True`` it forces the merged-before-review + path even if the report text omits the phrasing; proof fields are + still required in the report body for durability. + + Returns a dict with ``proven``, ``block``, ``outcome``, + ``process_state_label``, ``reasons``, ``skipped`` and ``safe_next_action``. + Only blocks when the report either claims an active approval on a + merged/closed PR, or claims post-merge moot validation without proof. + """ + text = report_text or "" + + merged_signal = bool(_MERGED_BEFORE_REVIEW_RE.search(text)) or bool( + pr_merged_or_closed + ) + active_approval = bool(_ACTIVE_APPROVAL_RE.search(text)) + moot_wording = bool(_POST_MERGE_MOOT_RE.search(text)) + + # Nothing about merged-state or moot validation — not applicable here. + if not merged_signal and not moot_wording: + return { + "proven": True, + "block": False, + "outcome": None, + "process_state_label": None, + "reasons": [], + "skipped": True, + "safe_next_action": "", + } + + # An active approval on a PR already merged/closed before review, without + # the sanctioned moot wording, overstates confidence. + if merged_signal and active_approval and not moot_wording: + return { + "proven": False, + "block": True, + "outcome": POST_MERGE_MOOT_OUTCOME, + "process_state_label": LABEL_POST_MERGE_MOOT, + "reasons": [ + "PR was already merged/closed before review submission; an " + "active approval overstates confidence — record 'post-merge " + "moot validation' instead of a normal approval" + ], + "skipped": False, + "safe_next_action": ( + "classify the outcome as post-merge moot validation (not an " + "active approval); cite PR state merged/closed and the merge " + "commit SHA. Template:\n" + POST_MERGE_VALIDATION_TEMPLATE + ), + } + + # Post-merge moot validation claimed — require merged-state + commit proof. + if moot_wording: + reasons: list[str] = [] + if not _MERGED_STATE_PROOF_RE.search(text): + reasons.append( + "post-merge moot validation claimed without merged/closed state " + "proof (state: merged/closed, or merged_at/closed_at)" + ) + if not _MERGE_COMMIT_SHA_RE.search(text): + reasons.append( + "post-merge moot validation claimed without a merge commit SHA" + ) + if reasons: + return { + "proven": False, + "block": True, + "outcome": POST_MERGE_MOOT_OUTCOME, + "process_state_label": LABEL_POST_MERGE_MOOT, + "reasons": reasons, + "skipped": False, + "safe_next_action": ( + "prove the PR is merged/closed: cite PR state and the merge " + "commit SHA. Template:\n" + POST_MERGE_VALIDATION_TEMPLATE + ), + } + return { + "proven": True, + "block": False, + "outcome": POST_MERGE_MOOT_OUTCOME, + "process_state_label": LABEL_POST_MERGE_MOOT, + "reasons": [], + "skipped": False, + "safe_next_action": "", + } + + # Merged signal present but no approval claim and no moot wording yet — + # guide toward the moot outcome without blocking. + return { + "proven": True, + "block": False, + "outcome": POST_MERGE_MOOT_OUTCOME, + "process_state_label": LABEL_POST_MERGE_MOOT, + "reasons": [], + "skipped": False, + "safe_next_action": ( + "PR appears merged/closed; if submitting a verdict, record " + "post-merge moot validation rather than an active approval" + ), + } diff --git a/tests/test_post_merge_validation.py b/tests/test_post_merge_validation.py new file mode 100644 index 0000000..3e76b7d --- /dev/null +++ b/tests/test_post_merge_validation.py @@ -0,0 +1,119 @@ +"""Post-merge moot validation wording + label tests (#529, criteria 1/4/5).""" + +import unittest + +from post_merge_validation import ( + BASELINE_ACCEPTED_OUTCOME, + BLOCKED_OUTCOME, + CLEAN_PASS_OUTCOME, + LABEL_BASELINE_ACCEPTED, + LABEL_BLOCKED, + LABEL_CLEAN_PASS, + LABEL_POST_MERGE_MOOT, + POST_MERGE_MOOT_OUTCOME, + assess_post_merge_validation, + process_state_label, +) +from final_report_validator import assess_final_report_validator +from issue_workflow_labels import CANONICAL_LABELS, VALIDATION_LABELS + +_MOOT_PROOF = ( + "Validation status: post-merge moot validation\n" + "PR state: merged\n" + "Merge commit sha: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Validation finding: full suite passed, for the record.\n" +) + + +class TestPostMergeValidation(unittest.TestCase): + def test_not_applicable_when_no_merged_or_moot_signal(self): + result = assess_post_merge_validation( + "Review decision: approve. Validation: full suite passed." + ) + self.assertFalse(result["block"]) + self.assertTrue(result["skipped"]) + + def test_active_approval_on_merged_pr_blocked(self): + report = ( + "PR is already merged. Review decision: approve. " + "Validation: full suite passed." + ) + result = assess_post_merge_validation(report) + self.assertTrue(result["block"]) + self.assertEqual(result["outcome"], POST_MERGE_MOOT_OUTCOME) + self.assertEqual(result["process_state_label"], LABEL_POST_MERGE_MOOT) + + def test_active_approval_on_merged_pr_via_structured_signal_blocked(self): + report = "Review decision: approve. Validation: full suite passed." + result = assess_post_merge_validation(report, pr_merged_or_closed=True) + self.assertTrue(result["block"]) + + def test_moot_wording_without_proof_blocked(self): + report = "Recording post-merge moot validation for this PR." + result = assess_post_merge_validation(report) + self.assertTrue(result["block"]) + self.assertTrue(result["reasons"]) + + def test_moot_wording_with_full_proof_allowed(self): + result = assess_post_merge_validation(_MOOT_PROOF) + self.assertFalse(result["block"]) + self.assertEqual(result["outcome"], POST_MERGE_MOOT_OUTCOME) + + def test_merged_signal_only_guides_without_blocking(self): + result = assess_post_merge_validation( + "PR was already merged before review; recording findings." + ) + self.assertFalse(result["block"]) + self.assertTrue(result["safe_next_action"]) + + def test_process_state_label_mapping(self): + self.assertEqual(process_state_label(CLEAN_PASS_OUTCOME), LABEL_CLEAN_PASS) + self.assertEqual( + process_state_label(BASELINE_ACCEPTED_OUTCOME), LABEL_BASELINE_ACCEPTED + ) + self.assertEqual(process_state_label(BLOCKED_OUTCOME), LABEL_BLOCKED) + self.assertEqual( + process_state_label(POST_MERGE_MOOT_OUTCOME), LABEL_POST_MERGE_MOOT + ) + self.assertIsNone(process_state_label("nonsense")) + + +class TestValidationLabelsRegistered(unittest.TestCase): + def test_validation_labels_are_canonical(self): + for label in ( + LABEL_CLEAN_PASS, + LABEL_BASELINE_ACCEPTED, + LABEL_BLOCKED, + LABEL_POST_MERGE_MOOT, + ): + self.assertIn(label, VALIDATION_LABELS) + self.assertIn(label, CANONICAL_LABELS) + + +class TestPostMergeValidationWiredIntoReview(unittest.TestCase): + def test_review_pr_blocks_active_approval_on_merged_pr(self): + report = ( + "PR is already merged. Review decision: approve. " + "Validation: full suite passed." + ) + result = assess_final_report_validator(report, "review_pr") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reviewer.post_merge_validation" + for f in result["findings"] + ) + ) + + def test_review_pr_allows_proven_post_merge_moot(self): + result = assess_final_report_validator(_MOOT_PROOF, "review_pr") + self.assertFalse( + any( + f["rule_id"] == "reviewer.post_merge_validation" + for f in result["findings"] + ) + ) + + +if __name__ == "__main__": + unittest.main()