From 6be8f163517ec3c6d15dbbb4a801a85dde8e0bff Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:33:12 -0400 Subject: [PATCH 01/15] Add infra-stop repair handoff verifier for blocked reviewers (#289) Stop PR queue advancement when infra_stop blocks capability and require CONTROL-CHECKOUT REPAIR MODE handoffs with infra diagnostics. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 7 + reviewer_infra_stop_handoff.py | 162 ++++++++++++++++++++++ tests/test_llm_workflow_split.py | 6 + tests/test_reviewer_infra_stop_handoff.py | 106 ++++++++++++++ 4 files changed, 281 insertions(+) create mode 100644 reviewer_infra_stop_handoff.py create mode 100644 tests/test_reviewer_infra_stop_handoff.py diff --git a/review_proofs.py b/review_proofs.py index dab4d02..8f7a853 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -4024,3 +4024,10 @@ def assess_non_mergeable_skip_proof(report_text, **kwargs): from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess return _assess(report_text, **kwargs) + + +def assess_infra_stop_handoff_report(report_text, **kwargs): + """#289: repair handoff purity when infra_stop blocks reviewer capability.""" + from reviewer_infra_stop_handoff import assess_infra_stop_handoff_report as _assess + + return _assess(report_text, **kwargs) diff --git a/reviewer_infra_stop_handoff.py b/reviewer_infra_stop_handoff.py new file mode 100644 index 0000000..f39147d --- /dev/null +++ b/reviewer_infra_stop_handoff.py @@ -0,0 +1,162 @@ +"""Infra-stop repair handoff verifier for blocked reviewer workflows (#289).""" + +from __future__ import annotations + +import re +from typing import Any + +from capability_stop_terminal import ( + TERMINAL_REPORT_HEADING, + assess_capability_stop_report, +) + +_REPAIR_HANDOFF_RE = re.compile( + r"repair handoff|control-checkout repair mode", + re.IGNORECASE, +) +_CONTROL_REPAIR_RE = re.compile(r"control-checkout repair mode", re.IGNORECASE) +_PR_QUEUE_ADVANCE_RE = re.compile( + r"(?:selected pr|pr #\d+ (?:to review|selected)|eligible pr|" + r"next (?:eligible )?pr(?: to review)?|oldest eligible pr|pinned (?:review )?head)", + re.IGNORECASE, +) +_REVIEW_MUTATION_RE = re.compile( + r"(?:gitea_review_pr|gitea_merge_pr|request_changes|submitted\s+approve|" + r"merge result|review decision\s*:\s*approve)", + re.IGNORECASE, +) +_BACKGROUND_TOOL_RE = re.compile(r"\b(?:schedule|manage_task)\b", re.IGNORECASE) +_PR_REVIEW_REPORT_RE = re.compile( + r"(?:review summary|merge recommendation|validation result\s*:\s*pass|" + r"queue status report with selected pr)", + re.IGNORECASE, +) + +_DIAGNOSTIC_FIELDS = { + "mcp process root": re.compile(r"mcp process root\s*:", re.IGNORECASE), + "inspected git root": re.compile(r"inspected git root\s*:", re.IGNORECASE), + "conflict marker path": re.compile( + r"(?:conflict marker path|exact conflict marker path)\s*:", + re.IGNORECASE, + ), + "merge/rebase control path": re.compile( + r"(?:merge/rebase control path|exact merge/rebase control path)\s*:", + re.IGNORECASE, + ), + "safe next repair action": re.compile( + r"safe next (?:repair )?action\s*:", + re.IGNORECASE, + ), +} + + +def assess_infra_stop_handoff_report( + report_text: str, + *, + stop_session: dict | None = None, +) -> dict[str, Any]: + """Validate repair handoff purity when infra_stop blocks reviewer capability (#289).""" + text = report_text or "" + session = dict(stop_session or {}) + reasons: list[str] = [] + + infra_stop = bool( + session.get("infra_stop") + or session.get("infra_stop_blocked") + or session.get("route_result") == "infra_stop" + ) + if not infra_stop: + return { + "proven": True, + "block": False, + "reasons": [], + "infra_stop": False, + "safe_next_action": "proceed", + } + + lower = text.lower() + repair_handoff = bool( + _REPAIR_HANDOFF_RE.search(text) + or TERMINAL_REPORT_HEADING.lower() in lower + ) + if not repair_handoff: + reasons.append( + "infra_stop requires a repair handoff, not a PR review report" + ) + + if _PR_REVIEW_REPORT_RE.search(text) and not _REPAIR_HANDOFF_RE.search(text): + reasons.append( + "final output must be a repair handoff instead of stale PR review state" + ) + + if _PR_QUEUE_ADVANCE_RE.search(text): + reasons.append( + "infra_stop blocks PR queue advancement; do not select or advance next PR" + ) + + if _REVIEW_MUTATION_RE.search(text): + reasons.append( + "review, approval, request-changes, merge, or comment mutations " + "forbidden while infra_stop is active" + ) + + if session.get("pinned_head_sha") or re.search( + r"pinned (?:review )?head sha\s*:", text, re.IGNORECASE + ): + if session.get("capability_cleared") is not True: + reasons.append( + "cannot pin PR head SHA while review capability never cleared" + ) + + if session.get("main_checkout_diagnostics") and not _CONTROL_REPAIR_RE.search(text): + reasons.append( + "main-checkout diagnostics must be labeled CONTROL-CHECKOUT REPAIR MODE" + ) + + if _BACKGROUND_TOOL_RE.search(text): + reasons.append( + "background schedule/manage_task tools must not be used during " + "blocked infra_stop recovery" + ) + + assessment = session.get("infra_stop_assessment") or {} + if assessment.get("infra_stop") or infra_stop: + missing = [ + label + for label, pattern in _DIAGNOSTIC_FIELDS.items() + if not pattern.search(text) + ] + if missing and session.get("require_infra_diagnostics", True): + reasons.append( + "infra_stop repair handoff missing diagnostic fields: " + + ", ".join(missing) + ) + conflict_file = assessment.get("conflict_file") + if conflict_file and conflict_file.lower() not in lower: + reasons.append( + f"infra_stop assessment conflict file {conflict_file!r} " + "not reflected in repair handoff" + ) + + capability = assess_capability_stop_report( + text, + trust_gate_status=session.get("trust_gate_status"), + capability_denied=True, + ) + if not capability.get("pure"): + reasons.extend(capability.get("reasons") or []) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": list(dict.fromkeys(reasons)), + "infra_stop": True, + "repair_handoff": repair_handoff, + "safe_next_action": ( + "stop PR queue work; emit CONTROL-CHECKOUT REPAIR MODE handoff " + "with infra diagnostics and safe next repair action" + if reasons + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 9a96d17..79070cb 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -134,3 +134,9 @@ def test_non_mergeable_skip_verifier_exported(): from review_proofs import assess_non_mergeable_skip_proof assert callable(assess_non_mergeable_skip_proof) + + +def test_infra_stop_handoff_verifier_exported(): + from review_proofs import assess_infra_stop_handoff_report + + assert callable(assess_infra_stop_handoff_report) diff --git a/tests/test_reviewer_infra_stop_handoff.py b/tests/test_reviewer_infra_stop_handoff.py new file mode 100644 index 0000000..6d49dc3 --- /dev/null +++ b/tests/test_reviewer_infra_stop_handoff.py @@ -0,0 +1,106 @@ +"""Tests for infra-stop repair handoff verifier (#289).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from capability_stop_terminal import TERMINAL_REPORT_HEADING +from reviewer_infra_stop_handoff import assess_infra_stop_handoff_report # noqa: E402 + + +def _repair_handoff() -> str: + return "\n".join([ + TERMINAL_REPORT_HEADING, + "Repair handoff for infra_stop blocked reviewer workflow.", + "CONTROL-CHECKOUT REPAIR MODE", + "MCP process root: /Users/dev/Gitea-Tools", + "Inspected git root: /Users/dev/Gitea-Tools", + "Conflict marker path: gitea_mcp_server.py", + "Merge/rebase control path: none", + "Safe next repair action: resolve conflict markers and restart MCP", + "infra_stop: true", + ]) + + +class TestInfraStopHandoff(unittest.TestCase): + def test_repair_handoff_passes(self): + result = assess_infra_stop_handoff_report( + _repair_handoff(), + stop_session={ + "infra_stop": True, + "infra_stop_assessment": { + "infra_stop": True, + "conflict_file": "gitea_mcp_server.py", + }, + }, + ) + self.assertTrue(result["proven"], result["reasons"]) + + def test_next_pr_selection_blocks(self): + report = "\n".join([ + TERMINAL_REPORT_HEADING, + "Next eligible PR to review: PR #276", + "Pinned review head SHA: abc123", + ]) + result = assess_infra_stop_handoff_report( + report, + stop_session={"infra_stop": True, "require_infra_diagnostics": False}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("queue" in r.lower() for r in result["reasons"])) + + def test_missing_repair_handoff_blocks(self): + result = assess_infra_stop_handoff_report( + "Validation result: pass\nReview summary for PR #276", + stop_session={"infra_stop": True}, + ) + self.assertFalse(result["proven"]) + + def test_main_checkout_diagnostics_without_label_blocks(self): + report = "\n".join([ + TERMINAL_REPORT_HEADING, + "Repair handoff", + "Ran git status in main checkout for MCP diagnostics.", + ]) + result = assess_infra_stop_handoff_report( + report, + stop_session={ + "infra_stop": True, + "main_checkout_diagnostics": True, + "require_infra_diagnostics": False, + }, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("control-checkout" in r.lower() for r in result["reasons"])) + + def test_background_scheduling_blocks(self): + report = "\n".join([ + TERMINAL_REPORT_HEADING, + "Repair handoff", + "Used schedule/manage_task while waiting for MCP recovery.", + ]) + result = assess_infra_stop_handoff_report( + report, + stop_session={"infra_stop": True, "require_infra_diagnostics": False}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("schedule" in r.lower() for r in result["reasons"])) + + def test_non_infra_stop_skips(self): + result = assess_infra_stop_handoff_report( + "Selected PR #1 for review", + stop_session={"infra_stop": False}, + ) + self.assertTrue(result["proven"]) + + +class TestExport(unittest.TestCase): + def test_review_proofs_reexport(self): + from review_proofs import assess_infra_stop_handoff_report as exported + + self.assertTrue(callable(exported)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 1d3ee72274744efb9a991d5c5b8e8795362046f8 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:39:43 -0400 Subject: [PATCH 02/15] feat: prove inventory pagination and worktree state in reviewer reports (#293) Add assess_inventory_worktree_report verifier to reject partial PR inventory claims, exactly-full first pages without finality proof, legacy scratch-worktree flags that contradict branches/ review worktrees, and contradictory checkout wording. Wire into build_final_report and export from review_proofs. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 21 ++ reviewer_inventory_worktree.py | 308 ++++++++++++++++++++++ tests/test_llm_workflow_split.py | 6 + tests/test_reviewer_inventory_worktree.py | 126 +++++++++ 4 files changed, 461 insertions(+) create mode 100644 reviewer_inventory_worktree.py create mode 100644 tests/test_reviewer_inventory_worktree.py diff --git a/review_proofs.py b/review_proofs.py index cbde5fd..db52131 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1405,6 +1405,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, if report_text else {"proven": True, "block": False, "reasons": [], "violations": []} ) + inventory_worktree = ( + assess_inventory_worktree_report(report_text) + if report_text + else {"proven": True, "block": False, "reasons": []} + ) if baseline_validation is None and report_text: baseline_validation = assess_reviewer_baseline_validation_proof( report_text=report_text, @@ -1561,6 +1566,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue-status report violates loaded workflow proof gates (#339)" ) downgrade_reasons.extend(queue_status_report.get("reasons", [])) + if not inventory_worktree.get("proven"): + downgrade_reasons.append( + "inventory pagination or worktree fields inconsistent (#293)" + ) + downgrade_reasons.extend(inventory_worktree.get("reasons", [])) if not baseline_validation.get("proven"): downgrade_reasons.append( "reviewer baseline validation proof missing or failed (#325)" @@ -1646,6 +1656,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue_status_violations": list( queue_status_report.get("violations") or [] ), + "inventory_worktree_proven": bool(inventory_worktree.get("proven")), + "inventory_worktree_violations": list( + inventory_worktree.get("reasons") or [] + ), "baseline_validation_proven": bool(baseline_validation.get("proven")), "baseline_validation_violations": list( baseline_validation.get("violations") or [] @@ -4064,3 +4078,10 @@ def assess_non_mergeable_skip_proof(report_text, **kwargs): from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess return _assess(report_text, **kwargs) + + +def assess_inventory_worktree_report(report_text, **kwargs): + """#293: prove inventory pagination and consistent worktree reporting.""" + from reviewer_inventory_worktree import assess_inventory_worktree_report as _assess + + return _assess(report_text, **kwargs) diff --git a/reviewer_inventory_worktree.py b/reviewer_inventory_worktree.py new file mode 100644 index 0000000..0ca95b3 --- /dev/null +++ b/reviewer_inventory_worktree.py @@ -0,0 +1,308 @@ +"""Reviewer inventory completeness and worktree state verifier (#293).""" + +from __future__ import annotations + +import re +from typing import Any + +_PAGINATION_FINALITY_EVIDENCE = re.compile( + r"pagination_complete\s*:\s*true|inventory_complete\s*:\s*true|" + r"is_final_page\s*:\s*true|pages_fetched|has_more\s*:\s*false|" + r"no next page|final[- ]page|pr_inventory_trust_gate\.status|" + r"total_count\s*:|pagination.*(?:final|complete)", + re.I, +) + +_INCOMPLETE_PAGINATION_PROOF = re.compile( + r"first page only|partial page|page 1 only|truncated|" + r"returned \d+ open prs?(?:\s*$|\s*[,;])", + re.I, +) + +_DEFAULT_PAGE_SIZE_ASSUMPTION = re.compile( + r"(?:less|fewer) than (?:the )?(?:default )?(?:gitea )?page[- ]?(?:size|limit)|" + r"default (?:gitea )?page[- ]?size|under (?:the )?50[- ]?(?:item )?limit|" + r"(?:complete|exhaustive).*(?:default )?page[- ]?size|" + r"page[- ]?size assumption|assumed complete", + re.I, +) + +_ELIGIBILITY_CLAIM_RE = re.compile( + r"\b(?:oldest eligible pr|next eligible pr|next pr to review|" + r"inventory (?:is )?complete|inventory exhaustive|exhaustive inventory)\b", + re.I, +) + +_EXACT_PAGE_LIMIT_RE = re.compile( + r"(?:returned|listed|fetched)\s+(\d+)\s+open prs?|" + r"open pr count\s*:\s*(\d+)|" + r"page[- ]?size\s*(?:=|:)\s*(\d+)", + re.I, +) + +_LEGACY_SCRATCH_FALSE_RE = re.compile( + r"scratch worktree used\s*:\s*false", + re.I, +) + +_BRANCHES_WORKTREE_RE = re.compile(r"\bbranches/", re.I) + +_CONTRADICTORY_HEAD_RE = re.compile( + r"detached head\s*/\s*branch\s+master|" + r"branch\s+master\s*/\s*detached head|" + r"detached head.*branch\s+(?:master|main|dev)\b.*detached|" + r"checkout\s*:\s*detached head\s*/\s*branch", + re.I, +) + +_MAIN_CHECKOUT_BRANCH_RE = re.compile( + r"main checkout branch\s*:\s*(\S+)", + re.I, +) + +_REVIEW_HEAD_STATE_RE = re.compile( + r"review worktree head state\s*:\s*(\S+)", + re.I, +) + +_HANDOFF_SECTION_RE = re.compile( + r"^##\s*Controller Handoff\s*$", + re.I | re.M, +) + + +def _handoff_field_map(report_text: str) -> dict[str, str]: + """Parse ``- Field: value`` lines from the Controller Handoff section.""" + text = report_text or "" + match = _HANDOFF_SECTION_RE.search(text) + if not match: + return {} + section = text[match.end() :] + fields: dict[str, str] = {} + for line in section.splitlines(): + stripped = line.strip().lstrip("-*").strip() + if ":" not in stripped: + continue + key, value = stripped.split(":", 1) + fields[key.strip().lower()] = value.strip() + return fields + + +def _truthy_field(value: str) -> bool: + lowered = (value or "").strip().lower() + if not lowered: + return False + if lowered in {"false", "no", "none", "not applicable", "n/a", "—", "-"}: + return False + return True + + +def _field_proves_pagination(value: str) -> bool: + proof = (value or "").strip() + if not proof or proof.lower() in {"none", "n/a", "not applicable", "unknown"}: + return False + if _DEFAULT_PAGE_SIZE_ASSUMPTION.search(proof): + return False + if _INCOMPLETE_PAGINATION_PROOF.search(proof): + return False + return bool(_PAGINATION_FINALITY_EVIDENCE.search(proof)) + + +def _pagination_proven(text: str, session: dict, *, pagination_field: str = "") -> bool: + if session.get("pagination_complete") or session.get("inventory_complete"): + return True + session_proof = session.get("inventory_pagination_proof") or "" + if _field_proves_pagination(str(session_proof)): + return True + if _field_proves_pagination(pagination_field): + return True + if _PAGINATION_FINALITY_EVIDENCE.search(text): + return True + return False + + +def _exact_page_limit_unproven( + text: str, session: dict, *, pagination_proven: bool = False +) -> bool: + """True when a full page was returned without final-page proof.""" + if pagination_proven: + return False + requested = session.get("requested_page_size") + returned = session.get("returned_page_size") + if isinstance(requested, int) and isinstance(returned, int): + if returned >= requested > 0: + return True + for match in _EXACT_PAGE_LIMIT_RE.finditer(text): + count = next((g for g in match.groups() if g), None) + if not count: + continue + try: + n = int(count) + except ValueError: + continue + if n in {10, 20, 50}: + return True + return False + + +def assess_inventory_worktree_report( + report_text: str, + *, + inventory_session: dict | None = None, +) -> dict[str, Any]: + """Validate PR inventory pagination proof and worktree field consistency (#293).""" + text = report_text or "" + session = dict(inventory_session or {}) + reasons: list[str] = [] + + fields = _handoff_field_map(text) + pagination_proof_field = fields.get("inventory pagination proof", "") + + pagination_proven = _pagination_proven( + text, session, pagination_field=pagination_proof_field + ) + + if _ELIGIBILITY_CLAIM_RE.search(text): + if not pagination_proven: + reasons.append( + "oldest/next eligible PR or inventory-complete claim requires " + "final-page/no-next-page/pagination_complete proof" + ) + + if _DEFAULT_PAGE_SIZE_ASSUMPTION.search(text): + if not pagination_proven: + reasons.append( + "inventory pagination assumed from default page size without " + "final-page/no-next-page/total-count/traversal proof" + ) + + if pagination_proof_field: + if _DEFAULT_PAGE_SIZE_ASSUMPTION.search(pagination_proof_field): + reasons.append( + "Inventory pagination proof field relies on page-size assumption" + ) + elif _INCOMPLETE_PAGINATION_PROOF.search(pagination_proof_field): + reasons.append( + "Inventory pagination proof field does not prove final page" + ) + elif not _field_proves_pagination(pagination_proof_field): + if _ELIGIBILITY_CLAIM_RE.search(text) or _EXACT_PAGE_LIMIT_RE.search(text): + reasons.append( + "Inventory pagination proof field missing final-page metadata" + ) + + if _exact_page_limit_unproven(text, session, pagination_proven=pagination_proven): + reasons.append( + "exactly-full first page returned without final-page or " + "no-next-page proof" + ) + + review_worktree_used = fields.get("review worktree used", "") + review_worktree_path = fields.get("review worktree path", "") + scratch_used = fields.get("scratch worktree used", "") + inside_branches = fields.get("review worktree inside branches:", "") or fields.get( + "review worktree inside branches", "" + ) + + branches_path_in_text = bool( + _BRANCHES_WORKTREE_RE.search(review_worktree_path) + or _BRANCHES_WORKTREE_RE.search(text) + ) + + if branches_path_in_text: + if review_worktree_used and not _truthy_field(review_worktree_used): + reasons.append( + "reports using a branches/ review worktree must set " + "Review worktree used: true" + ) + if scratch_used and re.search(r"\bfalse\b", scratch_used, re.I): + reasons.append( + "Scratch worktree used: false rejected when a branches/ " + "review worktree was created or used" + ) + if _LEGACY_SCRATCH_FALSE_RE.search(text) and not _truthy_field( + review_worktree_used + ): + reasons.append( + "legacy Scratch worktree used: false contradicts branches/ " + "review worktree usage" + ) + + if review_worktree_path and _truthy_field(review_worktree_used): + if "branches/" not in review_worktree_path.replace("\\", "/").lower(): + reasons.append( + "Review worktree path must be under branches/ when " + "Review worktree used is true" + ) + if inside_branches and re.search(r"\bfalse\b", inside_branches, re.I): + reasons.append( + "Review worktree inside branches must be true when path is " + "under branches/" + ) + + main_branch = ( + fields.get("main checkout branch", "") + or _MAIN_CHECKOUT_BRANCH_RE.search(text).group(1) + if _MAIN_CHECKOUT_BRANCH_RE.search(text) + else "" + ) + head_state = ( + fields.get("review worktree head state", "") + or ( + _REVIEW_HEAD_STATE_RE.search(text).group(1) + if _REVIEW_HEAD_STATE_RE.search(text) + else "" + ) + ) + if main_branch and head_state: + main_norm = main_branch.strip().lower() + head_norm = head_state.strip().lower() + if head_norm in {"branch", "on branch"} and main_norm in { + "master", + "main", + "dev", + }: + if "detached" in text.lower() and "review worktree" in text.lower(): + reasons.append( + "review worktree HEAD state must not reuse main checkout " + "branch wording" + ) + + if _CONTRADICTORY_HEAD_RE.search(text): + reasons.append( + "contradictory checkout wording such as 'Detached HEAD / Branch master'" + ) + + if review_worktree_used and _truthy_field(review_worktree_used): + if not review_worktree_path or review_worktree_path.lower() in { + "none", + "n/a", + "not applicable", + }: + reasons.append( + "Review worktree used: true requires Review worktree path" + ) + if not head_state or head_state.lower() in { + "none", + "n/a", + "not applicable", + "unknown", + }: + reasons.append( + "Review worktree used: true requires Review worktree HEAD state" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": list(dict.fromkeys(reasons)), + "pagination_proven": pagination_proven, + "branches_worktree_used": branches_path_in_text, + "safe_next_action": ( + "prove inventory pagination with final-page metadata; align " + "Review worktree used/path/HEAD state with branches/ usage" + if reasons + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index e1ec43a..24ea6fa 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -140,3 +140,9 @@ def test_non_mergeable_skip_verifier_exported(): from review_proofs import assess_non_mergeable_skip_proof assert callable(assess_non_mergeable_skip_proof) + + +def test_inventory_worktree_verifier_exported(): + from review_proofs import assess_inventory_worktree_report + + assert callable(assess_inventory_worktree_report) diff --git a/tests/test_reviewer_inventory_worktree.py b/tests/test_reviewer_inventory_worktree.py new file mode 100644 index 0000000..15f77f7 --- /dev/null +++ b/tests/test_reviewer_inventory_worktree.py @@ -0,0 +1,126 @@ +"""Tests for reviewer inventory/worktree verifier (#293).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from reviewer_inventory_worktree import assess_inventory_worktree_report # noqa: E402 + + +def _good_handoff() -> str: + return "\n".join([ + "## Controller Handoff", + "- Selected PR: 280", + "- Inventory pagination proof: is_final_page: true, has_more: false, total_count: 6", + "- Review worktree used: true", + "- Review worktree path: branches/review-pr280-feat-issue-275-claim", + "- Review worktree inside branches: true", + "- Review worktree HEAD state: detached", + "- Main checkout branch: master", + "- Current status: reviewed PR #280", + ]) + + +class TestInventoryPaginationProof(unittest.TestCase): + def test_no_next_page_proof_passes(self): + result = assess_inventory_worktree_report(_good_handoff()) + self.assertTrue(result["proven"], result["reasons"]) + self.assertTrue(result["pagination_proven"]) + + def test_rejects_partial_first_page_eligibility_claim(self): + report = "\n".join([ + "Oldest eligible PR is #280.", + "gitea_list_prs returned 10 open PRs on the first page.", + "## Controller Handoff", + "- Inventory pagination proof: first page only", + ]) + result = assess_inventory_worktree_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(result["block"]) + self.assertTrue(any("eligible" in r.lower() for r in result["reasons"])) + + def test_rejects_default_page_size_assumption(self): + report = ( + "Inventory exhaustive because fewer than the default Gitea page size " + "were returned." + ) + result = assess_inventory_worktree_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("page size" in r.lower() for r in result["reasons"])) + + def test_rejects_exactly_full_first_page_without_finality(self): + report = "\n".join([ + "Next eligible PR: #290 based on complete inventory.", + "gitea_list_prs returned 50 open PRs.", + "## Controller Handoff", + "- Inventory pagination proof: returned 50 open PRs", + ]) + result = assess_inventory_worktree_report( + report, + inventory_session={"requested_page_size": 50, "returned_page_size": 50}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("full first page" in r.lower() for r in result["reasons"])) + + def test_allows_exact_page_with_session_pagination_complete(self): + report = "Oldest eligible PR: #12 after queue inventory." + result = assess_inventory_worktree_report( + report, + inventory_session={"pagination_complete": True}, + ) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestWorktreeConsistency(unittest.TestCase): + def test_branches_worktree_requires_review_worktree_used_true(self): + report = "\n".join([ + "## Controller Handoff", + "- Inventory pagination proof: is_final_page: true", + "- Review worktree used: false", + "- Review worktree path: branches/review-pr280-feat", + "- Scratch worktree used: false", + ]) + result = assess_inventory_worktree_report(report) + self.assertFalse(result["proven"]) + joined = " ".join(result["reasons"]).lower() + self.assertIn("review worktree used", joined) + self.assertIn("scratch worktree", joined) + + def test_rejects_contradictory_detached_and_branch_wording(self): + report = "\n".join([ + "Checkout: Detached HEAD / Branch master", + "## Controller Handoff", + "- Inventory pagination proof: is_final_page: true", + "- Review worktree used: true", + "- Review worktree path: branches/review-pr280-feat", + "- Review worktree HEAD state: detached", + "- Main checkout branch: master", + ]) + result = assess_inventory_worktree_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("contradictory" in r.lower() for r in result["reasons"]) + ) + + def test_requires_head_state_when_review_worktree_used(self): + report = "\n".join([ + "## Controller Handoff", + "- Inventory pagination proof: pagination_complete: true", + "- Review worktree used: true", + "- Review worktree path: branches/review-pr280-feat", + ]) + result = assess_inventory_worktree_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("head state" in r.lower() for r in result["reasons"])) + + +class TestExport(unittest.TestCase): + def test_review_proofs_reexport(self): + from review_proofs import assess_inventory_worktree_report as exported + + self.assertTrue(callable(exported)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 2c65cf05ca8cc085f01a1c2233b6a27b2120e6a4 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:42:08 -0400 Subject: [PATCH 03/15] feat: reject stale already-landed controller handoff fields (#299) Add assess_already_landed_handoff_report verifier to block Pinned reviewed head, Scratch worktree used, legacy Mutations/Workspace mutations None when git fetch or other mutations occurred, and narrative/handoff drift after the already-landed gate fires. Wire into build_final_report and review_proofs. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 21 ++ reviewer_already_landed_handoff.py | 216 ++++++++++++++++++ tests/test_llm_workflow_split.py | 6 + tests/test_reviewer_already_landed_handoff.py | 131 +++++++++++ 4 files changed, 374 insertions(+) create mode 100644 reviewer_already_landed_handoff.py create mode 100644 tests/test_reviewer_already_landed_handoff.py diff --git a/review_proofs.py b/review_proofs.py index cbde5fd..28f00cb 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1405,6 +1405,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, if report_text else {"proven": True, "block": False, "reasons": [], "violations": []} ) + already_landed_handoff = ( + assess_already_landed_handoff_report(report_text) + if report_text + else {"proven": True, "block": False, "reasons": []} + ) if baseline_validation is None and report_text: baseline_validation = assess_reviewer_baseline_validation_proof( report_text=report_text, @@ -1561,6 +1566,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue-status report violates loaded workflow proof gates (#339)" ) downgrade_reasons.extend(queue_status_report.get("reasons", [])) + if not already_landed_handoff.get("proven"): + downgrade_reasons.append( + "already-landed controller handoff has stale or inconsistent fields (#299)" + ) + downgrade_reasons.extend(already_landed_handoff.get("reasons", [])) if not baseline_validation.get("proven"): downgrade_reasons.append( "reviewer baseline validation proof missing or failed (#325)" @@ -1646,6 +1656,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue_status_violations": list( queue_status_report.get("violations") or [] ), + "already_landed_handoff_proven": bool(already_landed_handoff.get("proven")), + "already_landed_handoff_violations": list( + already_landed_handoff.get("reasons") or [] + ), "baseline_validation_proven": bool(baseline_validation.get("proven")), "baseline_validation_violations": list( baseline_validation.get("violations") or [] @@ -4064,3 +4078,10 @@ def assess_non_mergeable_skip_proof(report_text, **kwargs): from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess return _assess(report_text, **kwargs) + + +def assess_already_landed_handoff_report(report_text, **kwargs): + """#299: reject stale fields after the already-landed gate fires.""" + from reviewer_already_landed_handoff import assess_already_landed_handoff_report as _assess + + return _assess(report_text, **kwargs) diff --git a/reviewer_already_landed_handoff.py b/reviewer_already_landed_handoff.py new file mode 100644 index 0000000..0642644 --- /dev/null +++ b/reviewer_already_landed_handoff.py @@ -0,0 +1,216 @@ +"""Already-landed controller handoff consistency verifier (#299).""" + +from __future__ import annotations + +import re +from typing import Any + +from review_proofs import git_ref_mutating_commands + +ALREADY_LANDED_STATE = "ALREADY_LANDED_RECONCILE_REQUIRED" + +_HANDOFF_SECTION_RE = re.compile(r"^##\s*Controller Handoff\s*$", re.I | re.M) + +_STALE_HANDOFF_FIELDS = ( + "pinned reviewed head", + "scratch worktree used", +) + +_LEGACY_MUTATIONS_NONE_RE = re.compile( + r"^\s*[-*]?\s*mutations\s*:\s*none\s*$", + re.I | re.M, +) +_LEGACY_WORKSPACE_NONE_RE = re.compile( + r"^\s*[-*]?\s*workspace\s+mutations\s*:\s*none\s*$", + re.I | re.M, +) + +_FORBIDDEN_REVIEW_STATES_RE = re.compile( + r"review decision\s*:\s*approved?\b|" + r"merge result\s*:\s*merged\b|" + r"\bready[_ ]to[_ ]merge\b", + re.I, +) + +_NARRATIVE_ELIGIBILITY_RE = re.compile( + r"eligibility class\s*:\s*([^\n]+)", + re.I, +) +_NARRATIVE_REVIEWED_SHA_RE = re.compile( + r"reviewed head sha\s*:\s*([^\n]+)", + re.I, +) +_NARRATIVE_WORKTREE_RE = re.compile( + r"review worktree used\s*:\s*([^\n]+)", + re.I, +) + + +def _handoff_field_map(report_text: str) -> dict[str, str]: + text = report_text or "" + match = _HANDOFF_SECTION_RE.search(text) + if not match: + return {} + fields: dict[str, str] = {} + for line in text[match.end() :].splitlines(): + stripped = line.strip().lstrip("-*").strip() + if ":" not in stripped: + continue + key, value = stripped.split(":", 1) + fields[key.strip().lower()] = value.strip() + return fields + + +def _narrative_before_handoff(report_text: str) -> str: + text = report_text or "" + match = _HANDOFF_SECTION_RE.search(text) + if not match: + return text + return text[: match.start()] + + +def _is_truthy(value: str) -> bool: + lowered = (value or "").strip().lower() + return lowered not in {"", "none", "n/a", "not applicable", "false", "no", "—", "-"} + + +def _gate_active(text: str, fields: dict[str, str], session: dict) -> bool: + if session.get("gate_fired") or session.get("already_landed_gate_fired"): + return True + eligibility = (fields.get("eligibility class") or "").upper() + if ALREADY_LANDED_STATE in eligibility: + return True + return ALREADY_LANDED_STATE.lower() in text.lower() + + +def assess_already_landed_handoff_report( + report_text: str, + *, + handoff_session: dict | None = None, + command_log: list | None = None, +) -> dict[str, Any]: + """Reject stale handoff fields and narrative/handoff drift after the gate (#299).""" + text = report_text or "" + session = dict(handoff_session or {}) + fields = _handoff_field_map(text) + reasons: list[str] = [] + + if not _gate_active(text, fields, session): + return { + "proven": True, + "block": False, + "reasons": [], + "gate_active": False, + "safe_next_action": "proceed", + } + + for stale_field in _STALE_HANDOFF_FIELDS: + if stale_field in fields: + reasons.append( + f"already-landed handoff must not include stale field " + f"'{stale_field.title()}' (#299)" + ) + + if _LEGACY_WORKSPACE_NONE_RE.search(text): + reasons.append( + "already-landed handoff must not include legacy " + "'Workspace mutations: None' (#299)" + ) + + ref_commands = git_ref_mutating_commands(command_log or session.get("command_log")) + mutations_observed = bool( + ref_commands + or session.get("mutations_observed") + or session.get("mcp_mutations") + or session.get("review_mutations") + ) + if mutations_observed and _LEGACY_MUTATIONS_NONE_RE.search(text): + reasons.append( + "already-landed handoff must not claim 'Mutations: None' when " + "git ref, MCP, review, merge, or cleanup mutations occurred (#299)" + ) + + git_ref_value = fields.get("git ref mutations", "").strip().lower() + if ref_commands and (not git_ref_value or git_ref_value == "none"): + reasons.append( + "git fetch/ref updates must be reported under Git ref mutations (#299)" + ) + + reviewed_sha = fields.get("reviewed head sha", "") + if reviewed_sha and _is_truthy(reviewed_sha): + reasons.append( + "already-landed handoff must use 'Reviewed head SHA: none' (#299)" + ) + + worktree_used = fields.get("review worktree used", "") + if worktree_used and _is_truthy(worktree_used): + reasons.append( + "already-landed handoff must set Review worktree used: false (#299)" + ) + + candidate_sha = fields.get("candidate head sha", "") + if not candidate_sha or candidate_sha.lower() in {"none", "n/a", "unknown"}: + reasons.append( + "already-landed handoff must include Candidate head SHA (#299)" + ) + + if _FORBIDDEN_REVIEW_STATES_RE.search(text): + reasons.append( + "already-landed handoff must not claim approved/merged/ready-to-merge (#299)" + ) + + narrative = _narrative_before_handoff(text) + handoff_eligibility = (fields.get("eligibility class") or "").strip() + narrative_eligibility = ( + _NARRATIVE_ELIGIBILITY_RE.search(narrative).group(1).strip() + if _NARRATIVE_ELIGIBILITY_RE.search(narrative) + else "" + ) + if handoff_eligibility and narrative_eligibility: + if handoff_eligibility.upper() != narrative_eligibility.upper(): + reasons.append( + "narrative Eligibility class disagrees with controller handoff (#299)" + ) + + narrative_reviewed = ( + _NARRATIVE_REVIEWED_SHA_RE.search(narrative).group(1).strip() + if _NARRATIVE_REVIEWED_SHA_RE.search(narrative) + else "" + ) + if narrative_reviewed and reviewed_sha: + if narrative_reviewed.lower() != reviewed_sha.lower(): + reasons.append( + "narrative Reviewed head SHA disagrees with controller handoff (#299)" + ) + + narrative_worktree = ( + _NARRATIVE_WORKTREE_RE.search(narrative).group(1).strip() + if _NARRATIVE_WORKTREE_RE.search(narrative) + else "" + ) + if narrative_worktree and worktree_used: + if narrative_worktree.lower() != worktree_used.lower(): + reasons.append( + "narrative Review worktree used disagrees with controller handoff (#299)" + ) + + git_ref_narrative = "git ref mutations" in narrative.lower() + if git_ref_narrative and git_ref_value: + if "none" in git_ref_value and ref_commands: + reasons.append( + "narrative and handoff disagree on git ref mutation reporting (#299)" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": list(dict.fromkeys(reasons)), + "gate_active": True, + "safe_next_action": ( + "emit canonical ALREADY_LANDED_RECONCILE_REQUIRED handoff without " + "stale review/merge fields; align narrative and controller handoff" + if reasons + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index e1ec43a..310a911 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -140,3 +140,9 @@ def test_non_mergeable_skip_verifier_exported(): from review_proofs import assess_non_mergeable_skip_proof assert callable(assess_non_mergeable_skip_proof) + + +def test_already_landed_handoff_verifier_exported(): + from review_proofs import assess_already_landed_handoff_report + + assert callable(assess_already_landed_handoff_report) diff --git a/tests/test_reviewer_already_landed_handoff.py b/tests/test_reviewer_already_landed_handoff.py new file mode 100644 index 0000000..cf1c779 --- /dev/null +++ b/tests/test_reviewer_already_landed_handoff.py @@ -0,0 +1,131 @@ +"""Tests for already-landed handoff verifier (#299).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from reviewer_already_landed_handoff import ( # noqa: E402 + ALREADY_LANDED_STATE, + assess_already_landed_handoff_report, +) + + +def _good_report() -> str: + return "\n".join([ + "## PR reconciliation summary", + f"Eligibility class: {ALREADY_LANDED_STATE}", + "Candidate head SHA: 2a544f582026b72a229d59a172c0a63ac4aaeaf9", + "Reviewed head SHA: none", + "Review worktree used: false", + "Ancestor proof: PR head is ancestor of prgs/master", + "", + "## Controller Handoff", + "- Selected PR: #278", + f"- Eligibility class: {ALREADY_LANDED_STATE}", + "- Candidate head SHA: 2a544f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: none", + "- Target branch: master", + "- Target branch SHA: abc123def4567890abcdef1234567890abcdef12", + "- Review worktree used: false", + "- Git ref mutations: git fetch prgs master", + "- Review decision: none", + "- Merge result: none", + "- Linked issue status: open", + "- Safe next action: reconcile open PR and linked issue", + ]) + + +class TestAlreadyLandedHandoff(unittest.TestCase): + def test_clean_handoff_passes(self): + result = assess_already_landed_handoff_report(_good_report()) + self.assertTrue(result["proven"], result["reasons"]) + self.assertTrue(result["gate_active"]) + + def test_inactive_gate_skips_checks(self): + report = "## Controller Handoff\n- Selected PR: #12\n- Review decision: approve" + result = assess_already_landed_handoff_report(report) + self.assertTrue(result["proven"]) + self.assertFalse(result["gate_active"]) + + def test_rejects_stale_pinned_reviewed_head(self): + report = _good_report().replace( + "- Candidate head SHA:", + "- Pinned reviewed head: 2a544f582026b72a229d59a172c0a63ac4aaeaf9\n" + "- Candidate head SHA:", + ) + result = assess_already_landed_handoff_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("pinned reviewed head" in r.lower() for r in result["reasons"])) + + def test_rejects_scratch_worktree_used(self): + report = _good_report() + "\n- Scratch worktree used: false" + result = assess_already_landed_handoff_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("scratch worktree" in r.lower() for r in result["reasons"])) + + def test_rejects_mutations_none_with_git_fetch(self): + report = _good_report().replace( + "- Git ref mutations: git fetch prgs master", + "- Git ref mutations: none\n- Mutations: None", + ) + result = assess_already_landed_handoff_report( + report, + command_log=[{"command": "git fetch prgs master", "performed": True}], + ) + self.assertFalse(result["proven"]) + joined = " ".join(result["reasons"]).lower() + self.assertIn("mutations", joined) + self.assertIn("git ref", joined) + + def test_rejects_workspace_mutations_none(self): + report = _good_report() + "\n- Workspace mutations: None" + result = assess_already_landed_handoff_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("workspace mutations" in r.lower() for r in result["reasons"])) + + def test_rejects_reviewed_head_sha_when_gate_fired(self): + report = _good_report().replace("- Reviewed head SHA: none", "- Reviewed head SHA: abc123") + result = assess_already_landed_handoff_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("reviewed head sha" in r.lower() for r in result["reasons"])) + + def test_rejects_narrative_handoff_eligibility_mismatch(self): + report = _good_report().replace( + f"Eligibility class: {ALREADY_LANDED_STATE}", + "Eligibility class: NORMAL_REVIEW_CANDIDATE", + 1, + ) + result = assess_already_landed_handoff_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("eligibility class" in r.lower() for r in result["reasons"])) + + def test_session_gate_fired_without_text_marker(self): + report = "## Controller Handoff\n- Selected PR: #1\n- Pinned reviewed head: abc" + result = assess_already_landed_handoff_report( + report, + handoff_session={"gate_fired": True}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(result["gate_active"]) + + def test_infra_stop_style_normal_handoff_not_blocked(self): + report = "\n".join([ + "## Controller Handoff", + "- Selected PR: #12", + "- Review decision: request_changes", + "- Pinned reviewed head: abc123def4567890abcdef1234567890abcdef12", + ]) + result = assess_already_landed_handoff_report(report) + self.assertTrue(result["proven"]) + + +class TestExport(unittest.TestCase): + def test_review_proofs_reexport(self): + from review_proofs import assess_already_landed_handoff_report as exported + + self.assertTrue(callable(exported)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 3764ba056634145f5d69a97e97f953ed0cfb04f6 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:44:18 -0400 Subject: [PATCH 04/15] feat: require live linked issue proof in reconciliation handoffs (#300) Add assess_reconcile_linked_issue_report verifier to block open/closed/resolved linked issue status claims without gitea_view_issue proof in the current session, require not-verified wording when the issue is missing, and gate recommended issue close/reopen/comment actions on exact capability proof. Wire into build_final_report and export from review_proofs. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 26 +++ reviewer_reconcile_linked_issue.py | 191 ++++++++++++++++++ tests/test_llm_workflow_split.py | 6 + tests/test_reviewer_reconcile_linked_issue.py | 117 +++++++++++ 4 files changed, 340 insertions(+) create mode 100644 reviewer_reconcile_linked_issue.py create mode 100644 tests/test_reviewer_reconcile_linked_issue.py diff --git a/review_proofs.py b/review_proofs.py index 77986a7..9b0bb43 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1405,6 +1405,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, if report_text else {"proven": True, "block": False, "reasons": [], "violations": []} ) + reconcile_linked_issue = ( + assess_reconcile_linked_issue_report(report_text) + if report_text and _RECONCILE_LINKED_ISSUE_HINT.search(report_text) + else {"proven": True, "block": False, "reasons": []} + ) if baseline_validation is None and report_text: baseline_validation = assess_reviewer_baseline_validation_proof( report_text=report_text, @@ -1561,6 +1566,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue-status report violates loaded workflow proof gates (#339)" ) downgrade_reasons.extend(queue_status_report.get("reasons", [])) + if not reconcile_linked_issue.get("proven"): + downgrade_reasons.append( + "reconciliation linked issue status lacks live fetch proof (#300)" + ) + downgrade_reasons.extend(reconcile_linked_issue.get("reasons", [])) if not baseline_validation.get("proven"): downgrade_reasons.append( "reviewer baseline validation proof missing or failed (#325)" @@ -1646,6 +1656,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue_status_violations": list( queue_status_report.get("violations") or [] ), + "reconcile_linked_issue_proven": bool(reconcile_linked_issue.get("proven")), + "reconcile_linked_issue_violations": list( + reconcile_linked_issue.get("reasons") or [] + ), "baseline_validation_proven": bool(baseline_validation.get("proven")), "baseline_validation_violations": list( baseline_validation.get("violations") or [] @@ -3682,6 +3696,11 @@ _QUEUE_STATUS_REPORT_HINT = re.compile( re.I, ) +_RECONCILE_LINKED_ISSUE_HINT = re.compile( + r"already[- ]landed|reconcil|linked issue(?:\s+live)?\s+status", + re.I, +) + _GATE_PASSED_VALUE = re.compile(r"\bpassed\b", re.I) _NOT_APPLICABLE_VALUE = re.compile( @@ -4071,3 +4090,10 @@ def assess_non_mergeable_skip_proof(report_text, **kwargs): from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess return _assess(report_text, **kwargs) + + +def assess_reconcile_linked_issue_report(report_text, **kwargs): + """#300: prove linked issue status was fetched live in reconciliation handoffs.""" + from reviewer_reconcile_linked_issue import assess_reconcile_linked_issue_report as _assess + + return _assess(report_text, **kwargs) diff --git a/reviewer_reconcile_linked_issue.py b/reviewer_reconcile_linked_issue.py new file mode 100644 index 0000000..8a01ee0 --- /dev/null +++ b/reviewer_reconcile_linked_issue.py @@ -0,0 +1,191 @@ +"""Reconciliation linked-issue live proof verifier (#300).""" + +from __future__ import annotations + +import re +from typing import Any + +_HANDOFF_SECTION_RE = re.compile(r"^##\s*Controller Handoff\s*$", re.I | re.M) + +_LINKED_ISSUE_FIELD_RE = re.compile( + r"linked issue\s*:\s*(.+)$", + re.I | re.M, +) +_LINKED_ISSUE_STATUS_RE = re.compile( + r"linked issue(?:\s+live)?\s+status\s*:\s*(.+)$", + re.I | re.M, +) + +_STATUS_CLAIM_RE = re.compile( + r"\b(?:issue\s+#?\d+\s*\()?(open|closed|resolved)\b|" + r"issue\s+(?:is\s+)?(open|closed|resolved)\b|" + r"linked issue.*\b(open|closed|resolved)\b", + re.I, +) +_NOT_VERIFIED_RE = re.compile(r"not verified in this session", re.I) + +_LIVE_FETCH_PROOF_RE = re.compile( + r"gitea_view_issue|issue fetched live|live issue fetch|" + r"fetched linked issue.*(?:current|this) session|" + r"linked issue (?:fetch|proof).*(?:current|this) session|" + r"live linked issue proof", + re.I, +) + +_ISSUE_ACTION_RECOMMEND_RE = re.compile( + r"(?:recommend(?:ed)?|should|must)\s+(?:close|reopen|comment on)\s+" + r"(?:the\s+)?(?:linked\s+)?issue|" + r"(?:close|reopen|comment on)\s+linked issue\s+#?\d+", + re.I, +) +_CAPABILITY_PROOF_RE = re.compile( + r"capability proof|exact capability|gitea_close_issue|" + r"gitea_mark_issue|gitea_create_issue_comment|gitea_edit_issue", + re.I, +) + +_NO_LINKED_ISSUE_VALUES = frozenset({ + "", + "none", + "n/a", + "not applicable", + "no linked issue", + "unknown", +}) + + +def _handoff_field_map(report_text: str) -> dict[str, str]: + text = report_text or "" + match = _HANDOFF_SECTION_RE.search(text) + if not match: + return {} + fields: dict[str, str] = {} + for line in text[match.end() :].splitlines(): + stripped = line.strip().lstrip("-*").strip() + if ":" not in stripped: + continue + key, value = stripped.split(":", 1) + fields[key.strip().lower()] = value.strip() + return fields + + +def _extract_linked_issue_number(text: str, fields: dict[str, str]) -> int | None: + linked = fields.get("linked issue", "") + if linked.lower() in _NO_LINKED_ISSUE_VALUES: + return None + match = re.search(r"#?(\d+)", linked) + if match: + return int(match.group(1)) + field_match = _LINKED_ISSUE_FIELD_RE.search(text) + if field_match: + num = re.search(r"#?(\d+)", field_match.group(1)) + if num: + return int(num.group(1)) + return None + + +def _status_value(text: str, fields: dict[str, str]) -> str: + for key in ("linked issue live status", "linked issue status"): + if fields.get(key): + return fields[key] + match = _LINKED_ISSUE_STATUS_RE.search(text) + return match.group(1).strip() if match else "" + + +def _live_proof_present(text: str, session: dict, issue_number: int | None) -> bool: + if session.get("live_fetched") or session.get("verified_live"): + return True + if session.get("issue_fetch_proof"): + return True + if _LIVE_FETCH_PROOF_RE.search(text): + return True + if issue_number is not None: + pattern = re.compile( + rf"gitea_view_issue.*#?{issue_number}|" + rf"issue\s+#?{issue_number}.*(?:fetched|viewed).*(?:current|this) session", + re.I, + ) + if pattern.search(text): + return True + return False + + +def assess_reconcile_linked_issue_report( + report_text: str, + *, + reconcile_session: dict | None = None, +) -> dict[str, Any]: + """Validate linked issue status claims in reconciliation handoffs (#300).""" + text = report_text or "" + session = dict(reconcile_session or {}) + fields = _handoff_field_map(text) + reasons: list[str] = [] + + issue_number = session.get("linked_issue_number") + if issue_number is None: + issue_number = _extract_linked_issue_number(text, fields) + + if issue_number is None: + status = _status_value(text, fields) + if status and status.lower() not in _NO_LINKED_ISSUE_VALUES: + if _STATUS_CLAIM_RE.search(status): + reasons.append( + "linked issue status reported without identifying the linked issue (#300)" + ) + if not reasons: + return { + "proven": True, + "block": False, + "reasons": [], + "linked_issue_number": None, + "safe_next_action": "proceed", + } + + status = _status_value(text, fields) + status_lower = status.lower() + + if session.get("issue_missing"): + if status_lower and "not verified" not in status_lower: + reasons.append( + "missing linked issue must be reported as " + "'not verified in this session' (#300)" + ) + elif status: + if _NOT_VERIFIED_RE.search(status): + pass + elif _STATUS_CLAIM_RE.search(status): + if not _live_proof_present(text, session, issue_number): + reasons.append( + "linked issue open/closed/resolved status requires live " + "gitea_view_issue proof in the current session (#300)" + ) + elif status_lower not in _NO_LINKED_ISSUE_VALUES: + if not _live_proof_present(text, session, issue_number): + reasons.append( + "linked issue status claim requires live fetch proof or " + "'not verified in this session' (#300)" + ) + + if _ISSUE_ACTION_RECOMMEND_RE.search(text): + if not _CAPABILITY_PROOF_RE.search(text) and not session.get( + "issue_action_capability_proven" + ): + reasons.append( + "recommended linked issue close/reopen/comment requires " + "exact capability proof (#300)" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": list(dict.fromkeys(reasons)), + "linked_issue_number": issue_number, + "live_proof_present": _live_proof_present(text, session, issue_number), + "safe_next_action": ( + "fetch linked issue with gitea_view_issue in this session or " + "report 'Linked issue status: not verified in this session'" + if reasons + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 8f208c4..5d3b08c 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -146,3 +146,9 @@ def test_non_mergeable_skip_verifier_exported(): from review_proofs import assess_non_mergeable_skip_proof assert callable(assess_non_mergeable_skip_proof) + + +def test_reconcile_linked_issue_verifier_exported(): + from review_proofs import assess_reconcile_linked_issue_report + + assert callable(assess_reconcile_linked_issue_report) diff --git a/tests/test_reviewer_reconcile_linked_issue.py b/tests/test_reviewer_reconcile_linked_issue.py new file mode 100644 index 0000000..1640f4f --- /dev/null +++ b/tests/test_reviewer_reconcile_linked_issue.py @@ -0,0 +1,117 @@ +"""Tests for reconciliation linked-issue live proof verifier (#300).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from reviewer_reconcile_linked_issue import assess_reconcile_linked_issue_report # noqa: E402 + + +def _good_handoff() -> str: + return "\n".join([ + "## PR reconciliation summary", + "Fetched linked issue #263 live with gitea_view_issue in this session.", + "", + "## Controller Handoff", + "- Selected PR: #278", + "- Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED", + "- Linked issue: #263", + "- Linked issue live status: Issue #263 (open)", + "- Safe next action: reconcile open PR", + ]) + + +class TestReconcileLinkedIssueLiveProof(unittest.TestCase): + def test_live_fetch_with_open_status_passes(self): + result = assess_reconcile_linked_issue_report( + _good_handoff(), + reconcile_session={"live_fetched": True, "linked_issue_number": 263}, + ) + self.assertTrue(result["proven"], result["reasons"]) + self.assertEqual(result["linked_issue_number"], 263) + + def test_no_linked_issue_passes(self): + report = "\n".join([ + "## Controller Handoff", + "- Selected PR: #99", + "- Linked issue: none", + "- Linked issue live status: not applicable", + ]) + result = assess_reconcile_linked_issue_report(report) + self.assertTrue(result["proven"]) + self.assertIsNone(result["linked_issue_number"]) + + def test_closed_status_without_live_proof_blocks(self): + report = "\n".join([ + "## Controller Handoff", + "- Linked issue: #263", + "- Linked issue live status: Issue #263 (closed)", + ]) + result = assess_reconcile_linked_issue_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(result["block"]) + self.assertTrue(any("live" in r.lower() for r in result["reasons"])) + + def test_not_verified_wording_passes_without_live_proof(self): + report = "\n".join([ + "## Controller Handoff", + "- Linked issue: #263", + "- Linked issue live status: not verified in this session", + ]) + result = assess_reconcile_linked_issue_report(report) + self.assertTrue(result["proven"], result["reasons"]) + + def test_missing_issue_requires_not_verified(self): + report = "\n".join([ + "## Controller Handoff", + "- Linked issue: #999", + "- Linked issue live status: Issue #999 (open)", + ]) + result = assess_reconcile_linked_issue_report( + report, + reconcile_session={"issue_missing": True, "linked_issue_number": 999}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("not verified" in r.lower() for r in result["reasons"])) + + def test_text_live_proof_without_session_lock_passes(self): + report = _good_handoff() + result = assess_reconcile_linked_issue_report(report) + self.assertTrue(result["proven"], result["reasons"]) + self.assertTrue(result["live_proof_present"]) + + def test_issue_action_recommendation_requires_capability_proof(self): + report = "\n".join([ + _good_handoff(), + "Recommended: close linked issue #263 after PR reconciliation.", + ]) + result = assess_reconcile_linked_issue_report( + report, + reconcile_session={"live_fetched": True}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("capability" in r.lower() for r in result["reasons"])) + + def test_issue_action_with_capability_proof_passes(self): + report = "\n".join([ + _good_handoff(), + "Recommended: close linked issue #263.", + "Capability proof: gitea_close_issue allowed for prgs-author.", + ]) + result = assess_reconcile_linked_issue_report( + report, + reconcile_session={"live_fetched": True}, + ) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestExport(unittest.TestCase): + def test_review_proofs_reexport(self): + from review_proofs import assess_reconcile_linked_issue_report as exported + + self.assertTrue(callable(exported)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 012b8b387a6ee32b8c0df565545d6d944cb44b7b Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:45:51 -0400 Subject: [PATCH 05/15] feat: add dedicated reconciliation controller-handoff schema (Closes #303) Add assess_reconciliation_handoff to review_proofs: already-landed PR reconciliation runs must emit a dedicated handoff schema (selected PR, live state, candidate head SHA, target branch + SHA, ancestor proof, linked issue + live status, eligibility class ALREADY_LANDED_RECONCILE_REQUIRED, capability proofs, per-category mutation ledger, blocker, safe next action, no-review/merge confirmation) instead of author or reviewer fields. Stale fields (PR number opened, Pinned reviewed head, Scratch worktree used, Workspace mutations, Issue lock proof, Claim/comment status) fail validation, a wrong eligibility class fails, and an observed git fetch/pull must be reported under Git ref mutations rather than claimed none. Fails closed on a missing Controller Handoff section. Tests cover blocked reconciliation, successful PR close, comment-only, no-op already-closed, missing linked-issue proof, wrong eligibility class, each stale field, and unreported observed fetch. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 115 +++++++++++++++++++ tests/test_reconciliation_handoff.py | 162 +++++++++++++++++++++++++++ 2 files changed, 277 insertions(+) create mode 100644 tests/test_reconciliation_handoff.py diff --git a/review_proofs.py b/review_proofs.py index cbde5fd..52772bd 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1171,6 +1171,121 @@ def assess_workspace_mutation_consistency(report_text, observed_commands=None): } +# ── Reconciliation controller-handoff schema (Issue #303) ──────────────────── +# +# Already-landed PR reconciliation is neither author issue work nor +# reviewer merge work; its handoff needs its own schema. Stale +# author/reviewer fields fail validation, and observed git ref +# mutations (fetch) must be reported under the precise category. + +RECONCILIATION_ELIGIBILITY_CLASS = "ALREADY_LANDED_RECONCILE_REQUIRED" + +RECONCILIATION_HANDOFF_FIELDS = ( + ("Task", ("task",)), + ("Repo", ("repo",)), + ("Role/profile", ("role/profile", "role", "profile")), + ("Identity", ("identity",)), + ("Selected PR", ("selected pr",)), + ("PR live state", ("pr live state",)), + ("Candidate head SHA", ("candidate head sha",)), + ("Target branch", ("target branch",)), + ("Target branch SHA", ("target branch sha",)), + ("Ancestor proof", ("ancestor proof",)), + ("Linked issue", ("linked issue",)), + ("Linked issue live status", ("linked issue live status",)), + ("Eligibility class", ("eligibility class",)), + ("Capabilities proven", ("capabilities proven",)), + ("Missing capabilities", ("missing capabilities",)), + ("PR comments posted", ("pr comments posted",)), + ("Issue comments posted", ("issue comments posted",)), + ("PRs closed", ("prs closed",)), + ("Issues closed", ("issues closed",)), + ("Git ref mutations", ("git ref mutations",)), + ("Worktree mutations", ("worktree mutations",)), + ("MCP/Gitea mutations", ("mcp/gitea mutations",)), + ("External-state mutations", ("external-state mutations",)), + ("Read-only diagnostics", ("read-only diagnostics",)), + ("Blocker", ("blocker",)), + ("Safe next action", ("safe next action",)), + ("No review/merge confirmation", ("no review/merge",)), +) + +RECONCILIATION_FORBIDDEN_FIELDS = ( + "pr number opened", + "pinned reviewed head", + "scratch worktree used", + "workspace mutations", + "issue lock proof", + "claim/comment status", +) + + +def assess_reconciliation_handoff(report_text, observed_commands=None): + """#303: validate the dedicated reconciliation handoff schema. + + Requires a Controller Handoff section carrying every reconciliation + field, ``Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED``, and + no stale author/reviewer fields. *observed_commands* is the git + command log; an observed fetch/pull must be reported under ``Git ref + mutations`` (never claimed none). + + Returns {'complete', 'downgraded', 'reasons'}; fails closed. + """ + section = _handoff_section_lines(report_text) + if section is None: + return { + "complete": False, + "downgraded": True, + "reasons": [ + "reconciliation report has no section titled exactly " + f"'{HANDOFF_HEADING}'" + ], + } + + fields = {} + for line in section: + stripped = line.strip().lstrip("-*").strip() + if ":" in stripped: + k, v = stripped.split(":", 1) + fields[k.strip().lower()] = v.strip() + + reasons = [] + + for name, aliases in RECONCILIATION_HANDOFF_FIELDS: + if not any(label.startswith(alias) + for label in fields for alias in aliases): + reasons.append( + f"reconciliation handoff missing required field: {name}" + ) + + for stale in RECONCILIATION_FORBIDDEN_FIELDS: + if any(label.startswith(stale) for label in fields): + reasons.append( + f"reconciliation handoff must not include stale field " + f"'{stale}'" + ) + + eligibility = fields.get("eligibility class", "") + if eligibility and RECONCILIATION_ELIGIBILITY_CLASS not in eligibility.upper(): + reasons.append( + "reconciliation handoff eligibility class must be " + f"'{RECONCILIATION_ELIGIBILITY_CLASS}' (got '{eligibility}')" + ) + + _, ref_cmds = _classify_git_commands(observed_commands) + if ref_cmds and _field_claims_none(fields, "git ref mutations"): + reasons.append( + "observed git ref mutation not reported under 'Git ref " + f"mutations': {ref_cmds[0]}" + ) + + return { + "complete": not reasons, + "downgraded": bool(reasons), + "reasons": reasons, + } + + def assess_role_boundary(proof=None, *, task_role=None, namespaces_used=None, justification=None): """Assess reviewer/author role separation for blind queue workflows. diff --git a/tests/test_reconciliation_handoff.py b/tests/test_reconciliation_handoff.py new file mode 100644 index 0000000..baa2787 --- /dev/null +++ b/tests/test_reconciliation_handoff.py @@ -0,0 +1,162 @@ +"""Tests for the dedicated reconciliation controller-handoff schema (#303). + +Already-landed PR reconciliation runs must emit the reconciliation +schema, not stale author/reviewer handoff fields; observed git ref +mutations (fetch) must be reported, and legacy fields fail validation. +""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from review_proofs import assess_reconciliation_handoff # noqa: E402 + + +def _good_handoff(**overrides): + fields = { + "Task": "Reconcile already-landed open PRs", + "Repo": "prgs/Scaled-Tech-Consulting/Gitea-Tools", + "Role/profile": "prgs-reconciler", + "Identity": "jcwalker3", + "Selected PR": "278", + "PR live state": "open", + "Candidate head SHA": "2a544b7", + "Target branch": "master", + "Target branch SHA": "fa0cb12", + "Ancestor proof": "candidate head is ancestor of target branch SHA", + "Linked issue": "263", + "Linked issue live status": "open (fetched live this session)", + "Eligibility class": "ALREADY_LANDED_RECONCILE_REQUIRED", + "Capabilities proven": "gitea.pr.comment", + "Missing capabilities": "gitea.pr.close", + "PR comments posted": "1 (PR #278 reconciliation notice)", + "Issue comments posted": "none", + "PRs closed": "none", + "Issues closed": "none", + "Git ref mutations": "git fetch prgs master", + "Worktree mutations": "none", + "MCP/Gitea mutations": "PR comment on #278", + "External-state mutations": "none", + "Read-only diagnostics": "gitea_view_pr 278, gitea_view_issue 263", + "Blocker": "PR close capability missing", + "Safe next action": "operator closes PR #278 or grants close capability", + "No review/merge confirmation": "no review or merge mutations performed", + } + fields.update(overrides) + lines = ["Controller Handoff"] + lines += [f"- {k}: {v}" for k, v in fields.items() if v is not None] + return "\n".join(lines) + "\n" + + +class TestReconciliationSchemaPasses(unittest.TestCase): + def test_blocked_reconciliation_passes(self): + result = assess_reconciliation_handoff( + _good_handoff(), + observed_commands=["git fetch prgs master"], + ) + self.assertTrue(result["complete"]) + self.assertEqual(result["reasons"], []) + + def test_successful_pr_close_passes(self): + report = _good_handoff(**{ + "Missing capabilities": "none", + "PRs closed": "PR #278", + "Blocker": "none", + "Safe next action": "none; reconciliation complete", + }) + result = assess_reconciliation_handoff( + report, observed_commands=["git fetch prgs master"] + ) + self.assertTrue(result["complete"]) + + def test_comment_only_reconciliation_passes(self): + report = _good_handoff(**{ + "Git ref mutations": "none", + }) + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertTrue(result["complete"]) + + def test_noop_already_closed_passes(self): + report = _good_handoff(**{ + "PR live state": "closed", + "PR comments posted": "none", + "MCP/Gitea mutations": "none", + "Git ref mutations": "none", + "Blocker": "none", + "Safe next action": "none; PR already closed", + }) + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertTrue(result["complete"]) + + +class TestSchemaViolationsFail(unittest.TestCase): + def test_missing_linked_issue_proof_fails(self): + report = _good_handoff(**{"Linked issue live status": None}) + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + self.assertTrue( + any("linked issue live status" in r.lower() + for r in result["reasons"]) + ) + + def test_wrong_eligibility_class_fails(self): + report = _good_handoff(**{"Eligibility class": "REVIEW_ELIGIBLE"}) + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + + def test_missing_handoff_section_fails(self): + result = assess_reconciliation_handoff( + "no handoff here", observed_commands=[] + ) + self.assertFalse(result["complete"]) + + +class TestStaleFieldsFail(unittest.TestCase): + def test_pr_number_opened_fails(self): + report = _good_handoff() + "- PR number opened: 278\n" + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + self.assertTrue( + any("pr number opened" in r.lower() for r in result["reasons"]) + ) + + def test_pinned_reviewed_head_fails(self): + report = _good_handoff() + "- Pinned reviewed head: 2a544b7\n" + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + + def test_scratch_worktree_used_fails(self): + report = _good_handoff() + "- Scratch worktree used: False\n" + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + + def test_workspace_mutations_fails(self): + report = _good_handoff() + "- Workspace mutations: None\n" + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + + def test_author_lock_fields_fail(self): + report = ( + _good_handoff() + + "- Issue lock proof: locked before diff\n" + + "- Claim/comment status: claimed\n" + ) + result = assess_reconciliation_handoff(report, observed_commands=[]) + self.assertFalse(result["complete"]) + + +class TestObservedFetchMustBeReported(unittest.TestCase): + def test_fetch_with_ref_mutations_none_fails(self): + report = _good_handoff(**{"Git ref mutations": "none"}) + result = assess_reconciliation_handoff( + report, observed_commands=["git fetch prgs master"] + ) + self.assertFalse(result["complete"]) + self.assertTrue( + any("git ref mutation" in r.lower() for r in result["reasons"]) + ) + + +if __name__ == "__main__": + unittest.main() From 852ac1f56b572e7aad7ff4cc306e9b77cee1bb03 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:47:16 -0400 Subject: [PATCH 06/15] feat: comment-then-stop policy for partial PR reconciliation (Closes #302) Already-landed PR reconciliation could prove a PR should be closed but stop silently when gitea.pr.close was unavailable, even with comment capability present. Adopt Option B (comment-then-stop) as the durable policy and enforce it: - resolve_partial_reconciliation_plan maps proven capabilities to one of four outcomes: FULL_RECONCILE_CLOSE_ALLOWED (close_pr proven), PARTIAL_RECONCILE_COMMENT_THEN_STOP (comment_pr proven, close_pr missing; one reconciliation comment with PR head SHA, target branch SHA, ancestor proof, linked issue status, and the missing capability, then stop), RECOVERY_HANDOFF_ONLY (no comment capability; no Gitea mutation), and GATE_NOT_PROVEN (no ancestry proof; no mutation regardless of capability). Missing capability dicts fail closed. - assess_partial_reconciliation_report requires final reports to explain why the comment was or was not posted, name the missing capability, and state the close result or that no mutation ran. - workflows/reconcile-landed-pr.md gains section 12A documenting the policy; the workflow contract test locks the new markers. Tests cover close capability present, close missing with comment allowed, comment capability missing, all capabilities missing, unproven ancestry, fail-closed capability dict, and report checks for each outcome. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 160 ++++++++++++++++++ .../workflows/reconcile-landed-pr.md | 22 +++ tests/test_llm_workflow_split.py | 6 + tests/test_review_proofs.py | 129 ++++++++++++++ 4 files changed, 317 insertions(+) diff --git a/review_proofs.py b/review_proofs.py index 77986a7..e34a7c0 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -3581,6 +3581,166 @@ def assess_reviewer_baseline_validation_proof( } +# --------------------------------------------------------------------------- +# Partial reconciliation policy (#302) +# --------------------------------------------------------------------------- + +# Durable policy choice for already-landed PR reconciliation when +# ``gitea.pr.close`` is unavailable: Option B, comment-then-stop. +PARTIAL_RECONCILIATION_POLICY = "comment_then_stop" + +PARTIAL_RECONCILIATION_COMMENT_FIELDS = ( + "PR head SHA", + "target branch SHA", + "ancestor proof", + "linked issue status", + "required missing capability", +) + + +def resolve_partial_reconciliation_plan( + *, + ancestor_proven: bool, + capabilities: dict | None, +) -> dict: + """#302: decide reconciliation mutations from proven capabilities. + + Implements the comment-then-stop policy (Option B): with ancestor + proof but no ``close_pr`` capability, a reconciliation comment with + the full proof is posted when ``comment_pr`` is proven, then the + workflow stops for an authorized close. Missing comment capability + degrades to a recovery handoff with no Gitea mutation. Unproven + ancestry blocks every mutation regardless of capability. + + *capabilities* maps ``close_pr``/``comment_pr``/``close_issue``/ + ``comment_issue`` to proven booleans; ``None`` or missing keys fail + closed. + """ + caps = capabilities or {} + if not ancestor_proven: + return { + "policy": PARTIAL_RECONCILIATION_POLICY, + "outcome": "GATE_NOT_PROVEN", + "allowed_mutations": (), + "required_comment_fields": (), + "report_requirements": ( + "state that ancestry was not proven and no Gitea mutation " + "was performed", + ), + "reasons": [ + "ancestor proof missing; no reconciliation mutation may " + "run (#302)" + ], + "safe_next_action": ( + "run the already-landed ancestry check before any " + "reconciliation mutation" + ), + } + + if caps.get("close_pr") is True: + return { + "policy": PARTIAL_RECONCILIATION_POLICY, + "outcome": "FULL_RECONCILE_CLOSE_ALLOWED", + "allowed_mutations": ("close_pr", "comment_pr"), + "required_comment_fields": (), + "report_requirements": ( + "report the PR close result", + ), + "reasons": [], + "safe_next_action": ( + "close the already-landed PR with the proven close_pr " + "capability and report the close result" + ), + } + + if caps.get("comment_pr") is True: + return { + "policy": PARTIAL_RECONCILIATION_POLICY, + "outcome": "PARTIAL_RECONCILE_COMMENT_THEN_STOP", + "allowed_mutations": ("comment_pr",), + "required_comment_fields": PARTIAL_RECONCILIATION_COMMENT_FIELDS, + "report_requirements": ( + "report that the reconciliation comment was posted", + "report the missing close capability that prevented the " + "PR close", + ), + "reasons": [ + "close_pr capability missing; policy is comment-then-stop " + "(#302)" + ], + "safe_next_action": ( + "post one reconciliation comment carrying the ancestor " + "proof, then stop for a human or authorized close" + ), + } + + return { + "policy": PARTIAL_RECONCILIATION_POLICY, + "outcome": "RECOVERY_HANDOFF_ONLY", + "allowed_mutations": (), + "required_comment_fields": (), + "report_requirements": ( + "report that no reconciliation comment was posted and name " + "the missing capability", + "report that no Gitea mutation was performed", + ), + "reasons": [ + "close_pr and comment_pr capabilities missing; recovery " + "handoff only (#302)" + ], + "safe_next_action": ( + "produce a recovery handoff recording the intended comment " + "and required capabilities; perform no Gitea mutation" + ), + } + + +def assess_partial_reconciliation_report( + report_text: str | None, + *, + plan: dict, +) -> dict: + """#302: final reports must explain why comments were or were not posted.""" + text = (report_text or "").lower() + outcome = (plan or {}).get("outcome", "") + reasons: list[str] = [] + + if outcome == "FULL_RECONCILE_CLOSE_ALLOWED": + if "close" not in text or "result" not in text: + reasons.append( + "full reconciliation report must state the PR close " + "result (#302)" + ) + elif outcome == "PARTIAL_RECONCILE_COMMENT_THEN_STOP": + if "comment" not in text: + reasons.append( + "partial reconciliation report must state that the " + "reconciliation comment was posted (#302)" + ) + if "capability" not in text and "close_pr" not in text: + reasons.append( + "partial reconciliation report must name the missing " + "close capability (#302)" + ) + elif outcome == "RECOVERY_HANDOFF_ONLY": + if "comment" not in text or "capability" not in text: + reasons.append( + "recovery handoff report must explain that no comment was " + "posted and name the missing capability (#302)" + ) + if "no gitea mutation" not in text and "no mutation" not in text: + reasons.append( + "recovery handoff report must state that no Gitea " + "mutation was performed (#302)" + ) + + return { + "complete": not reasons, + "block": bool(reasons), + "reasons": reasons, + } + + # --------------------------------------------------------------------------- # Identity disclosure (#305) # --------------------------------------------------------------------------- diff --git a/skills/llm-project-workflow/workflows/reconcile-landed-pr.md b/skills/llm-project-workflow/workflows/reconcile-landed-pr.md index 80a7c4b..10ba60d 100644 --- a/skills/llm-project-workflow/workflows/reconcile-landed-pr.md +++ b/skills/llm-project-workflow/workflows/reconcile-landed-pr.md @@ -248,6 +248,28 @@ Post a reconciliation comment only if: If comment capability is missing, record the intended comment in the final handoff only. +## 12A. Partial reconciliation policy (#302) + +The durable policy when `close_pr` capability is missing is +**comment-then-stop** (`resolve_partial_reconciliation_plan` in +`review_proofs.py` enforces it): + +* `close_pr` proven → full reconciliation: close the PR and report the + close result (`FULL_RECONCILE_CLOSE_ALLOWED`). +* `close_pr` missing, `comment_pr` proven → post exactly one + reconciliation comment carrying PR head SHA, target branch SHA, + ancestor proof, linked issue status, and the required missing + capability, then stop for a human or authorized close + (`PARTIAL_RECONCILE_COMMENT_THEN_STOP`). +* `comment_pr` also missing → no Gitea mutation; produce a recovery + handoff recording the intended comment and required capabilities + (`RECOVERY_HANDOFF_ONLY`). +* Ancestry not proven → no mutation regardless of capability + (`GATE_NOT_PROVEN`). + +Final reports must explain why the comment was or was not posted and +name the missing capability (`assess_partial_reconciliation_report`). + ## 13. PR close rules Close a PR only if: diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 8f208c4..741d028 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -78,6 +78,12 @@ def test_reconcile_landed_workflow_contract(): assert "canonical: true" in text assert "Do not review or merge normal PRs" in text assert "Already-landed proof" in text + # Issue #302: partial reconciliation policy must stay documented. + assert "## 12A. Partial reconciliation policy (#302)" in text + assert "comment-then-stop" in text + assert "PARTIAL_RECONCILE_COMMENT_THEN_STOP" in text + assert "RECOVERY_HANDOFF_ONLY" in text + assert "resolve_partial_reconciliation_plan" in text def test_create_issue_workflow_contract(): diff --git a/tests/test_review_proofs.py b/tests/test_review_proofs.py index 3a3ea7a..6403381 100644 --- a/tests/test_review_proofs.py +++ b/tests/test_review_proofs.py @@ -24,6 +24,8 @@ from review_proofs import ( # noqa: E402 ISSUE_SELECTION_CONTINUATION_EXPLICIT, ISSUE_SELECTION_REPRESENTED_BY_OPEN_PR, assess_author_pr_report, + assess_partial_reconciliation_report, + resolve_partial_reconciliation_plan, assess_queue_ordering_proof, assess_reviewer_baseline_validation_proof, assess_capability_evidence, @@ -2539,5 +2541,132 @@ class TestReviewerBaselineValidationProof(unittest.TestCase): self.assertFalse(report["baseline_validation_proven"]) +class TestPartialReconciliationPlan(unittest.TestCase): + """Issue #302: policy when PR-close capability is missing (Option B).""" + + def _plan(self, **overrides): + kwargs = { + "ancestor_proven": True, + "capabilities": { + "close_pr": False, + "comment_pr": True, + "close_issue": True, + "comment_issue": True, + }, + } + kwargs.update(overrides) + return resolve_partial_reconciliation_plan(**kwargs) + + def test_close_capability_present_allows_full_reconcile(self): + plan = self._plan(capabilities={ + "close_pr": True, "comment_pr": True, + "close_issue": True, "comment_issue": True, + }) + self.assertEqual(plan["outcome"], "FULL_RECONCILE_CLOSE_ALLOWED") + self.assertIn("close_pr", plan["allowed_mutations"]) + + def test_close_missing_with_comment_posts_comment_then_stops(self): + plan = self._plan() + self.assertEqual( + plan["outcome"], "PARTIAL_RECONCILE_COMMENT_THEN_STOP") + self.assertEqual(plan["allowed_mutations"], ("comment_pr",)) + for field in ( + "PR head SHA", + "target branch SHA", + "ancestor proof", + "linked issue status", + "required missing capability", + ): + self.assertIn(field, plan["required_comment_fields"]) + + def test_comment_capability_missing_produces_handoff_only(self): + plan = self._plan(capabilities={ + "close_pr": False, "comment_pr": False, + "close_issue": True, "comment_issue": True, + }) + self.assertEqual(plan["outcome"], "RECOVERY_HANDOFF_ONLY") + self.assertEqual(plan["allowed_mutations"], ()) + + def test_all_capabilities_missing_produces_handoff_only(self): + plan = self._plan(capabilities={ + "close_pr": False, "comment_pr": False, + "close_issue": False, "comment_issue": False, + }) + self.assertEqual(plan["outcome"], "RECOVERY_HANDOFF_ONLY") + self.assertEqual(plan["allowed_mutations"], ()) + + def test_unproven_ancestry_blocks_all_mutations(self): + plan = self._plan(ancestor_proven=False, capabilities={ + "close_pr": True, "comment_pr": True, + "close_issue": True, "comment_issue": True, + }) + self.assertEqual(plan["outcome"], "GATE_NOT_PROVEN") + self.assertEqual(plan["allowed_mutations"], ()) + + def test_missing_capability_dict_fails_closed(self): + plan = self._plan(capabilities=None) + self.assertEqual(plan["outcome"], "RECOVERY_HANDOFF_ONLY") + self.assertEqual(plan["allowed_mutations"], ()) + + +class TestPartialReconciliationReport(unittest.TestCase): + """Issue #302: reports must explain why comments were or were not posted.""" + + def _plan(self, outcome): + capabilities = { + "FULL_RECONCILE_CLOSE_ALLOWED": { + "close_pr": True, "comment_pr": True, + "close_issue": True, "comment_issue": True, + }, + "PARTIAL_RECONCILE_COMMENT_THEN_STOP": { + "close_pr": False, "comment_pr": True, + "close_issue": True, "comment_issue": True, + }, + "RECOVERY_HANDOFF_ONLY": { + "close_pr": False, "comment_pr": False, + "close_issue": False, "comment_issue": False, + }, + }[outcome] + return resolve_partial_reconciliation_plan( + ancestor_proven=True, capabilities=capabilities) + + def test_partial_report_requires_comment_and_missing_capability(self): + plan = self._plan("PARTIAL_RECONCILE_COMMENT_THEN_STOP") + good = ( + "Reconciliation comment posted on PR #278 with ancestor proof. " + "PR close skipped: close capability gitea.pr.close missing; " + "stopped for authorized close." + ) + result = assess_partial_reconciliation_report(good, plan=plan) + self.assertTrue(result["complete"]) + + bad = "Stopped. Nothing done." + result = assess_partial_reconciliation_report(bad, plan=plan) + self.assertTrue(result["block"]) + + def test_handoff_only_report_requires_no_comment_explanation(self): + plan = self._plan("RECOVERY_HANDOFF_ONLY") + good = ( + "No reconciliation comment posted: comment capability missing. " + "No Gitea mutation performed; recovery handoff produced." + ) + result = assess_partial_reconciliation_report(good, plan=plan) + self.assertTrue(result["complete"]) + + bad = "Recovery handoff produced." + result = assess_partial_reconciliation_report(bad, plan=plan) + self.assertTrue(result["block"]) + + def test_full_reconcile_report_requires_close_result(self): + plan = self._plan("FULL_RECONCILE_CLOSE_ALLOWED") + good = "PR close result: closed PR #278 after ancestor proof." + result = assess_partial_reconciliation_report(good, plan=plan) + self.assertTrue(result["complete"]) + + bad = "Reconciliation done." + result = assess_partial_reconciliation_report(bad, plan=plan) + self.assertTrue(result["block"]) + + if __name__ == "__main__": unittest.main() From 6e774e191a1ef570031db78b73204916d979c1c0 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:08:38 -0400 Subject: [PATCH 07/15] feat: require pagination proof in reconciliation PR inventory (#308) Add assess_reconcile_inventory_report verifier to block complete queue scan and all-already-landed claims without final-page metadata, requested page size, pages fetched, per-page counts, and no-next-page proof. Wire into build_final_report and export from review_proofs. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 27 ++++ reviewer_reconcile_inventory.py | 162 +++++++++++++++++++++ tests/test_llm_workflow_split.py | 6 + tests/test_reviewer_reconcile_inventory.py | 102 +++++++++++++ 4 files changed, 297 insertions(+) create mode 100644 reviewer_reconcile_inventory.py create mode 100644 tests/test_reviewer_reconcile_inventory.py diff --git a/review_proofs.py b/review_proofs.py index fdebea9..62344f0 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1476,6 +1476,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, if report_text else {"proven": True, "block": False, "reasons": [], "violations": []} ) + reconcile_inventory = ( + assess_reconcile_inventory_report(report_text) + if report_text and _RECONCILE_INVENTORY_HINT.search(report_text) + else {"proven": True, "block": False, "reasons": []} + ) if baseline_validation is None and report_text: baseline_validation = assess_reviewer_baseline_validation_proof( report_text=report_text, @@ -1632,6 +1637,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue-status report violates loaded workflow proof gates (#339)" ) downgrade_reasons.extend(queue_status_report.get("reasons", [])) + if not reconcile_inventory.get("proven"): + downgrade_reasons.append( + "reconciliation PR inventory lacks pagination proof (#308)" + ) + downgrade_reasons.extend(reconcile_inventory.get("reasons", [])) if not baseline_validation.get("proven"): downgrade_reasons.append( "reviewer baseline validation proof missing or failed (#325)" @@ -1717,6 +1727,10 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue_status_violations": list( queue_status_report.get("violations") or [] ), + "reconcile_inventory_proven": bool(reconcile_inventory.get("proven")), + "reconcile_inventory_violations": list( + reconcile_inventory.get("reasons") or [] + ), "baseline_validation_proven": bool(baseline_validation.get("proven")), "baseline_validation_violations": list( baseline_validation.get("violations") or [] @@ -3753,6 +3767,12 @@ _QUEUE_STATUS_REPORT_HINT = re.compile( re.I, ) +_RECONCILE_INVENTORY_HINT = re.compile( + r"already[- ]landed reconciliation|reconcile[- ]landed|" + r"open pr inventory|inventory pagination proof", + re.I, +) + _GATE_PASSED_VALUE = re.compile(r"\bpassed\b", re.I) _NOT_APPLICABLE_VALUE = re.compile( @@ -4142,3 +4162,10 @@ def assess_non_mergeable_skip_proof(report_text, **kwargs): from reviewer_mergeability_skip import assess_non_mergeable_skip_proof as _assess return _assess(report_text, **kwargs) + + +def assess_reconcile_inventory_report(report_text, **kwargs): + """#308: prove PR inventory pagination in reconciliation reports.""" + from reviewer_reconcile_inventory import assess_reconcile_inventory_report as _assess + + return _assess(report_text, **kwargs) diff --git a/reviewer_reconcile_inventory.py b/reviewer_reconcile_inventory.py new file mode 100644 index 0000000..9dfc0e5 --- /dev/null +++ b/reviewer_reconcile_inventory.py @@ -0,0 +1,162 @@ +"""Reconciliation PR inventory pagination proof verifier (#308).""" + +from __future__ import annotations + +import re +from typing import Any + +_COMPLETE_SCAN_CLAIM_RE = re.compile( + r"\b(?:all already[- ]landed(?:\s+prs?)?(?:\s+were)?\s+found|" + r"complete queue scan|all open prs? (?:were )?(?:found|checked|inventoried|listed)|" + r"inventory (?:is )?complete|exhaustive (?:pr )?inventory|" + r"no additional already[- ]landed|scanned (?:the )?(?:full )?open pr queue)\b", + re.I, +) + +_FINALITY_RE = re.compile( + r"is_final_page\s*:\s*true|has_more\s*:\s*false|no next page|final[- ]page|" + r"pagination_complete\s*:\s*true|inventory pagination proof\s*:\s*(?!assumed|none\b).+", + re.I, +) + +_REQUESTED_PAGE_SIZE_RE = re.compile( + r"requested page size\s*:\s*(\d+)|page[- ]?size\s*(?:=|:)\s*(\d+)", + re.I, +) +_PAGES_FETCHED_RE = re.compile( + r"pages? fetched\s*:\s*(\d+)|page count\s*:\s*(\d+)", + re.I, +) +_RETURNED_PER_PAGE_RE = re.compile( + r"returned pr count(?: per page)?\s*:|open pr count per page|" + r"returned \d+ open prs? per page|per[- ]page counts?\s*:", + re.I, +) +_EXACT_LIMIT_RETURN_RE = re.compile( + r"returned\s+(\d+)\s+open prs?|listed\s+(\d+)\s+open prs?", + re.I, +) + +_DEFAULT_PAGE_SIZE_ASSUMPTION = re.compile( + r"(?:less|fewer) than (?:the )?(?:default )?(?:gitea )?page[- ]?(?:size|limit)|" + r"default (?:gitea )?page[- ]?size|assumed complete", + re.I, +) + + +def _int_from_groups(match: re.Match[str] | None) -> int | None: + if not match: + return None + for group in match.groups(): + if group: + return int(group) + return None + + +def _pagination_proven(text: str, session: dict) -> bool: + if session.get("pagination_complete") or session.get("inventory_complete"): + return True + if _FINALITY_RE.search(text): + return True + pages = session.get("pages_fetched") + if isinstance(pages, int) and pages >= 1 and session.get("is_final_page"): + return True + return False + + +def _metadata_present(text: str, session: dict) -> list[str]: + missing: list[str] = [] + has_page_size = bool( + _REQUESTED_PAGE_SIZE_RE.search(text) + or session.get("requested_page_size") + ) + has_pages_fetched = bool( + _PAGES_FETCHED_RE.search(text) or session.get("pages_fetched") + ) + has_returned_counts = bool( + _RETURNED_PER_PAGE_RE.search(text) or session.get("returned_per_page") + ) + if not has_page_size: + missing.append("requested page size") + if not has_pages_fetched: + missing.append("page count fetched") + if not has_returned_counts: + missing.append("returned PR count per page") + if not _pagination_proven(text, session): + missing.append("final-page/no-next-page proof") + return missing + + +def assess_reconcile_inventory_report( + report_text: str, + *, + inventory_session: dict | None = None, +) -> dict[str, Any]: + """Validate PR inventory pagination proof in reconciliation reports (#308).""" + text = report_text or "" + session = dict(inventory_session or {}) + reasons: list[str] = [] + + claims_scan = bool(_COMPLETE_SCAN_CLAIM_RE.search(text)) + lists_open_prs = bool( + re.search(r"open prs? listed|listed open prs?|pr inventory", text, re.I) + ) + + if not claims_scan and not lists_open_prs and not session.get("inventory_required"): + return { + "proven": True, + "block": False, + "reasons": [], + "inventory_claimed": False, + "safe_next_action": "proceed", + } + + if _DEFAULT_PAGE_SIZE_ASSUMPTION.search(text) and not _pagination_proven(text, session): + reasons.append( + "reconciliation inventory assumed complete from page-size guess " + "without final-page proof (#308)" + ) + + if claims_scan and not _pagination_proven(text, session): + reasons.append( + "complete queue scan or all already-landed claim requires " + "pagination finality proof (#308)" + ) + + missing = _metadata_present(text, session) + if (claims_scan or lists_open_prs) and missing: + reasons.append( + "reconciliation inventory missing pagination metadata: " + + ", ".join(missing) + ) + + requested = session.get("requested_page_size") + returned = session.get("returned_page_size") + if isinstance(requested, int) and isinstance(returned, int): + if returned >= requested > 0 and not _pagination_proven(text, session): + reasons.append( + "exactly-full first reconciliation page without final-page proof (#308)" + ) + else: + for match in _EXACT_LIMIT_RETURN_RE.finditer(text): + count = _int_from_groups(match) + if count in {10, 20, 50} and not _pagination_proven(text, session): + reasons.append( + "exactly-full first reconciliation page without final-page proof (#308)" + ) + break + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": list(dict.fromkeys(reasons)), + "inventory_claimed": claims_scan or lists_open_prs, + "pagination_proven": _pagination_proven(text, session), + "safe_next_action": ( + "include requested page size, pages fetched, per-page counts, and " + "final-page/no-next-page proof before claiming complete scan" + if reasons + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 8f208c4..cfb916a 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -146,3 +146,9 @@ def test_non_mergeable_skip_verifier_exported(): from review_proofs import assess_non_mergeable_skip_proof assert callable(assess_non_mergeable_skip_proof) + + +def test_reconcile_inventory_verifier_exported(): + from review_proofs import assess_reconcile_inventory_report + + assert callable(assess_reconcile_inventory_report) diff --git a/tests/test_reviewer_reconcile_inventory.py b/tests/test_reviewer_reconcile_inventory.py new file mode 100644 index 0000000..7dfb6b9 --- /dev/null +++ b/tests/test_reviewer_reconcile_inventory.py @@ -0,0 +1,102 @@ +"""Tests for reconciliation inventory pagination verifier (#308).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from reviewer_reconcile_inventory import assess_reconcile_inventory_report # noqa: E402 + + +def _good_report() -> str: + return "\n".join([ + "## Already-landed reconciliation", + "Open PR inventory:", + "- Requested page size: 50", + "- Pages fetched: 2", + "- Returned PR count per page: page1=50, page2=6", + "- Inventory pagination proof: is_final_page: true, has_more: false", + "All already-landed PRs in the scanned open queue were found.", + "", + "## Controller Handoff", + "- Selected PR: #278", + "- Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED", + ]) + + +class TestReconcileInventoryPagination(unittest.TestCase): + def test_complete_metadata_passes(self): + result = assess_reconcile_inventory_report(_good_report()) + self.assertTrue(result["proven"], result["reasons"]) + self.assertTrue(result["pagination_proven"]) + + def test_no_inventory_claim_skips_checks(self): + report = "## Controller Handoff\n- Selected PR: #278" + result = assess_reconcile_inventory_report(report) + self.assertTrue(result["proven"]) + self.assertFalse(result["inventory_claimed"]) + + def test_first_page_below_limit_with_finality_passes(self): + report = "\n".join([ + "Open PRs listed for reconciliation.", + "- Requested page size: 50", + "- Pages fetched: 1", + "- Returned PR count per page: page1=6", + "- Inventory pagination proof: is_final_page: true", + ]) + result = assess_reconcile_inventory_report(report) + self.assertTrue(result["proven"], result["reasons"]) + + def test_exactly_full_first_page_without_finality_blocks(self): + report = "\n".join([ + "Complete queue scan of open PRs.", + "- Requested page size: 50", + "- Pages fetched: 1", + "- Returned PR count per page: page1=50", + "Listed 50 open PRs on the first page.", + ]) + result = assess_reconcile_inventory_report( + report, + inventory_session={"requested_page_size": 50, "returned_page_size": 50}, + ) + self.assertFalse(result["proven"]) + self.assertTrue(any("full first" in r.lower() for r in result["reasons"])) + + def test_missing_pagination_metadata_blocks(self): + report = "All open PRs were inventoried. Inventory is complete." + result = assess_reconcile_inventory_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("metadata" in r.lower() for r in result["reasons"])) + + def test_default_page_size_assumption_blocks(self): + report = ( + "Exhaustive PR inventory: fewer than the default Gitea page size returned." + ) + result = assess_reconcile_inventory_report(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("page-size" in r.lower() or "page size" in r.lower() + for r in result["reasons"])) + + def test_session_pagination_complete_passes(self): + report = "All already-landed PRs were found after scanning open PRs." + result = assess_reconcile_inventory_report( + report, + inventory_session={ + "pagination_complete": True, + "requested_page_size": 50, + "pages_fetched": 2, + "returned_per_page": [50, 3], + }, + ) + self.assertTrue(result["proven"], result["reasons"]) + + +class TestExport(unittest.TestCase): + def test_review_proofs_reexport(self): + from review_proofs import assess_reconcile_inventory_report as exported + + self.assertTrue(callable(exported)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 6743cc11b92d1d6671b886a2b65a3838b4d1e2dc Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:11:15 -0400 Subject: [PATCH 08/15] feat: add native MCP preference gate and shell circuit breaker (Closes #270) Enforce native MCP-first paths for Gitea mutations with fail-closed assessment helpers, shell spawn circuit breaker tracking, and MCP tools for operation-path assessment and shell health reporting. Closes #270 --- gitea_mcp_server.py | 73 ++++++ native_mcp_preference.py | 369 ++++++++++++++++++++++++++++ tests/test_native_mcp_preference.py | 121 +++++++++ 3 files changed, 563 insertions(+) create mode 100644 native_mcp_preference.py create mode 100644 tests/test_native_mcp_preference.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index b5415eb..65bd6a5 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -477,6 +477,7 @@ import review_proofs # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import merged_cleanup_reconcile # noqa: E402 +import native_mcp_preference # noqa: E402 # Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, @@ -4082,6 +4083,14 @@ _GUIDE_RULES = { "inventory, summarize) needs no explicit authorization. Enforced " "fail closed via subagent_gate.assess_subagent_delegation and " "validate_subagent_report (#266)."), + "native_mcp_preference": ( + "Gitea mutations must use native MCP tools first. When MCP is " + "available, block shell scripts, direct API calls, WebFetch, " + "Playwright, helper encoders, and profile-secret reads unless " + "explicit recovery-mode proof is complete. If MCP transport is " + "broken or the shell circuit breaker trips, emit a terminal recovery " + "report instead of improvising unsafe fallback. Reviewers must not " + "touch or restart MCP server files/processes (#270)."), } _COMMON_WORKFLOWS = [ @@ -4786,6 +4795,69 @@ def _build_runtime_task_capabilities( } +@mcp.tool() +@mcp.tool() +def gitea_assess_gitea_operation_path( + task: str, + path_kind: str | None = None, + command_or_detail: str | None = None, + mcp_available: bool = True, + mcp_tool_visible: bool = True, + recovery_mode: bool = False, + recovery_proof_complete: bool = False, + remote: str = "dadeschools", + host: str | None = None, +) -> dict: + """Read-only: assess whether a proposed Gitea mutation path is allowed (#270). + + Native MCP must be preferred when available. Shell scripts, direct API + calls, WebFetch, Playwright, and helper encoders are blocked unless + explicit recovery-mode proof is supplied. + """ + profile = get_profile() + role = _role_kind( + profile.get("allowed_operations") or [], + profile.get("forbidden_operations") or [], + ) + return native_mcp_preference.assess_gitea_operation_path( + task=task, + path_kind=path_kind, + command_or_detail=command_or_detail, + mcp_available=mcp_available, + mcp_tool_visible=mcp_tool_visible, + recovery_mode=recovery_mode, + recovery_proof_complete=recovery_proof_complete, + role=role, + active_profile=profile.get("profile_name"), + ) + + +@mcp.tool() +def gitea_record_shell_spawn_outcome( + exit_code: int | None = None, + stdout: str = "", + stderr: str = "", + spawn_failure: bool | None = None, + probe_attempted: bool = False, + probe_succeeded: bool | None = None, +) -> dict: + """Record shell spawn outcome and update the session circuit breaker (#270).""" + return native_mcp_preference.record_shell_spawn_outcome( + exit_code=exit_code, + stdout=stdout, + stderr=stderr, + spawn_failure=spawn_failure, + probe_attempted=probe_attempted, + probe_succeeded=probe_succeeded, + ) + + +@mcp.tool() +def gitea_get_shell_health() -> dict: + """Read-only: return shell spawn circuit-breaker state for this MCP session (#270).""" + return native_mcp_preference.shell_health_status() + + @mcp.tool() def gitea_get_runtime_context( remote: str = "dadeschools", @@ -4904,6 +4976,7 @@ def gitea_get_runtime_context( "preflight_block_reasons": preflight["preflight_block_reasons"], "preflight_workspace": preflight.get("preflight_workspace"), "session_capabilities": session_capabilities, + "shell_health": native_mcp_preference.shell_health_status(), } if reveal and h: diff --git a/native_mcp_preference.py b/native_mcp_preference.py new file mode 100644 index 0000000..872b800 --- /dev/null +++ b/native_mcp_preference.py @@ -0,0 +1,369 @@ +"""Native MCP preference gate and shell health circuit breaker (#270). + +Gitea mutations must use native MCP tools first. Shell scripts, direct API +calls, browser helpers, and improvised encoders are blocked when MCP is +available unless explicit recovery-mode proof is supplied. +""" + +from __future__ import annotations + +import re +from typing import Any + +from reviewer_fallback import LOCAL_GITEA_SCRIPT_NAMES + +# Tasks that must prefer native MCP over shell/API/helper fallbacks. +GITEA_MUTATION_TASKS = frozenset({ + "comment_issue", + "mark_issue", + "lock_issue", + "set_issue_labels", + "create_issue", + "close_issue", + "create_branch", + "push_branch", + "create_pr", + "close_pr", + "comment_pr", + "review_pr", + "merge_pr", + "approve_pr", + "request_changes_pr", + "commit_files", + "gitea_commit_files", + "delete_branch", + "address_pr_change_requests", +}) + +ALLOWED_PATH_KINDS = frozenset({ + "mcp_native", + "recovery_fallback", +}) + +BLOCKED_PATH_KINDS = frozenset({ + "shell_script", + "direct_api", + "webfetch", + "playwright", + "helper_script", + "unsafe_helper", + "mcp_server_touch", +}) + +SHELL_SPAWN_FAILURE_THRESHOLD = 2 + +TERMINAL_REPORT_HEADING = ( + "MCP transport unavailable or shell circuit breaker tripped. " + "No unsafe Gitea fallback performed." +) + +_RECOVERY_MODE_RE = re.compile( + r"\b(?:recovery mode|explicit recovery|mcp unavailable|mcp not available|" + r"mcp tools unavailable|no mcp path)\b", + re.IGNORECASE, +) +_LOCAL_SCRIPT_RE = re.compile( + r"(?:^|[\s\"'`/])(?:python3?|bash|sh)?\s*(?:" + + "|".join(re.escape(name) for name in LOCAL_GITEA_SCRIPT_NAMES) + + r")", + re.IGNORECASE, +) +_WEBFETCH_RE = re.compile(r"\b(?:webfetch|mcp_web_fetch|fetch\s+url)\b", re.IGNORECASE) +_PLAYWRIGHT_RE = re.compile(r"\b(?:playwright|browser_navigate|browser_click)\b", re.IGNORECASE) +_HELPER_SCRIPT_RE = re.compile( + r"(?:^|[\s\"'`/])(?:_encode_|_emit_|_inline_)[\w-]+\.py", + re.IGNORECASE, +) +_MANUAL_BASE64_RE = re.compile( + r"\b(?:manual\s+base64|llm-generated\s+base64|base64-encode\s+in\s+chat)\b", + re.IGNORECASE, +) +_DIRECT_API_RE = re.compile( + r"\b(?:api_request|urllib\.request|requests\.(?:post|patch|put)|curl\s+-X\s+POST)\b", + re.IGNORECASE, +) +_MCP_SERVER_TOUCH_RE = re.compile( + r"\b(?:kill|pkill|restart|reload|touch|edit|modify|write)\b.{0,40}\b(?:mcp_server|mcp-server|gitea_mcp_server)\b", + re.IGNORECASE, +) +_CLI_AUTH_DIVERGENCE_RE = re.compile( + r"GITEA_MCP_PROFILE\s*=\s*['\"]?([^\s'\";]+)", + re.IGNORECASE, +) + +_session_shell: dict[str, Any] = { + "consecutive_spawn_failures": 0, + "shell_unavailable": False, + "hard_stopped": False, + "last_exit_code": None, +} + + +def _clean(value: str | None) -> str: + return (value or "").strip() + + +def is_spawn_failure( + *, + exit_code: int | None = None, + stdout: str | None = None, + stderr: str | None = None, + spawn_failure: bool | None = None, +) -> bool: + """True for executor spawn failures (exit_code -1, empty output).""" + if spawn_failure is True: + return True + if spawn_failure is False: + return False + if exit_code != -1: + return False + return not (_clean(stdout) or _clean(stderr)) + + +def record_shell_spawn_outcome( + *, + exit_code: int | None = None, + stdout: str | None = None, + stderr: str | None = None, + spawn_failure: bool | None = None, + probe_attempted: bool = False, + probe_succeeded: bool | None = None, +) -> dict[str, Any]: + """Track shell spawn outcomes and trip the session circuit breaker (#270 AC4).""" + global _session_shell + failed = is_spawn_failure( + exit_code=exit_code, + stdout=stdout, + stderr=stderr, + spawn_failure=spawn_failure, + ) + + if failed: + _session_shell["consecutive_spawn_failures"] = ( + int(_session_shell.get("consecutive_spawn_failures") or 0) + 1 + ) + _session_shell["last_exit_code"] = exit_code + if probe_attempted and probe_succeeded is False: + _session_shell["shell_unavailable"] = True + else: + _session_shell["consecutive_spawn_failures"] = 0 + _session_shell["shell_unavailable"] = False + _session_shell["hard_stopped"] = False + _session_shell["last_exit_code"] = exit_code + + failures = int(_session_shell["consecutive_spawn_failures"]) + if failures >= SHELL_SPAWN_FAILURE_THRESHOLD: + _session_shell["shell_unavailable"] = True + _session_shell["hard_stopped"] = True + + return shell_health_status() + + +def shell_health_status() -> dict[str, Any]: + """Return the current shell health / circuit-breaker state.""" + failures = int(_session_shell.get("consecutive_spawn_failures") or 0) + hard_stopped = bool(_session_shell.get("hard_stopped")) + shell_unavailable = bool(_session_shell.get("shell_unavailable")) + return { + "consecutive_spawn_failures": failures, + "shell_unavailable": shell_unavailable, + "hard_stopped": hard_stopped, + "threshold": SHELL_SPAWN_FAILURE_THRESHOLD, + "shell_use_allowed": not hard_stopped, + "last_exit_code": _session_shell.get("last_exit_code"), + "safe_next_action": ( + "emit terminal recovery report; prefer native MCP for remaining Gitea mutations" + if hard_stopped + else ( + "probe shell once with echo/pwd; if probe fails mark shell unavailable" + if failures == 1 + else "shell healthy" + ) + ), + } + + +def clear_shell_health_for_tests() -> None: + """Reset session shell state (tests only).""" + global _session_shell + _session_shell = { + "consecutive_spawn_failures": 0, + "shell_unavailable": False, + "hard_stopped": False, + "last_exit_code": None, + } + + +def classify_command_path(command_or_detail: str | None) -> str: + """Classify a proposed command/detail into a path kind.""" + text = command_or_detail or "" + if _MCP_SERVER_TOUCH_RE.search(text): + return "mcp_server_touch" + if _WEBFETCH_RE.search(text): + return "webfetch" + if _PLAYWRIGHT_RE.search(text): + return "playwright" + if _HELPER_SCRIPT_RE.search(text) or _MANUAL_BASE64_RE.search(text): + return "unsafe_helper" + if _LOCAL_SCRIPT_RE.search(text): + return "shell_script" + if _DIRECT_API_RE.search(text): + return "direct_api" + return "mcp_native" + + +def detect_cli_auth_divergence( + command_or_detail: str | None, + *, + active_profile: str | None, +) -> list[str]: + """Flag shell commands that override GITEA_MCP_PROFILE away from the session.""" + reasons: list[str] = [] + text = command_or_detail or "" + active = _clean(active_profile) + match = _CLI_AUTH_DIVERGENCE_RE.search(text) + if not match or not active: + return reasons + requested = _clean(match.group(1)) + if requested and requested != active: + reasons.append( + f"CLI auth divergence: command sets GITEA_MCP_PROFILE='{requested}' " + f"but active session profile is '{active}' (fail closed)" + ) + return reasons + + +def format_mcp_unavailable_terminal_report( + *, + task: str | None = None, + reasons: list[str] | None = None, +) -> str: + """Terminal report when MCP is broken and unsafe recovery is forbidden (#270 AC3).""" + lines = [TERMINAL_REPORT_HEADING, ""] + if task: + lines.append(f"Blocked task: {task}") + if reasons: + lines.extend(["", "Reasons:"]) + lines.extend(f"- {reason}" for reason in reasons) + lines.extend([ + "", + "Required recovery (no improvised fallback):", + "- Restart the MCP session", + "- Kill hung background terminals holding the shell executor", + "- Retry the native MCP tool once after reconnect", + "- If MCP remains unavailable, stop and hand off to the operator", + "- Do not run local Gitea scripts, WebFetch, Playwright, or manual base64 encoders", + ]) + return "\n".join(lines) + + +def assess_gitea_operation_path( + *, + task: str, + path_kind: str | None = None, + command_or_detail: str | None = None, + mcp_available: bool = True, + mcp_tool_visible: bool = True, + recovery_mode: bool = False, + recovery_proof_complete: bool = False, + role: str | None = None, + active_profile: str | None = None, +) -> dict[str, Any]: + """Fail closed when a Gitea mutation would bypass native MCP (#270).""" + task_name = _clean(task) + resolved_kind = _clean(path_kind) or classify_command_path(command_or_detail) + reasons: list[str] = [] + + if task_name and task_name not in GITEA_MUTATION_TASKS: + reasons.append( + f"unknown or non-mutation Gitea task '{task_name}'; " + "native MCP preference gate applies only to Gitea mutations" + ) + + shell = shell_health_status() + if shell["hard_stopped"] and resolved_kind != "mcp_native": + reasons.append( + "shell circuit breaker is hard-stopped after consecutive spawn failures; " + "use native MCP or emit terminal recovery report" + ) + + recovery_declared = recovery_mode or bool( + _RECOVERY_MODE_RE.search(command_or_detail or "") + ) + recovery_allowed = ( + recovery_declared + and recovery_proof_complete + and not mcp_available + and resolved_kind in BLOCKED_PATH_KINDS - {"mcp_server_touch"} + ) + + if resolved_kind in BLOCKED_PATH_KINDS and not recovery_allowed: + reasons.append(f"path kind '{resolved_kind}' is not an approved Gitea mutation path") + + if resolved_kind == "mcp_server_touch": + if _clean(role) == "reviewer" or ( + active_profile and "reviewer" in active_profile.lower() + ): + reasons.append( + "reviewer workflows must not touch, restart, or kill MCP server files/processes" + ) + else: + reasons.append( + "MCP server files/processes must not be modified during normal Gitea workflows" + ) + + reasons.extend( + detect_cli_auth_divergence(command_or_detail, active_profile=active_profile) + ) + + if ( + mcp_available + and mcp_tool_visible + and resolved_kind != "mcp_native" + and not recovery_allowed + ): + if not recovery_declared: + reasons.append( + "native MCP tools are available; shell/API/helper fallback is forbidden" + ) + elif not recovery_proof_complete: + reasons.append( + "recovery-mode fallback requires complete recovery proof before proceeding" + ) + + if not mcp_available and resolved_kind != "mcp_native" and not recovery_allowed: + if not recovery_declared: + reasons.append( + "MCP transport unavailable; produce terminal recovery report instead of " + "improvising unsafe fallback" + ) + + block = bool(reasons) + terminal_report = None + if block and (not mcp_available or shell["hard_stopped"]): + terminal_report = format_mcp_unavailable_terminal_report( + task=task_name or None, + reasons=reasons, + ) + + return { + "task": task_name or None, + "path_kind": resolved_kind, + "mcp_available": mcp_available, + "mcp_tool_visible": mcp_tool_visible, + "recovery_mode": recovery_declared, + "shell_health": shell, + "block": block, + "allowed": not block, + "reasons": reasons, + "terminal_report": terminal_report, + "safe_next_action": ( + "use the native MCP tool for this task" + if mcp_available and mcp_tool_visible and block + else ( + terminal_report or "proceed with native MCP" + if block + else "proceed with native MCP" + ) + ), + } \ No newline at end of file diff --git a/tests/test_native_mcp_preference.py b/tests/test_native_mcp_preference.py new file mode 100644 index 0000000..7607814 --- /dev/null +++ b/tests/test_native_mcp_preference.py @@ -0,0 +1,121 @@ +"""Tests for native MCP preference gate and shell circuit breaker (#270).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import native_mcp_preference as nmp # noqa: E402 + + +class TestShellHealthCircuitBreaker(unittest.TestCase): + def setUp(self): + nmp.clear_shell_health_for_tests() + + def test_two_spawn_failures_hard_stop(self): + nmp.record_shell_spawn_outcome(exit_code=-1, stdout="", stderr="") + status = nmp.record_shell_spawn_outcome(exit_code=-1, stdout="", stderr="") + self.assertTrue(status["hard_stopped"]) + self.assertFalse(status["shell_use_allowed"]) + self.assertEqual(status["consecutive_spawn_failures"], 2) + + def test_success_resets_breaker(self): + nmp.record_shell_spawn_outcome(exit_code=-1, stdout="", stderr="") + status = nmp.record_shell_spawn_outcome(exit_code=0, stdout="ok", stderr="") + self.assertFalse(status["hard_stopped"]) + self.assertEqual(status["consecutive_spawn_failures"], 0) + + +class TestNativeMcpPreferenceGate(unittest.TestCase): + def setUp(self): + nmp.clear_shell_health_for_tests() + + def test_mcp_available_blocks_local_script(self): + result = nmp.assess_gitea_operation_path( + task="create_pr", + command_or_detail="python create_pr.py --remote prgs --title T --head h", + mcp_available=True, + mcp_tool_visible=True, + ) + self.assertTrue(result["block"]) + self.assertEqual(result["path_kind"], "shell_script") + + def test_mcp_native_path_allowed(self): + result = nmp.assess_gitea_operation_path( + task="comment_issue", + path_kind="mcp_native", + mcp_available=True, + mcp_tool_visible=True, + ) + self.assertFalse(result["block"]) + self.assertTrue(result["allowed"]) + + def test_mcp_unavailable_requires_terminal_report(self): + result = nmp.assess_gitea_operation_path( + task="merge_pr", + path_kind="shell_script", + mcp_available=False, + mcp_tool_visible=False, + ) + self.assertTrue(result["block"]) + self.assertTrue( + any("terminal recovery report" in r for r in result["reasons"]) + ) + self.assertIn(nmp.TERMINAL_REPORT_HEADING, result["terminal_report"]) + + def test_recovery_fallback_allowed_with_proof(self): + result = nmp.assess_gitea_operation_path( + task="merge_pr", + path_kind="shell_script", + command_or_detail="Recovery mode: MCP tools unavailable in this session.", + mcp_available=False, + recovery_mode=True, + recovery_proof_complete=True, + ) + self.assertFalse(result["block"]) + + def test_cli_auth_divergence_blocked(self): + result = nmp.assess_gitea_operation_path( + task="create_pr", + command_or_detail="GITEA_MCP_PROFILE=prgs-reviewer python create_pr.py", + mcp_available=True, + active_profile="prgs-author", + ) + self.assertTrue(result["block"]) + self.assertTrue(any("CLI auth divergence" in r for r in result["reasons"])) + + def test_unsafe_helper_fallback_denied(self): + result = nmp.assess_gitea_operation_path( + task="commit_files", + command_or_detail="python _encode_payload.py", + mcp_available=True, + mcp_tool_visible=True, + ) + self.assertTrue(result["block"]) + self.assertEqual(result["path_kind"], "unsafe_helper") + + def test_reviewer_mcp_server_touch_blocked(self): + result = nmp.assess_gitea_operation_path( + task="review_pr", + command_or_detail="pkill -f gitea_mcp_server.py", + mcp_available=True, + role="reviewer", + active_profile="prgs-reviewer", + ) + self.assertTrue(result["block"]) + self.assertEqual(result["path_kind"], "mcp_server_touch") + + def test_hard_stopped_shell_blocks_non_mcp_path(self): + nmp.record_shell_spawn_outcome(exit_code=-1, stdout="", stderr="") + nmp.record_shell_spawn_outcome(exit_code=-1, stdout="", stderr="") + result = nmp.assess_gitea_operation_path( + task="lock_issue", + path_kind="shell_script", + mcp_available=True, + ) + self.assertTrue(result["block"]) + self.assertTrue(any("circuit breaker" in r for r in result["reasons"])) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 8d32af14d6e67243c670bdd3b4a4155fde40a98a Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:14:29 -0400 Subject: [PATCH 09/15] feat: enforce PR review/merge workflow state machine gates (Closes #290) Add fail-closed review/merge state progression helpers, pre-merge gate requirements, blocked recovery handoff checks, and MCP assessment tool. Closes #290 --- gitea_mcp_server.py | 69 +++++ review_merge_state_machine.py | 332 +++++++++++++++++++++++ tests/test_review_merge_state_machine.py | 121 +++++++++ 3 files changed, 522 insertions(+) create mode 100644 review_merge_state_machine.py create mode 100644 tests/test_review_merge_state_machine.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index b5415eb..9e8a6e8 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -477,6 +477,7 @@ import review_proofs # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import merged_cleanup_reconcile # noqa: E402 +import review_merge_state_machine # noqa: E402 # Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, @@ -4082,6 +4083,14 @@ _GUIDE_RULES = { "inventory, summarize) needs no explicit authorization. Enforced " "fail closed via subagent_gate.assess_subagent_delegation and " "validate_subagent_report (#266)."), + "review_merge_state_machine": ( + "PR review/merge must follow the enforced state machine: PRECHECK → " + "INVENTORY → SELECT_PR → PIN_HEAD_SHA → CREATE_BRANCHES_WORKTREE → " + "VALIDATE → REVIEW_DECISION → APPROVE_OR_REQUEST_CHANGES → " + "PRE_MERGE_RECHECK → MERGE → POST_MERGE_REPORT. Failed upstream " + "states forbid downstream approve/merge. infra_stop and capability " + "blocks stop immediately. Blocked recovery handoffs must restart the " + "full workflow — never replay approve/merge commands (#290)."), } _COMMON_WORKFLOWS = [ @@ -4786,6 +4795,66 @@ def _build_runtime_task_capabilities( } +@mcp.tool() +def gitea_assess_review_merge_state_machine( + target_state: str | None = None, + state_completion: dict[str, bool] | None = None, + pre_merge_gates: dict[str, bool] | None = None, + infra_stop: bool = False, + capability_blocked: bool = False, + recovery_handoff_text: str | None = None, + final_report_text: str | None = None, +) -> dict: + """Read-only: assess enforced PR review/merge workflow state (#290).""" + completion = state_completion or {} + blockers = review_merge_state_machine.assess_workflow_blockers( + infra_stop=infra_stop, + capability_blocked=capability_blocked, + ) + result = { + "workflow": review_merge_state_machine.workflow_status( + completion, + infra_stop=infra_stop, + capability_blocked=capability_blocked, + ), + "blockers": blockers, + "approve": review_merge_state_machine.can_approve( + completion, + infra_stop=infra_stop, + capability_blocked=capability_blocked, + ), + "merge": review_merge_state_machine.can_merge( + completion, + pre_merge_gates=pre_merge_gates, + infra_stop=infra_stop, + capability_blocked=capability_blocked, + ), + } + if target_state: + result["advancement"] = review_merge_state_machine.assess_state_advancement( + completion, + target_state=target_state, + infra_stop=infra_stop, + capability_blocked=capability_blocked, + ) + if recovery_handoff_text is not None: + result["recovery_handoff"] = ( + review_merge_state_machine.assess_blocked_recovery_handoff( + recovery_handoff_text + ) + ) + if final_report_text is not None: + result["final_report_claims"] = ( + review_merge_state_machine.assess_final_report_state_claims( + final_report_text, + state_completion=completion, + approve_completed=bool(completion.get("APPROVE_OR_REQUEST_CHANGES")), + merge_completed=bool(completion.get("MERGE")), + ) + ) + return result + + @mcp.tool() def gitea_get_runtime_context( remote: str = "dadeschools", diff --git a/review_merge_state_machine.py b/review_merge_state_machine.py new file mode 100644 index 0000000..33c9705 --- /dev/null +++ b/review_merge_state_machine.py @@ -0,0 +1,332 @@ +"""Enforced PR review/merge workflow state machine (#290). + +Review and merge must advance through explicit states. Any failed upstream +gate forbids downstream approve/merge mutations until the full workflow is +restarted after blockers clear. +""" + +from __future__ import annotations + +import re +from typing import Any + +REVIEW_MERGE_STATES: tuple[str, ...] = ( + "PRECHECK", + "INVENTORY", + "SELECT_PR", + "PIN_HEAD_SHA", + "CREATE_BRANCHES_WORKTREE", + "VALIDATE", + "REVIEW_DECISION", + "APPROVE_OR_REQUEST_CHANGES", + "PRE_MERGE_RECHECK", + "MERGE", + "POST_MERGE_REPORT", +) + +TERMINAL_BLOCKED_HEADING = ( + "PR review/merge workflow blocked. Restart the full workflow after blockers clear." +) + +_FORBIDDEN_RECOVERY_REPLAY_RE = re.compile( + r"\b(?:approve(?:\s+pr)?\s*#?\d+|merge(?:\s+pr)?\s*#?\d+|gitea_merge_pr|" + r"gitea_submit_pr_review|submit\s+approve|run\s+merge)\b", + re.IGNORECASE, +) +_RESTART_WORKFLOW_RE = re.compile( + r"restart(?:\s+the)?\s+full\s+workflow|rerun\s+the\s+full\s+workflow", + re.IGNORECASE, +) +_READY_TO_MERGE_RE = re.compile( + r"\bready\s+to\s+merge\b", + re.IGNORECASE, +) +_REVIEWED_VALIDATED_RE = re.compile( + r"\b(?:reviewed|validated|approval\s+submitted)\b", + re.IGNORECASE, +) + +_PRE_MERGE_REQUIRED_GATES = ( + "whoami_verified", + "profile_runtime_verified", + "merge_capability_verified", + "pr_refetched", + "reviewed_head_sha_unchanged", + "pr_mergeable", + "checks_passed", + "reviewer_not_author", + "worktree_clean", +) + + +def _clean(value: str | None) -> str: + return (value or "").strip() + + +def state_index(state: str) -> int: + name = _clean(state).upper() + try: + return REVIEW_MERGE_STATES.index(name) + except ValueError as exc: + raise ValueError(f"unknown review/merge state '{state}' (fail closed)") from exc + + +def downstream_states(from_state: str) -> list[str]: + idx = state_index(from_state) + return list(REVIEW_MERGE_STATES[idx + 1 :]) + + +def _completed_through(state_completion: dict[str, bool], state: str) -> bool: + return bool(state_completion.get(state)) + + +def _first_incomplete_state(state_completion: dict[str, bool]) -> str | None: + for state in REVIEW_MERGE_STATES: + if not _completed_through(state_completion, state): + return state + return None + + +def assess_workflow_blockers( + *, + infra_stop: bool = False, + capability_blocked: bool = False, + mcp_reconnect_failed: bool = False, + stale_capability_state: bool = False, +) -> dict[str, Any]: + """Return hard blockers that forbid all PR queue work (#290 AC3).""" + reasons: list[str] = [] + if infra_stop: + reasons.append("infra_stop is active; PR selection/review/merge is forbidden") + if capability_blocked: + reasons.append("gitea_resolve_task_capability returned blocked/stop_required") + if mcp_reconnect_failed: + reasons.append("MCP reconnect failed; stale session state cannot be reused") + if stale_capability_state: + reasons.append("stale MCP capability state detected after reconnect failure") + return { + "block": bool(reasons), + "reasons": reasons, + "forbidden_states": list(REVIEW_MERGE_STATES) if reasons else [], + "safe_next_action": ( + TERMINAL_BLOCKED_HEADING if reasons else "proceed with PRECHECK" + ), + } + + +def assess_state_advancement( + state_completion: dict[str, bool] | None, + *, + target_state: str, + infra_stop: bool = False, + capability_blocked: bool = False, +) -> dict[str, Any]: + """Fail closed when *target_state* is requested before upstream gates pass.""" + completion = dict(state_completion or {}) + target = _clean(target_state).upper() + blockers = assess_workflow_blockers( + infra_stop=infra_stop, + capability_blocked=capability_blocked, + ) + reasons = list(blockers["reasons"]) + + try: + target_idx = state_index(target) + except ValueError as exc: + return { + "target_state": target, + "allowed": False, + "block": True, + "reasons": [str(exc)], + "next_allowed_state": None, + "safe_next_action": TERMINAL_BLOCKED_HEADING, + } + + if blockers["block"]: + return { + "target_state": target, + "allowed": False, + "block": True, + "reasons": reasons, + "next_allowed_state": None, + "safe_next_action": blockers["safe_next_action"], + } + + next_allowed = _first_incomplete_state(completion) + if next_allowed is None: + allowed = target_idx == len(REVIEW_MERGE_STATES) - 1 + if not allowed: + reasons.append("workflow already completed through POST_MERGE_REPORT") + else: + allowed = state_index(next_allowed) >= target_idx + if not allowed: + reasons.append( + f"state '{target}' is forbidden until '{next_allowed}' completes" + ) + + return { + "target_state": target, + "allowed": allowed and not reasons, + "block": bool(reasons), + "reasons": reasons, + "next_allowed_state": next_allowed, + "completed_states": [s for s in REVIEW_MERGE_STATES if completion.get(s)], + "safe_next_action": ( + f"complete state '{next_allowed}' before advancing" + if next_allowed and not allowed + else ( + TERMINAL_BLOCKED_HEADING + if reasons + else f"advance to {target}" + ) + ), + } + + +def can_approve(state_completion: dict[str, bool] | None, **blocker_kwargs) -> dict[str, Any]: + """Approval requires all states through REVIEW_DECISION (#290 AC).""" + required = REVIEW_MERGE_STATES[: REVIEW_MERGE_STATES.index("REVIEW_DECISION") + 1] + completion = dict(state_completion or {}) + advance = assess_state_advancement( + completion, + target_state="APPROVE_OR_REQUEST_CHANGES", + **blocker_kwargs, + ) + missing = [s for s in required if not completion.get(s)] + reasons = list(advance["reasons"]) + if missing: + reasons.append( + "approval blocked: incomplete states: " + ", ".join(missing) + ) + block = bool(reasons) or advance["block"] + return { + "allowed": not block, + "block": block, + "reasons": reasons, + "missing_states": missing, + "safe_next_action": advance["safe_next_action"], + } + + +def can_merge( + state_completion: dict[str, bool] | None, + *, + pre_merge_gates: dict[str, bool] | None = None, + **blocker_kwargs, +) -> dict[str, Any]: + """Merge requires approve path plus fresh PRE_MERGE_RECHECK gates (#290 AC6).""" + completion = dict(state_completion or {}) + approve = can_approve(completion, **blocker_kwargs) + reasons = list(approve["reasons"]) + + if not completion.get("APPROVE_OR_REQUEST_CHANGES"): + reasons.append("merge blocked: APPROVE_OR_REQUEST_CHANGES not completed") + + gate_map = dict(pre_merge_gates or {}) + missing_gates = [g for g in _PRE_MERGE_REQUIRED_GATES if not gate_map.get(g)] + if missing_gates: + reasons.append( + "merge blocked: pre-merge gates incomplete: " + ", ".join(missing_gates) + ) + + advance = assess_state_advancement( + completion, + target_state="MERGE", + **blocker_kwargs, + ) + reasons.extend(advance["reasons"]) + + block = bool(reasons) + return { + "allowed": not block, + "block": block, + "reasons": reasons, + "missing_pre_merge_gates": missing_gates, + "safe_next_action": ( + "complete PRE_MERGE_RECHECK with fresh whoami/capability/PR re-fetch " + "before merge" + if block + else "merge allowed" + ), + } + + +def assess_blocked_recovery_handoff(report_text: str) -> dict[str, Any]: + """Blocked handoffs must not replay approve/merge commands (#290 AC4).""" + text = report_text or "" + reasons: list[str] = [] + if _FORBIDDEN_RECOVERY_REPLAY_RE.search(text): + reasons.append( + "blocked recovery handoff contains direct approve/merge replay command" + ) + if reasons and not _RESTART_WORKFLOW_RE.search(text): + reasons.append( + "blocked recovery handoff must direct operator to restart full workflow" + ) + return { + "block": bool(reasons), + "allowed": not reasons, + "reasons": reasons, + "safe_next_action": ( + "remove approve/merge replay commands and say to restart full workflow " + "after blockers clear" + if reasons + else "recovery handoff wording acceptable" + ), + } + + +def assess_final_report_state_claims( + report_text: str, + *, + state_completion: dict[str, bool] | None = None, + approve_completed: bool = False, + merge_completed: bool = False, +) -> dict[str, Any]: + """Reports must not claim reviewed/ready-to-merge without gate proof (#290 AC10).""" + text = report_text or "" + completion = dict(state_completion or {}) + reasons: list[str] = [] + + if _READY_TO_MERGE_RE.search(text) and not ( + merge_completed or completion.get("PRE_MERGE_RECHECK") + ): + reasons.append( + "report claims ready-to-merge without PRE_MERGE_RECHECK completion" + ) + + if _REVIEWED_VALIDATED_RE.search(text): + if not (approve_completed or completion.get("VALIDATE")): + reasons.append( + "report claims reviewed/validated without VALIDATE/APPROVE proof" + ) + + return { + "block": bool(reasons), + "allowed": not reasons, + "reasons": reasons, + "safe_next_action": ( + "remove stale reviewed/ready-to-merge claims unless gates passed" + if reasons + else "final report state claims consistent with workflow" + ), + } + + +def workflow_status( + state_completion: dict[str, bool] | None, + **blocker_kwargs, +) -> dict[str, Any]: + """Summarize current state-machine position for MCP/runtime reporting.""" + completion = dict(state_completion or {}) + blockers = assess_workflow_blockers(**blocker_kwargs) + next_state = _first_incomplete_state(completion) + return { + "states": list(REVIEW_MERGE_STATES), + "completed_states": [s for s in REVIEW_MERGE_STATES if completion.get(s)], + "next_required_state": next_state, + "workflow_complete": next_state is None, + "approve_allowed": can_approve(completion, **blocker_kwargs)["allowed"], + "merge_allowed": can_merge(completion, **blocker_kwargs)["allowed"], + "blockers": blockers, + } \ No newline at end of file diff --git a/tests/test_review_merge_state_machine.py b/tests/test_review_merge_state_machine.py new file mode 100644 index 0000000..b3930d6 --- /dev/null +++ b/tests/test_review_merge_state_machine.py @@ -0,0 +1,121 @@ +"""Tests for enforced PR review/merge state machine (#290).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import review_merge_state_machine as rmsm # noqa: E402 + +FULL_REVIEW_COMPLETION = {state: True for state in rmsm.REVIEW_MERGE_STATES} + + +def _completion_through(state_name: str) -> dict[str, bool]: + idx = rmsm.state_index(state_name) + return {state: i <= idx for i, state in enumerate(rmsm.REVIEW_MERGE_STATES)} + + +class TestWorkflowBlockers(unittest.TestCase): + def test_infra_stop_blocks_all_states(self): + result = rmsm.assess_workflow_blockers(infra_stop=True) + self.assertTrue(result["block"]) + self.assertEqual(result["forbidden_states"], list(rmsm.REVIEW_MERGE_STATES)) + + def test_infra_stop_forbids_advancement(self): + result = rmsm.assess_state_advancement( + {}, + target_state="INVENTORY", + infra_stop=True, + ) + self.assertTrue(result["block"]) + self.assertFalse(result["allowed"]) + + +class TestStateAdvancement(unittest.TestCase): + def test_cannot_skip_inventory(self): + result = rmsm.assess_state_advancement( + {"PRECHECK": True}, + target_state="SELECT_PR", + ) + self.assertTrue(result["block"]) + self.assertEqual(result["next_allowed_state"], "INVENTORY") + + def test_sequential_advance_allowed(self): + completion = _completion_through("INVENTORY") + result = rmsm.assess_state_advancement( + completion, + target_state="SELECT_PR", + ) + self.assertFalse(result["block"]) + self.assertTrue(result["allowed"]) + + +class TestApproveAndMergeGates(unittest.TestCase): + def test_approve_blocked_without_validate(self): + completion = _completion_through("PIN_HEAD_SHA") + result = rmsm.can_approve(completion) + self.assertTrue(result["block"]) + self.assertIn("VALIDATE", ",".join(result["missing_states"])) + + def test_approve_allowed_when_review_path_complete(self): + completion = _completion_through("REVIEW_DECISION") + result = rmsm.can_approve(completion) + self.assertFalse(result["block"]) + self.assertTrue(result["allowed"]) + + def test_merge_blocked_without_pre_merge_gates(self): + completion = _completion_through("APPROVE_OR_REQUEST_CHANGES") + result = rmsm.can_merge(completion) + self.assertTrue(result["block"]) + self.assertTrue(result["missing_pre_merge_gates"]) + + def test_merge_allowed_with_pre_merge_gates(self): + completion = _completion_through("PRE_MERGE_RECHECK") + gates = {gate: True for gate in rmsm._PRE_MERGE_REQUIRED_GATES} + result = rmsm.can_merge(completion, pre_merge_gates=gates) + self.assertFalse(result["block"]) + self.assertTrue(result["allowed"]) + + def test_merge_blocked_when_head_sha_gate_fails(self): + completion = _completion_through("PRE_MERGE_RECHECK") + gates = {gate: True for gate in rmsm._PRE_MERGE_REQUIRED_GATES} + gates["reviewed_head_sha_unchanged"] = False + result = rmsm.can_merge(completion, pre_merge_gates=gates) + self.assertTrue(result["block"]) + self.assertIn("reviewed_head_sha_unchanged", result["missing_pre_merge_gates"]) + + +class TestRecoveryHandoff(unittest.TestCase): + def test_blocked_handoff_without_replay_passes(self): + report = ( + "Blocked recovery handoff.\n" + "Restart the full workflow after infra_stop clears.\n" + "No approve/merge performed." + ) + result = rmsm.assess_blocked_recovery_handoff(report) + self.assertFalse(result["block"]) + + def test_blocked_handoff_with_merge_replay_fails(self): + report = "Please merge PR #12 now using gitea_merge_pr." + result = rmsm.assess_blocked_recovery_handoff(report) + self.assertTrue(result["block"]) + + +class TestFinalReportClaims(unittest.TestCase): + def test_ready_to_merge_claim_without_gates_fails(self): + result = rmsm.assess_final_report_state_claims( + "PR is ready to merge after validation.", + state_completion=_completion_through("VALIDATE"), + ) + self.assertTrue(result["block"]) + + def test_reviewed_claim_without_validate_fails(self): + result = rmsm.assess_final_report_state_claims( + "PR reviewed and looks good.", + state_completion={"PRECHECK": True}, + ) + self.assertTrue(result["block"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 958d470f9810efd46e63943a1ce04767ca23e410 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:15:13 -0400 Subject: [PATCH 10/15] feat: dedicated reconciler capability for closing landed PRs (Closes #309) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Already-landed open PRs block the review queue: the already-landed gate correctly stops approval/merge, but no task path could close the redundant PR. Add the reconciler close path: - task_capability_map: new reconcile_close_landed_pr (gitea.pr.close) and reconcile_close_landed_issue (gitea.issue.close) tasks under a dedicated "reconciler" role. Exact close capabilities only — the role grants no review, approve, request-changes, or merge authority, and normal reviewer profiles keep no broad PR-close authority. - role_session_router: reconciler tasks route wrong_role_stop in author/reviewer sessions with a dedicated recovery message; matching reconciler sessions route allowed_current_session. - review_proofs.assess_reconciler_close_gate: PR close allowed only when every proof passes — PR live-verified open, candidate head SHA pinned and matching the live head, target branch freshly fetched with a full SHA, head an ancestor of the target, exact gitea.pr.close proven. Non-landed PRs return NOT_LANDED_CLOSE_BLOCKED; missing close capability returns RECOVERY_HANDOFF_REQUIRED; incomplete proof fails closed as GATE_NOT_PROVEN. Linked-issue close is skipped when the issue is already closed and allowed only for an open issue resolved by the landed commits with exact gitea.issue.close capability. The gate also returns the required final-report field list. 17 new tests cover the capability map, wrong-role routing, and every gate path (landed close, non-landed block, stale target, changed head, missing capabilities, linked-issue variants, review-mutation denial). Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 163 ++++++++++++++++++++++++ role_session_router.py | 28 +++++ task_capability_map.py | 10 ++ tests/test_reconciler_close_gate.py | 188 ++++++++++++++++++++++++++++ 4 files changed, 389 insertions(+) create mode 100644 tests/test_reconciler_close_gate.py diff --git a/review_proofs.py b/review_proofs.py index e0e948a..bf126b2 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -3759,6 +3759,169 @@ def assess_full_suite_failure_approval_gate( } +# --------------------------------------------------------------------------- +# Reconciler close gate (#309) +# --------------------------------------------------------------------------- + +RECONCILER_CLOSE_REPORT_FIELDS = ( + "identity/profile", + "close capability proof", + "PR live state", + "candidate head SHA", + "target branch SHA", + "ancestor proof", + "linked issue status", + "PR close result", + "issue close result", + "no review/merge confirmation", +) + + +def assess_reconciler_close_gate( + *, + pr_number: int | None, + pr_state: str | None, + candidate_head_sha: str | None, + live_head_sha: str | None, + target_branch: str | None, + target_branch_sha: str | None, + head_is_ancestor_of_target: bool | None, + close_pr_capability: bool, + close_issue_capability: bool = False, + linked_issue_state: str | None = None, + issue_resolved_by_landing: bool = False, +) -> dict: + """#309: gate the dedicated reconciler close path for landed PRs. + + The reconciler path may close a PR only with exact ``gitea.pr.close`` + capability and full already-landed proof: live open PR, pinned head, + freshly fetched target branch SHA, and ancestry of the head in the + target. Non-landed PRs are never closable here, and the gate never + grants review, approval, request-changes, or merge. + + Linked-issue closure: skipped when the issue is already closed; + allowed only when the issue is open, resolved by the landed commits, + and exact ``gitea.issue.close`` capability is proven. + """ + reasons: list[str] = [] + + if not isinstance(pr_number, int) or pr_number <= 0: + reasons.append("PR number missing or invalid (#309)") + if (pr_state or "").strip().lower() != "open": + reasons.append( + "PR is not live-verified open at mutation time; re-fetch the " + "PR before any reconciler close (#309)" + ) + if not _FULL_SHA.match((candidate_head_sha or "").strip()): + reasons.append( + "candidate head SHA is not a full 40-hex commit SHA (#309)" + ) + if live_head_sha is not None and candidate_head_sha and ( + live_head_sha.strip() != candidate_head_sha.strip() + ): + reasons.append( + "PR head changed since the ancestry proof was pinned; re-run " + "the already-landed check (#309)" + ) + if not (target_branch or "").strip(): + reasons.append("target branch missing (#309)") + if not _FULL_SHA.match((target_branch_sha or "").strip()): + reasons.append( + "target branch SHA missing or not a full 40-hex SHA; fetch the " + "target branch freshly before the ancestry check (#309)" + ) + if head_is_ancestor_of_target is None: + reasons.append( + "ancestry of the PR head against the target branch was not " + "checked (#309)" + ) + + never_allowed = { + "review_allowed": False, + "approve_allowed": False, + "request_changes_allowed": False, + "merge_allowed": False, + } + + if reasons: + return { + "outcome": "GATE_NOT_PROVEN", + "pr_close_allowed": False, + "issue_close_allowed": False, + "reasons": reasons, + "required_report_fields": RECONCILER_CLOSE_REPORT_FIELDS, + "safe_next_action": ( + "re-fetch the PR and target branch, pin SHAs, run the " + "ancestry check, then re-run this gate" + ), + **never_allowed, + } + + if head_is_ancestor_of_target is False: + return { + "outcome": "NOT_LANDED_CLOSE_BLOCKED", + "pr_close_allowed": False, + "issue_close_allowed": False, + "reasons": [ + f"PR #{pr_number} head is not an ancestor of " + f"{target_branch}; non-landed PRs cannot be closed through " + "the reconciler path (#309)" + ], + "required_report_fields": RECONCILER_CLOSE_REPORT_FIELDS, + "safe_next_action": ( + "route the PR through the normal review workflow; the " + "reconciler path only closes already-landed PRs" + ), + **never_allowed, + } + + if close_pr_capability is not True: + return { + "outcome": "RECOVERY_HANDOFF_REQUIRED", + "pr_close_allowed": False, + "issue_close_allowed": False, + "reasons": [ + "exact gitea.pr.close capability not proven; produce a " + "recovery handoff instead of closing (#309)" + ], + "required_report_fields": RECONCILER_CLOSE_REPORT_FIELDS, + "safe_next_action": ( + "produce a recovery handoff naming the missing " + "gitea.pr.close capability and the completed ancestor proof" + ), + **never_allowed, + } + + issue_close_allowed = ( + (linked_issue_state or "").strip().lower() == "open" + and issue_resolved_by_landing is True + and close_issue_capability is True + ) + issue_close_reasons = [] + if (linked_issue_state or "").strip().lower() == "closed": + issue_close_reasons.append( + "linked issue already closed; no issue close attempted (#309)" + ) + elif not issue_close_allowed: + issue_close_reasons.append( + "issue close requires an open linked issue resolved by the " + "landed commits and exact gitea.issue.close capability (#309)" + ) + + return { + "outcome": "CLOSE_ALLOWED", + "pr_close_allowed": True, + "issue_close_allowed": issue_close_allowed, + "reasons": issue_close_reasons, + "required_report_fields": RECONCILER_CLOSE_REPORT_FIELDS, + "safe_next_action": ( + "close the already-landed PR, report the close result, and " + "handle the linked issue per the proven capability" + ), + **never_allowed, + } + + # --------------------------------------------------------------------------- # Identity disclosure (#305) # --------------------------------------------------------------------------- diff --git a/role_session_router.py b/role_session_router.py index a7db807..ff75025 100644 --- a/role_session_router.py +++ b/role_session_router.py @@ -90,12 +90,20 @@ TASK_REQUIRED_ROLE = { "blind_pr_queue_review": "reviewer", "request_changes_pr": "reviewer", "approve_pr": "reviewer", + # #309: reconciler tasks close already-landed PRs/issues only. + "reconcile_close_landed_pr": "reconciler", + "reconcile_close_landed_issue": "reconciler", } WRONG_ROLE_REVIEWER_MSG = ( "Wrong role/session for reviewer task. Launch reviewer MCP namespace." ) +WRONG_ROLE_RECONCILER_MSG = ( + "Wrong role/session for reconciler task. Launch a reconciler-capable " + "MCP namespace/profile with exact close capability." +) + _session_last_route: dict | None = None @@ -191,6 +199,26 @@ def route_task_session( _record_route(result) return result + if required_role == "reconciler": + result = { + "task_type": task_type, + "required_role": required_role, + "active_role": active_role_kind, + "active_profile": active_profile, + "route_result": ROUTE_WRONG_ROLE, + "downstream_allowed": False, + "reasons": [ + WRONG_ROLE_RECONCILER_MSG, + "Reconciler tasks cannot run in author or reviewer " + "sessions without exact close capability.", + ], + "message": WRONG_ROLE_RECONCILER_MSG, + "runtime_switching_supported": runtime_switching_supported, + "profile_switch_blocked": not runtime_switching_supported, + } + _record_route(result) + return result + if required_role == "author": route = ROUTE_TO_AUTHOR message = ( diff --git a/task_capability_map.py b/task_capability_map.py index 9d162c1..7e252a8 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -96,6 +96,16 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.read", "role": "author", }, + # #309: dedicated reconciler path for already-landed open PRs. Exact + # close capabilities only — never review/approve/request_changes/merge. + "reconcile_close_landed_pr": { + "permission": "gitea.pr.close", + "role": "reconciler", + }, + "reconcile_close_landed_issue": { + "permission": "gitea.issue.close", + "role": "reconciler", + }, } # Issue-mutating MCP tools and their resolver task keys. diff --git a/tests/test_reconciler_close_gate.py b/tests/test_reconciler_close_gate.py new file mode 100644 index 0000000..bae8438 --- /dev/null +++ b/tests/test_reconciler_close_gate.py @@ -0,0 +1,188 @@ +"""Issue #309: dedicated reconciler capability to close already-landed PRs. + +The reconciler path must prove exact ``gitea.pr.close`` capability, must +never imply review/approve/request-changes/merge capability, and may close +a PR only after full already-landed proof. +""" + +import sys +import unittest + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import role_session_router # noqa: E402 +import task_capability_map # noqa: E402 +from review_proofs import assess_reconciler_close_gate # noqa: E402 + +PINNED = "0fdc8f582026b72a229d59a172c0a63ac4aaeaf9" +OTHER = "a4060c5de00f2b1c9e88f4f6f0f3f9a7b2c1d0e9" + + +class TestReconcilerCapabilityMap(unittest.TestCase): + def test_reconcile_close_pr_task_maps_to_pr_close(self): + self.assertEqual( + task_capability_map.required_permission( + "reconcile_close_landed_pr"), + "gitea.pr.close", + ) + self.assertEqual( + task_capability_map.required_role("reconcile_close_landed_pr"), + "reconciler", + ) + + def test_reconcile_close_issue_task_maps_to_issue_close(self): + self.assertEqual( + task_capability_map.required_permission( + "reconcile_close_landed_issue"), + "gitea.issue.close", + ) + self.assertEqual( + task_capability_map.required_role( + "reconcile_close_landed_issue"), + "reconciler", + ) + + def test_reconciler_tasks_do_not_grant_review_permissions(self): + for task in ("reconcile_close_landed_pr", + "reconcile_close_landed_issue"): + permission = task_capability_map.required_permission(task) + self.assertNotIn("review", permission) + self.assertNotIn("approve", permission) + self.assertNotIn("merge", permission) + self.assertNotIn("request_changes", permission) + + +class TestReconcilerRouting(unittest.TestCase): + def test_reconciler_task_requires_reconciler_role(self): + self.assertEqual( + role_session_router.required_role_for_task( + "reconcile_close_landed_pr"), + "reconciler", + ) + + def test_reconciler_task_blocked_in_author_session(self): + result = role_session_router.route_task_session( + "reconcile_close_landed_pr", + active_profile="prgs-author", + active_role_kind="author", + allowed_in_current_session=False, + ) + self.assertEqual(result["route_result"], "wrong_role_stop") + self.assertFalse(result["downstream_allowed"]) + + def test_reconciler_task_allowed_in_matching_session(self): + result = role_session_router.route_task_session( + "reconcile_close_landed_pr", + active_profile="prgs-reconciler", + active_role_kind="reconciler", + allowed_in_current_session=True, + ) + self.assertEqual(result["route_result"], "allowed_current_session") + self.assertTrue(result["downstream_allowed"]) + + +class TestReconcilerCloseGate(unittest.TestCase): + def _gate(self, **overrides): + kwargs = { + "pr_number": 278, + "pr_state": "open", + "candidate_head_sha": PINNED, + "live_head_sha": PINNED, + "target_branch": "master", + "target_branch_sha": OTHER, + "head_is_ancestor_of_target": True, + "close_pr_capability": True, + "close_issue_capability": False, + "linked_issue_state": "closed", + } + kwargs.update(overrides) + return assess_reconciler_close_gate(**kwargs) + + def test_already_landed_open_pr_close_allowed(self): + result = self._gate() + self.assertEqual(result["outcome"], "CLOSE_ALLOWED") + self.assertTrue(result["pr_close_allowed"]) + + def test_not_landed_pr_cannot_be_closed(self): + result = self._gate(head_is_ancestor_of_target=False) + self.assertEqual(result["outcome"], "NOT_LANDED_CLOSE_BLOCKED") + self.assertFalse(result["pr_close_allowed"]) + + def test_unchecked_ancestry_blocks_close(self): + result = self._gate(head_is_ancestor_of_target=None) + self.assertEqual(result["outcome"], "GATE_NOT_PROVEN") + self.assertFalse(result["pr_close_allowed"]) + + def test_stale_target_branch_sha_blocks_close(self): + result = self._gate(target_branch_sha=None) + self.assertEqual(result["outcome"], "GATE_NOT_PROVEN") + self.assertFalse(result["pr_close_allowed"]) + + def test_closed_pr_state_blocks_close(self): + result = self._gate(pr_state="closed") + self.assertEqual(result["outcome"], "GATE_NOT_PROVEN") + self.assertFalse(result["pr_close_allowed"]) + + def test_changed_head_blocks_close(self): + result = self._gate(live_head_sha=OTHER) + self.assertEqual(result["outcome"], "GATE_NOT_PROVEN") + self.assertFalse(result["pr_close_allowed"]) + + def test_missing_close_capability_produces_recovery_handoff(self): + result = self._gate(close_pr_capability=False) + self.assertEqual(result["outcome"], "RECOVERY_HANDOFF_REQUIRED") + self.assertFalse(result["pr_close_allowed"]) + self.assertTrue(any( + "gitea.pr.close" in reason for reason in result["reasons"] + )) + + def test_linked_issue_already_closed_skips_issue_close(self): + result = self._gate(linked_issue_state="closed", + close_issue_capability=True) + self.assertFalse(result["issue_close_allowed"]) + + def test_linked_open_issue_close_requires_capability(self): + allowed = self._gate( + linked_issue_state="open", + issue_resolved_by_landing=True, + close_issue_capability=True, + ) + self.assertTrue(allowed["issue_close_allowed"]) + + blocked = self._gate( + linked_issue_state="open", + issue_resolved_by_landing=True, + close_issue_capability=False, + ) + self.assertFalse(blocked["issue_close_allowed"]) + + def test_gate_never_allows_review_mutations(self): + for result in ( + self._gate(), + self._gate(head_is_ancestor_of_target=False), + self._gate(close_pr_capability=False), + ): + self.assertFalse(result["review_allowed"]) + self.assertFalse(result["approve_allowed"]) + self.assertFalse(result["request_changes_allowed"]) + self.assertFalse(result["merge_allowed"]) + + def test_report_fields_listed_for_close(self): + result = self._gate() + for field in ( + "identity/profile", + "close capability proof", + "PR live state", + "candidate head SHA", + "target branch SHA", + "ancestor proof", + "linked issue status", + "PR close result", + "issue close result", + "no review/merge confirmation", + ): + self.assertIn(field, result["required_report_fields"]) + + +if __name__ == "__main__": + unittest.main() From 6946534f3e8d1e6cdae2e281a3bb03a7f082ca16 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:19:33 -0400 Subject: [PATCH 11/15] feat: verify canonical workflow citation and version in review reports (Closes #296) The canonical PR review/merge workflow already lives in skills/llm-project-workflow/workflows/review-merge-pr.md and is exposed through mcp_list_project_skills / mcp_get_skill_guide (#333-#335), and its final-report schema already demands a workflow version. What was missing is enforcement: add compute_workflow_hash (short sha256 version hash of workflow text) and assess_review_workflow_source to review_proofs. Review reports must cite the canonical workflow file and carry a populated Workflow version field; when the current canonical hash is supplied, a mismatched citation fails as a stale workflow copy, directing the agent to reload via mcp_get_skill_guide. Fails closed. Tests cover hash determinism and content sensitivity, matching-hash pass, missing citation, missing version field, stale hash, version none, and offline validation without a canonical hash. Co-Authored-By: Claude Opus 4.8 (1M context) --- review_proofs.py | 62 +++++++++++++++ tests/test_review_workflow_source.py | 108 +++++++++++++++++++++++++++ 2 files changed, 170 insertions(+) create mode 100644 tests/test_review_workflow_source.py diff --git a/review_proofs.py b/review_proofs.py index 5ea057e..21dd0f7 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -14,6 +14,7 @@ is usable from prompts, harness assertions, and tests. The MCP-level gates here weakens or replaces them. """ +import hashlib import re import issue_duplicate_gate @@ -3097,6 +3098,67 @@ def assess_issue_filing_duplicate_summary( } +# ── Canonical review-workflow citation and version proof (Issue #296) ──────── +# +# The canonical PR review/merge workflow lives in +# skills/llm-project-workflow/workflows/review-merge-pr.md and is exposed +# via mcp_get_skill_guide. Review reports must prove they loaded it and +# cite the version hash used, so stale or shortened prompt copies are +# rejected. + +REVIEW_WORKFLOW_MARKERS = ( + "workflows/review-merge-pr.md", + "review-merge-pr.md", +) + + +def compute_workflow_hash(workflow_text): + """Return the short (12-hex) sha256 version hash of a workflow text.""" + digest = hashlib.sha256((workflow_text or "").encode("utf-8")) + return digest.hexdigest()[:12] + + +def assess_review_workflow_source(report_text, *, canonical_hash=None): + """#296: review reports must cite the canonical workflow and version. + + The report must reference the canonical workflow file and carry a + populated ``Workflow version`` (or ``Workflow hash``) field. When + *canonical_hash* — ``compute_workflow_hash`` of the current + workflows/review-merge-pr.md content — is supplied, the cited hash + must match it, rejecting stale copies. + + Returns {'complete', 'downgraded', 'reasons'}; fails closed. + """ + lower = (report_text or "").lower() + reasons = [] + + if not any(marker in lower for marker in REVIEW_WORKFLOW_MARKERS): + reasons.append( + "review report missing canonical workflow source citation " + "(workflows/review-merge-pr.md)" + ) + + fields = _report_labeled_fields(report_text) + version = fields.get("workflow version") or fields.get("workflow hash") + if not version or version.strip().lower().startswith(("none", "unknown")): + reasons.append( + "review report missing populated 'Workflow version' field " + "(version/hash of the canonical workflow used)" + ) + elif canonical_hash and canonical_hash.lower() not in version.lower(): + reasons.append( + f"review report cites stale workflow version '{version}'; " + f"current canonical hash is '{canonical_hash}' — reload the " + "workflow via mcp_get_skill_guide" + ) + + return { + "complete": not reasons, + "downgraded": bool(reasons), + "reasons": reasons, + } + + def assess_create_issue_workflow_source(report_text: str) -> dict: """#337: create-issue reports must cite the canonical workflow file.""" lower = (report_text or "").lower() diff --git a/tests/test_review_workflow_source.py b/tests/test_review_workflow_source.py new file mode 100644 index 0000000..07552ca --- /dev/null +++ b/tests/test_review_workflow_source.py @@ -0,0 +1,108 @@ +"""Tests for canonical review-workflow citation and version proof (#296). + +PR review/merge reports must prove they loaded the canonical workflow +(workflows/review-merge-pr.md) and cite the workflow version/hash used; +a stale or missing hash fails so prompt drift is caught. +""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from review_proofs import ( # noqa: E402 + assess_review_workflow_source, + compute_workflow_hash, +) + + +CANONICAL_TEXT = "# review-merge-pr workflow v1\nstep one\n" +CANONICAL_HASH = compute_workflow_hash(CANONICAL_TEXT) + + +def _report(citation=True, version=CANONICAL_HASH): + lines = ["Task: review next eligible PR"] + if citation: + lines.append( + "Workflow source: skills/llm-project-workflow/" + "workflows/review-merge-pr.md (loaded via mcp_get_skill_guide)" + ) + if version is not None: + lines.append(f"- Workflow version: {version}") + return "\n".join(lines) + "\n" + + +class TestComputeWorkflowHash(unittest.TestCase): + def test_deterministic(self): + self.assertEqual( + compute_workflow_hash(CANONICAL_TEXT), + compute_workflow_hash(CANONICAL_TEXT), + ) + + def test_changes_with_content(self): + self.assertNotEqual( + compute_workflow_hash(CANONICAL_TEXT), + compute_workflow_hash(CANONICAL_TEXT + "extra rule\n"), + ) + + def test_short_hex(self): + h = compute_workflow_hash(CANONICAL_TEXT) + self.assertEqual(len(h), 12) + int(h, 16) # raises if not hex + + +class TestReviewWorkflowSource(unittest.TestCase): + def test_citation_and_matching_hash_passes(self): + result = assess_review_workflow_source( + _report(), canonical_hash=CANONICAL_HASH + ) + self.assertTrue(result["complete"]) + self.assertEqual(result["reasons"], []) + + def test_missing_citation_fails(self): + result = assess_review_workflow_source( + _report(citation=False), canonical_hash=CANONICAL_HASH + ) + self.assertFalse(result["complete"]) + self.assertTrue( + any("review-merge-pr" in r.lower() for r in result["reasons"]) + ) + + def test_missing_version_field_fails(self): + result = assess_review_workflow_source( + _report(version=None), canonical_hash=CANONICAL_HASH + ) + self.assertFalse(result["complete"]) + self.assertTrue( + any("workflow version" in r.lower() for r in result["reasons"]) + ) + + def test_stale_hash_fails(self): + result = assess_review_workflow_source( + _report(version="deadbeef0000"), canonical_hash=CANONICAL_HASH + ) + self.assertFalse(result["complete"]) + self.assertTrue( + any("stale" in r.lower() or "does not match" in r.lower() + for r in result["reasons"]) + ) + + def test_version_none_fails(self): + result = assess_review_workflow_source( + _report(version="none"), canonical_hash=CANONICAL_HASH + ) + self.assertFalse(result["complete"]) + + def test_without_canonical_hash_field_still_required(self): + # No canonical hash supplied (e.g. offline validation): citation + # and a populated version field are still required. + self.assertTrue( + assess_review_workflow_source(_report())["complete"] + ) + self.assertFalse( + assess_review_workflow_source(_report(version=None))["complete"] + ) + + +if __name__ == "__main__": + unittest.main() From 9cf1b41b77794ff72efaae40f1ed78ce4ca0c916 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:20:50 -0400 Subject: [PATCH 12/15] feat: add prgs-reconciler profile and gated already-landed PR close (Closes #310) Introduce a dedicated reconciler role with gitea_reconcile_already_landed_pr, which closes open PRs only after live fetch and ancestor proof against a fresh target branch. Document the gitea-reconciler namespace and extend task routing. --- already_landed_reconcile.py | 142 +++++++++++++ docs/gitea-dual-namespace-deployment.md | 1 + docs/gitea-execution-profiles.md | 18 ++ docs/llm-workflow-runbooks.md | 19 ++ gitea_mcp_server.py | 196 +++++++++++++++++- migrate_profiles.py | 2 + role_session_router.py | 43 ++++ task_capability_map.py | 4 + tests/test_already_landed_reconcile.py | 131 ++++++++++++ .../test_reconcile_already_landed_pr_tool.py | 149 +++++++++++++ tests/test_resolve_task_capability.py | 12 ++ tests/test_role_session_router.py | 35 ++++ 12 files changed, 750 insertions(+), 2 deletions(-) create mode 100644 already_landed_reconcile.py create mode 100644 tests/test_already_landed_reconcile.py create mode 100644 tests/test_reconcile_already_landed_pr_tool.py diff --git a/already_landed_reconcile.py b/already_landed_reconcile.py new file mode 100644 index 0000000..b2e195c --- /dev/null +++ b/already_landed_reconcile.py @@ -0,0 +1,142 @@ +"""Already-landed open PR reconciliation gates (#310). + +Reconciler workflows may close an open PR only when the PR head SHA is proven +an ancestor of a freshly fetched target branch. Arbitrary PR closure is denied. +""" + +from __future__ import annotations + +import subprocess +from typing import Any + +from merged_cleanup_reconcile import extract_linked_issue, is_head_ancestor_of_ref + +ELIGIBILITY_ALREADY_LANDED = "ALREADY_LANDED_RECONCILE_REQUIRED" +ELIGIBILITY_NOT_LANDED = "NOT_ALREADY_LANDED" +ELIGIBILITY_STALE_TARGET = "TARGET_BRANCH_UNVERIFIED" +ELIGIBILITY_PR_NOT_OPEN = "PR_NOT_OPEN" + + +def fetch_target_branch(project_root: str, remote: str, branch: str) -> dict[str, Any]: + """Fetch *branch* from *remote* and return the resolved SHA.""" + ref = f"{remote}/{branch}" + fetch = subprocess.run( + ["git", "-C", project_root, "fetch", remote, branch], + capture_output=True, + text=True, + check=False, + ) + if fetch.returncode != 0: + return { + "success": False, + "target_branch": branch, + "target_ref": ref, + "target_branch_sha": None, + "reasons": [ + f"git fetch {remote} {branch} failed: " + f"{(fetch.stderr or fetch.stdout or '').strip()}" + ], + } + + rev = subprocess.run( + ["git", "-C", project_root, "rev-parse", ref], + capture_output=True, + text=True, + check=False, + ) + if rev.returncode != 0: + return { + "success": False, + "target_branch": branch, + "target_ref": ref, + "target_branch_sha": None, + "reasons": [ + f"git rev-parse {ref} failed: " + f"{(rev.stderr or rev.stdout or '').strip()}" + ], + } + + return { + "success": True, + "target_branch": branch, + "target_ref": ref, + "target_branch_sha": (rev.stdout or "").strip(), + "reasons": [], + "git_fetch_command": f"git fetch {remote} {branch}", + } + + +def assess_already_landed_reconciliation( + *, + pr: dict[str, Any], + project_root: str, + remote: str, + target_branch: str, + target_fetch: dict[str, Any] | None = None, +) -> dict[str, Any]: + """Return eligibility and proof for reconciling an open PR.""" + pr_number = int(pr.get("number") or 0) + pr_state = (pr.get("state") or "").strip().lower() + head_sha = (pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None + if not head_sha and isinstance(pr.get("head"), str): + head_sha = None + head_ref = (pr.get("head") or {}).get("ref") if isinstance(pr.get("head"), dict) else pr.get("head") + base_ref = (pr.get("base") or {}).get("ref") if isinstance(pr.get("base"), dict) else pr.get("base") + title = pr.get("title") or "" + body = pr.get("body") or "" + + fetch_result = target_fetch or fetch_target_branch(project_root, remote, target_branch) + linked_issue = extract_linked_issue(title, body) + + result: dict[str, Any] = { + "pr_number": pr_number, + "pr_state": pr_state, + "candidate_head_sha": head_sha, + "head_ref": head_ref, + "base_ref": base_ref or target_branch, + "target_branch": target_branch, + "target_branch_sha": fetch_result.get("target_branch_sha"), + "linked_issue": linked_issue, + "git_ref_mutations": [], + "reasons": [], + } + if fetch_result.get("git_fetch_command"): + result["git_ref_mutations"].append(fetch_result["git_fetch_command"]) + + if pr_state != "open": + result["eligibility_class"] = ELIGIBILITY_PR_NOT_OPEN + result["ancestor_proof"] = None + result["close_allowed"] = False + result["reasons"].append(f"PR #{pr_number} state is {pr_state!r}, not open") + return result + + if not fetch_result.get("success"): + result["eligibility_class"] = ELIGIBILITY_STALE_TARGET + result["ancestor_proof"] = None + result["close_allowed"] = False + result["reasons"].extend(fetch_result.get("reasons") or []) + return result + + target_ref = fetch_result.get("target_ref") or f"{remote}/{target_branch}" + ancestor = is_head_ancestor_of_ref(project_root, head_sha, target_ref) + result["ancestor_proof"] = ancestor + + if ancestor is None: + result["eligibility_class"] = ELIGIBILITY_STALE_TARGET + result["close_allowed"] = False + result["reasons"].append( + f"ancestor check failed for head {head_sha!r} against {target_ref}" + ) + return result + + if ancestor: + result["eligibility_class"] = ELIGIBILITY_ALREADY_LANDED + result["close_allowed"] = True + return result + + result["eligibility_class"] = ELIGIBILITY_NOT_LANDED + result["close_allowed"] = False + result["reasons"].append( + f"PR head {head_sha} is not an ancestor of {target_ref}" + ) + return result \ No newline at end of file diff --git a/docs/gitea-dual-namespace-deployment.md b/docs/gitea-dual-namespace-deployment.md index aa4c03d..1b815dc 100644 --- a/docs/gitea-dual-namespace-deployment.md +++ b/docs/gitea-dual-namespace-deployment.md @@ -23,6 +23,7 @@ launched with exactly one static execution profile: |-----------------------------|----------------|-------------| | `gitea-author` | an author profile | implement issues, push branches, open PRs, comment | | `gitea-reviewer` | a reviewer profile | review, approve/request changes, merge | +| `gitea-reconciler` | a reconciler profile | close already-landed open PRs after ancestry proof (#310) | Properties: diff --git a/docs/gitea-execution-profiles.md b/docs/gitea-execution-profiles.md index 061ae70..225d36b 100644 --- a/docs/gitea-execution-profiles.md +++ b/docs/gitea-execution-profiles.md @@ -226,6 +226,24 @@ explicit operator-directed closure of a contaminated PR). If `close_pr` ever resolves as unknown, agents must fail closed rather than fall back to the edit path. +## Reconciler profile for already-landed open PRs (#310) + +Normal author and reviewer profiles must not gain broad PR-close authority. +Already-landed open PRs (head SHA is an ancestor of the target branch) need a +dedicated reconciler profile such as `prgs-reconciler` with: + +- `gitea.read` +- `gitea.pr.comment` +- `gitea.issue.comment` +- `gitea.issue.close` +- `gitea.pr.close` + +Use the `gitea-reconciler` MCP namespace (static profile launch) and the +`gitea_reconcile_already_landed_pr` tool. The resolver task is +`reconcile_already_landed_pr`. PR close is allowed only after live PR fetch, +fresh target-branch fetch, recorded target SHA, and ancestor proof. PRs whose +heads are not already landed cannot be closed through this path. + ## Identity and fail-closed rules Before **any** mutating action, a workflow must know both: diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index a5a51b2..39e4399 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -169,6 +169,25 @@ To avoid the bottleneck of relaunching/restarting the MCP server to switch betwe * `mcp__gitea-reviewer__*` (for reviewing PRs, approving, requesting changes, merging) * **Trust Model:** Separate tokens remain separate in the keychain/environment. Each instance operates under its own `GITEA_MCP_PROFILE` and enforces its own `allowed_operations`. A runtime `whoami` identity check is still performed independently, and self-review/self-merge checks remain strictly mandatory. The dual-server pattern is a operational convenience and never a security bypass. * **Reviewer-Identity PR Creation Deadlock:** Reviewer/merge identities must not create PRs or push branches. Doing so makes the reviewer identity the PR author in Gitea, blocking subsequent independent review and causing a review deadlock. Normally, PRs must be created by the author/work identity (`gitea-author`), leaving the reviewer identity (`gitea-reviewer`) clean and available for independent review and merge. +* **Reconciler namespace (#310):** Register a third static instance for + already-landed PR cleanup when review queues block on open PRs whose heads + already landed on `master`: + +```json +"gitea-reconciler": { + "command": "/path/to/Gitea-Tools/venv/bin/python3", + "args": ["/path/to/Gitea-Tools/mcp_server.py"], + "env": { + "GITEA_MCP_CONFIG": "/path/to/.config/gitea-tools/profiles.json", + "GITEA_MCP_PROFILE": "prgs-reconciler" + } +} +``` + + The reconciler profile grants `gitea.pr.close` only for + `gitea_reconcile_already_landed_pr` after ancestry proof — not for normal + review or author workflows. + * **Fallback:** If the dual-profile MCP launcher pattern is not supported or configured in the client, the LLM must relaunch or restart the client/MCP with the correct profile environment variable before claiming or working on any tasks. ## Setup runbook — interactive menu diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index b5415eb..b1aa8be 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -476,6 +476,7 @@ import task_capability_map # noqa: E402 import review_proofs # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 +import already_landed_reconcile # noqa: E402 import merged_cleanup_reconcile # noqa: E402 @@ -3425,6 +3426,186 @@ def gitea_reconcile_merged_cleanups( return {"success": True, "performed": True, **report} +@mcp.tool() +def gitea_reconcile_already_landed_pr( + pr_number: int, + close_pr: bool = False, + close_linked_issue: bool = False, + post_comment: bool = False, + comment_body: str = "", + target_branch: str = "master", + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Reconcile an open PR whose head is already on the target branch (#310). + + Read-only assessment always runs. PR close requires ``gitea.pr.close`` and + proof that the pinned PR head SHA is an ancestor of a freshly fetched + target branch. Arbitrary PR closure is denied. + + Args: + pr_number: Open PR to reconcile. + close_pr: When True, close the PR after already-landed proof passes. + close_linked_issue: When True, close a linked issue after live fetch + when the issue is open and ``gitea.issue.close`` is allowed. + post_comment: When True, post ``comment_body`` on the PR thread. + comment_body: PR comment text when ``post_comment`` is True. + target_branch: Target branch used for ancestry proof (default master). + remote: Known instance — 'dadeschools' or 'prgs'. + host: Override the Gitea host. + org: Override the owner/organization. + repo: Override the repository name. + + Returns: + dict with eligibility proof, optional mutations, and recovery guidance. + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "performed": False, + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + + if close_pr: + close_block = _profile_operation_gate("gitea.pr.close") + if close_block: + return { + "success": False, + "performed": False, + "pr_number": pr_number, + "required_permission": "gitea.pr.close", + "reasons": close_block, + "permission_report": _permission_block_report("gitea.pr.close"), + "safe_next_action": ( + "Launch the prgs-reconciler MCP namespace or configure a " + "profile with gitea.pr.close for already-landed cleanup." + ), + } + + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + base = repo_api_url(h, o, r) + pr_url = f"{base}/pulls/{pr_number}" + pr = api_request("GET", pr_url, auth) + + assessment = already_landed_reconcile.assess_already_landed_reconciliation( + pr=pr, + project_root=PROJECT_ROOT, + remote=remote, + target_branch=target_branch, + ) + + result: dict = { + "success": True, + "performed": False, + "pr_number": pr_number, + "pr_state": assessment.get("pr_state"), + "candidate_head_sha": assessment.get("candidate_head_sha"), + "target_branch": assessment.get("target_branch"), + "target_branch_sha": assessment.get("target_branch_sha"), + "ancestor_proof": assessment.get("ancestor_proof"), + "eligibility_class": assessment.get("eligibility_class"), + "linked_issue": assessment.get("linked_issue"), + "close_allowed": assessment.get("close_allowed"), + "git_ref_mutations": assessment.get("git_ref_mutations") or [], + "reasons": list(assessment.get("reasons") or []), + "pr_closed": False, + "issue_closed": False, + "pr_comment_posted": False, + } + + if not assessment.get("close_allowed"): + result["success"] = False + result["safe_next_action"] = ( + "Do not close this PR via the reconciler path until " + "already-landed proof passes." + ) + return result + + verify_preflight_purity(remote) + + if post_comment and comment_body.strip(): + comment_block = _profile_operation_gate("gitea.pr.comment") + if comment_block: + result["success"] = False + result["reasons"].extend(comment_block) + result["permission_report"] = _permission_block_report( + "gitea.pr.comment" + ) + return result + comment_url = f"{base}/issues/{pr_number}/comments" + with _audited( + "comment_pr", + host=h, + remote=remote, + org=o, + repo=r, + pr_number=pr_number, + request_metadata={"source": "reconcile_already_landed_pr"}, + ): + api_request("POST", comment_url, auth, {"body": comment_body.strip()}) + result["pr_comment_posted"] = True + result["performed"] = True + + if close_pr: + patch_url = f"{base}/pulls/{pr_number}" + request_metadata = { + "source": "reconcile_already_landed_pr", + "required_permission": "gitea.pr.close", + "candidate_head_sha": assessment.get("candidate_head_sha"), + "target_branch_sha": assessment.get("target_branch_sha"), + "ancestor_proof": assessment.get("ancestor_proof"), + } + with _audited( + "close_pr", + host=h, + remote=remote, + org=o, + repo=r, + pr_number=pr_number, + request_metadata=request_metadata, + ): + closed = api_request("PATCH", patch_url, auth, {"state": "closed"}) + result["pr_closed"] = True + result["performed"] = True + result["pr_state"] = closed.get("state", "closed") + + linked_issue = assessment.get("linked_issue") + if close_linked_issue and linked_issue: + issue_block = _profile_operation_gate("gitea.issue.close") + if issue_block: + result["reasons"].extend(issue_block) + result["issue_close_blocked"] = True + return result + issue_url = f"{base}/issues/{linked_issue}" + issue = api_request("GET", issue_url, auth) + result["linked_issue_live_status"] = issue.get("state") + if (issue.get("state") or "").lower() == "open": + with _audited( + "close_issue", + host=h, + remote=remote, + org=o, + repo=r, + issue_number=linked_issue, + request_metadata={"source": "reconcile_already_landed_pr"}, + ): + api_request( + "PATCH", + issue_url, + auth, + {"state": "closed"}, + ) + result["issue_closed"] = True + result["performed"] = True + + return result + + @mcp.tool() def gitea_close_issue( issue_number: int, @@ -3935,17 +4116,26 @@ def _role_kind(allowed, forbidden) -> str: """Classify the active profile from its normalized operations. 'author' can create PRs / push branches; 'reviewer' can approve/merge; - 'mixed' can do both (a config smell — the reviewer-deadlock invariant - forbids it in v2 configs); 'limited' can do neither. + 'reconciler' can close already-landed PRs via ``gitea.pr.close`` without + review/author powers; 'mixed' can do both (a config smell — the + reviewer-deadlock invariant forbids it in v2 configs); 'limited' can do + neither. """ def can(op): return gitea_config.check_operation(op, allowed, forbidden)[0] review = can("gitea.pr.approve") or can("gitea.pr.merge") author = can("gitea.pr.create") or can("gitea.branch.push") + reconciler = ( + can("gitea.pr.close") + and not review + and not author + ) if review and author: return "mixed" if review: return "reviewer" + if reconciler: + return "reconciler" if author: return "author" return "limited" @@ -4709,6 +4899,7 @@ _RUNTIME_CAPABILITY_TASKS = ( "review_pr", "merge_pr", "close_issue", + "reconcile_already_landed_pr", ) @@ -4760,6 +4951,7 @@ def _build_runtime_task_capabilities( "review_pr": "can_review_prs", "merge_pr": "can_merge_prs", "close_issue": "can_close_issues", + "reconcile_already_landed_pr": "can_reconcile_already_landed_prs", } for task in _RUNTIME_CAPABILITY_TASKS: permission = task_capability_map.required_permission(task) diff --git a/migrate_profiles.py b/migrate_profiles.py index 3613c04..6bf150c 100755 --- a/migrate_profiles.py +++ b/migrate_profiles.py @@ -31,6 +31,8 @@ REVIEWER_DEFAULT_FORBIDDEN = ["branch", "commit", "push", "open_pr"] def infer_role(name, execution_profile): """Return the unambiguous role for a legacy profile name, or None.""" haystack = f"{name} {execution_profile or ''}".lower() + if "reconciler" in haystack: + return "reconciler" has_author = "author" in haystack has_reviewer = "reviewer" in haystack if has_author == has_reviewer: diff --git a/role_session_router.py b/role_session_router.py index a7db807..1d0c125 100644 --- a/role_session_router.py +++ b/role_session_router.py @@ -72,6 +72,14 @@ AUTHOR_TASKS = frozenset({ "comment_pr", "address_pr_change_requests", "delete_branch", + "work_issue", + "work-issue", +}) + +RECONCILER_TASKS = frozenset({ + "reconcile_already_landed_pr", + "reconcile_already_landed", + "reconcile-landed-pr", }) TASK_REQUIRED_ROLE = { @@ -90,12 +98,21 @@ TASK_REQUIRED_ROLE = { "blind_pr_queue_review": "reviewer", "request_changes_pr": "reviewer", "approve_pr": "reviewer", + "work_issue": "author", + "work-issue": "author", + "reconcile_already_landed_pr": "reconciler", + "reconcile_already_landed": "reconciler", + "reconcile-landed-pr": "reconciler", } WRONG_ROLE_REVIEWER_MSG = ( "Wrong role/session for reviewer task. Launch reviewer MCP namespace." ) +WRONG_ROLE_RECONCILER_MSG = ( + "Wrong role/session for reconciler task. Launch reconciler MCP namespace." +) + _session_last_route: dict | None = None @@ -191,6 +208,32 @@ def route_task_session( _record_route(result) return result + if required_role == "reconciler": + message = WRONG_ROLE_RECONCILER_MSG + if active_role_kind == "reconciler": + message = ( + "Reconciler task requires gitea.pr.close on a dedicated " + "reconciler profile." + ) + result = { + "task_type": task_type, + "required_role": required_role, + "active_role": active_role_kind, + "active_profile": active_profile, + "route_result": ROUTE_WRONG_ROLE, + "downstream_allowed": False, + "reasons": [ + message, + "Reconciler tasks cannot run in author or reviewer sessions.", + "Static-profile mode does not permit in-place role switching.", + ], + "message": WRONG_ROLE_RECONCILER_MSG, + "runtime_switching_supported": runtime_switching_supported, + "profile_switch_blocked": not runtime_switching_supported, + } + _record_route(result) + return result + if required_role == "author": route = ROUTE_TO_AUTHOR message = ( diff --git a/task_capability_map.py b/task_capability_map.py index 9d162c1..d932781 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -96,6 +96,10 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.read", "role": "author", }, + "reconcile_already_landed_pr": { + "permission": "gitea.pr.close", + "role": "reconciler", + }, } # Issue-mutating MCP tools and their resolver task keys. diff --git a/tests/test_already_landed_reconcile.py b/tests/test_already_landed_reconcile.py new file mode 100644 index 0000000..72a99a9 --- /dev/null +++ b/tests/test_already_landed_reconcile.py @@ -0,0 +1,131 @@ +"""Tests for already-landed PR reconciliation gates (#310).""" +import os +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import already_landed_reconcile + + +class TestAssessAlreadyLandedReconciliation(unittest.TestCase): + def test_open_pr_already_landed_allows_close(self): + with patch( + "already_landed_reconcile.is_head_ancestor_of_ref", + return_value=True, + ): + assessment = already_landed_reconcile.assess_already_landed_reconciliation( + pr={ + "number": 278, + "state": "open", + "title": "Fix thing (Closes #263)", + "body": "", + "head": {"ref": "feat/x", "sha": "abc123"}, + "base": {"ref": "master"}, + }, + project_root="/tmp/repo", + remote="prgs", + target_branch="master", + target_fetch={ + "success": True, + "target_branch": "master", + "target_ref": "prgs/master", + "target_branch_sha": "deadbeef", + "git_fetch_command": "git fetch prgs master", + }, + ) + self.assertTrue(assessment["close_allowed"]) + self.assertEqual( + assessment["eligibility_class"], + already_landed_reconcile.ELIGIBILITY_ALREADY_LANDED, + ) + self.assertEqual(assessment["linked_issue"], 263) + self.assertTrue(assessment["ancestor_proof"]) + + def test_not_landed_pr_denies_close(self): + with patch( + "already_landed_reconcile.is_head_ancestor_of_ref", + return_value=False, + ): + assessment = already_landed_reconcile.assess_already_landed_reconciliation( + pr={ + "number": 99, + "state": "open", + "title": "WIP", + "body": "", + "head": {"ref": "feat/y", "sha": "fff111"}, + "base": {"ref": "master"}, + }, + project_root="/tmp/repo", + remote="prgs", + target_branch="master", + target_fetch={ + "success": True, + "target_branch": "master", + "target_ref": "prgs/master", + "target_branch_sha": "deadbeef", + }, + ) + self.assertFalse(assessment["close_allowed"]) + self.assertEqual( + assessment["eligibility_class"], + already_landed_reconcile.ELIGIBILITY_NOT_LANDED, + ) + + def test_stale_target_branch_denies_close(self): + assessment = already_landed_reconcile.assess_already_landed_reconciliation( + pr={ + "number": 99, + "state": "open", + "title": "WIP", + "body": "", + "head": {"ref": "feat/y", "sha": "fff111"}, + "base": {"ref": "master"}, + }, + project_root="/tmp/repo", + remote="prgs", + target_branch="master", + target_fetch={ + "success": False, + "target_branch": "master", + "target_ref": "prgs/master", + "target_branch_sha": None, + "reasons": ["git fetch failed"], + }, + ) + self.assertFalse(assessment["close_allowed"]) + self.assertEqual( + assessment["eligibility_class"], + already_landed_reconcile.ELIGIBILITY_STALE_TARGET, + ) + + def test_closed_pr_denies_close(self): + assessment = already_landed_reconcile.assess_already_landed_reconciliation( + pr={ + "number": 50, + "state": "closed", + "title": "Done", + "body": "", + "head": {"ref": "feat/z", "sha": "aaa"}, + "base": {"ref": "master"}, + }, + project_root="/tmp/repo", + remote="prgs", + target_branch="master", + target_fetch={ + "success": True, + "target_branch": "master", + "target_ref": "prgs/master", + "target_branch_sha": "deadbeef", + }, + ) + self.assertFalse(assessment["close_allowed"]) + self.assertEqual( + assessment["eligibility_class"], + already_landed_reconcile.ELIGIBILITY_PR_NOT_OPEN, + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_reconcile_already_landed_pr_tool.py b/tests/test_reconcile_already_landed_pr_tool.py new file mode 100644 index 0000000..4f1ee05 --- /dev/null +++ b/tests/test_reconcile_already_landed_pr_tool.py @@ -0,0 +1,149 @@ +"""Tests for gitea_reconcile_already_landed_pr MCP tool (#310).""" +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import mcp_server +from mcp_server import gitea_reconcile_already_landed_pr + +RECONCILER_PROFILE = { + "profile_name": "prgs-reconciler", + "allowed_operations": [ + "gitea.read", + "gitea.pr.comment", + "gitea.issue.comment", + "gitea.issue.close", + "gitea.pr.close", + ], + "forbidden_operations": [ + "gitea.pr.approve", + "gitea.pr.merge", + "gitea.pr.create", + "gitea.branch.push", + ], + "audit_label": "prgs-reconciler", +} + +AUTHOR_PROFILE = { + "profile_name": "prgs-author", + "allowed_operations": [ + "gitea.read", + "gitea.pr.create", + "gitea.issue.comment", + ], + "forbidden_operations": ["gitea.pr.close", "gitea.pr.approve", "gitea.pr.merge"], + "audit_label": "prgs-author", +} + +OPEN_LANDED_PR = { + "number": 278, + "title": "Already landed (Closes #263)", + "body": "", + "state": "open", + "head": {"ref": "feat/issue-263", "sha": "2a544b78d1371925761d16f1c451bb3b2984470e"}, + "base": {"ref": "master"}, +} + + +class TestReconcileAlreadyLandedPrTool(unittest.TestCase): + def setUp(self): + self.mock_api = patch("mcp_server.api_request").start() + self.mock_auth = patch( + "mcp_server.get_auth_header", return_value="token test" + ).start() + patch("mcp_server.verify_preflight_purity").start() + patch("gitea_audit.audit_enabled", return_value=False).start() + mcp_server._IDENTITY_CACHE.clear() + + def tearDown(self): + patch.stopall() + mcp_server._IDENTITY_CACHE.clear() + + def _set_profile(self, profile): + patch("mcp_server.get_profile", return_value=profile).start() + + def test_close_blocked_without_pr_close_capability(self): + self._set_profile(AUTHOR_PROFILE) + res = gitea_reconcile_already_landed_pr( + pr_number=278, + close_pr=True, + remote="prgs", + ) + self.assertFalse(res["success"]) + self.assertFalse(res["performed"]) + self.assertEqual(res["required_permission"], "gitea.pr.close") + self.mock_api.assert_not_called() + + def test_not_landed_pr_close_denied_after_live_fetch(self): + self._set_profile(RECONCILER_PROFILE) + self.mock_api.return_value = OPEN_LANDED_PR + with patch( + "mcp_server.already_landed_reconcile.assess_already_landed_reconciliation", + return_value={ + "pr_state": "open", + "candidate_head_sha": OPEN_LANDED_PR["head"]["sha"], + "target_branch": "master", + "target_branch_sha": "deadbeef", + "ancestor_proof": False, + "eligibility_class": "NOT_ALREADY_LANDED", + "linked_issue": 263, + "close_allowed": False, + "git_ref_mutations": ["git fetch prgs master"], + "reasons": ["not ancestor"], + }, + ): + res = gitea_reconcile_already_landed_pr( + pr_number=278, + close_pr=True, + remote="prgs", + ) + self.assertFalse(res["success"]) + self.assertFalse(res.get("pr_closed")) + patch_calls = [ + c for c in self.mock_api.call_args_list if c.args[0] == "PATCH" + ] + self.assertEqual(patch_calls, []) + + def test_already_landed_close_allowed_with_capability(self): + self._set_profile(RECONCILER_PROFILE) + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/pulls/278" in url: + return dict(OPEN_LANDED_PR) + if method == "PATCH" and "/pulls/278" in url: + closed = dict(OPEN_LANDED_PR) + closed["state"] = "closed" + return closed + return {} + + self.mock_api.side_effect = api_side_effect + with patch( + "mcp_server.already_landed_reconcile.assess_already_landed_reconciliation", + return_value={ + "pr_state": "open", + "candidate_head_sha": OPEN_LANDED_PR["head"]["sha"], + "target_branch": "master", + "target_branch_sha": "e017d80256217f17b0f421cd051cfa5cbcf5ae0f", + "ancestor_proof": True, + "eligibility_class": "ALREADY_LANDED_RECONCILE_REQUIRED", + "linked_issue": 263, + "close_allowed": True, + "git_ref_mutations": ["git fetch prgs master"], + "reasons": [], + }, + ): + res = gitea_reconcile_already_landed_pr( + pr_number=278, + close_pr=True, + remote="prgs", + ) + self.assertTrue(res["success"]) + self.assertTrue(res["performed"]) + self.assertTrue(res["pr_closed"]) + self.assertEqual(res["eligibility_class"], "ALREADY_LANDED_RECONCILE_REQUIRED") + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_resolve_task_capability.py b/tests/test_resolve_task_capability.py index 3545675..3bed849 100644 --- a/tests/test_resolve_task_capability.py +++ b/tests/test_resolve_task_capability.py @@ -275,6 +275,18 @@ class TestResolveTaskCapability(unittest.TestCase): self.assertEqual(res["required_role_kind"], "author") self.assertIn("author", res["exact_safe_next_action"].lower()) + @patch("mcp_server.api_request", return_value={"login": "author-user"}) + @patch("mcp_server.get_auth_header", return_value="token author-pass") + def test_resolve_reconcile_already_landed_pr_requires_close(self, _auth, _api): + with patch.dict(os.environ, self._env("author-profile")): + res = mcp_server.gitea_resolve_task_capability( + task="reconcile_already_landed_pr", remote="prgs" + ) + self.assertEqual(res["requested_task"], "reconcile_already_landed_pr") + self.assertEqual(res["required_operation_permission"], "gitea.pr.close") + self.assertEqual(res["required_role_kind"], "reconciler") + self.assertFalse(res["allowed_in_current_session"]) + @patch("mcp_server.api_request", return_value={"login": "author-user"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_close_pr_known_and_lookalike_tasks_still_fail_closed(self, _auth, _api): diff --git a/tests/test_role_session_router.py b/tests/test_role_session_router.py index 6204b6d..98eab56 100644 --- a/tests/test_role_session_router.py +++ b/tests/test_role_session_router.py @@ -52,6 +52,22 @@ CONFIG = { ], "execution_profile": "prgs-reviewer", }, + "prgs-reconciler": { + "enabled": True, + "context": "ctx", + "role": "reconciler", + "username": "sysadmin", + "auth": {"type": "env", "name": "GITEA_TOKEN_RECONCILER"}, + "allowed_operations": [ + "gitea.read", "gitea.pr.comment", "gitea.issue.comment", + "gitea.issue.close", "gitea.pr.close", + ], + "forbidden_operations": [ + "gitea.pr.approve", "gitea.pr.merge", "gitea.pr.create", + "gitea.branch.push", + ], + "execution_profile": "prgs-reconciler", + }, }, "rules": {"allow_runtime_switching": False}, } @@ -88,6 +104,7 @@ class TestRoleSessionRouter(unittest.TestCase): "GITEA_MCP_PROFILE": profile, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_RECONCILER": "reconciler-pass", } def test_reviewer_task_under_author_profile_wrong_role_stop(self): @@ -179,6 +196,24 @@ class TestRoleSessionRouter(unittest.TestCase): ) self.assertFalse(route["downstream_allowed"]) + @patch("mcp_server.api_request", return_value={"login": "sysadmin"}) + @patch("mcp_server.get_auth_header", return_value="token reconciler-pass") + def test_reconciler_task_under_reconciler_profile_allowed(self, _auth, _api): + with patch.dict(os.environ, self._env("prgs-reconciler")): + route = mcp_server.gitea_route_task_session( + task_type="reconcile_already_landed_pr", remote="prgs" + ) + self.assertEqual(route["route_result"], role_session_router.ROUTE_ALLOWED) + self.assertTrue(route["downstream_allowed"]) + + def test_reconciler_task_under_author_profile_wrong_role_stop(self): + with patch.dict(os.environ, self._env("prgs-author")): + route = mcp_server.gitea_route_task_session( + task_type="reconcile_already_landed_pr", remote="prgs" + ) + self.assertEqual(route["route_result"], role_session_router.ROUTE_WRONG_ROLE) + self.assertFalse(route["downstream_allowed"]) + def test_activate_profile_blocked_in_static_mode(self): with patch.dict(os.environ, self._env("prgs-author")): result = mcp_server.gitea_activate_profile( From 2577489c2ac73347c266b99c73bfd617a084b90e Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:22:29 -0400 Subject: [PATCH 13/15] feat: enforce already-landed review report states --- final_report_validator.py | 133 ++++++++++++++++++++++++++- tests/test_final_report_validator.py | 62 ++++++++++++- 2 files changed, 190 insertions(+), 5 deletions(-) diff --git a/final_report_validator.py b/final_report_validator.py index bff9467..b6cc93c 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -87,14 +87,34 @@ _ALREADY_LANDED_RE = re.compile( r"already[- ]landed|ancestor proof\s*:\s*passed|eligibility class\s*:\s*already_landed", re.IGNORECASE, ) +_ALREADY_LANDED_GATE_FIRED_RE = re.compile( + r"\b(?:fired|passed|true|yes|already_landed_reconcile_required)\b", + re.IGNORECASE, +) _ELIGIBLE_REVIEW_RE = re.compile( r"eligible for (?:review|merge)|ready for (?:review|merge)|next eligible pr", re.IGNORECASE, ) +_APPROVED_STATE_RE = re.compile(r"\bapproved?\b", re.IGNORECASE) +_MERGED_STATE_RE = re.compile(r"\bmerged\b", re.IGNORECASE) +_READY_TO_MERGE_STATE_RE = re.compile(r"\bready_to_merge\b|\bready to merge\b", re.IGNORECASE) +_NEGATED_STATE_RE = re.compile( + r"\b(?:not|no|none|n/a|never)\b|request_changes|not attempted", + re.IGNORECASE, +) _REVIEWED_LANDED_RE = re.compile( r"(?:reviewed|approved).{0,40}already[- ]landed|already[- ]landed.{0,40}(?:reviewed|eligible)", re.IGNORECASE | re.DOTALL, ) +_TARGET_BRANCH_UP_TO_DATE_RE = re.compile( + r"\btarget branch (?:is )?up[- ]to[- ]date\b", + re.IGNORECASE, +) +_TARGET_BRANCH_SHA_RE = re.compile( + r"target branch sha\s*:\s*[0-9a-f]{40}", + re.IGNORECASE, +) +_FULL_SHA_RE = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE) _RECONCILE_STALE_FIELDS = ( "pr number opened", "pinned reviewed head", @@ -190,6 +210,54 @@ def _handoff_fields(report_text: str) -> dict[str, str]: return fields +def _already_landed_gate_fired(report_text: str) -> bool: + text = report_text or "" + fields = _handoff_fields(text) + + gate_value = fields.get("already-landed gate", "") + if gate_value and _ALREADY_LANDED_GATE_FIRED_RE.search(gate_value): + return True + + ancestor_value = fields.get("ancestor proof", "") + if re.search(r"\bpassed\b", ancestor_value, re.IGNORECASE): + return True + + eligibility_value = fields.get("eligibility class", "") + if "already_landed" in eligibility_value.lower(): + return True + + if re.search( + r"\bpr\b.{0,60}\balready[- ]landed\b|\balready[- ]landed\b.{0,60}\bpr\b", + text, + re.IGNORECASE | re.DOTALL, + ): + return True + + return bool(_ALREADY_LANDED_RE.search(text)) + + +def _positive_review_state_terms(fields: dict[str, str]) -> list[str]: + terms: list[str] = [] + for key, value in fields.items(): + lowered_key = key.lower() + lowered_value = value.lower() + if _NEGATED_STATE_RE.search(value): + continue + if lowered_key == "review decision" and _APPROVED_STATE_RE.search(value): + terms.append("APPROVED") + elif lowered_key == "merge result" and _MERGED_STATE_RE.search(value): + terms.append("MERGED") + elif _READY_TO_MERGE_STATE_RE.search(value): + terms.append("READY_TO_MERGE") + elif lowered_value in {"approved", "approve"}: + terms.append("APPROVED") + elif lowered_value == "merged": + terms.append("MERGED") + elif lowered_value == "ready_to_merge": + terms.append("READY_TO_MERGE") + return sorted(set(terms)) + + def _rule_shared_controller_handoff( report_text: str, task_kind: str, @@ -492,7 +560,7 @@ def _rule_reviewer_main_checkout_path(report_text: str) -> list[dict[str, str]]: def _rule_reviewer_already_landed_eligible(report_text: str) -> list[dict[str, str]]: text = report_text or "" - if not _ALREADY_LANDED_RE.search(text): + if not _already_landed_gate_fired(text): return [] if not _ELIGIBLE_REVIEW_RE.search(text): return [] @@ -507,6 +575,65 @@ def _rule_reviewer_already_landed_eligible(report_text: str) -> list[dict[str, s ] +def _rule_reviewer_already_landed_state(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + if not _already_landed_gate_fired(text): + return [] + + terms = _positive_review_state_terms(_handoff_fields(text)) + if not terms: + return [] + + return [ + validator_finding( + "reviewer.already_landed_review_state", + "block", + "Review decision", + "already-landed gate fired but final report claims " + f"{', '.join(terms)} state", + "stop normal review/merge; classify as ALREADY_LANDED_RECONCILE_REQUIRED", + ) + ] + + +def _rule_reviewer_target_branch_freshness( + report_text: str, + *, + action_log: list[dict] | None = None, +) -> list[dict[str, str]]: + text = report_text or "" + if not _TARGET_BRANCH_UP_TO_DATE_RE.search(text): + return [] + + fields = _handoff_fields(text) + fetch_reported = bool(_GIT_FETCH_RE.search(text)) or any( + _GIT_FETCH_RE.search(str(e.get("command") or e.get("action") or "")) + for e in (action_log or []) + ) + target_sha_reported = bool(_TARGET_BRANCH_SHA_RE.search(text)) or any( + "target branch" in key and "sha" in key and _FULL_SHA_RE.search(value) + for key, value in fields.items() + ) + if fetch_reported and target_sha_reported: + return [] + + missing = [] + if not fetch_reported: + missing.append("fresh git fetch") + if not target_sha_reported: + missing.append("target branch SHA") + return [ + validator_finding( + "reviewer.target_branch_freshness_proof", + "block", + "Target branch SHA", + "target branch up-to-date claim lacks " + " and ".join(missing), + "fetch the target branch and report the exact target branch SHA before " + "claiming it is up to date", + ) + ] + + def _rule_reconcile_controller_handoff(report_text: str) -> list[dict[str, str]]: from review_proofs import _handoff_section_lines @@ -706,6 +833,8 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_main_checkout_baseline, _rule_reviewer_main_checkout_path, _rule_reviewer_already_landed_eligible, + _rule_reviewer_already_landed_state, + _rule_reviewer_target_branch_freshness, _rule_reviewer_mutation_ledger, _rule_reviewer_review_mutation, ], @@ -875,4 +1004,4 @@ def assess_final_report_validator( "safe_next_action": _primary_safe_next_action(findings), "complete": grade == "A", "handoff_heading": HANDOFF_HEADING, - } \ No newline at end of file + } diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index f8e8848..72ef13d 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -25,11 +25,15 @@ def _review_handoff(**overrides): "- Files changed: review_proofs.py", "- Validation: pytest tests/test_review_proofs.py -q in branches/review-203", "- Mutations: review only", - "- Workspace mutations: none", "- File edits by reviewer: none", "- Worktree/index mutations: none", "- Git ref mutations: git fetch prgs master", "- MCP/Gitea mutations: review submitted", + "- Review mutations: submitted request_changes review", + "- Merge mutations: none", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: issue and PR metadata inspected", "- Current status: PR open", "- Blockers: none", "- Next: await author", @@ -190,11 +194,63 @@ class TestReviewPrRules(unittest.TestCase): ) ) + def test_already_landed_gate_blocks_review_terminal_states(self): + cases = { + "APPROVED": ("- Review decision: request_changes", "- Review decision: APPROVED"), + "MERGED": ("- Merge result: not attempted", "- Merge result: MERGED"), + "READY_TO_MERGE": ("- Current status: PR open", "- Current status: READY_TO_MERGE"), + } + for state, (old, new) in cases.items(): + with self.subTest(state=state): + report = ( + _review_handoff() + .replace("- Reviewer eligibility: eligible", "- Reviewer eligibility: blocked") + .replace(old, new) + + "\n- Already-landed gate: fired" + + "\n- Ancestor proof: passed" + + "\n- Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED" + ) + result = assess_final_report_validator(report, "review_pr") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reviewer.already_landed_review_state" + for f in result["findings"] + ) + ) + + def test_target_branch_up_to_date_requires_fetch_and_sha(self): + report = ( + _review_handoff() + .replace("- Git ref mutations: git fetch prgs master", "- Git ref mutations: none") + + "\nTarget branch up to date." + ) + result = assess_final_report_validator(report, "review_pr") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reviewer.target_branch_freshness_proof" + for f in result["findings"] + ) + ) + + def test_target_branch_up_to_date_with_fetch_and_sha_passes(self): + report = ( + _review_handoff() + + "\n- Target branch SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9" + + "\nTarget branch up to date." + ) + result = assess_final_report_validator(report, "review_pr") + self.assertEqual(result["grade"], "A") + def test_git_fetch_must_not_be_readonly_only(self): report = ( _review_handoff() .replace("- Git ref mutations: git fetch prgs master", "- Git ref mutations: none") - + "\n- Read-only diagnostics: git fetch prgs master" + .replace( + "- Read-only diagnostics: issue and PR metadata inspected", + "- Read-only diagnostics: git fetch prgs master", + ) ) result = assess_final_report_validator( report, @@ -270,4 +326,4 @@ class TestEntryPoint(unittest.TestCase): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() From 91c67930ecba04520b5079af34bdc38c8b9b3b96 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 10:02:49 -0400 Subject: [PATCH 14/15] feat: add PR-only queue cleanup mode with per-PR dispatch gates (Closes #390) Adds canonical pr-queue-cleanup workflow, report verifier, reviewer-only task routing, and runbook guidance so operators can drain open PRs one canonical review at a time with fail-closed boundaries on batching and unauthorized merges. Co-Authored-By: Claude Opus 4.8 (1M context) --- capability_stop_terminal.py | 2 + docs/llm-workflow-runbooks.md | 19 ++ pr_queue_cleanup.py | 225 ++++++++++++++++++ review_proofs.py | 29 +++ role_session_router.py | 4 + skills/llm-project-workflow/SKILL.md | 6 + .../schemas/pr-queue-cleanup-final-report.md | 31 +++ .../templates/pr-queue-cleanup.md | 25 ++ .../workflows/pr-queue-cleanup.md | 86 +++++++ task_capability_map.py | 8 + tests/test_llm_workflow_split.py | 50 +++- tests/test_pr_queue_cleanup.py | 118 +++++++++ tests/test_resolve_task_capability.py | 22 ++ tests/test_role_session_router.py | 18 ++ 14 files changed, 642 insertions(+), 1 deletion(-) create mode 100644 pr_queue_cleanup.py create mode 100644 skills/llm-project-workflow/schemas/pr-queue-cleanup-final-report.md create mode 100644 skills/llm-project-workflow/templates/pr-queue-cleanup.md create mode 100644 skills/llm-project-workflow/workflows/pr-queue-cleanup.md create mode 100644 tests/test_pr_queue_cleanup.py diff --git a/capability_stop_terminal.py b/capability_stop_terminal.py index 70ff264..d7f9751 100644 --- a/capability_stop_terminal.py +++ b/capability_stop_terminal.py @@ -13,6 +13,8 @@ REVIEWER_CAPABILITY_TASKS = frozenset({ "review_pr", "merge_pr", "blind_pr_queue_review", + "pr_queue_cleanup", + "pr-queue-cleanup", "request_changes_pr", "approve_pr", }) diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index a5a51b2..f163ae6 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -749,3 +749,22 @@ scripts/release-tag v0.4.0 --notes-file /tmp/release-notes.md --push - [`credential-isolation.md`](credential-isolation.md) — credential handling. - [`release-workflows.md`](release-workflows.md) — release/merge workflow. - [`../README.md`](../README.md) — canonical config, thin launchers, the menu. + +## PR-only queue cleanup mode (#390) + +Use `pr-queue-cleanup` mode when the queue holds many open PRs and the goal +is to drain reviews deterministically. One run = one PR = one canonical +review (`skills/llm-project-workflow/workflows/pr-queue-cleanup.md`). + +* Cleanup runs are reviewer-role only (`pr_queue_cleanup` resolver task); + author sessions get `wrong_role_stop`. +* Forbidden in cleanup mode: issue claiming, branch creation, implementation + edits, new issue filing, and any second-PR review after a terminal + mutation. +* Terminal chain: stop after `REQUEST_CHANGES`; after `APPROVED` continue + only to same-PR merge with explicit per-PR operator authorization and + passing gates; stop after merge or merge blocker. +* Every run reports the next suggested PR without continuing to it; the next + PR requires a fresh run with fresh identity/capability/inventory proof. +* Use full `review-merge-pr` mode instead when the operator wants a single + targeted review, and `work-issue` mode for any authoring. diff --git a/pr_queue_cleanup.py b/pr_queue_cleanup.py new file mode 100644 index 0000000..c1237aa --- /dev/null +++ b/pr_queue_cleanup.py @@ -0,0 +1,225 @@ +"""PR-only queue cleanup mode gates and report verifier (#390). + +Cleanup mode dispatches exactly one canonical review run per PR. It forbids +author-side mutations (issue claiming, branch creation, implementation edits, +issue filing) and enforces the terminal-mutation chain: stop after +``REQUEST_CHANGES``; after ``APPROVED`` continue only to same-PR merge when +merge is explicitly authorized for that PR and merge gates pass; stop after +merge or a merge blocker. The next PR always requires a fresh run. +""" + +from __future__ import annotations + +import re + +CLEANUP_WORKFLOW_PATH = "workflows/pr-queue-cleanup.md" + +# Author-side resolver tasks that must never run inside cleanup mode. +CLEANUP_FORBIDDEN_TASKS = frozenset({ + "claim_issue", + "mark_issue", + "lock_issue", + "create_issue", + "create_branch", + "push_branch", + "create_pr", + "commit_files", + "gitea_commit_files", + "address_pr_change_requests", +}) + +# Run-state outcomes for one cleanup dispatch. +STOP_AFTER_REQUEST_CHANGES = "STOP_AFTER_REQUEST_CHANGES" +STOP_AFTER_DECISION = "STOP_AFTER_DECISION" +CONTINUE_TO_SAME_PR_MERGE = "CONTINUE_TO_SAME_PR_MERGE" +STOP_APPROVED_NO_MERGE_AUTH = "STOP_APPROVED_NO_MERGE_AUTH" +STOP_APPROVED_MERGE_GATES_FAILED = "STOP_APPROVED_MERGE_GATES_FAILED" +STOP_AFTER_MERGE = "STOP_AFTER_MERGE" +STOP_AFTER_MERGE_BLOCKER = "STOP_AFTER_MERGE_BLOCKER" +STOP_GATE_NOT_PROVEN = "STOP_GATE_NOT_PROVEN" + +_TERMINAL_DECISIONS = frozenset({"approved", "request_changes", "comment", "skip"}) + + +def resolve_cleanup_run_state( + decision: str | None, + *, + merge_authorized_for_pr: bool = False, + merge_gates_passed: bool | None = None, + merge_completed: bool = False, + merge_blocker: bool = False, +) -> dict: + """Resolve what one cleanup run may do after its terminal review decision. + + Fail closed: unknown decisions stop the run with no further mutation. + """ + normalized = (decision or "").strip().lower() + + if normalized not in _TERMINAL_DECISIONS: + return { + "outcome": STOP_GATE_NOT_PROVEN, + "further_mutation_allowed": False, + "reasons": [ + f"unknown terminal decision {decision!r}; cleanup run stops " + "(fail closed)" + ], + } + + if normalized == "request_changes": + return { + "outcome": STOP_AFTER_REQUEST_CHANGES, + "further_mutation_allowed": False, + "reasons": ["REQUEST_CHANGES is terminal in cleanup mode"], + } + + if normalized in {"comment", "skip"}: + return { + "outcome": STOP_AFTER_DECISION, + "further_mutation_allowed": False, + "reasons": [f"{normalized} decision ends the cleanup run"], + } + + # normalized == "approved" + if merge_completed: + return { + "outcome": STOP_AFTER_MERGE, + "further_mutation_allowed": False, + "reasons": ["merge completed; cleanup run stops"], + } + if merge_blocker: + return { + "outcome": STOP_AFTER_MERGE_BLOCKER, + "further_mutation_allowed": False, + "reasons": ["merge blocker recorded; cleanup run stops"], + } + if not merge_authorized_for_pr: + return { + "outcome": STOP_APPROVED_NO_MERGE_AUTH, + "further_mutation_allowed": False, + "reasons": [ + "APPROVED without explicit per-PR merge authorization; " + "cleanup run stops" + ], + } + if merge_gates_passed is not True: + return { + "outcome": STOP_APPROVED_MERGE_GATES_FAILED, + "further_mutation_allowed": False, + "reasons": [ + "merge gates not proven passed; cleanup run stops before merge" + ], + } + return { + "outcome": CONTINUE_TO_SAME_PR_MERGE, + "further_mutation_allowed": True, + "reasons": [], + "allowed_mutation": "merge same PR only", + } + + +def check_cleanup_task_allowed(task: str) -> tuple[bool, list[str]]: + """Fail closed on any author-side mutation task inside cleanup mode.""" + normalized = (task or "").strip().lower() + if normalized in CLEANUP_FORBIDDEN_TASKS: + return False, [ + f"task '{normalized}' is forbidden in PR-only cleanup mode: " + "no issue claiming, branch creation, implementation edits, or " + "issue filing" + ] + return True, [] + + +_SELECTED_PR_RE = re.compile( + r"^\s*[-*]?\s*selected pr\s*:\s*#?(\d+)", re.IGNORECASE | re.MULTILINE +) +_NEXT_SUGGESTED_RE = re.compile( + r"^\s*[-*]?\s*next suggested pr\s*:", re.IGNORECASE | re.MULTILINE +) +_TERMINAL_MUTATION_RE = re.compile( + r"^\s*[-*]?\s*(?:review (?:decision|verdict)|terminal (?:review )?" + r"(?:decision|mutation))\s*:\s*" + r"(approved|request_changes|request changes|merged|comment)", + re.IGNORECASE | re.MULTILINE, +) +_PAGINATION_HINT_RE = re.compile( + r"inventory_complete|final page|has_more\s*[=:]\s*false|total[_ ]count", + re.IGNORECASE, +) +_MERGE_RESULT_RE = re.compile( + r"^\s*[-*]?\s*merge result\s*:\s*(?!none\b|not attempted\b)\S", + re.IGNORECASE | re.MULTILINE, +) +_MERGE_AUTHORIZED_RE = re.compile( + r"^\s*[-*]?\s*merge authorized(?: for pr)?\s*:\s*true", + re.IGNORECASE | re.MULTILINE, +) +_ISSUE_MUTATION_CLAIM_RE = re.compile( + r"^\s*[-*]?\s*issue mutations\s*:\s*(?!none\b)\S", + re.IGNORECASE | re.MULTILINE, +) +_BRANCH_MUTATION_CLAIM_RE = re.compile( + r"^\s*[-*]?\s*branch mutations\s*:\s*(?!none\b)\S", + re.IGNORECASE | re.MULTILINE, +) + + +def assess_pr_queue_cleanup_report(report_text: str) -> dict: + """Validate a PR-only cleanup run report (single dispatch, fail closed).""" + reasons: list[str] = [] + text = report_text or "" + + if CLEANUP_WORKFLOW_PATH not in text: + reasons.append( + f"report must cite the canonical cleanup workflow " + f"({CLEANUP_WORKFLOW_PATH})" + ) + + selected = _SELECTED_PR_RE.findall(text) + if len(selected) == 0: + reasons.append("report must name exactly one Selected PR") + elif len(set(selected)) > 1: + reasons.append( + "cleanup run selected multiple PRs " + f"({', '.join(sorted(set(selected)))}); one canonical review per " + "PR per run" + ) + + terminal = _TERMINAL_MUTATION_RE.findall(text) + if len(terminal) > 1: + reasons.append( + "multiple terminal review mutations reported in one cleanup run" + ) + + if not _PAGINATION_HINT_RE.search(text): + reasons.append( + "report missing PR inventory pagination proof " + "(inventory_complete / final page / total_count)" + ) + + if not _NEXT_SUGGESTED_RE.search(text): + reasons.append( + "report must include 'Next suggested PR' (without continuing to it)" + ) + + if _MERGE_RESULT_RE.search(text) and not _MERGE_AUTHORIZED_RE.search(text): + reasons.append( + "merge reported without explicit per-PR merge authorization " + "('Merge authorized: true')" + ) + + if _ISSUE_MUTATION_CLAIM_RE.search(text): + reasons.append("issue mutations are forbidden in PR-only cleanup mode") + if _BRANCH_MUTATION_CLAIM_RE.search(text): + reasons.append("branch mutations are forbidden in PR-only cleanup mode") + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "safe_next_action": ( + "proceed" if proven else + "fix the cleanup report: one PR, one terminal mutation, pagination " + "proof, next-suggested-PR field, and no issue/branch mutations" + ), + } diff --git a/review_proofs.py b/review_proofs.py index e56185d..871b370 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1699,6 +1699,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, if report_text and _QUEUE_STATUS_REPORT_HINT.search(report_text) else {"proven": True, "block": False, "reasons": [], "violations": []} ) + pr_queue_cleanup_report = ( + assess_pr_queue_cleanup_report(report_text) + if report_text and _PR_QUEUE_CLEANUP_REPORT_HINT.search(report_text) + else {"proven": True, "block": False, "reasons": [], "violations": []} + ) proof_wording = ( assess_proof_wording(report_text) if report_text @@ -1875,6 +1880,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue-status report violates loaded workflow proof gates (#339)" ) downgrade_reasons.extend(queue_status_report.get("reasons", [])) + if not pr_queue_cleanup_report.get("proven"): + downgrade_reasons.append( + "PR-only cleanup report violates cleanup-mode proof gates (#390)" + ) + downgrade_reasons.extend(pr_queue_cleanup_report.get("reasons", [])) if not already_landed_handoff.get("proven"): downgrade_reasons.append( "already-landed controller handoff has stale or inconsistent fields (#299)" @@ -1978,6 +1988,12 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "queue_status_violations": list( queue_status_report.get("violations") or [] ), + "pr_queue_cleanup_report_proven": bool( + pr_queue_cleanup_report.get("proven") + ), + "pr_queue_cleanup_violations": list( + pr_queue_cleanup_report.get("reasons") or [] + ), "already_landed_handoff_proven": bool(already_landed_handoff.get("proven")), "already_landed_handoff_violations": list( already_landed_handoff.get("reasons") or [] @@ -4809,6 +4825,19 @@ _QUEUE_STATUS_REPORT_HINT = re.compile( re.I, ) +_PR_QUEUE_CLEANUP_REPORT_HINT = re.compile( + r"workflows/pr-queue-cleanup\.md|task:\s*pr-queue-cleanup|" + r"pr[- ]only queue cleanup", + re.I, +) + + +def assess_pr_queue_cleanup_report(report_text: str | None) -> dict: + """#390: validate PR-only cleanup final reports (one PR per run).""" + from pr_queue_cleanup import assess_pr_queue_cleanup_report as _assess + + return _assess(report_text or "") + _GATE_PASSED_VALUE = re.compile(r"\bpassed\b", re.I) _NOT_APPLICABLE_VALUE = re.compile( diff --git a/role_session_router.py b/role_session_router.py index ff75025..454c48c 100644 --- a/role_session_router.py +++ b/role_session_router.py @@ -57,6 +57,8 @@ REVIEWER_TASKS = frozenset({ "review_pr", "merge_pr", "blind_pr_queue_review", + "pr_queue_cleanup", + "pr-queue-cleanup", "request_changes_pr", "approve_pr", }) @@ -88,6 +90,8 @@ TASK_REQUIRED_ROLE = { "review_pr": "reviewer", "merge_pr": "reviewer", "blind_pr_queue_review": "reviewer", + "pr_queue_cleanup": "reviewer", + "pr-queue-cleanup": "reviewer", "request_changes_pr": "reviewer", "approve_pr": "reviewer", # #309: reconciler tasks close already-landed PRs/issues only. diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index 0ef7db1..dc2f961 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -22,6 +22,7 @@ workflow file. | Reconcile already-landed open PRs | [`workflows/reconcile-landed-pr.md`](workflows/reconcile-landed-pr.md) | [`schemas/reconcile-landed-final-report.md`](schemas/reconcile-landed-final-report.md) | | Create or update Gitea issues | [`workflows/create-issue.md`](workflows/create-issue.md) | [`schemas/create-issue-final-report.md`](schemas/create-issue-final-report.md) | | Work on an assigned issue / author code | [`workflows/work-issue.md`](workflows/work-issue.md) | [`schemas/work-issue-final-report.md`](schemas/work-issue-final-report.md) | +| PR-only queue cleanup (one canonical review per PR) | [`workflows/pr-queue-cleanup.md`](workflows/pr-queue-cleanup.md) | [`schemas/pr-queue-cleanup-final-report.md`](schemas/pr-queue-cleanup-final-report.md) | ## Universal rules @@ -50,6 +51,10 @@ changes, merge, implement fixes, create branches, commit, push, or create PRs. A run that starts in `work-issue` mode may not review, approve, request changes, merge, close PRs, or act as reviewer. +A run that starts in `pr-queue-cleanup` mode may not claim issues, create +branches, edit implementation files, file new issues, or review a second PR +after any terminal review mutation. + If the task requires a different mode, stop and produce a handoff for the correct workflow. @@ -156,6 +161,7 @@ Ready-to-copy task prompts live in [`templates/`](templates/): - [`start-issue.md`](templates/start-issue.md) — author work (loads `work-issue.md`) - [`review-pr.md`](templates/review-pr.md) — review (loads `review-merge-pr.md`) +- [`pr-queue-cleanup.md`](templates/pr-queue-cleanup.md) — one PR per cleanup run - [`merge-pr.md`](templates/merge-pr.md) — merge (loads `review-merge-pr.md`) - [`recover-bad-state.md`](templates/recover-bad-state.md) - [`reconcile-closed-not-merged-pr.md`](templates/reconcile-closed-not-merged-pr.md) diff --git a/skills/llm-project-workflow/schemas/pr-queue-cleanup-final-report.md b/skills/llm-project-workflow/schemas/pr-queue-cleanup-final-report.md new file mode 100644 index 0000000..c7abe8c --- /dev/null +++ b/skills/llm-project-workflow/schemas/pr-queue-cleanup-final-report.md @@ -0,0 +1,31 @@ +# PR-queue-cleanup final report schema + +One report per cleanup run (one run = one PR). Fields must all be present; +use `none` where nothing occurred. Validated by +`pr_queue_cleanup.assess_pr_queue_cleanup_report` (fail closed). + +* Task: pr-queue-cleanup +* Workflow source: workflows/pr-queue-cleanup.md (+ version/commit/hash) +* Repo: +* Role/profile: +* Identity: +* PR inventory pagination proof: (inventory_complete / final page / total_count) +* Queue ordering proof: +* Earlier PRs skipped: (with live per-PR proof) +* Selected PR: (exactly one) +* Pinned head SHA: +* Review decision: (single terminal decision) +* Merge authorized for PR: true/false (explicit per-PR operator authorization) +* Merge gates result: +* Merge result: (none / not attempted / merged SHA / blocker) +* Run stop point: (which §4 chain rule ended the run) +* Next suggested PR: (named, not continued to) +* File edits by reviewer: +* Worktree/index mutations: +* Git ref mutations: +* MCP/Gitea mutations: +* Issue mutations: none (required — forbidden in cleanup mode) +* Branch mutations: none (required — forbidden in cleanup mode) +* Read-only diagnostics: +* Blockers: +* Safe next action: (fresh run for the next PR) diff --git a/skills/llm-project-workflow/templates/pr-queue-cleanup.md b/skills/llm-project-workflow/templates/pr-queue-cleanup.md new file mode 100644 index 0000000..d3913f1 --- /dev/null +++ b/skills/llm-project-workflow/templates/pr-queue-cleanup.md @@ -0,0 +1,25 @@ +# Template: PR-only queue cleanup (one PR per run) + +Copy, fill the `<...>` fields, and paste as the task prompt. + +```text +Task: PR-only queue cleanup for . + +Load workflows/pr-queue-cleanup.md and workflows/review-merge-pr.md before any +mutation. Route pr_queue_cleanup through gitea_route_task_session (reviewer +profile required). + +Rules: +- One run = exactly one selected PR = one terminal review decision. +- Build full open-PR inventory with pagination proof before selection. +- Forbidden: issue claiming, branch creation, implementation edits, issue filing, + reviewing a second PR after a terminal mutation in this run. +- After REQUEST_CHANGES: stop. After APPROVED: merge only this PR and only if + operator explicitly authorized merge for this PR in this run. +- Report Next suggested PR without continuing to it. + +Operator PR list (optional): +Merge authorized for selected PR in this run: + +End with the pr-queue-cleanup final report schema. +``` \ No newline at end of file diff --git a/skills/llm-project-workflow/workflows/pr-queue-cleanup.md b/skills/llm-project-workflow/workflows/pr-queue-cleanup.md new file mode 100644 index 0000000..8a1be98 --- /dev/null +++ b/skills/llm-project-workflow/workflows/pr-queue-cleanup.md @@ -0,0 +1,86 @@ +--- +task_mode: pr-queue-cleanup +canonical: true +final_report_schema: ../schemas/pr-queue-cleanup-final-report.md +--- + +# PR-only queue cleanup workflow (canonical) + +**Task mode:** `pr-queue-cleanup` + +Reviewer-role mode for cleanup periods when the queue holds many open PRs. +Each run dispatches **exactly one** canonical review for **exactly one** PR, +then stops after any terminal review mutation. The next PR always requires a +new run with fresh identity, capability, and inventory proof. + +This mode composes with the canonical review workflow: for the selected PR, +load and follow [`workflows/review-merge-pr.md`](review-merge-pr.md) in full. +This file adds the cleanup-mode boundaries around that per-PR run; it does +not replace the review workflow. + +## 0. Load the canonical workflow first + +Load this file and `workflows/review-merge-pr.md` before any mutation and +report source, version/commit/hash, and any conflict with the operator +prompt. If either cannot be loaded, stop and produce a recovery handoff. + +## 1. Mode isolation — forbidden actions + +PR-only cleanup mode forbids, with no exceptions: + +* issue claiming (`claim_issue`, `mark_issue`, `lock_issue`) +* branch creation or push (`create_branch`, `push_branch`) +* implementation edits of any kind +* new issue filing (`create_issue`) +* PR creation (`create_pr`) and commit tools (`commit_files`) +* reviewing a second PR after any terminal review mutation + +`pr_queue_cleanup.check_cleanup_task_allowed` fails closed on these tasks. +If author-side work is needed, stop and hand off to `work-issue` mode in a +separate session. + +## 2. Identity, capability, and routing + +Route `pr_queue_cleanup` through `gitea_route_task_session` — reviewer role +required; author sessions receive `wrong_role_stop`. Prove `gitea.pr.review` +capability before selection. Merge additionally requires `gitea.pr.merge` +plus the explicit per-PR authorization in §4. + +## 3. Inventory and selection + +Build the complete open-PR inventory with pagination proof +(`inventory_complete`, final page, `total_count`) before any selection +claim. Select exactly one PR according to project queue ordering rules +(oldest-first unless the operator queue says otherwise), skipping earlier +PRs only with live per-PR proof. No multi-PR validation and no batch report +may substitute for per-PR proof. + +## 4. Terminal mutation chain + +`pr_queue_cleanup.resolve_cleanup_run_state` is the authority: + +* After `REQUEST_CHANGES` → the run stops. +* After `COMMENT` or a proof-backed skip → the run stops. +* After `APPROVED` → the run may continue **only** to same-PR merge, and only + when the operator explicitly authorized merge for that specific PR in this + run (`Merge authorized: true`) and every merge gate passes. +* After merge, or on any merge blocker → the run stops. +* Any terminal mutation targeting a PR other than the selected PR is a hard + stop and must be reported as a violation. + +## 5. Fresh run per PR + +The next PR requires a new run with fresh identity, capability, inventory, +and ordering proof. Do not carry pinned SHAs, eligibility classes, or +validation results across runs. + +## 6. Final report + +Use the schema in +[`schemas/pr-queue-cleanup-final-report.md`](../schemas/pr-queue-cleanup-final-report.md). +The report must include the **Next suggested PR** (from the proven ordering) +without continuing to it, exactly one **Selected PR**, the single terminal +decision, pagination proof, and `Issue mutations: none` / `Branch mutations: +none`. `pr_queue_cleanup.assess_pr_queue_cleanup_report` validates these +fields and fails closed on batch reviews, missing pagination proof, missing +next-suggested-PR, unauthorized merges, or any issue/branch mutation. diff --git a/task_capability_map.py b/task_capability_map.py index 8453255..6bbad48 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -72,6 +72,14 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.pr.review", "role": "reviewer", }, + "pr_queue_cleanup": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, + "pr-queue-cleanup": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, "request_changes_pr": { "permission": "gitea.pr.request_changes", "role": "reviewer", diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 39c92d0..40c2764 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -16,6 +16,7 @@ def test_all_workflow_files_exist(): "reconcile-landed-pr.md", "create-issue.md", "work-issue.md", + "pr-queue-cleanup.md", ): assert (SKILL_DIR / "workflows" / name).is_file(), name @@ -26,6 +27,7 @@ def test_skill_references_all_workflow_files(): "workflows/reconcile-landed-pr.md", "workflows/create-issue.md", "workflows/work-issue.md", + "workflows/pr-queue-cleanup.md", ): assert name in SKILL @@ -36,6 +38,7 @@ def test_skill_contains_mode_isolation_language(): assert "reconcile-landed-pr" in SKILL assert "create-issue" in SKILL assert "work-issue" in SKILL + assert "pr-queue-cleanup" in SKILL assert "Do not mix modes" in SKILL or "do not mix modes" in SKILL.lower() @@ -99,12 +102,24 @@ def test_work_issue_workflow_contract(): assert "## 21. PR creation rules" in text +def test_pr_queue_cleanup_workflow_contract(): + text = (SKILL_DIR / "workflows" / "pr-queue-cleanup.md").read_text( + encoding="utf-8" + ) + assert "canonical: true" in text + assert "exactly one" in text + assert "check_cleanup_task_allowed" in text + assert "resolve_cleanup_run_state" in text + assert "Next suggested PR" in text + + def test_final_report_schemas_exist(): for name in ( "review-merge-final-report.md", "reconcile-landed-final-report.md", "create-issue-final-report.md", "work-issue-final-report.md", + "pr-queue-cleanup-final-report.md", ): assert (SKILL_DIR / "schemas" / name).is_file(), name @@ -124,6 +139,20 @@ def test_start_issue_template_references_work_issue_workflow(): assert "workflows/work-issue.md" in text +def test_pr_queue_cleanup_template_references_workflow(): + text = (SKILL_DIR / "templates" / "pr-queue-cleanup.md").read_text( + encoding="utf-8" + ) + assert "workflows/pr-queue-cleanup.md" in text + assert "pr_queue_cleanup" in text + + +def test_pr_queue_cleanup_verifier_exported(): + from review_proofs import assess_pr_queue_cleanup_report + + assert callable(assess_pr_queue_cleanup_report) + + def test_reviewer_fallback_verifier_exported(): from review_proofs import assess_reviewer_fallback_report @@ -199,4 +228,23 @@ def test_mutation_categories_verifier_exported(): def test_worktree_ownership_verifier_exported(): from review_proofs import assess_worktree_ownership_report - assert callable(assess_worktree_ownership_report) \ No newline at end of file + assert callable(assess_worktree_ownership_report) + + +def test_pr_queue_cleanup_verifier_exported(): + from review_proofs import assess_pr_queue_cleanup_report + + assert callable(assess_pr_queue_cleanup_report) + + +def test_pr_queue_cleanup_workflow_contract(): + text = (SKILL_DIR / "workflows" / "pr-queue-cleanup.md").read_text(encoding="utf-8") + assert "canonical: true" in text + assert "task_mode: pr-queue-cleanup" in text + assert "## 1. Mode isolation" in text + assert "## 4. Terminal mutation chain" in text + assert "## 5. Fresh run per PR" in text + assert "exactly one" in text.lower() + assert "review-merge-pr.md" in text + assert "Next suggested PR" in text + assert "schemas/pr-queue-cleanup-final-report.md" in text diff --git a/tests/test_pr_queue_cleanup.py b/tests/test_pr_queue_cleanup.py new file mode 100644 index 0000000..3cec75a --- /dev/null +++ b/tests/test_pr_queue_cleanup.py @@ -0,0 +1,118 @@ +"""Tests for PR-only queue cleanup mode (#390).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from pr_queue_cleanup import ( + CLEANUP_WORKFLOW_PATH, + STOP_AFTER_REQUEST_CHANGES, + STOP_APPROVED_NO_MERGE_AUTH, + CONTINUE_TO_SAME_PR_MERGE, + assess_pr_queue_cleanup_report, + check_cleanup_task_allowed, + resolve_cleanup_run_state, +) +from review_proofs import assess_pr_queue_cleanup_report as proofs_assess + + +GOOD_REPORT = f""" +Task: pr-queue-cleanup +Workflow source: {CLEANUP_WORKFLOW_PATH} +PR inventory pagination proof: inventory_complete=true, total_count=12 +Selected PR: #401 +Review decision: approved +Merge authorized for PR: true +Merge result: not attempted +Next suggested PR: #402 +Issue mutations: none +Branch mutations: none +""" + + +class TestCleanupRunState(unittest.TestCase): + def test_request_changes_stops_run(self): + result = resolve_cleanup_run_state("request_changes") + self.assertEqual(result["outcome"], STOP_AFTER_REQUEST_CHANGES) + self.assertFalse(result["further_mutation_allowed"]) + + def test_approved_without_merge_auth_stops(self): + result = resolve_cleanup_run_state("approved") + self.assertEqual(result["outcome"], STOP_APPROVED_NO_MERGE_AUTH) + self.assertFalse(result["further_mutation_allowed"]) + + def test_approved_with_merge_auth_continues_to_merge(self): + result = resolve_cleanup_run_state( + "approved", + merge_authorized_for_pr=True, + merge_gates_passed=True, + ) + self.assertEqual(result["outcome"], CONTINUE_TO_SAME_PR_MERGE) + self.assertTrue(result["further_mutation_allowed"]) + + +class TestCleanupTaskGate(unittest.TestCase): + def test_author_tasks_blocked(self): + for task in ( + "lock_issue", + "create_issue", + "create_branch", + "create_pr", + "commit_files", + ): + allowed, reasons = check_cleanup_task_allowed(task) + self.assertFalse(allowed, task) + self.assertTrue(reasons) + + +class TestCleanupReportVerifier(unittest.TestCase): + def test_good_report_passes(self): + result = assess_pr_queue_cleanup_report(GOOD_REPORT) + self.assertTrue(result["proven"]) + self.assertFalse(result["block"]) + + def test_multi_pr_selection_blocked(self): + report = GOOD_REPORT + "\nSelected PR: #403\n" + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertIn("multiple prs", result["reasons"][0].lower()) + + def test_missing_pagination_proof_blocked(self): + report = GOOD_REPORT.replace( + "PR inventory pagination proof: inventory_complete=true, total_count=12", + "PR inventory pagination proof: inventory listed", + ) + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("pagination" in r.lower() for r in result["reasons"]) + ) + + def test_merge_without_authorization_blocked(self): + report = GOOD_REPORT.replace( + "Merge authorized for PR: true", + "Merge authorized for PR: false", + ).replace("Merge result: not attempted", "Merge result: merged abc123") + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("merge" in r.lower() for r in result["reasons"]) + ) + + def test_issue_mutations_forbidden(self): + report = GOOD_REPORT.replace( + "Issue mutations: none", + "Issue mutations: gitea_mark_issue", + ) + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + + def test_review_proofs_wrapper_matches_module(self): + direct = assess_pr_queue_cleanup_report(GOOD_REPORT) + wrapped = proofs_assess(GOOD_REPORT) + self.assertEqual(direct["proven"], wrapped["proven"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_resolve_task_capability.py b/tests/test_resolve_task_capability.py index 3545675..fca97aa 100644 --- a/tests/test_resolve_task_capability.py +++ b/tests/test_resolve_task_capability.py @@ -275,6 +275,28 @@ class TestResolveTaskCapability(unittest.TestCase): self.assertEqual(res["required_role_kind"], "author") self.assertIn("author", res["exact_safe_next_action"].lower()) + @patch("mcp_server.api_request", return_value={"login": "author-user"}) + @patch("mcp_server.get_auth_header", return_value="token author-pass") + def test_resolve_pr_queue_cleanup_author_profile_blocked(self, _auth, _api): + with patch.dict(os.environ, self._env("author-profile")): + res = mcp_server.gitea_resolve_task_capability( + task="pr_queue_cleanup", remote="prgs" + ) + self.assertEqual(res["required_role_kind"], "reviewer") + self.assertFalse(res["allowed_in_current_session"]) + self.assertTrue(res["stop_required"]) + + @patch("mcp_server.api_request", return_value={"login": "reviewer-user"}) + @patch("mcp_server.get_auth_header", return_value="token reviewer-pass") + def test_resolve_pr_queue_cleanup_reviewer_profile_allowed(self, _auth, _api): + with patch.dict(os.environ, self._env("reviewer-profile")): + res = mcp_server.gitea_resolve_task_capability( + task="pr_queue_cleanup", remote="prgs" + ) + self.assertEqual(res["required_operation_permission"], "gitea.pr.review") + self.assertTrue(res["allowed_in_current_session"]) + self.assertFalse(res["stop_required"]) + @patch("mcp_server.api_request", return_value={"login": "author-user"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_close_pr_known_and_lookalike_tasks_still_fail_closed(self, _auth, _api): diff --git a/tests/test_role_session_router.py b/tests/test_role_session_router.py index 6204b6d..2099407 100644 --- a/tests/test_role_session_router.py +++ b/tests/test_role_session_router.py @@ -99,6 +99,24 @@ class TestRoleSessionRouter(unittest.TestCase): self.assertFalse(route["downstream_allowed"]) self.assertIn("Wrong role/session for reviewer task", route["message"]) + def test_pr_queue_cleanup_under_author_profile_wrong_role_stop(self): + with patch.dict(os.environ, self._env("prgs-author")): + route = mcp_server.gitea_route_task_session( + task_type="pr_queue_cleanup", remote="prgs" + ) + self.assertEqual(route["route_result"], role_session_router.ROUTE_WRONG_ROLE) + self.assertFalse(route["downstream_allowed"]) + + @patch("mcp_server.api_request", return_value={"login": "sysadmin"}) + @patch("mcp_server.get_auth_header", return_value="token reviewer-pass") + def test_pr_queue_cleanup_under_reviewer_profile_allowed(self, _auth, _api): + with patch.dict(os.environ, self._env("prgs-reviewer")): + route = mcp_server.gitea_route_task_session( + task_type="pr_queue_cleanup", remote="prgs" + ) + self.assertEqual(route["route_result"], role_session_router.ROUTE_ALLOWED) + self.assertTrue(route["downstream_allowed"]) + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_issue_creation_blocked_after_reviewer_wrong_role_stop( From a2bd23da144a7186d214aa148071953ad4cea926 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 10:06:50 -0400 Subject: [PATCH 15/15] test: expand PR-queue-cleanup coverage to full acceptance matrix (#390) Superset of the initial test slice: adds capability-map and router routing cases (reviewer-role enforcement, author wrong_role_stop), the full terminal-chain matrix (comment/skip stop, gates-failed stop, merge completed/blocker stop, unknown-decision fail-closed), and report verifier cases for two terminal mutations, missing next-suggested-PR, branch mutations, and missing workflow citation. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests/test_pr_queue_cleanup.py | 259 ++++++++++++++++++++++++++------- 1 file changed, 205 insertions(+), 54 deletions(-) diff --git a/tests/test_pr_queue_cleanup.py b/tests/test_pr_queue_cleanup.py index 3cec75a..e26a026 100644 --- a/tests/test_pr_queue_cleanup.py +++ b/tests/test_pr_queue_cleanup.py @@ -6,113 +6,264 @@ from pathlib import Path sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) from pr_queue_cleanup import ( + CLEANUP_FORBIDDEN_TASKS, CLEANUP_WORKFLOW_PATH, - STOP_AFTER_REQUEST_CHANGES, - STOP_APPROVED_NO_MERGE_AUTH, CONTINUE_TO_SAME_PR_MERGE, + STOP_AFTER_DECISION, + STOP_AFTER_MERGE, + STOP_AFTER_MERGE_BLOCKER, + STOP_AFTER_REQUEST_CHANGES, + STOP_APPROVED_MERGE_GATES_FAILED, + STOP_APPROVED_NO_MERGE_AUTH, + STOP_GATE_NOT_PROVEN, assess_pr_queue_cleanup_report, check_cleanup_task_allowed, resolve_cleanup_run_state, ) from review_proofs import assess_pr_queue_cleanup_report as proofs_assess +from role_session_router import REVIEWER_TASKS, route_task_session +from task_capability_map import required_permission, required_role -GOOD_REPORT = f""" -Task: pr-queue-cleanup -Workflow source: {CLEANUP_WORKFLOW_PATH} -PR inventory pagination proof: inventory_complete=true, total_count=12 -Selected PR: #401 -Review decision: approved -Merge authorized for PR: true -Merge result: not attempted -Next suggested PR: #402 -Issue mutations: none -Branch mutations: none -""" +class TestCleanupCapabilityMapping(unittest.TestCase): + def test_cleanup_task_is_reviewer_role(self): + for key in ("pr_queue_cleanup", "pr-queue-cleanup"): + self.assertEqual(required_permission(key), "gitea.pr.review") + self.assertEqual(required_role(key), "reviewer") + def test_cleanup_task_registered_as_reviewer_route(self): + self.assertIn("pr_queue_cleanup", REVIEWER_TASKS) -class TestCleanupRunState(unittest.TestCase): - def test_request_changes_stops_run(self): - result = resolve_cleanup_run_state("request_changes") - self.assertEqual(result["outcome"], STOP_AFTER_REQUEST_CHANGES) - self.assertFalse(result["further_mutation_allowed"]) - - def test_approved_without_merge_auth_stops(self): - result = resolve_cleanup_run_state("approved") - self.assertEqual(result["outcome"], STOP_APPROVED_NO_MERGE_AUTH) - self.assertFalse(result["further_mutation_allowed"]) - - def test_approved_with_merge_auth_continues_to_merge(self): - result = resolve_cleanup_run_state( - "approved", - merge_authorized_for_pr=True, - merge_gates_passed=True, + def test_author_session_gets_wrong_role_stop(self): + result = route_task_session( + "pr_queue_cleanup", + active_profile="prgs-author", + active_role_kind="author", + allowed_in_current_session=False, ) - self.assertEqual(result["outcome"], CONTINUE_TO_SAME_PR_MERGE) - self.assertTrue(result["further_mutation_allowed"]) + self.assertEqual(result["route_result"], "wrong_role_stop") + self.assertFalse(result["downstream_allowed"]) + + def test_reviewer_session_allowed(self): + result = route_task_session( + "pr_queue_cleanup", + active_profile="prgs-reviewer", + active_role_kind="reviewer", + allowed_in_current_session=True, + ) + self.assertEqual(result["route_result"], "allowed_current_session") + self.assertTrue(result["downstream_allowed"]) class TestCleanupTaskGate(unittest.TestCase): def test_author_tasks_blocked(self): for task in ( + "claim_issue", + "mark_issue", "lock_issue", "create_issue", "create_branch", + "push_branch", "create_pr", "commit_files", ): allowed, reasons = check_cleanup_task_allowed(task) self.assertFalse(allowed, task) - self.assertTrue(reasons) + self.assertTrue(reasons, task) + + def test_review_task_allowed(self): + allowed, reasons = check_cleanup_task_allowed("review_pr") + self.assertTrue(allowed) + self.assertEqual(reasons, []) + + def test_forbidden_set_covers_issue_filing_and_impl(self): + self.assertIn("create_issue", CLEANUP_FORBIDDEN_TASKS) + self.assertIn("create_branch", CLEANUP_FORBIDDEN_TASKS) + self.assertIn("commit_files", CLEANUP_FORBIDDEN_TASKS) + + +class TestCleanupRunState(unittest.TestCase): + def test_request_changes_stops_run(self): + state = resolve_cleanup_run_state("request_changes") + self.assertEqual(state["outcome"], STOP_AFTER_REQUEST_CHANGES) + self.assertFalse(state["further_mutation_allowed"]) + + def test_comment_and_skip_stop_run(self): + for decision in ("comment", "skip"): + state = resolve_cleanup_run_state(decision) + self.assertEqual(state["outcome"], STOP_AFTER_DECISION) + self.assertFalse(state["further_mutation_allowed"]) + + def test_approved_without_merge_auth_stops(self): + state = resolve_cleanup_run_state("approved", merge_gates_passed=True) + self.assertEqual(state["outcome"], STOP_APPROVED_NO_MERGE_AUTH) + self.assertFalse(state["further_mutation_allowed"]) + + def test_approved_with_merge_auth_continues_to_merge(self): + state = resolve_cleanup_run_state( + "approved", + merge_authorized_for_pr=True, + merge_gates_passed=True, + ) + self.assertEqual(state["outcome"], CONTINUE_TO_SAME_PR_MERGE) + self.assertTrue(state["further_mutation_allowed"]) + self.assertEqual(state["allowed_mutation"], "merge same PR only") + + def test_approved_with_auth_but_failed_gates_stops(self): + state = resolve_cleanup_run_state( + "approved", + merge_authorized_for_pr=True, + merge_gates_passed=False, + ) + self.assertEqual(state["outcome"], STOP_APPROVED_MERGE_GATES_FAILED) + self.assertFalse(state["further_mutation_allowed"]) + + def test_merge_completed_stops(self): + state = resolve_cleanup_run_state( + "approved", + merge_authorized_for_pr=True, + merge_gates_passed=True, + merge_completed=True, + ) + self.assertEqual(state["outcome"], STOP_AFTER_MERGE) + self.assertFalse(state["further_mutation_allowed"]) + + def test_merge_blocker_stops(self): + state = resolve_cleanup_run_state( + "approved", + merge_authorized_for_pr=True, + merge_gates_passed=True, + merge_blocker=True, + ) + self.assertEqual(state["outcome"], STOP_AFTER_MERGE_BLOCKER) + self.assertFalse(state["further_mutation_allowed"]) + + def test_unknown_decision_fails_closed(self): + state = resolve_cleanup_run_state("ship_it") + self.assertEqual(state["outcome"], STOP_GATE_NOT_PROVEN) + self.assertFalse(state["further_mutation_allowed"]) + + +def _clean_report(**overrides): + lines = { + "task": "Task: pr-queue-cleanup", + "workflow": f"Workflow source: {CLEANUP_WORKFLOW_PATH} (hash abc123def456)", + "pagination": ( + "PR inventory pagination proof: inventory_complete=true, final " + "page, total_count 15" + ), + "selected": "Selected PR: #371", + "decision": "Review decision: approved", + "merge_auth": "Merge authorized for PR: false", + "merge_result": "Merge result: not attempted", + "next": "Next suggested PR: #372", + "issue_mut": "Issue mutations: none", + "branch_mut": "Branch mutations: none", + } + lines.update(overrides) + return "\n".join(v for v in lines.values() if v) class TestCleanupReportVerifier(unittest.TestCase): - def test_good_report_passes(self): - result = assess_pr_queue_cleanup_report(GOOD_REPORT) - self.assertTrue(result["proven"]) + def test_clean_single_pr_report_passes(self): + result = assess_pr_queue_cleanup_report(_clean_report()) + self.assertTrue(result["proven"], result["reasons"]) self.assertFalse(result["block"]) + def test_approved_and_merged_with_authorization_passes(self): + report = _clean_report( + merge_auth="Merge authorized for PR: true", + merge_result="Merge result: merged 0123456789ab", + ) + result = assess_pr_queue_cleanup_report(report) + self.assertTrue(result["proven"], result["reasons"]) + + def test_request_changes_then_stop_passes(self): + report = _clean_report(decision="Review decision: request_changes") + result = assess_pr_queue_cleanup_report(report) + self.assertTrue(result["proven"], result["reasons"]) + def test_multi_pr_selection_blocked(self): - report = GOOD_REPORT + "\nSelected PR: #403\n" + report = _clean_report() + "\nSelected PR: #403\n" result = assess_pr_queue_cleanup_report(report) self.assertFalse(result["proven"]) - self.assertIn("multiple prs", result["reasons"][0].lower()) + self.assertTrue( + any("multiple prs" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_two_terminal_mutations_blocked(self): + report = _clean_report() + "\nReview decision: request_changes" + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("terminal review mutations" in r for r in result["reasons"]), + result["reasons"], + ) def test_missing_pagination_proof_blocked(self): - report = GOOD_REPORT.replace( - "PR inventory pagination proof: inventory_complete=true, total_count=12", - "PR inventory pagination proof: inventory listed", + report = _clean_report( + pagination="PR inventory pagination proof: inventory listed" ) result = assess_pr_queue_cleanup_report(report) self.assertFalse(result["proven"]) self.assertTrue( - any("pagination" in r.lower() for r in result["reasons"]) + any("pagination" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_missing_next_suggested_pr_blocked(self): + report = _clean_report(next="") + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("Next suggested PR" in r for r in result["reasons"]), + result["reasons"], ) def test_merge_without_authorization_blocked(self): - report = GOOD_REPORT.replace( - "Merge authorized for PR: true", - "Merge authorized for PR: false", - ).replace("Merge result: not attempted", "Merge result: merged abc123") + report = _clean_report( + merge_result="Merge result: merged abc123abc123", + ) result = assess_pr_queue_cleanup_report(report) self.assertFalse(result["proven"]) self.assertTrue( - any("merge" in r.lower() for r in result["reasons"]) + any("authorization" in r.lower() for r in result["reasons"]), + result["reasons"], ) def test_issue_mutations_forbidden(self): - report = GOOD_REPORT.replace( - "Issue mutations: none", - "Issue mutations: gitea_mark_issue", - ) + report = _clean_report(issue_mut="Issue mutations: gitea_mark_issue") result = assess_pr_queue_cleanup_report(report) self.assertFalse(result["proven"]) + self.assertTrue( + any("issue mutations" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_branch_mutations_forbidden(self): + report = _clean_report(branch_mut="Branch mutations: created feat/x") + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("branch mutations" in r.lower() for r in result["reasons"]), + result["reasons"], + ) + + def test_missing_workflow_citation_blocked(self): + report = _clean_report(workflow="Workflow source: none") + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("pr-queue-cleanup.md" in r for r in result["reasons"]), + result["reasons"], + ) def test_review_proofs_wrapper_matches_module(self): - direct = assess_pr_queue_cleanup_report(GOOD_REPORT) - wrapped = proofs_assess(GOOD_REPORT) + direct = assess_pr_queue_cleanup_report(_clean_report()) + wrapped = proofs_assess(_clean_report()) self.assertEqual(direct["proven"], wrapped["proven"]) if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main()