feat: block issue closure on unproven expected pre-existing failure (#529)
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) <[email protected]>
This commit is contained in:
@@ -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 "",
|
||||||
|
}
|
||||||
@@ -40,6 +40,7 @@ FINAL_REPORT_TASK_KINDS = frozenset({
|
|||||||
"issue_filing",
|
"issue_filing",
|
||||||
"issue_selection",
|
"issue_selection",
|
||||||
"inventory",
|
"inventory",
|
||||||
|
"controller_close",
|
||||||
})
|
})
|
||||||
|
|
||||||
_TASK_KIND_ALIASES = {
|
_TASK_KIND_ALIASES = {
|
||||||
@@ -51,6 +52,9 @@ _TASK_KIND_ALIASES = {
|
|||||||
"reconcile_already_landed": "reconcile_already_landed",
|
"reconcile_already_landed": "reconcile_already_landed",
|
||||||
"work-issue": "work_issue",
|
"work-issue": "work_issue",
|
||||||
"create_issue": "issue_filing",
|
"create_issue": "issue_filing",
|
||||||
|
"close_issue": "controller_close",
|
||||||
|
"close-issue": "controller_close",
|
||||||
|
"controller-close": "controller_close",
|
||||||
}
|
}
|
||||||
|
|
||||||
_HANDOFF_ROLE_BY_TASK = {
|
_HANDOFF_ROLE_BY_TASK = {
|
||||||
@@ -62,6 +66,7 @@ _HANDOFF_ROLE_BY_TASK = {
|
|||||||
"issue_filing": "issue_filing",
|
"issue_filing": "issue_filing",
|
||||||
"issue_selection": None,
|
"issue_selection": None,
|
||||||
"inventory": "inventory",
|
"inventory": "inventory",
|
||||||
|
"controller_close": None,
|
||||||
}
|
}
|
||||||
|
|
||||||
_LEGACY_WORKSPACE_MUTATIONS_RE = re.compile(
|
_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_CANONICAL_COMMENT_RULES,
|
||||||
*_SHARED_ISSUE_LOCK_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,
|
||||||
|
],
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -6070,6 +6070,7 @@ def gitea_close_issue(
|
|||||||
host: str | None = None,
|
host: str | None = None,
|
||||||
org: str | None = None,
|
org: str | None = None,
|
||||||
repo: str | None = None,
|
repo: str | None = None,
|
||||||
|
closure_report: str | None = None,
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""Close an issue by setting its state to 'closed'.
|
"""Close an issue by setting its state to 'closed'.
|
||||||
|
|
||||||
@@ -6079,6 +6080,9 @@ def gitea_close_issue(
|
|||||||
host: Override the Gitea host.
|
host: Override the Gitea host.
|
||||||
org: Override the owner/organization.
|
org: Override the owner/organization.
|
||||||
repo: Override the repository name.
|
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:
|
Returns:
|
||||||
dict with 'success' boolean and 'message'.
|
dict with 'success' boolean and 'message'.
|
||||||
@@ -6088,6 +6092,25 @@ def gitea_close_issue(
|
|||||||
if blocked:
|
if blocked:
|
||||||
return blocked
|
return blocked
|
||||||
verify_preflight_purity(remote, task="close_issue")
|
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)
|
h, o, r = _resolve(remote, host, org, repo)
|
||||||
auth = _auth(h)
|
auth = _auth(h)
|
||||||
url = f"{repo_api_url(h, o, r)}/issues/{issue_number}"
|
url = f"{repo_api_url(h, o, r)}/issues/{issue_number}"
|
||||||
|
|||||||
@@ -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()
|
||||||
@@ -2672,6 +2672,56 @@ class TestTrackerHygieneCleanup(unittest.TestCase):
|
|||||||
self.assertTrue(res["success"])
|
self.assertTrue(res["success"])
|
||||||
self.assertEqual(res["cleanup_status"].get(1), "not present")
|
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):
|
def test_merge_pr_with_closes_removes_label(self):
|
||||||
import reviewer_pr_lease
|
import reviewer_pr_lease
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user