feat: reject eligible/reviewed wording for already-landed PRs (Closes #298) #367
@@ -1171,6 +1171,102 @@ def assess_workspace_mutation_consistency(report_text, observed_commands=None):
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
# ── Already-landed report wording (Issue #298) ───────────────────────────────
|
||||||
|
#
|
||||||
|
# An already-landed PR is reconciliation-only: it must never be called
|
||||||
|
# eligible, its head SHA must never be called reviewed, and the legacy
|
||||||
|
# eligible/reviewed/scratch-worktree handoff fields must not appear.
|
||||||
|
# Reviewed-head claims in any report require validation + diff review to
|
||||||
|
# have actually passed.
|
||||||
|
|
||||||
|
ALREADY_LANDED_FORBIDDEN_PHRASES = (
|
||||||
|
"oldest eligible pr",
|
||||||
|
"next eligible pr",
|
||||||
|
"pinned reviewed head",
|
||||||
|
"scratch worktree used",
|
||||||
|
)
|
||||||
|
|
||||||
|
ALREADY_LANDED_ELIGIBILITY_CLASS = "ALREADY_LANDED_RECONCILE_REQUIRED"
|
||||||
|
|
||||||
|
|
||||||
|
def assess_already_landed_report_wording(report_text, *, already_landed,
|
||||||
|
review_validated=False):
|
||||||
|
"""#298: reject eligible/reviewed wording for already-landed PRs.
|
||||||
|
|
||||||
|
*already_landed* states whether the already-landed gate fired for the
|
||||||
|
selected PR. *review_validated* states whether validation and diff
|
||||||
|
review actually passed this session; only then may a reviewed head
|
||||||
|
SHA be populated in a non-already-landed report.
|
||||||
|
|
||||||
|
Returns {'complete', 'downgraded', 'reasons'}; fails closed.
|
||||||
|
"""
|
||||||
|
text_lower = (report_text or "").lower()
|
||||||
|
fields = _report_labeled_fields(report_text)
|
||||||
|
reasons = []
|
||||||
|
|
||||||
|
reviewed_head = fields.get("reviewed head sha")
|
||||||
|
reviewed_head_populated = bool(
|
||||||
|
reviewed_head and not reviewed_head.strip().lower().startswith("none")
|
||||||
|
)
|
||||||
|
pinned_head_present = "pinned reviewed head" in fields
|
||||||
|
|
||||||
|
if already_landed:
|
||||||
|
for phrase in ALREADY_LANDED_FORBIDDEN_PHRASES:
|
||||||
|
if phrase in text_lower:
|
||||||
|
reasons.append(
|
||||||
|
f"already-landed report must not use '{phrase}'; the PR "
|
||||||
|
"is reconciliation-only, not review/merge eligible"
|
||||||
|
)
|
||||||
|
|
||||||
|
eligibility = fields.get("eligibility class", "")
|
||||||
|
if ALREADY_LANDED_ELIGIBILITY_CLASS not in eligibility.upper():
|
||||||
|
reasons.append(
|
||||||
|
"already-landed report missing 'Eligibility class: "
|
||||||
|
f"{ALREADY_LANDED_ELIGIBILITY_CLASS}'"
|
||||||
|
)
|
||||||
|
|
||||||
|
for required in ("oldest open pr requiring action",
|
||||||
|
"candidate head sha"):
|
||||||
|
if required not in fields:
|
||||||
|
reasons.append(
|
||||||
|
f"already-landed report missing '{required}' field"
|
||||||
|
)
|
||||||
|
|
||||||
|
if "reviewed head sha" not in fields:
|
||||||
|
reasons.append(
|
||||||
|
"already-landed report must state 'Reviewed head SHA: none'"
|
||||||
|
)
|
||||||
|
elif reviewed_head_populated:
|
||||||
|
reasons.append(
|
||||||
|
"already-landed report must not claim a reviewed head SHA; "
|
||||||
|
"no validation or diff review passed for this PR"
|
||||||
|
)
|
||||||
|
|
||||||
|
worktree_used = fields.get("review worktree used", "")
|
||||||
|
if worktree_used.strip().lower() not in ("false", "no"):
|
||||||
|
reasons.append(
|
||||||
|
"already-landed report must state 'Review worktree used: "
|
||||||
|
"false'"
|
||||||
|
)
|
||||||
|
elif not review_validated:
|
||||||
|
if pinned_head_present:
|
||||||
|
reasons.append(
|
||||||
|
"'Pinned reviewed head' populated before validation and "
|
||||||
|
"diff review passed"
|
||||||
|
)
|
||||||
|
if reviewed_head_populated:
|
||||||
|
reasons.append(
|
||||||
|
"reviewed head SHA populated before validation and diff "
|
||||||
|
"review passed"
|
||||||
|
)
|
||||||
|
|
||||||
|
return {
|
||||||
|
"complete": not reasons,
|
||||||
|
"downgraded": bool(reasons),
|
||||||
|
"reasons": reasons,
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
def assess_role_boundary(proof=None, *, task_role=None, namespaces_used=None,
|
def assess_role_boundary(proof=None, *, task_role=None, namespaces_used=None,
|
||||||
justification=None):
|
justification=None):
|
||||||
"""Assess reviewer/author role separation for blind queue workflows.
|
"""Assess reviewer/author role separation for blind queue workflows.
|
||||||
|
|||||||
@@ -0,0 +1,151 @@
|
|||||||
|
"""Tests for already-landed final-report wording validator (#298).
|
||||||
|
|
||||||
|
An already-landed PR is reconciliation-only, never review/merge
|
||||||
|
eligible, and a head SHA may not be called reviewed unless validation
|
||||||
|
and diff review actually passed. Final reports and controller handoffs
|
||||||
|
must use the reconciliation wording, not the legacy eligible/reviewed
|
||||||
|
fields.
|
||||||
|
"""
|
||||||
|
import sys
|
||||||
|
import unittest
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
|
from review_proofs import assess_already_landed_report_wording # noqa: E402
|
||||||
|
|
||||||
|
|
||||||
|
GOOD_ALREADY_LANDED = (
|
||||||
|
"Controller Handoff\n"
|
||||||
|
"- Oldest open PR requiring action: PR #278\n"
|
||||||
|
"- Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED\n"
|
||||||
|
"- Candidate head SHA: 2a544b7\n"
|
||||||
|
"- Reviewed head SHA: none\n"
|
||||||
|
"- Review worktree used: false\n"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestAlreadyLandedWording(unittest.TestCase):
|
||||||
|
def test_correct_reconciliation_wording_passes(self):
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
GOOD_ALREADY_LANDED, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertTrue(result["complete"])
|
||||||
|
self.assertFalse(result["downgraded"])
|
||||||
|
self.assertEqual(result["reasons"], [])
|
||||||
|
|
||||||
|
def test_oldest_eligible_wording_fails(self):
|
||||||
|
report = GOOD_ALREADY_LANDED + "- oldest eligible PR: PR #278\n"
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("oldest eligible pr" in r.lower() for r in result["reasons"])
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_next_eligible_wording_fails(self):
|
||||||
|
report = GOOD_ALREADY_LANDED + "- next eligible PR: PR #278\n"
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
|
||||||
|
def test_pinned_reviewed_head_fails(self):
|
||||||
|
report = GOOD_ALREADY_LANDED + "- Pinned reviewed head: 2a544b7\n"
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("pinned reviewed head" in r.lower() for r in result["reasons"])
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_scratch_worktree_field_fails(self):
|
||||||
|
report = GOOD_ALREADY_LANDED + "- Scratch worktree used: False\n"
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
|
||||||
|
def test_missing_eligibility_class_fails(self):
|
||||||
|
report = (
|
||||||
|
"Controller Handoff\n"
|
||||||
|
"- Oldest open PR requiring action: PR #278\n"
|
||||||
|
"- Candidate head SHA: 2a544b7\n"
|
||||||
|
"- Reviewed head SHA: none\n"
|
||||||
|
"- Review worktree used: false\n"
|
||||||
|
)
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("eligibility class" in r.lower() for r in result["reasons"])
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_populated_reviewed_head_sha_fails(self):
|
||||||
|
report = GOOD_ALREADY_LANDED.replace(
|
||||||
|
"- Reviewed head SHA: none\n",
|
||||||
|
"- Reviewed head SHA: 2a544b7\n",
|
||||||
|
)
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=True
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("reviewed head" in r.lower() for r in result["reasons"])
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestNonAlreadyLandedReports(unittest.TestCase):
|
||||||
|
def test_normal_reviewed_report_passes(self):
|
||||||
|
report = (
|
||||||
|
"- Selected PR: 281\n"
|
||||||
|
"- Pinned reviewed head: abc1234\n"
|
||||||
|
"- Review decision: approve\n"
|
||||||
|
)
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=False, review_validated=True
|
||||||
|
)
|
||||||
|
self.assertTrue(result["complete"])
|
||||||
|
|
||||||
|
def test_blocked_infra_report_with_pinned_head_fails(self):
|
||||||
|
report = (
|
||||||
|
"- Selected PR: 281\n"
|
||||||
|
"- Pinned reviewed head: abc1234\n"
|
||||||
|
"- Current status: blocked on infra_stop\n"
|
||||||
|
)
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=False, review_validated=False
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
self.assertTrue(
|
||||||
|
any("before validation" in r.lower() for r in result["reasons"])
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_validation_failed_report_with_reviewed_sha_fails(self):
|
||||||
|
report = (
|
||||||
|
"- Selected PR: 281\n"
|
||||||
|
"- Reviewed head SHA: abc1234\n"
|
||||||
|
"- Validation: full suite failed\n"
|
||||||
|
)
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=False, review_validated=False
|
||||||
|
)
|
||||||
|
self.assertFalse(result["complete"])
|
||||||
|
|
||||||
|
def test_validation_failed_report_with_reviewed_none_passes(self):
|
||||||
|
report = (
|
||||||
|
"- Selected PR: 281\n"
|
||||||
|
"- Reviewed head SHA: none\n"
|
||||||
|
"- Validation: full suite failed\n"
|
||||||
|
)
|
||||||
|
result = assess_already_landed_report_wording(
|
||||||
|
report, already_landed=False, review_validated=False
|
||||||
|
)
|
||||||
|
self.assertTrue(result["complete"])
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
Reference in New Issue
Block a user