feat: post-merge moot validation wording + validation:* labels (#529)
Adds #529 acceptance criteria 1/4/5. - post_merge_validation.py: assess_post_merge_validation defines the four canonical validation distinctions (clean pass / baseline-accepted / blocked / post-merge moot) and fails closed on two mistakes: (a) an active approval on a PR already merged/closed before review submission, and (b) post-merge moot validation claimed without merged-state + merge-commit proof. - issue_workflow_labels.py: durable validation:* process-state labels (clean-pass / baseline-accepted / blocked / post-merge-moot) registered in CANONICAL_LABEL_SPECS so manage_labels creates them. - final_report_validator.py: _rule_reviewer_post_merge_validation wired into the review_pr rule set. - tests/test_post_merge_validation.py: module behavior, label registration, and the wired review rule. Refs #529 Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
Claude Opus 4.8
@@ -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,
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
@@ -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: <what the validation run showed, for the record>\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"
|
||||
),
|
||||
}
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user