feat: block approval on full-suite failure without baseline proof (Closes #323) #361
@@ -3548,6 +3548,113 @@ def assess_reviewer_baseline_validation_proof(
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Full-suite failure approval gate (#323)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def assess_full_suite_failure_approval_gate(
|
||||
*,
|
||||
review_decision: str,
|
||||
full_suite_passed: bool | None,
|
||||
baseline_validation: dict | None = None,
|
||||
baseline_proof: dict | None = None,
|
||||
report_text: str | None = None,
|
||||
project_root: str | None = None,
|
||||
) -> dict:
|
||||
"""#323: approving a PR whose full suite fails requires baseline proof.
|
||||
|
||||
Default action on a full-suite failure is REQUEST_CHANGES. Approval is
|
||||
allowed only when the #325 baseline proof passes in full (branches/
|
||||
worktree, pinned SHAs, matching failure signatures) and the extra #323
|
||||
fields prove the failures are pre-existing: ``pr_suite_command``,
|
||||
``baseline_suite_command``, ``new_tests_passed``, and a non-empty
|
||||
``unrelated_explanation``.
|
||||
|
||||
*review_decision* is the decision the workflow intends to submit; only
|
||||
``approve`` can be blocked. ``block`` is True when an approval was
|
||||
requested that the proofs do not allow.
|
||||
"""
|
||||
decision = (review_decision or "").strip().lower().replace("-", "_")
|
||||
wants_approval = decision == "approve"
|
||||
reasons: list[str] = []
|
||||
|
||||
if full_suite_passed is True:
|
||||
return {
|
||||
"approve_allowed": True,
|
||||
"block": False,
|
||||
"default_action": "proceed",
|
||||
"reasons": [],
|
||||
}
|
||||
|
||||
default_action = "request_changes"
|
||||
|
||||
if full_suite_passed is None:
|
||||
reasons.append(
|
||||
"full-suite result not proven; approval requires a recorded "
|
||||
"full-suite command and result (#323)"
|
||||
)
|
||||
else:
|
||||
if baseline_validation is None:
|
||||
baseline_validation = assess_reviewer_baseline_validation_proof(
|
||||
baseline_proof=baseline_proof,
|
||||
report_text=report_text,
|
||||
project_root=project_root,
|
||||
)
|
||||
proof = baseline_proof or {}
|
||||
|
||||
if not proof:
|
||||
reasons.append(
|
||||
"full suite failed but no baseline proof was provided; "
|
||||
"default action is REQUEST_CHANGES (#323)"
|
||||
)
|
||||
if not baseline_validation.get("proven"):
|
||||
reasons.append(
|
||||
"baseline validation proof missing or failed (#323/#325)"
|
||||
)
|
||||
reasons.extend(baseline_validation.get("reasons", []))
|
||||
|
||||
if proof:
|
||||
worktree = (proof.get("worktree_path") or "").strip()
|
||||
if not _path_under_branches(worktree, project_root):
|
||||
reasons.append(
|
||||
"baseline comparison must run in a clean baseline "
|
||||
"worktree under branches/, not the main checkout (#323)"
|
||||
)
|
||||
if not (proof.get("pr_suite_command") or "").strip():
|
||||
reasons.append(
|
||||
"PR full-suite command/result missing from baseline "
|
||||
"proof (#323)"
|
||||
)
|
||||
if not (proof.get("baseline_suite_command") or "").strip():
|
||||
reasons.append(
|
||||
"baseline full-suite command/result missing from "
|
||||
"baseline proof (#323)"
|
||||
)
|
||||
if proof.get("new_tests_passed") is not True:
|
||||
reasons.append(
|
||||
"proof that new/changed tests passed is missing (#323)"
|
||||
)
|
||||
if not (proof.get("unrelated_explanation") or "").strip():
|
||||
reasons.append(
|
||||
"explanation why failures are unrelated to the PR is "
|
||||
"missing (#323)"
|
||||
)
|
||||
if proof.get("failure_signatures_match") is not True:
|
||||
reasons.append(
|
||||
"PR and baseline failure signatures do not match; "
|
||||
"request changes (#323)"
|
||||
)
|
||||
|
||||
approve_allowed = not reasons
|
||||
return {
|
||||
"approve_allowed": approve_allowed,
|
||||
"block": wants_approval and not approve_allowed,
|
||||
"default_action": "proceed" if approve_allowed else default_action,
|
||||
"reasons": reasons,
|
||||
}
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Identity disclosure (#305)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
@@ -24,6 +24,7 @@ from review_proofs import ( # noqa: E402
|
||||
ISSUE_SELECTION_CONTINUATION_EXPLICIT,
|
||||
ISSUE_SELECTION_REPRESENTED_BY_OPEN_PR,
|
||||
assess_author_pr_report,
|
||||
assess_full_suite_failure_approval_gate,
|
||||
assess_queue_ordering_proof,
|
||||
assess_reviewer_baseline_validation_proof,
|
||||
assess_capability_evidence,
|
||||
@@ -2387,5 +2388,170 @@ class TestReviewerBaselineValidationProof(unittest.TestCase):
|
||||
self.assertFalse(report["baseline_validation_proven"])
|
||||
|
||||
|
||||
class TestFullSuiteFailureApprovalGate(unittest.TestCase):
|
||||
"""Issue #323: full-suite failure blocks approval without baseline proof."""
|
||||
|
||||
ROOT = "/repo/Gitea-Tools"
|
||||
|
||||
def _good_proof(self, **overrides):
|
||||
proof = {
|
||||
"worktree_path": f"{self.ROOT}/branches/baseline-master-pr280",
|
||||
"baseline_target_sha": PINNED,
|
||||
"pr_head_sha": OTHER,
|
||||
"baseline_failures": ["tests/test_foo.py::test_bar"],
|
||||
"pr_failures": ["tests/test_foo.py::test_bar"],
|
||||
"failure_signatures_match": True,
|
||||
"clean_before": True,
|
||||
"clean_after": True,
|
||||
"pr_suite_command": "venv/bin/pytest tests/",
|
||||
"baseline_suite_command": "venv/bin/pytest tests/",
|
||||
"new_tests_passed": True,
|
||||
"unrelated_explanation": (
|
||||
"failures touch agent_temp_artifacts only; PR changes "
|
||||
"review_proofs handoff fields"
|
||||
),
|
||||
}
|
||||
proof.update(overrides)
|
||||
return proof
|
||||
|
||||
def _good_report(self):
|
||||
return (
|
||||
"Full suite failed on PR worktree and on baseline.\n"
|
||||
"- Validation command: venv/bin/pytest tests/\n"
|
||||
f"- Working directory: {self.ROOT}/branches/review-pr-280\n"
|
||||
"Failures are pre-existing on master.\n"
|
||||
)
|
||||
|
||||
def test_full_suite_pass_allows_approval_without_baseline(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=True,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertTrue(result["approve_allowed"])
|
||||
self.assertFalse(result["block"])
|
||||
self.assertEqual(result["default_action"], "proceed")
|
||||
|
||||
def test_full_suite_failure_defaults_to_request_changes(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="request_changes",
|
||||
full_suite_passed=False,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["block"])
|
||||
self.assertEqual(result["default_action"], "request_changes")
|
||||
|
||||
def test_approval_with_failure_and_no_proof_blocks(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
self.assertEqual(result["default_action"], "request_changes")
|
||||
|
||||
def test_approval_with_matching_baseline_proof_allowed(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertTrue(result["approve_allowed"])
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
def test_mismatched_failure_signatures_block_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(
|
||||
failure_signatures_match=False,
|
||||
pr_failures=["tests/test_other.py::test_other"],
|
||||
),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_main_checkout_baseline_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(worktree_path=self.ROOT),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_missing_new_tests_proof_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(new_tests_passed=None),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(any(
|
||||
"new" in reason.lower() and "test" in reason.lower()
|
||||
for reason in result["reasons"]
|
||||
))
|
||||
|
||||
def test_missing_unrelated_explanation_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(unrelated_explanation=""),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
|
||||
def test_missing_suite_commands_block_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
baseline_proof=self._good_proof(
|
||||
pr_suite_command="", baseline_suite_command=""),
|
||||
report_text=self._good_report(),
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(any(
|
||||
"command" in reason.lower() for reason in result["reasons"]
|
||||
))
|
||||
|
||||
def test_unknown_suite_result_blocks_approval(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=None,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_vague_same_as_master_claim_without_proof_blocks(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="approve",
|
||||
full_suite_passed=False,
|
||||
report_text="Validation: failures same as master; approving.",
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["approve_allowed"])
|
||||
self.assertTrue(result["block"])
|
||||
|
||||
def test_non_approve_decision_never_blocks(self):
|
||||
result = assess_full_suite_failure_approval_gate(
|
||||
review_decision="comment",
|
||||
full_suite_passed=None,
|
||||
project_root=self.ROOT,
|
||||
)
|
||||
self.assertFalse(result["block"])
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user