From eddb68818e023ed7d3e922f8ca2d8facec1f5d29 Mon Sep 17 00:00:00 2001 From: Jason Walker Date: Thu, 9 Jul 2026 15:25:27 -0400 Subject: [PATCH] feat: block issue closure on unproven expected pre-existing failure (#529) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Delivers #529 acceptance criterion 6: controller issue closure must be blocked when the closure report calls a non-zero test-suite exit an "expected pre-existing"/baseline failure without pre-merge proof. - controller_closure_baseline_proof.py: assess_controller_closure_baseline_proof reuses the #533 pre-merge baseline verifier; empty report -> skipped/allowed, clean pass -> allowed, proven baseline -> allowed, unproven baseline -> block. - gitea_close_issue: optional closure_report param; fails closed (no state PATCH) when the gate blocks. - final_report_validator: new controller_close task kind (alias close_issue) wired to the pre-merge baseline proof rule so a closure can be pre-checked. - Tests: tests/test_controller_closure_baseline_proof.py plus two gitea_close_issue gate tests. Scope: criteria 2/3 (non-zero exit not a clean pass; baseline claims need proof) are already enforced on master via premerge_baseline_proof (#533) and the reviewer schema rules. Criteria 1/4/5 (post-merge moot canonical wording and validation:* process-state labels) remain follow-up work. Validation: full suite 2365 passed, 6 skipped; the 9 remaining failures (tests/test_config.py TestAuthIntegration, tests/test_credentials.py TestGetCredentials, test_issue_540 reviewer-role) are baseline-proven — identical failures on clean prgs/master 6913ac9 (keychain/env dependent), unrelated to this change. Refs #529 Co-Authored-By: Claude Opus 4.8 (1M context) --- controller_closure_baseline_proof.py | 63 ++++++++++ final_report_validator.py | 12 ++ gitea_mcp_server.py | 23 ++++ .../test_controller_closure_baseline_proof.py | 113 ++++++++++++++++++ tests/test_mcp_server.py | 50 ++++++++ 5 files changed, 261 insertions(+) create mode 100644 controller_closure_baseline_proof.py create mode 100644 tests/test_controller_closure_baseline_proof.py diff --git a/controller_closure_baseline_proof.py b/controller_closure_baseline_proof.py new file mode 100644 index 0000000..49948d7 --- /dev/null +++ b/controller_closure_baseline_proof.py @@ -0,0 +1,63 @@ +"""Controller-closure baseline-proof gate (#529, criterion 6). + +A controller (or any session) may close a tracking issue with a closure +report that summarizes validation. When that report calls a non-zero +test-suite exit an "expected pre-existing failure" (or baseline / known +failure) it must carry pre-merge proof; otherwise the closure buries an +unproven regression as an accepted baseline and weakens controller +confidence in the durable state. + +This module fails closed on exactly that case by reusing the pre-merge +baseline proof verifier (#533). A closure report is allowed when it is +empty (no validation claim), a clean pass, or a baseline failure proven on +the PR pre-merge base commit (or a documented known-failure record that +predates the PR). +""" + +from __future__ import annotations + +from typing import Any + +from premerge_baseline_proof import CLEAN_PASS, assess_premerge_baseline_proof + + +def assess_controller_closure_baseline_proof( + closure_report: str | None, +) -> dict[str, Any]: + """Assess whether an issue-closure report may proceed. + + Returns a dict with ``block``, ``proven``, ``label``, ``reasons``, + ``skipped`` and ``safe_next_action``. Only blocks when the closure + report claims a non-zero validation exit is an expected + pre-existing/baseline failure without valid pre-merge proof. + """ + text = (closure_report or "").strip() + if not text: + return { + "block": False, + "proven": True, + "label": CLEAN_PASS, + "reasons": [], + "skipped": True, + "safe_next_action": "", + } + + result = assess_premerge_baseline_proof(text) + block = bool(result.get("block")) + return { + "block": block, + "proven": not block, + "label": result.get("label"), + "reasons": list(result.get("reasons") or []), + "skipped": bool(result.get("skipped", False)), + "safe_next_action": ( + result.get("safe_next_action") + or ( + "provide pre-merge baseline proof (base commit, tested commit, " + "command, exit status, failure signature) before closing on an " + "expected pre-existing failure" + ) + ) + if block + else "", + } diff --git a/final_report_validator.py b/final_report_validator.py index 143e1ae..8b43d63 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -40,6 +40,7 @@ FINAL_REPORT_TASK_KINDS = frozenset({ "issue_filing", "issue_selection", "inventory", + "controller_close", }) _TASK_KIND_ALIASES = { @@ -51,6 +52,9 @@ _TASK_KIND_ALIASES = { "reconcile_already_landed": "reconcile_already_landed", "work-issue": "work_issue", "create_issue": "issue_filing", + "close_issue": "controller_close", + "close-issue": "controller_close", + "controller-close": "controller_close", } _HANDOFF_ROLE_BY_TASK = { @@ -62,6 +66,7 @@ _HANDOFF_ROLE_BY_TASK = { "issue_filing": "issue_filing", "issue_selection": None, "inventory": "inventory", + "controller_close": None, } _LEGACY_WORKSPACE_MUTATIONS_RE = re.compile( @@ -1606,6 +1611,13 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, ], + # Controller issue closure (#529): a closure report must not bury an + # unproven non-zero suite exit as an "expected pre-existing failure". + # Kept intentionally narrow so a closure pre-check does not demand the + # full reviewer/author handoff schema. + "controller_close": [ + _rule_reviewer_premerge_baseline_proof, + ], } diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index dd50942..f8339c2 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -6070,6 +6070,7 @@ def gitea_close_issue( host: str | None = None, org: str | None = None, repo: str | None = None, + closure_report: str | None = None, ) -> dict: """Close an issue by setting its state to 'closed'. @@ -6079,6 +6080,9 @@ def gitea_close_issue( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + closure_report: Optional validation/closure summary. When it claims a + non-zero test-suite exit is an "expected pre-existing"/baseline + failure without pre-merge proof, the close fails closed (#529). Returns: dict with 'success' boolean and 'message'. @@ -6088,6 +6092,25 @@ def gitea_close_issue( if blocked: return blocked verify_preflight_purity(remote, task="close_issue") + + # #529: block closure that buries an unproven non-zero suite exit as an + # "expected pre-existing failure". Skipped when no closure report is given. + from controller_closure_baseline_proof import ( + assess_controller_closure_baseline_proof, + ) + closure_gate = assess_controller_closure_baseline_proof(closure_report) + if closure_gate["block"]: + return { + "success": False, + "blocked": True, + "message": ( + f"Issue #{issue_number} close blocked (#529): closure report " + "claims an expected pre-existing/baseline failure without " + "pre-merge proof." + ), + "reasons": closure_gate["reasons"], + "safe_next_action": closure_gate["safe_next_action"], + } h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) url = f"{repo_api_url(h, o, r)}/issues/{issue_number}" diff --git a/tests/test_controller_closure_baseline_proof.py b/tests/test_controller_closure_baseline_proof.py new file mode 100644 index 0000000..2db5742 --- /dev/null +++ b/tests/test_controller_closure_baseline_proof.py @@ -0,0 +1,113 @@ +"""Controller-closure baseline-proof gate tests (#529, criterion 6).""" + +import unittest + +from controller_closure_baseline_proof import ( + assess_controller_closure_baseline_proof, +) +from premerge_baseline_proof import ( + CLEAN_PASS, + PREMERGE_BASELINE_PROVEN_FAILURE, + UNRESOLVED_REGRESSION_RISK, +) +from final_report_validator import assess_final_report_validator + +# Complete pre-merge proof fields for a baseline claim (see #533). +_PROOF_FIELDS = ( + "Pre-merge base commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Tested commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Command: venv/bin/python -m pytest tests/test_foo.py -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" +) + + +class TestControllerClosureBaselineProof(unittest.TestCase): + def test_empty_report_skipped_and_allowed(self): + result = assess_controller_closure_baseline_proof("") + self.assertFalse(result["block"]) + self.assertTrue(result["skipped"]) + self.assertEqual(result["label"], CLEAN_PASS) + + def test_none_report_allowed(self): + result = assess_controller_closure_baseline_proof(None) + self.assertFalse(result["block"]) + self.assertTrue(result["skipped"]) + + def test_clean_pass_closure_allowed(self): + result = assess_controller_closure_baseline_proof( + "Closing #529. Validation: full suite passed, exit status 0. Clean pass." + ) + self.assertFalse(result["block"]) + self.assertEqual(result["label"], CLEAN_PASS) + + def test_expected_preexisting_failure_without_proof_blocked(self): + report = ( + "Closing #529. Full suite: 1 failed. This is an expected " + "pre-existing failure, safe to close." + ) + result = assess_controller_closure_baseline_proof(report) + self.assertTrue(result["block"]) + self.assertFalse(result["proven"]) + self.assertEqual(result["label"], UNRESOLVED_REGRESSION_RISK) + self.assertTrue(result["reasons"]) + self.assertTrue(result["safe_next_action"]) + + def test_current_master_reproduction_only_blocked(self): + report = ( + "Closing #529. Full suite: 1 failed. Pre-existing baseline failure — " + "reproduced on current master after merge.\n" + "Command: venv/bin/python -m pytest tests/test_foo.py -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" + ) + result = assess_controller_closure_baseline_proof(report) + self.assertTrue(result["block"]) + + def test_proven_baseline_closure_allowed(self): + report = ( + "Closing #529. Full suite: 1 failed, an expected pre-existing " + "baseline failure proven on the PR pre-merge base commit.\n" + + _PROOF_FIELDS + ) + result = assess_controller_closure_baseline_proof(report) + self.assertFalse(result["block"]) + self.assertEqual(result["label"], PREMERGE_BASELINE_PROVEN_FAILURE) + + +class TestControllerCloseTaskKind(unittest.TestCase): + """The composable validator must cover the controller_close task kind.""" + + def test_controller_close_blocks_unproven_baseline(self): + report = ( + "Full suite: 1 failed. Expected pre-existing failure, closing the " + "tracking issue." + ) + result = assess_final_report_validator(report, "controller_close") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + "premerge_baseline_proof" in f["rule_id"] + for f in result["findings"] + ) + ) + + def test_controller_close_alias_close_issue(self): + report = ( + "Full suite: 1 failed. Expected pre-existing failure, closing the " + "tracking issue." + ) + result = assess_final_report_validator(report, "close_issue") + self.assertTrue(result["blocked"]) + + def test_controller_close_allows_proven_baseline(self): + report = ( + "Full suite: 1 failed, expected pre-existing baseline failure proven " + "on the PR pre-merge base commit.\n" + _PROOF_FIELDS + ) + result = assess_final_report_validator(report, "controller_close") + self.assertFalse(result["blocked"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index d52263b..4b25963 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -2672,6 +2672,56 @@ class TestTrackerHygieneCleanup(unittest.TestCase): self.assertTrue(res["success"]) self.assertEqual(res["cleanup_status"].get(1), "not present") + def test_close_issue_blocks_unproven_expected_preexisting_failure(self): + # #529: a closure report calling a non-zero suite exit an "expected + # pre-existing failure" without pre-merge proof must fail closed and + # never PATCH the issue state. + patched = [] + + def api_side_effect(method, url, auth, payload=None): + if method == "PATCH": + patched.append(url) + return {} + self.mock_api.side_effect = api_side_effect + + res = gitea_close_issue( + issue_number=1, + closure_report=( + "Full suite: 1 failed. Expected pre-existing failure, safe to close." + ), + ) + self.assertFalse(res["success"]) + self.assertTrue(res.get("blocked")) + self.assertTrue(res.get("reasons")) + self.assertFalse( + patched, "must not PATCH issue state when the closure gate blocks" + ) + + def test_close_issue_allows_proven_baseline_closure(self): + def api_side_effect(method, url, auth, payload=None): + if method == "PATCH" and "issues/1" in url: + return {"state": "closed"} + if method == "GET" and "labels" in url and "issues" not in url: + return [{"name": "bug", "id": 2}] + if method == "GET" and "issues/1" in url: + return {"labels": [{"name": "bug"}]} + return {} + self.mock_api.side_effect = api_side_effect + + res = gitea_close_issue( + issue_number=1, + closure_report=( + "Full suite: 1 failed, expected pre-existing baseline failure " + "proven on the PR pre-merge base commit.\n" + "Pre-merge base commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Tested commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Command: venv/bin/python -m pytest -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" + ), + ) + self.assertTrue(res["success"]) + def test_merge_pr_with_closes_removes_label(self): import reviewer_pr_lease