feat: block approval on full-suite failure without baseline proof (Closes #323)
Approving a PR whose full test suite fails is now gated behind complete baseline proof. New assess_full_suite_failure_approval_gate: - Full-suite pass: approval allowed, no baseline required. - Full-suite failure: default action is REQUEST_CHANGES; approval is blocked unless the #325 baseline validation proof passes (branches/ worktree, pinned baseline/PR SHAs, matching failure signatures, clean before/after) and the proof also carries the PR and baseline full-suite commands, evidence that new/changed tests passed, and a non-empty explanation of why the failures are unrelated to the PR. - Unknown/unproven full-suite result blocks approval. - Main-checkout baseline comparison is rejected. - Vague "same as master" claims without proof are rejected via the existing pre-existing-failure claim detection. - Non-approve decisions are never blocked; the gate only reports the request-changes default. Tests cover matching baseline failures, mismatched signatures, incomplete proof (missing commands, new-test evidence, explanation), main-checkout baseline, unknown suite result, vague claims, and full-suite pass. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
@@ -3218,6 +3218,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)
|
# Identity disclosure (#305)
|
||||||
# ---------------------------------------------------------------------------
|
# ---------------------------------------------------------------------------
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ from review_proofs import ( # noqa: E402
|
|||||||
ISSUE_SELECTION_CONTINUATION_EXPLICIT,
|
ISSUE_SELECTION_CONTINUATION_EXPLICIT,
|
||||||
ISSUE_SELECTION_REPRESENTED_BY_OPEN_PR,
|
ISSUE_SELECTION_REPRESENTED_BY_OPEN_PR,
|
||||||
assess_author_pr_report,
|
assess_author_pr_report,
|
||||||
|
assess_full_suite_failure_approval_gate,
|
||||||
assess_reviewer_baseline_validation_proof,
|
assess_reviewer_baseline_validation_proof,
|
||||||
assess_capability_evidence,
|
assess_capability_evidence,
|
||||||
assess_capability_proof,
|
assess_capability_proof,
|
||||||
@@ -2281,5 +2282,170 @@ class TestReviewerBaselineValidationProof(unittest.TestCase):
|
|||||||
self.assertFalse(report["baseline_validation_proven"])
|
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__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
Reference in New Issue
Block a user