From 934688a71dd19d9b46369e73fb3ac356ee0cc211 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 13:11:36 -0400 Subject: [PATCH 1/7] feat: add master-parity staleness gate for MCP mutations (Closes #420) The MCP server loads capability-gate code into memory at startup, so a newly merged gate (e.g. the branch-delete capability gate #408/#410) does not take effect until the process restarts. A long-running server could therefore still perform a mutation the updated codebase forbids. Runtime profile/config data is already read live from disk each call (gitea_config.load_config re-reads the JSON), so allowed_operations changes apply without a restart. This closes the remaining gap: *code* parity. - master_parity_gate.py: capture the process's startup commit and, at mutation time, compare it against the on-disk master HEAD; pure, injectable assessment plus block-reasons and a restart-required report. - gitea_mcp_server.py: capture _STARTUP_PARITY at import; _profile_operation_gate fails closed for mutating ops when stale while leaving gitea.read allowed; gitea_get_runtime_context surfaces master_parity; new read-only gitea_assess_master_parity tool reports parity/restart-required. - Escape hatches: GITEA_MCP_DISABLE_PARITY_GATE (ops), GITEA_TEST_CURRENT_HEAD (tests). - tests/test_master_parity_gate.py: 18 cases (pure logic, block/report, read override, and server wiring: reads pass, mutations blocked when stale). Validation: venv/bin/python -m pytest -q -> 1618 passed, 6 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 93 ++++++++++++++++- master_parity_gate.py | 168 +++++++++++++++++++++++++++++++ tests/test_master_parity_gate.py | 152 ++++++++++++++++++++++++++++ 3 files changed, 412 insertions(+), 1 deletion(-) create mode 100644 master_parity_gate.py create mode 100644 tests/test_master_parity_gate.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 0bc82e4..a6a1501 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -508,6 +508,13 @@ import reconciler_profile # noqa: E402 import reconciliation_workflow # noqa: E402 import review_merge_state_machine # noqa: E402 import native_mcp_preference # noqa: E402 +import master_parity_gate # noqa: E402 + +# Master-parity baseline (#420): the commit this server process started at. +# Captured once so that, at mutation time, we can detect when the on-disk +# master has advanced past the running code and fail closed until restart. +# Read-only operations are never blocked by staleness. +_STARTUP_PARITY = master_parity_gate.capture_startup_parity(PROJECT_ROOT) # Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, @@ -4252,15 +4259,41 @@ def _try_auto_switch_for_operation(op: str, host: str | None = None) -> bool: return False +def _current_master_parity() -> dict: + """Assess this process's code against the on-disk master HEAD (#420).""" + current_head = master_parity_gate.read_git_head(PROJECT_ROOT) + return master_parity_gate.assess_master_parity(_STARTUP_PARITY, current_head) + + +def _master_parity_block(op: str) -> list[str]: + """Fail-closed staleness reasons for a mutation *op* (#420). + + Read-only operations (``gitea.read``) are never blocked: a stale server can + still be inspected. Any other (mutating) operation is refused when the + on-disk master has definitively advanced past the running code, since newly + merged capability gates would not yet be loaded in memory. + """ + if op == "gitea.read": + return [] + return master_parity_gate.parity_block_reasons(_current_master_parity()) + + def _profile_operation_gate(op: str) -> list[str]: - """Profile permission check for a single gated operation (#126, #216). + """Profile permission check for a single gated operation (#126, #216, #420). Issue discussion comments are gated separately from the gitea.pr.* review/merge family: listing requires ``gitea.read``, creating requires ``gitea.issue.comment``. Closing a PR requires the distinct ``gitea.pr.close`` (#216). Returns a list of block reasons (empty = allowed); an unreadable profile fails closed. + + A mutating operation is additionally refused when the server code is stale + relative to master (#420), so a long-running process cannot bypass a + capability gate that has since been merged. """ + stale_reasons = _master_parity_block(op) + if stale_reasons: + return stale_reasons try: profile = get_profile() except Exception as exc: @@ -5673,12 +5706,70 @@ def gitea_get_runtime_context( "shell_health": native_mcp_preference.shell_health_status(), } + parity = _current_master_parity() + result["master_parity"] = { + "in_parity": parity["in_parity"], + "stale": parity["stale"], + "restart_required": parity["restart_required"], + "startup_head": parity["startup_head"], + "current_head": parity["current_head"], + "summary": master_parity_gate.format_parity(parity), + "mutation_gate_enforced": not master_parity_gate.gate_disabled(), + } + if parity["stale"] and not master_parity_gate.gate_disabled(): + safe_next_action = ( + "Server code is stale relative to master; restart the Gitea MCP " + "server to load current capability gates before mutating. " + f"({master_parity_gate.format_parity(parity)})" + ) + result["safe_next_action"] = safe_next_action + if reveal and h: result["server"] = gitea_url(h, "").rstrip("/") return result +@mcp.tool() +def gitea_assess_master_parity( + remote: str = "dadeschools", + host: str | None = None, +) -> dict: + """Read-only: is the running server code in parity with the on-disk master? + + The MCP server loads its capability-gate code into memory at startup; + ``master`` advancing (e.g. a newly merged security gate) does not take + effect until the process restarts. This tool compares the commit the + process started at against the current workspace ``HEAD`` and reports + whether a restart is required before mutations are safe again (#420). + + Never mutates and makes no network calls. Read-only operations are never + blocked by staleness; only mutating operations fail closed while stale. + + Returns: + dict with 'in_parity', 'stale', 'restart_required', 'startup_head', + 'current_head', 'mutation_gate_enforced', 'summary', 'reasons', and a + 'report' recovery payload when stale. + """ + parity = _current_master_parity() + enforced = not master_parity_gate.gate_disabled() + out = { + "in_parity": parity["in_parity"], + "stale": parity["stale"], + "restart_required": parity["restart_required"], + "determinable": parity["determinable"], + "startup_head": parity["startup_head"], + "current_head": parity["current_head"], + "mutation_gate_enforced": enforced, + "summary": master_parity_gate.format_parity(parity), + "reasons": parity["reasons"], + "process_root": PROJECT_ROOT, + } + if parity["stale"] and enforced: + out["report"] = master_parity_gate.parity_report(parity) + return out + + @mcp.tool() def gitea_list_profiles() -> dict: """Read-only: list all Gitea MCP profiles with redacted metadata. diff --git a/master_parity_gate.py b/master_parity_gate.py new file mode 100644 index 0000000..efb7314 --- /dev/null +++ b/master_parity_gate.py @@ -0,0 +1,168 @@ +"""Master-parity staleness gate (#420). + +The Gitea MCP server loads its capability-gate code and workflow logic into +memory when the process starts. When ``master`` advances -- for example a newly +merged security gate such as the branch-delete capability gate (#408/#410) -- +the running process keeps executing the *old* code until it is restarted. A +stale server can therefore still perform a mutation that the updated codebase +would forbid. + +Runtime profile/config data is already read live from disk on every call +(``gitea_config.load_config`` re-reads the JSON file each time), so profile +``allowed_operations`` changes take effect immediately without a restart. The +gap this module closes is *code* parity: it captures the server process's +source-tree commit at startup and detects, at mutation time, when the on-disk +``master`` HEAD has advanced past it. Detected staleness fails closed with a +restart-required recovery report, while read-only operations stay allowed so a +stale server can still be inspected. + +The core assessment is pure -- callers inject the observed HEAD SHAs -- so the +logic is fully unit-testable without a git checkout. +""" + +from __future__ import annotations + +import os +import subprocess + +# Environment escape hatches (ops + tests): +# GITEA_MCP_DISABLE_PARITY_GATE -> disable enforcement entirely (fail open). +# GITEA_TEST_CURRENT_HEAD -> force the "current" HEAD read, for tests. +ENV_DISABLE = "GITEA_MCP_DISABLE_PARITY_GATE" +ENV_TEST_CURRENT_HEAD = "GITEA_TEST_CURRENT_HEAD" + + +def read_git_head(root: str) -> str | None: + """Return the current ``HEAD`` commit SHA of *root*, or ``None``. + + ``None`` means the SHA could not be determined (not a git checkout, git + unavailable, or an error). A test override via ``GITEA_TEST_CURRENT_HEAD`` + takes precedence so the gate can be exercised deterministically. + """ + forced = os.environ.get(ENV_TEST_CURRENT_HEAD) + if forced is not None: + return forced.strip() or None + if not root: + return None + try: + res = subprocess.run( + ["git", "-C", root, "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=False, + ) + except Exception: + return None + if res.returncode != 0: + return None + return (res.stdout or "").strip() or None + + +def capture_startup_parity(root: str, head: str | None = None) -> dict: + """Capture the process source-tree baseline once at server startup. + + *head* may be injected (tests); otherwise it is read from *root*. The result + is an opaque baseline handed back to :func:`assess_master_parity`. + """ + startup_head = head if head is not None else read_git_head(root) + return {"root": root, "startup_head": startup_head} + + +def _short(sha: str | None) -> str: + return sha[:12] if sha else "unknown" + + +def assess_master_parity(startup: dict | None, current_head: str | None) -> dict: + """Compare the startup baseline against the current on-disk ``HEAD``. + + Pure: both HEADs are supplied by the caller. Returns a structured result: + + - ``in_parity`` -- server code matches the on-disk master (or parity + could not be determined, which is not treated as stale). + - ``stale`` -- the on-disk master has definitively advanced past the + running process. + - ``restart_required`` -- alias of ``stale``; the recovery action. + - ``determinable`` -- whether both HEADs were known well enough to compare. + - ``startup_head`` / ``current_head`` / ``reasons``. + """ + startup_head = (startup or {}).get("startup_head") + reasons: list[str] = [] + + if startup_head is None: + reasons.append( + "startup commit was not captured; code parity cannot be enforced") + return _result(True, False, False, startup_head, current_head, reasons) + + if current_head is None: + reasons.append( + "current workspace HEAD could not be read; code parity cannot be " + "enforced") + return _result(True, False, False, startup_head, current_head, reasons) + + if startup_head == current_head: + return _result(True, False, True, startup_head, current_head, reasons) + + reasons.append( + f"MCP server started at commit {_short(startup_head)} but the workspace " + f"master is now {_short(current_head)}; restart the server to load the " + f"current capability gates") + return _result(False, True, True, startup_head, current_head, reasons) + + +def _result(in_parity, stale, determinable, startup_head, current_head, reasons): + return { + "in_parity": in_parity, + "stale": stale, + "restart_required": stale, + "determinable": determinable, + "startup_head": startup_head, + "current_head": current_head, + "reasons": list(reasons), + } + + +def gate_disabled() -> bool: + """Whether the parity gate is disabled by env escape hatch.""" + return bool((os.environ.get(ENV_DISABLE) or "").strip()) + + +def parity_block_reasons(assessment: dict) -> list[str]: + """Block reasons for a mutation gate (empty when the mutation may proceed). + + A disabled gate or an in-parity / non-determinable assessment yields no + reasons; only a definitively stale server blocks. + """ + if gate_disabled(): + return [] + if assessment.get("stale"): + return list(assessment.get("reasons") or + ["server code is stale relative to master (fail closed)"]) + return [] + + +def parity_report(assessment: dict) -> dict: + """Structured stale-server report for permission-block payloads.""" + return { + "kind": "server_stale", + "restart_required": True, + "startup_head": assessment.get("startup_head"), + "current_head": assessment.get("current_head"), + "reasons": list(assessment.get("reasons") or []), + "recovery": [ + "The running MCP server is executing code older than the current " + "master and may not enforce newly merged capability gates.", + "Restart the Gitea MCP server so it reloads master's capability " + "gates and execution profiles before retrying the mutation.", + ], + } + + +def format_parity(assessment: dict) -> str: + """One-line human summary for logs / runtime context.""" + if assessment.get("stale"): + return (f"STALE: started {_short(assessment.get('startup_head'))}, " + f"master now {_short(assessment.get('current_head'))} " + f"(restart required)") + if not assessment.get("determinable"): + return "parity indeterminate (baseline or current HEAD unknown)" + return f"in parity at {_short(assessment.get('current_head'))}" diff --git a/tests/test_master_parity_gate.py b/tests/test_master_parity_gate.py new file mode 100644 index 0000000..33086f0 --- /dev/null +++ b/tests/test_master_parity_gate.py @@ -0,0 +1,152 @@ +"""Tests for the master-parity staleness gate (#420). + +Covers the pure assessment logic, the mutation-block helper, the env escape +hatches, and the server-side wiring: reads are never blocked by staleness, a +mutation is refused when the on-disk master has advanced, and the read-only +gitea_assess_master_parity tool reports the stale state. +""" +import os +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import master_parity_gate as mp # noqa: E402 + + +SHA_A = "a" * 40 +SHA_B = "b" * 40 + + +class TestAssessMasterParity(unittest.TestCase): + def test_in_parity_when_heads_match(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_A) + self.assertTrue(res["in_parity"]) + self.assertFalse(res["stale"]) + self.assertFalse(res["restart_required"]) + self.assertTrue(res["determinable"]) + + def test_stale_when_master_advanced(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + self.assertFalse(res["in_parity"]) + self.assertTrue(res["stale"]) + self.assertTrue(res["restart_required"]) + self.assertTrue(res["determinable"]) + self.assertTrue(any("restart" in r for r in res["reasons"])) + + def test_missing_startup_head_not_determinable(self): + res = mp.assess_master_parity({"startup_head": None}, SHA_B) + self.assertFalse(res["determinable"]) + self.assertFalse(res["stale"]) + self.assertTrue(res["in_parity"]) + + def test_missing_current_head_not_determinable(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, None) + self.assertFalse(res["determinable"]) + self.assertFalse(res["stale"]) + self.assertTrue(res["in_parity"]) + + def test_none_startup_baseline(self): + res = mp.assess_master_parity(None, SHA_A) + self.assertFalse(res["determinable"]) + self.assertFalse(res["stale"]) + + +class TestBlockReasonsAndReport(unittest.TestCase): + def test_stale_produces_block_reasons(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + self.assertTrue(mp.parity_block_reasons(res)) + + def test_in_parity_no_block_reasons(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_A) + self.assertEqual(mp.parity_block_reasons(res), []) + + def test_disable_env_suppresses_block(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + with patch.dict(os.environ, {mp.ENV_DISABLE: "1"}): + self.assertTrue(mp.gate_disabled()) + self.assertEqual(mp.parity_block_reasons(res), []) + + def test_report_shape_when_stale(self): + res = mp.assess_master_parity({"startup_head": SHA_A}, SHA_B) + report = mp.parity_report(res) + self.assertEqual(report["kind"], "server_stale") + self.assertTrue(report["restart_required"]) + self.assertEqual(report["startup_head"], SHA_A) + self.assertEqual(report["current_head"], SHA_B) + self.assertTrue(report["recovery"]) + + +class TestReadGitHead(unittest.TestCase): + def test_test_override_takes_precedence(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + self.assertEqual(mp.read_git_head("/nonexistent"), SHA_B) + + def test_blank_override_is_none(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: " "}): + self.assertIsNone(mp.read_git_head("/nonexistent")) + + def test_empty_root_is_none(self): + # No override set; empty root cannot resolve a HEAD. + env = {k: v for k, v in os.environ.items() + if k != mp.ENV_TEST_CURRENT_HEAD} + with patch.dict(os.environ, env, clear=True): + self.assertIsNone(mp.read_git_head("")) + + +class TestServerWiring(unittest.TestCase): + """Integration with the gate choke point in the server namespace.""" + + def setUp(self): + import mcp_server + self.srv = mcp_server + # Pin a known startup baseline so the current-HEAD override can diverge. + self._saved = self.srv._STARTUP_PARITY + self.srv._STARTUP_PARITY = {"root": self.srv.PROJECT_ROOT, + "startup_head": SHA_A} + + def tearDown(self): + self.srv._STARTUP_PARITY = self._saved + + def test_reads_not_blocked_when_stale(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + self.assertEqual(self.srv._master_parity_block("gitea.read"), []) + + def test_mutation_blocked_when_stale(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + reasons = self.srv._master_parity_block("gitea.pr.create") + self.assertTrue(reasons) + # The profile-operation choke point surfaces the same block. + self.assertTrue(self.srv._profile_operation_gate("gitea.pr.create")) + + def test_mutation_allowed_when_in_parity(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_A}): + self.assertEqual( + self.srv._master_parity_block("gitea.pr.create"), []) + + def test_disable_env_lets_mutation_through(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B, + mp.ENV_DISABLE: "1"}): + self.assertEqual( + self.srv._master_parity_block("gitea.pr.create"), []) + + def test_assess_tool_reports_stale(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_B}): + out = self.srv.gitea_assess_master_parity(remote="prgs") + self.assertTrue(out["stale"]) + self.assertTrue(out["restart_required"]) + self.assertEqual(out["startup_head"], SHA_A) + self.assertEqual(out["current_head"], SHA_B) + self.assertIn("report", out) + + def test_assess_tool_reports_in_parity(self): + with patch.dict(os.environ, {mp.ENV_TEST_CURRENT_HEAD: SHA_A}): + out = self.srv.gitea_assess_master_parity(remote="prgs") + self.assertFalse(out["stale"]) + self.assertTrue(out["in_parity"]) + self.assertNotIn("report", out) + + +if __name__ == "__main__": + unittest.main() From 8f866ae75336d830ec804d2d227263059f2c0c85 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 01:47:34 -0400 Subject: [PATCH 2/7] feat: add skip-stale-REQUEST_CHANGES reviewer menu prompt (#482) Add a second reviewer workflow prompt to the root MCP operator menu: "Skip already-reviewed stale REQUEST_CHANGES PR and hand off to author". show_reviewer_prompts becomes a small 2-option submenu (mirroring the existing show_author_prompts idiom): option 1 keeps the standard PR review prompt; option 2 is the new prompt. It instructs the reviewer to determine whether the current head already carries a non-stale REQUEST_CHANGES verdict and, if so, to submit no new terminal review mutation, run only handoff diagnostics, and return an author-ready fix prompt with the full 8-field handoff. Stacked on PR #479 (feat/issue-478-mcp-menu-shell); the menu is not yet on master. Diff limited to the new prompt: mcp-menu.sh, docs/mcp-menu.md, and a discoverability test in tests/test_mcp_menu_script.py. Closes #482. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/mcp-menu.md | 2 +- mcp-menu.sh | 39 ++++++++++++++++++++++++++++++++++- tests/test_mcp_menu_script.py | 13 ++++++++++++ 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/docs/mcp-menu.md b/docs/mcp-menu.md index 0b028f9..e028a40 100644 --- a/docs/mcp-menu.md +++ b/docs/mcp-menu.md @@ -41,7 +41,7 @@ The script must be executable (`chmod +x mcp-menu.sh`). It uses bash with |--------|-------------| | Project status / root checkout health | Shows cwd, branch, `git status --short --branch`, HEAD SHA, `prgs/master` SHA, and warnings when the root checkout is dirty or off `master`. | | Author workflow prompts | Ready-to-copy prompts for issue work, conflict-fix sessions, and root checkout recovery. | -| Reviewer workflow prompts | PR review prompt (review-only; no merge). | +| Reviewer workflow prompts | Standard PR review prompt, and a skip-already-reviewed-stale-`REQUEST_CHANGES` prompt that hands off to the author without a duplicate terminal mutation (review-only; no merge). | | Merger workflow prompts | PR merge prompt (merge gates and explicit approval). | | Reconciler workflow prompts | Already-landed / closed PR reconciliation prompt. | | Onboarding new project | Checklist prompt for adding a repository to the MCP workflow. | diff --git a/mcp-menu.sh b/mcp-menu.sh index 4d124f2..215f017 100755 --- a/mcp-menu.sh +++ b/mcp-menu.sh @@ -115,7 +115,15 @@ Workflow: } show_reviewer_prompts() { - print_prompt_block "Reviewer — PR review" \ + while true; do + printf '\n--- Reviewer workflow prompts ---\n' + printf ' 1) Standard PR review\n' + printf ' 2) Skip already-reviewed stale REQUEST_CHANGES PR and hand off to author\n' + printf ' 0) Back\n' + read -r -p 'Choice: ' choice + case "$choice" in + 1) + print_prompt_block "Reviewer — PR review" \ "You are the REVIEWER session for /. Goal: review PR #

only — do not merge unless explicitly switched to merger mode. @@ -126,6 +134,35 @@ Workflow: 3. gitea_view_pr, validate scope, run required checks in the correct worktree. 4. gitea_review_pr with approve, request-changes, or comment as warranted. 5. Final report: PR head SHA, verdict, validation evidence, mutation ledger. No merge in reviewer-only runs." + ;; + 2) + print_prompt_block "Reviewer — skip already-reviewed stale REQUEST_CHANGES PR and hand off to author" \ +"You are the REVIEWER session for /. + +Goal: review PR #

only far enough to determine whether the current head already has a non-stale REQUEST_CHANGES verdict. If it does, do NOT submit another terminal review mutation — produce an author handoff and stop. + +Workflow: +1. gitea_view_pr; pin the current head SHA. +2. Load prior reviews; find the latest REQUEST_CHANGES and the head SHA it was bound to. +3. If that REQUEST_CHANGES is still bound to the current head (non-stale), do not submit another terminal review mutation. Confirm the binding and summarize the blockers. +4. Run only the diagnostics needed to produce a useful author handoff — no full re-review. +5. Duplicate/supersession check: confirm no newer canonical PR or superseding head changes the decision. +6. Stop — no new review mutation. + +Return: +- selected PR and issue +- current head SHA +- prior REQUEST_CHANGES proof (review id + bound head SHA) +- duplicate/supersession analysis +- validation summary +- why no new review mutation was submitted +- corrected mutation ledger, including local worktree create/remove if used +- author-ready fix prompt" + ;; + 0) return ;; + *) printf 'Invalid choice.\n' ;; + esac + done } show_merger_prompts() { diff --git a/tests/test_mcp_menu_script.py b/tests/test_mcp_menu_script.py index a447648..b52ecad 100644 --- a/tests/test_mcp_menu_script.py +++ b/tests/test_mcp_menu_script.py @@ -105,6 +105,19 @@ class TestMcpMenuScript(unittest.TestCase): self.assertIn("./mcp-menu.sh", docs_text) self.assertIn("placeholder", docs_text.lower()) + def test_reviewer_skip_stale_request_changes_prompt_discoverable(self): + # #482: the skip-already-reviewed-stale-REQUEST_CHANGES reviewer prompt + # must be reachable from the reviewer menu and documented. + label = "Skip already-reviewed stale REQUEST_CHANGES PR and hand off to author" + reviewer_fn = self._extract_function("show_reviewer_prompts") + self.assertIn(label, reviewer_fn, "new reviewer prompt label must appear in the reviewer menu") + # The prompt must instruct: no duplicate terminal review mutation for a + # non-stale REQUEST_CHANGES head, and must carry the author handoff. + self.assertIn("do NOT submit another terminal review mutation", reviewer_fn) + self.assertIn("author-ready fix prompt", reviewer_fn) + docs_text = DOCS.read_text(encoding="utf-8") + self.assertIn("REQUEST_CHANGES", docs_text) + def _extract_function(self, name: str) -> str: marker = f"{name}() {{" start = self.content.index(marker) From d50328aea45369d9e26ba1fc2a992a6e1f634c68 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 03:14:32 -0400 Subject: [PATCH 3/7] feat: add canonical state handoff ledger (#494) Add templates, validators, and documentation for discussion/issue/PR state comments and final-report next-action handoff fields. Wire enforcement into assess_final_report_validator across all workflow task kinds. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/llm-workflow-runbooks.md | 15 ++ docs/state-handoff-ledger.md | 86 ++++++ final_report_validator.py | 40 +++ state_handoff_ledger.py | 374 +++++++++++++++++++++++++++ tests/test_final_report_validator.py | 6 + tests/test_state_handoff_ledger.py | 243 +++++++++++++++++ 6 files changed, 764 insertions(+) create mode 100644 docs/state-handoff-ledger.md create mode 100644 state_handoff_ledger.py create mode 100644 tests/test_state_handoff_ledger.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index e0ff616..f71c825 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -830,6 +830,21 @@ 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. +- [`state-handoff-ledger.md`](state-handoff-ledger.md) — canonical state comments and next-action handoff (#494). + +## Canonical state handoff ledger (#494) + +Gitea comments and final reports must make continuation obvious without chat +history. See [`state-handoff-ledger.md`](state-handoff-ledger.md) for: + +- discussion → issue → PR → review → merge → reconcile lifecycle +- templates for discussion, issue, PR, and queue-controller state comments +- final-report requirements: Current status, Next actor, Next action, Next prompt +- queue-controller priority order and discussion ≥5-comment rule (urgent/trivial + exceptions) + +Helpers live in `state_handoff_ledger.py`; final-report enforcement is wired +through `assess_final_report_validator`. ## PR-only queue cleanup mode (#390) diff --git a/docs/state-handoff-ledger.md b/docs/state-handoff-ledger.md new file mode 100644 index 0000000..fa0ce76 --- /dev/null +++ b/docs/state-handoff-ledger.md @@ -0,0 +1,86 @@ +# Canonical state handoff ledger (#494) + +Gitea is the system of record for continuation. Every discussion, issue, and PR +should answer: current state, last proof, who acts next, and the exact prompt for +the next role. + +## Lifecycle + +1. Controller opens a discussion. +2. Discussion accumulates substantive comments (default minimum: five). +3. Controller posts a discussion summary when ready. +4. Controller creates linked issues from the summary. +5. Author locks issue, implements, opens PR. +6. Reviewer reviews at current head. +7. Merger merges after formal approval. +8. Reconciler closes superseded or already-landed PRs. +9. Issue/PR/discussion state comments make the next action obvious at every step. + +## Discussion rules + +- Do **not** convert a discussion into issues until it has at least **five + substantive comments**, unless the discussion state comment marks + `URGENCY: urgent` or `URGENCY: trivial`. +- Substantive comment types: proposal, risk/concern, acceptance criteria, + implementation approach, dependency/sequence, summary. +- Before issue creation, post a **discussion summary** with decision, issues to + create, non-goals, unresolved questions, and next prompt. +- Created issues must link back to the discussion; the discussion must link + forward to created issues. + +## State comment templates + +Use `state_handoff_ledger.py` helpers or copy the canonical blocks: + +- `render_discussion_state_comment(...)` +- `render_discussion_summary_comment(...)` +- `render_issue_state_comment(...)` +- `render_pr_state_comment(...)` +- `render_queue_controller_report(...)` + +Post state comments as the **latest canonical update** on the object. Do not +bury state inside PR bodies only. + +## Final report requirements + +Every final report must include a `Controller Handoff` section with: + +- **Current status** — live state after this session +- **Next actor** — `author`, `reviewer`, `merger`, `reconciler`, or `controller` +- **Next action** — one imperative step +- **Next prompt** — ready-to-paste prompt for the next role + +`assess_final_report_next_action_handoff` and +`assess_contradictory_state_handoff` enforce these fields and reject +contradictory claims (for example, ready-to-merge without approval, issue done +without PR proof, discussion complete without summary). + +## Queue controller selection (priority order) + +1. Merge clean approved PRs at current head. +2. Review PRs needing review. +3. Reconcile superseded or already-landed duplicates. +4. Continue blocked-but-now-unblocked issues. +5. Create issues from completed discussions. +6. Start new author work only when higher-priority queue items are clear. + +For each candidate object, read the **latest canonical state comment** before +choosing an action. Output the exact next-role prompt in the controller report. + +## Example workflow states + +| State | Next actor | Typical next action | +|-------|------------|---------------------| +| Discussion needs more comments | controller | Facilitate discussion until five substantive comments or urgent/trivial exception | +| Issue ready for author | author | Lock issue and implement in `branches/` worktree | +| PR needs review | reviewer | Review at pinned head in reviewer worktree | +| PR approved at head | merger | Merge with merger profile after eligibility proof | +| PR superseded | reconciler | Close duplicate/already-landed PR with proof | +| Issue blocked | controller | Post issue state comment with blockers and next prompt | + +## Non-goals + +- Chat history is not the source of truth. +- Do not weaken author/reviewer/merger/reconciler separation. +- Do not replace Gitea issues, PRs, or canonical workflow files under + `skills/llm-project-workflow/`. \ No newline at end of file diff --git a/final_report_validator.py b/final_report_validator.py index 352a3ff..b2f4c59 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -21,6 +21,7 @@ from review_proofs import ( assess_review_mutation_final_report, assess_validation_report, ) +from state_handoff_ledger import assess_state_handoff_ledger_report from validation_status_vocabulary import assess_validation_status_vocabulary FINAL_REPORT_TASK_KINDS = frozenset({ @@ -316,6 +317,38 @@ def _rule_shared_controller_handoff( ) +def _rule_shared_state_handoff_next_action(report_text: str) -> list[dict[str, str]]: + result = assess_state_handoff_ledger_report(report_text) + if result.get("complete"): + return [] + findings: list[dict[str, str]] = [] + next_action = result.get("next_action") or {} + contradictions = result.get("contradictions") or {} + if next_action.get("block"): + findings.extend( + _findings_from_reasons( + "shared.state_handoff_next_action", + next_action.get("reasons") or [], + field="Next action handoff", + severity="block", + safe_next_action=next_action.get("safe_next_action") + or "add next-action handoff fields to Controller Handoff", + ) + ) + if contradictions.get("block"): + findings.extend( + _findings_from_reasons( + "shared.state_handoff_contradiction", + contradictions.get("reasons") or [], + field="State handoff", + severity="block", + safe_next_action=contradictions.get("safe_next_action") + or "resolve contradictory state claims before submission", + ) + ) + return findings + + def _rule_shared_email_disclosure(report_text: str) -> list[dict[str, str]]: result = assess_email_disclosure(report_text) if result.get("proven"): @@ -1051,6 +1084,7 @@ _SHARED_ISSUE_LOCK_RULES = ( _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "review_pr": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_legacy_workspace_mutations, @@ -1075,6 +1109,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reconcile_stale_author_fields, @@ -1087,12 +1122,14 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { ], "author_issue": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_vague_mutations_none, ], "work_issue": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_vague_mutations_none, @@ -1100,17 +1137,20 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { ], "issue_filing": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, ], "inventory": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, _rule_reconcile_pagination_proof, ], "issue_selection": [ _rule_shared_controller_handoff, + _rule_shared_state_handoff_next_action, _rule_shared_email_disclosure, *_SHARED_ISSUE_LOCK_RULES, ], diff --git a/state_handoff_ledger.py b/state_handoff_ledger.py new file mode 100644 index 0000000..809043e --- /dev/null +++ b/state_handoff_ledger.py @@ -0,0 +1,374 @@ +"""Canonical state handoff ledger for discussions, issues, and PRs (#494).""" + +from __future__ import annotations + +import re +from typing import Any + +HANDOFF_HEADING = "Controller Handoff" + +ISSUE_STATE_FIELDS = ( + "STATE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "STATUS", + "RELATED_PRS", + "BRANCH", + "HEAD_SHA", + "VALIDATION", + "BLOCKERS", + "DUPLICATES_OR_SUPERSEDES", + "LAST_UPDATED_BY", +) + +PR_STATE_FIELDS = ( + "STATE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "ISSUE", + "BASE", + "HEAD", + "HEAD_SHA", + "REVIEW_STATUS", + "VALIDATION", + "BLOCKERS", + "SUPERSEDES", + "SUPERSEDED_BY", + "MERGE_READY", + "LAST_UPDATED_BY", +) + +DISCUSSION_STATE_FIELDS = ( + "STATE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "COMMENT_COUNT", + "SUBSTANTIVE_COMMENTS", + "URGENCY", + "BLOCKERS", + "LAST_UPDATED_BY", +) + +DISCUSSION_SUMMARY_FIELDS = ( + "DECISION", + "ISSUES_TO_CREATE", + "NON_GOALS", + "UNRESOLVED_QUESTIONS", + "NEXT_PROMPT", + "LAST_UPDATED_BY", +) + +QUEUE_CONTROLLER_FIELDS = ( + "QUEUE_STATE", + "SELECTED_OBJECT", + "SELECTED_OBJECT_TYPE", + "NEXT_ACTOR", + "NEXT_ACTION", + "NEXT_PROMPT", + "EVIDENCE", + "LAST_UPDATED_BY", +) + +FINAL_REPORT_NEXT_ACTION_FIELDS = ( + ("Current status", ("current status",)), + ("Next actor", ("next actor", "next_actor")), + ("Next action", ("next action", "next_action")), + ("Next prompt", ("next prompt", "next_prompt")), +) + +STATE_COMMENT_KINDS = frozenset({ + "issue_state", + "pr_state", + "discussion_state", + "discussion_summary", + "queue_controller", +}) + +_FIELDS_BY_KIND = { + "issue_state": ISSUE_STATE_FIELDS, + "pr_state": PR_STATE_FIELDS, + "discussion_state": DISCUSSION_STATE_FIELDS, + "discussion_summary": DISCUSSION_SUMMARY_FIELDS, + "queue_controller": QUEUE_CONTROLLER_FIELDS, +} + +_FIELD_LINE_RE = re.compile( + r"^([A-Z][A-Z0-9_]+)\s*:\s*(.*)$", + re.MULTILINE, +) +_HANDOFF_SECTION_RE = re.compile(r"^##\s*Controller Handoff\s*$", re.I | re.M) +_READY_TO_MERGE_RE = re.compile( + r"\b(?:ready[_ ]to[_ ]merge|merge[_ ]ready\s*:\s*(?:yes|true))\b", + re.IGNORECASE, +) +_APPROVAL_PROOF_RE = re.compile( + r"\b(?:review decision\s*:\s*approve|approved at|approval at current head|" + r"formal approval|review_status\s*:\s*approved)\b", + re.IGNORECASE, +) +_EXTERNAL_NONE_RE = re.compile( + r"^\s*[-*]?\s*external[- ]state mutations\s*:\s*none\s*$", + re.IGNORECASE | re.MULTILINE, +) +_LOCK_MUTATION_RE = re.compile( + r"\b(?:gitea_lock_issue|issue-locks/|lock file edited)\b", + re.IGNORECASE, +) +_ISSUE_DONE_RE = re.compile( + r"\b(?:issue (?:done|closed)\b|current status\s*:\s*(?:done|closed)\b|" + r"current status\s*:\s*issue complete\b)", + re.IGNORECASE, +) +_PR_MERGE_PROOF_RE = re.compile( + r"\b(?:pr (?:merged|number)|merge result\s*:\s*merged|pr mutations\s*:\s*created)\b", + re.IGNORECASE, +) +_DISCUSSION_COMPLETE_RE = re.compile( + r"\b(?:discussion complete|discussion ready for issue creation)\b", + re.IGNORECASE, +) +_DISCUSSION_SUMMARY_PROOF_RE = re.compile( + r"\b(?:discussion summary|issues to create|issues created|linked issues)\b", + re.IGNORECASE, +) + +MIN_DISCUSSION_COMMENTS_DEFAULT = 5 + + +def _render_fields(fields: tuple[str, ...], values: dict[str, Any]) -> str: + lines = ["```text"] + for name in fields: + raw = values.get(name, values.get(name.lower(), "none")) + text = str(raw).strip() if raw is not None else "none" + lines.append(f"{name}: {text or 'none'}") + lines.append("```") + return "\n".join(lines) + + +def render_issue_state_comment(**values: Any) -> str: + """Render canonical issue state comment body.""" + return _render_fields(ISSUE_STATE_FIELDS, values) + + +def render_pr_state_comment(**values: Any) -> str: + """Render canonical PR state comment body.""" + return _render_fields(PR_STATE_FIELDS, values) + + +def render_discussion_state_comment(**values: Any) -> str: + """Render canonical discussion state comment body.""" + return _render_fields(DISCUSSION_STATE_FIELDS, values) + + +def render_discussion_summary_comment(**values: Any) -> str: + """Render discussion-to-issue conversion summary comment.""" + return _render_fields(DISCUSSION_SUMMARY_FIELDS, values) + + +def render_queue_controller_report(**values: Any) -> str: + """Render queue-controller selection report.""" + return _render_fields(QUEUE_CONTROLLER_FIELDS, values) + + +def parse_state_comment(text: str) -> dict[str, str]: + """Parse KEY: value lines from a canonical state comment.""" + parsed: dict[str, str] = {} + for match in _FIELD_LINE_RE.finditer(text or ""): + parsed[match.group(1)] = match.group(2).strip() + return parsed + + +def _handoff_field_map(report_text: str) -> dict[str, str]: + section_match = _HANDOFF_SECTION_RE.search(report_text or "") + if not section_match: + return {} + section = (report_text or "")[section_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 assess_state_comment(text: str, *, kind: str) -> dict[str, Any]: + """Validate a canonical Gitea state comment for *kind*.""" + normalized = (kind or "").strip().lower() + if normalized not in STATE_COMMENT_KINDS: + return { + "complete": False, + "block": True, + "missing_fields": [], + "reasons": [f"unknown state comment kind '{kind}'"], + "safe_next_action": "use a supported state comment kind", + } + + required = _FIELDS_BY_KIND[normalized] + parsed = parse_state_comment(text) + missing = [name for name in required if name not in parsed or not parsed[name].strip()] + reasons = [f"state comment missing field: {name}" for name in missing] + + if normalized == "discussion_state": + urgency = parsed.get("URGENCY", "").strip().lower() + count_raw = parsed.get("COMMENT_COUNT", "").strip() + try: + count = int(count_raw) + except (TypeError, ValueError): + count = -1 + if ( + urgency not in {"urgent", "trivial"} + and count >= 0 + and count < MIN_DISCUSSION_COMMENTS_DEFAULT + ): + reasons.append( + f"discussion has {count} comments; need at least " + f"{MIN_DISCUSSION_COMMENTS_DEFAULT} substantive comments " + "or URGENCY: urgent|trivial" + ) + + block = bool(missing or reasons) + return { + "complete": not block, + "block": block, + "missing_fields": missing, + "parsed_fields": parsed, + "reasons": reasons, + "safe_next_action": ( + "fill all required state comment fields before posting" + if block + else "proceed" + ), + } + + +def assess_final_report_next_action_handoff(report_text: str) -> dict[str, Any]: + """Require current status plus next actor/action/prompt in Controller Handoff.""" + fields = _handoff_field_map(report_text) + if not fields: + return { + "complete": False, + "block": True, + "missing_fields": [name for name, _ in FINAL_REPORT_NEXT_ACTION_FIELDS], + "reasons": [f"final report has no section titled exactly '{HANDOFF_HEADING}'"], + "safe_next_action": f"add a complete {HANDOFF_HEADING} section", + } + + missing = [] + for name, aliases in FINAL_REPORT_NEXT_ACTION_FIELDS: + value = "" + for alias in aliases: + if alias in fields: + value = fields[alias] + break + if not value or value.lower() in {"none", "n/a", "not verified in this session", ""}: + missing.append(name) + + reasons = [f"handoff missing next-action field: {name}" for name in missing] + block = bool(missing) + return { + "complete": not block, + "block": block, + "missing_fields": missing, + "reasons": reasons, + "safe_next_action": ( + "add Current status, Next actor, Next action, and Next prompt to Controller Handoff" + if block + else "proceed" + ), + } + + +def assess_contradictory_state_handoff(report_text: str) -> dict[str, Any]: + """Fail closed on contradictory queue/state claims in final reports.""" + text = report_text or "" + fields = _handoff_field_map(text) + reasons: list[str] = [] + + review_decision = fields.get("review decision", fields.get("review status", "")) + has_approval = ( + review_decision.lower() in {"approve", "approved"} + or bool(_APPROVAL_PROOF_RE.search(text)) + ) + if _READY_TO_MERGE_RE.search(text) and not has_approval: + reasons.append( + "report claims ready to merge without approval-at-current-head proof" + ) + + lock_val = fields.get("external-state mutations", "") + mutation_lines = " ".join( + fields.get(key, "") + for key in ( + "issue mutations", + "mcp/gitea mutations", + "external-state mutations", + "claim/lock state", + ) + ) + if ( + (_EXTERNAL_NONE_RE.search(text) or lock_val.lower() == "none") + and _LOCK_MUTATION_RE.search(mutation_lines) + ): + reasons.append( + "report claims External-state mutations: none while lock " + "activity is documented in mutation fields" + ) + + if _ISSUE_DONE_RE.search(text) and not _PR_MERGE_PROOF_RE.search(text): + reasons.append( + "report claims issue done/complete without PR or merge proof" + ) + + if _DISCUSSION_COMPLETE_RE.search(text) and not _DISCUSSION_SUMMARY_PROOF_RE.search(text): + reasons.append( + "report claims discussion complete without summary or issue-creation proof" + ) + + review_status = fields.get("review status", fields.get("review decision", "")) + merge_ready = fields.get("merge ready", "") + if ( + merge_ready.lower() in {"yes", "true", "ready"} + and review_status.lower() not in {"approve", "approved"} + and not _APPROVAL_PROOF_RE.search(text) + ): + reasons.append( + "MERGE_READY or merge-ready claim without approved review status" + ) + + block = bool(reasons) + return { + "complete": not block, + "block": block, + "reasons": reasons, + "safe_next_action": ( + "correct contradictory state claims or add missing proof" + if block + else "proceed" + ), + } + + +def assess_state_handoff_ledger_report(report_text: str) -> dict[str, Any]: + """Composite #494 assessment for final reports.""" + next_action = assess_final_report_next_action_handoff(report_text) + contradictions = assess_contradictory_state_handoff(report_text) + reasons = list(next_action.get("reasons") or []) + list(contradictions.get("reasons") or []) + block = bool(next_action.get("block") or contradictions.get("block")) + return { + "complete": not block, + "block": block, + "next_action": next_action, + "contradictions": contradictions, + "reasons": reasons, + "safe_next_action": ( + next_action.get("safe_next_action") + if next_action.get("block") + else contradictions.get("safe_next_action") + if contradictions.get("block") + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 88c5b23..0de0a62 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -35,6 +35,9 @@ def _review_handoff(**overrides): "- External-state mutations: none", "- Read-only diagnostics: issue and PR metadata inspected", "- Current status: PR open", + "- Next actor: author", + "- Next action: address review feedback", + "- Next prompt: Fix PR #203 per reviewer request_changes and push updated head", "- Blockers: none", "- Next: await author", "- Safety: no self-review; no self-merge", @@ -92,6 +95,9 @@ def _reconcile_handoff(drop=(), **overrides): "- Read-only diagnostics: gitea_view_pr, gitea_view_issue", "- Reconciliation mutations: PR comment posted", "- Current status: reconciliation complete", + "- Next actor: reconciler", + "- Next action: close PR #99 after capability proof", + "- Next prompt: Reconcile already-landed PR #99 with prgs-reconciler profile", "- Blocker: missing gitea.pr.close capability", "- Safe next action: hand off to a profile with gitea.pr.close", "- No review/merge confirmation: confirmed", diff --git a/tests/test_state_handoff_ledger.py b/tests/test_state_handoff_ledger.py new file mode 100644 index 0000000..08f5c5f --- /dev/null +++ b/tests/test_state_handoff_ledger.py @@ -0,0 +1,243 @@ +"""Tests for canonical state handoff ledger (#494).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from final_report_validator import assess_final_report_validator # noqa: E402 +from state_handoff_ledger import ( # noqa: E402 + ISSUE_STATE_FIELDS, + assess_contradictory_state_handoff, + assess_final_report_next_action_handoff, + assess_state_comment, + assess_state_handoff_ledger_report, + parse_state_comment, + render_issue_state_comment, + render_pr_state_comment, + render_queue_controller_report, +) + + +def _work_issue_handoff(**overrides): + lines = [ + "## Controller Handoff", + "", + "- Task: work-issue", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: author", + "- Identity: jcwalker3 / prgs-author", + "- Selected issue: #494", + "- Current status: implementation complete; PR opened awaiting review", + "- Next actor: reviewer", + "- Next action: review PR at pinned head", + "- Next prompt: Review PR #500 for issue #494 at current head in reviewer worktree", + "- Safe next action: switch to reviewer session", + "- External-state mutations: none", + ] + text = "\n".join(lines) + for key, value in overrides.items(): + needle = f"- {key}:" + if needle in text: + prefix = text.split(needle)[0] + rest = text.split(needle, 1)[1] + suffix = rest.split("\n", 1)[1] if "\n" in rest else "" + text = f"{prefix}{needle} {value}\n{suffix}".rstrip() + else: + text += f"\n- {key}: {value}" + return text + + +class TestStateCommentTemplates(unittest.TestCase): + def test_render_issue_state_includes_all_fields(self): + body = render_issue_state_comment( + STATE="issue_ready_for_author", + NEXT_ACTOR="author", + NEXT_ACTION="lock and implement", + NEXT_PROMPT="Find issue #494 and open a PR", + LAST_UPDATED_BY="jcwalker3", + ) + parsed = parse_state_comment(body) + for field in ISSUE_STATE_FIELDS: + self.assertIn(field, parsed) + + def test_render_pr_state_parses(self): + body = render_pr_state_comment( + STATE="needs_review", + NEXT_ACTOR="reviewer", + NEXT_ACTION="review at head", + NEXT_PROMPT="Review PR #500", + ISSUE="#494", + HEAD_SHA="a" * 40, + LAST_UPDATED_BY="sysadmin", + ) + parsed = parse_state_comment(body) + self.assertEqual(parsed["STATE"], "needs_review") + self.assertEqual(parsed["NEXT_ACTOR"], "reviewer") + + def test_queue_controller_template(self): + body = render_queue_controller_report( + QUEUE_STATE="open_prs_need_review", + SELECTED_OBJECT="PR #500", + SELECTED_OBJECT_TYPE="pr", + NEXT_ACTOR="reviewer", + NEXT_ACTION="start review session", + NEXT_PROMPT="Review PR #500", + LAST_UPDATED_BY="controller", + ) + result = assess_state_comment(body, kind="queue_controller") + self.assertTrue(result["complete"]) + + +class TestStateCommentValidation(unittest.TestCase): + def test_complete_issue_state_comment(self): + body = render_issue_state_comment( + STATE="in_progress", + NEXT_ACTOR="author", + NEXT_ACTION="push branch", + NEXT_PROMPT="Continue issue #494", + STATUS="open", + RELATED_PRS="none", + BRANCH="feat/issue-494-state-handoff-ledger", + HEAD_SHA="b" * 40, + VALIDATION="pytest passed", + BLOCKERS="none", + DUPLICATES_OR_SUPERSEDES="none", + LAST_UPDATED_BY="jcwalker3", + ) + result = assess_state_comment(body, kind="issue_state") + self.assertTrue(result["complete"]) + self.assertFalse(result["block"]) + + def test_missing_fields_blocked(self): + body = "STATE: open\nNEXT_ACTOR: author" + result = assess_state_comment(body, kind="issue_state") + self.assertFalse(result["complete"]) + self.assertIn("NEXT_ACTION", result["missing_fields"]) + + def test_discussion_requires_five_comments_by_default(self): + body = render_issue_state_comment( + STATE="collecting_feedback", + NEXT_ACTOR="controller", + NEXT_ACTION="facilitate discussion", + NEXT_PROMPT="Add acceptance criteria", + STATUS="open", + RELATED_PRS="none", + BRANCH="none", + HEAD_SHA="none", + VALIDATION="none", + BLOCKERS="none", + DUPLICATES_OR_SUPERSEDES="none", + LAST_UPDATED_BY="controller", + ) + # Re-map to discussion fields manually for comment-count test + from state_handoff_ledger import render_discussion_state_comment + + disc = render_discussion_state_comment( + STATE="collecting_feedback", + NEXT_ACTOR="controller", + NEXT_ACTION="wait for comments", + NEXT_PROMPT="Continue discussion", + COMMENT_COUNT="2", + SUBSTANTIVE_COMMENTS="yes", + URGENCY="normal", + BLOCKERS="none", + LAST_UPDATED_BY="controller", + ) + result = assess_state_comment(disc, kind="discussion_state") + self.assertFalse(result["complete"]) + self.assertTrue(any("5" in reason for reason in result["reasons"])) + + def test_discussion_urgent_exception(self): + from state_handoff_ledger import render_discussion_state_comment + + disc = render_discussion_state_comment( + STATE="ready_for_summary", + NEXT_ACTOR="controller", + NEXT_ACTION="summarize", + NEXT_PROMPT="Post summary", + COMMENT_COUNT="2", + SUBSTANTIVE_COMMENTS="yes", + URGENCY="urgent", + BLOCKERS="none", + LAST_UPDATED_BY="controller", + ) + result = assess_state_comment(disc, kind="discussion_state") + self.assertTrue(result["complete"]) + + +class TestFinalReportNextAction(unittest.TestCase): + def test_complete_next_action_handoff(self): + report = _work_issue_handoff() + result = assess_final_report_next_action_handoff(report) + self.assertTrue(result["complete"]) + self.assertFalse(result["block"]) + + def test_missing_next_prompt_blocked(self): + report = _work_issue_handoff(**{"Next prompt": "none"}) + result = assess_final_report_next_action_handoff(report) + self.assertFalse(result["complete"]) + self.assertIn("Next prompt", result["missing_fields"]) + + def test_missing_handoff_section_blocked(self): + result = assess_final_report_next_action_handoff("no handoff here") + self.assertTrue(result["block"]) + + +class TestContradictoryState(unittest.TestCase): + def test_ready_to_merge_without_approval_blocked(self): + report = _work_issue_handoff( + **{ + "Current status": "PR ready to merge", + "Review decision": "not attempted", + } + ) + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + def test_ready_to_merge_with_approval_allowed(self): + report = _work_issue_handoff( + **{ + "Current status": "PR ready to merge", + "Review decision": "approve", + } + ) + result = assess_contradictory_state_handoff(report) + self.assertFalse(result["block"]) + + def test_issue_done_without_pr_proof_blocked(self): + report = _work_issue_handoff(**{"Current status": "issue complete"}) + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + def test_external_none_with_lock_activity_blocked(self): + report = _work_issue_handoff( + **{ + "External-state mutations": "none", + } + ) + report += "\n- Issue mutations: gitea_lock_issue adopted branch" + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + def test_discussion_complete_without_summary_blocked(self): + report = _work_issue_handoff(**{"Current status": "discussion complete"}) + result = assess_contradictory_state_handoff(report) + self.assertTrue(result["block"]) + + +class TestFinalReportValidatorIntegration(unittest.TestCase): + def test_work_issue_validator_flags_missing_next_prompt(self): + report = _work_issue_handoff(**{"Next prompt": ""}) + result = assess_final_report_validator(report, "work_issue") + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("shared.state_handoff_next_action", rule_ids) + + def test_work_issue_validator_accepts_complete_handoff(self): + report = _work_issue_handoff() + result = assess_state_handoff_ledger_report(report) + self.assertTrue(result["complete"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 7fb813446fe0389d73ce306199887002d455c1a4 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 23:04:44 -0400 Subject: [PATCH 4/7] feat: require true pre-merge baseline proof for non-zero validation exits (Closes #533) Add premerge_baseline_proof.py distinguishing four validation outcomes (clean pass / current-master failure reproduced / pre-merge baseline-proven failure / unresolved regression risk). A non-zero exit may only be labeled "baseline"/"pre-existing" with pre-merge proof: the failure reproduced on the PR's pre-merge base commit, or a documented known-failure record predating the PR. Current-master (post-merge) reproduction is explicitly rejected as sufficient baseline proof. - premerge_baseline_proof.py: assess_premerge_baseline_proof + labels - final_report_validator.py: reviewer.premerge_baseline_proof rule on review_pr and reconcile_already_landed tasks - skills/llm-project-workflow/templates/review-pr.md: baseline proof section requiring base commit, tested commit, command, exit status, failure signature - tests/test_premerge_baseline_proof.py: reject current-master-only proof, accept pre-merge base proof and documented known-failure records Co-Authored-By: Claude Opus 4.8 (1M context) --- final_report_validator.py | 21 +++ premerge_baseline_proof.py | 176 ++++++++++++++++++ .../templates/review-pr.md | 10 + tests/test_premerge_baseline_proof.py | 112 +++++++++++ 4 files changed, 319 insertions(+) create mode 100644 premerge_baseline_proof.py create mode 100644 tests/test_premerge_baseline_proof.py diff --git a/final_report_validator.py b/final_report_validator.py index a807ff9..f7cb1c7 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -715,6 +715,25 @@ def _rule_reviewer_main_checkout_baseline(report_text: str) -> list[dict[str, st ] +def _rule_reviewer_premerge_baseline_proof(report_text: str) -> list[dict[str, str]]: + from premerge_baseline_proof import assess_premerge_baseline_proof + + result = assess_premerge_baseline_proof(report_text) + if not result.get("block"): + return [] + return _findings_from_reasons( + "reviewer.premerge_baseline_proof", + result.get("reasons") or [], + field="Pre-merge baseline proof", + severity="block", + safe_next_action=result.get("safe_next_action") + or ( + "prove the failure on the PR pre-merge base commit or a known-failure " + "record predating the PR; current-master reproduction is not baseline proof" + ), + ) + + def _rule_reviewer_validation_status_vocabulary( report_text: str, *, @@ -1294,6 +1313,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_validation_structured, _rule_reviewer_linked_issue, _rule_reviewer_baseline_on_failure, + _rule_reviewer_premerge_baseline_proof, _rule_reviewer_validation_status_vocabulary, _rule_reviewer_main_checkout_baseline, _rule_reviewer_main_checkout_path, @@ -1318,6 +1338,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reconcile_linked_issue_live, _rule_reconcile_close_proof, _rule_reconcile_pagination_proof, + _rule_reviewer_premerge_baseline_proof, _rule_reviewer_git_fetch_readonly, _rule_reviewer_legacy_workspace_mutations, _rule_reviewer_vague_mutations_none, diff --git a/premerge_baseline_proof.py b/premerge_baseline_proof.py new file mode 100644 index 0000000..cf9b971 --- /dev/null +++ b/premerge_baseline_proof.py @@ -0,0 +1,176 @@ +"""Pre-merge baseline proof verifier for non-zero validation exits (#533). + +A controller/reviewer may reproduce a failing test on *current* master and +describe it as proof of a "known baseline failure". Reproducing a failure on +post-merge master only proves the failure exists on current master — it does +NOT prove the failure pre-existed the PR under review. A PR can introduce or +preserve a regression, then once merged the failure appears on master and gets +mislabeled "baseline", weakening validation gates. + +This module distinguishes four canonical validation outcomes: + +- ``CLEAN_PASS`` — the validation command exited zero. +- ``CURRENT_MASTER_FAILURE_REPRODUCED`` — the same failure reproduces on + current (post-merge) master. Not sufficient as baseline proof. +- ``PREMERGE_BASELINE_PROVEN_FAILURE`` — the failure is proven on the PR's + pre-merge base commit, or by a documented known-failure record that predates + the PR. +- ``UNRESOLVED_REGRESSION_RISK`` — a non-zero exit that is neither a clean pass + nor proven pre-existing. + +A report may only call a non-zero validation exit a "baseline"/"pre-existing" +failure when it carries pre-merge proof. +""" + +from __future__ import annotations + +import re +from typing import Any + +CLEAN_PASS = "clean pass" +CURRENT_MASTER_FAILURE_REPRODUCED = "current-master failure reproduced" +PREMERGE_BASELINE_PROVEN_FAILURE = "pre-merge baseline-proven failure" +UNRESOLVED_REGRESSION_RISK = "unresolved regression risk" + +# A non-zero validation exit is claimed somewhere in the report. +_NONZERO_EXIT_RE = re.compile( + r"(?:exit(?:\s*(?:status|code))?\s*[:=]?\s*(?:[1-9]\d*)" + r"|\b\d+\s+failed\b" + r"|\bnon[- ]zero\s+(?:exit|validation))", + re.IGNORECASE, +) +# The report claims the failure is pre-existing / a baseline failure. +_BASELINE_CLAIM_RE = re.compile( + r"(?:pre[- ]?existing(?:\s+(?:baseline|failure))?" + r"|baseline(?:[- ]proven)?\s+failure" + r"|known[- ]baseline" + r"|failure\s+is\s+baseline" + r"|already\s+fail(?:s|ing)\s+on\s+master)", + re.IGNORECASE, +) +# Only-current-master reproduction language (insufficient on its own). +_CURRENT_MASTER_RE = re.compile( + r"(?:current[- ]master\s+(?:reproduc|failure)" + r"|reproduc\w*\s+on\s+(?:current\s+)?master" + r"|post[- ]merge\s+master" + r"|fails\s+on\s+(?:current\s+)?master\s+(?:now|too|as\s+well))", + re.IGNORECASE, +) +# Pre-merge base proof: a base commit tested before the PR landed. +_PREMERGE_BASE_RE = re.compile( + r"(?:pre[- ]merge\s+base(?:\s+commit)?" + r"|pr\s+base\s+commit" + r"|merge[- ]base(?:\s+commit)?" + r"|base\s+commit\s+before\s+(?:the\s+)?pr)", + re.IGNORECASE, +) +# Documented known-failure record predating the PR. +_KNOWN_FAILURE_RECORD_RE = re.compile( + r"known[- ]failure\s+(?:record|reference|ledger)\b.{0,80}?" + r"(?:predat|before\s+(?:the\s+)?pr|pre[- ]existing|prior\s+to\s+(?:the\s+)?pr)", + re.IGNORECASE | re.DOTALL, +) + +# Required proof fields for a pre-merge baseline claim. +_FIELD_PATTERNS: dict[str, re.Pattern[str]] = { + "base commit": re.compile( + r"(?:pre[- ]merge\s+)?base\s+commit\s*[:=]\s*([0-9a-f]{7,40})", re.IGNORECASE + ), + "tested commit": re.compile( + r"tested\s+commit\s*[:=]\s*([0-9a-f]{7,40})", re.IGNORECASE + ), + "command": re.compile(r"command\s*[:=]\s*\S", re.IGNORECASE), + "exit status": re.compile(r"exit\s*(?:status|code)?\s*[:=]\s*\d+", re.IGNORECASE), + "failure signature": re.compile( + r"failure\s+signature\s*[:=]\s*\S", re.IGNORECASE + ), +} + + +def assess_premerge_baseline_proof(report_text: str) -> dict[str, Any]: + """Assess whether a claimed baseline failure carries pre-merge proof. + + Returns a dict with ``proven``, ``block``, ``label``, ``reasons``, + ``skipped`` and ``safe_next_action``. Only blocks when the report claims a + non-zero validation exit is baseline/pre-existing without valid pre-merge + proof. + """ + text = report_text or "" + + has_failure = bool(_NONZERO_EXIT_RE.search(text)) + claims_baseline = bool(_BASELINE_CLAIM_RE.search(text)) + + # No failure claimed, or no baseline assertion — nothing to enforce here. + if not has_failure and not claims_baseline: + return { + "proven": True, + "block": False, + "label": CLEAN_PASS, + "reasons": [], + "skipped": True, + "safe_next_action": "", + } + + if not claims_baseline: + # A non-zero exit not asserted as baseline — surface as regression risk, + # but do not block (the report never claimed a clean/baseline pass). + return { + "proven": True, + "block": False, + "label": UNRESOLVED_REGRESSION_RISK, + "reasons": [], + "skipped": False, + "safe_next_action": "", + } + + has_premerge_base = bool(_PREMERGE_BASE_RE.search(text)) + has_known_record = bool(_KNOWN_FAILURE_RECORD_RE.search(text)) + only_current_master = bool(_CURRENT_MASTER_RE.search(text)) + + reasons: list[str] = [] + + if not (has_premerge_base or has_known_record): + if only_current_master: + reasons.append( + "baseline failure claimed from current-master reproduction only; " + "that proves the failure on current master, not that it pre-existed " + "the PR — provide pre-merge base proof or a known-failure record" + ) + else: + reasons.append( + "baseline/pre-existing failure claimed without pre-merge proof " + "(no pre-merge base commit and no documented known-failure record " + "predating the PR)" + ) + + # Require the concrete proof fields regardless of proof source. + missing = [name for name, pat in _FIELD_PATTERNS.items() if not pat.search(text)] + if missing: + reasons.append( + "pre-merge baseline claim missing required proof field(s): " + + ", ".join(missing) + ) + + if reasons: + return { + "proven": False, + "block": True, + "label": UNRESOLVED_REGRESSION_RISK, + "reasons": reasons, + "skipped": False, + "safe_next_action": ( + "prove the failure on the PR pre-merge base commit (or cite a " + "known-failure record predating the PR) and state base commit, " + "tested commit, command, exit status, and failure signature; " + "current-master reproduction alone is not baseline proof" + ), + } + + return { + "proven": True, + "block": False, + "label": PREMERGE_BASELINE_PROVEN_FAILURE, + "reasons": [], + "skipped": False, + "safe_next_action": "", + } diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index a959634..bc5059a 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -126,4 +126,14 @@ including the review/merge role fields: Selected PR, Reviewer eligibility, Pinned reviewed head, Review decision, Merge result, Linked issue status, Cleanup status. If you could not merge, name the exact gate. Reports missing the handoff are downgraded (review_proofs.assess_controller_handoff). + +Baseline failure proof (#533): if a validation command exits non-zero, do NOT +call it a clean pass. Only label a failure "baseline"/"pre-existing" with +pre-merge proof — the failure reproduced on the PR's pre-merge base commit, or a +documented known-failure record predating the PR. Reproducing on current +(post-merge) master is "current-master failure reproduced", NOT baseline proof. +State: base commit, tested commit, command, exit status, failure signature. +Use one label: clean pass / current-master failure reproduced / pre-merge +baseline-proven failure / unresolved regression risk +(final_report_validator: reviewer.premerge_baseline_proof). ``` diff --git a/tests/test_premerge_baseline_proof.py b/tests/test_premerge_baseline_proof.py new file mode 100644 index 0000000..865f093 --- /dev/null +++ b/tests/test_premerge_baseline_proof.py @@ -0,0 +1,112 @@ +import unittest + +from premerge_baseline_proof import ( + CLEAN_PASS, + PREMERGE_BASELINE_PROVEN_FAILURE, + UNRESOLVED_REGRESSION_RISK, + assess_premerge_baseline_proof, +) +from final_report_validator import assess_final_report_validator + +_FIELDS = ( + "Pre-merge base commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Tested commit: 1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b\n" + "Command: venv/bin/python -m pytest tests/test_foo.py -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" +) + +# Current-master-only reproduction carries no pre-merge base proof (that is the +# whole defect #533 guards against): a command/exit/signature but no base commit. +_CURRENT_ONLY_FIELDS = ( + "Command: venv/bin/python -m pytest tests/test_foo.py -q\n" + "Exit status: 1\n" + "Failure signature: AssertionError: None is not true\n" +) + + +class TestPremergeBaselineProof(unittest.TestCase): + def test_clean_pass_not_blocked(self): + result = assess_premerge_baseline_proof( + "Validation: full suite passed, exit status 0. Clean pass." + ) + self.assertFalse(result["block"]) + self.assertTrue(result["skipped"]) + self.assertEqual(result["label"], CLEAN_PASS) + + def test_nonzero_exit_not_claimed_baseline_not_blocked(self): + result = assess_premerge_baseline_proof( + "Full suite: 1 failed. Investigating the regression; not yet classified." + ) + self.assertFalse(result["block"]) + self.assertEqual(result["label"], UNRESOLVED_REGRESSION_RISK) + + def test_current_master_only_reproduction_rejected(self): + report = ( + "Full suite: 1 failed. This is a pre-existing baseline failure — " + "reproduced on current master after the PR merged.\n" + _CURRENT_ONLY_FIELDS + ) + result = assess_premerge_baseline_proof(report) + self.assertTrue(result["block"]) + self.assertTrue( + any("current-master reproduction only" in r for r in result["reasons"]) + ) + + def test_baseline_claim_missing_fields_blocked(self): + report = ( + "Full suite: 1 failed. This failure is baseline / pre-existing. " + "Verified against the pre-merge base commit." + ) + result = assess_premerge_baseline_proof(report) + self.assertTrue(result["block"]) + self.assertTrue( + any("missing required proof field" in r for r in result["reasons"]) + ) + + def test_premerge_base_proof_accepted(self): + report = ( + "Full suite: 1 failed. This is a pre-existing baseline failure, proven " + "on the PR pre-merge base commit.\n" + _FIELDS + ) + result = assess_premerge_baseline_proof(report) + self.assertFalse(result["block"]) + self.assertTrue(result["proven"]) + self.assertEqual(result["label"], PREMERGE_BASELINE_PROVEN_FAILURE) + + def test_documented_known_failure_record_accepted(self): + report = ( + "Full suite: 1 failed. Baseline failure: cited known-failure record #529 " + "which predates the PR.\n" + _FIELDS + ) + result = assess_premerge_baseline_proof(report) + self.assertFalse(result["block"]) + self.assertEqual(result["label"], PREMERGE_BASELINE_PROVEN_FAILURE) + + +class TestPremergeBaselineProofValidatorIntegration(unittest.TestCase): + def _has_rule(self, report): + result = assess_final_report_validator(report, "review_pr") + return any( + f["rule_id"] == "reviewer.premerge_baseline_proof" + for f in result["findings"] + ) + + def test_review_report_current_master_only_flagged(self): + report = ( + "## Reviewer final report\n" + "Full suite: 1 failed. Pre-existing baseline failure — reproduced on " + "post-merge master.\n" + _CURRENT_ONLY_FIELDS + ) + self.assertTrue(self._has_rule(report)) + + def test_review_report_premerge_proof_clean(self): + report = ( + "## Reviewer final report\n" + "Full suite: 1 failed. Pre-existing baseline failure proven on the " + "pre-merge base commit.\n" + _FIELDS + ) + self.assertFalse(self._has_rule(report)) + + +if __name__ == "__main__": + unittest.main() From a2b7327447839ad2a0d5d3b1d289a235564ea8ac Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 23:08:55 -0400 Subject: [PATCH 5/7] feat: check approvals or issues next if the PR queue is empty (Closes #572) --- skills/llm-project-workflow/templates/pr-queue-cleanup.md | 2 +- skills/llm-project-workflow/templates/review-pr.md | 3 ++- skills/llm-project-workflow/workflows/pr-queue-cleanup.md | 4 ++++ skills/llm-project-workflow/workflows/review-merge-pr.md | 4 ++++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/skills/llm-project-workflow/templates/pr-queue-cleanup.md b/skills/llm-project-workflow/templates/pr-queue-cleanup.md index d3913f1..1031a2a 100644 --- a/skills/llm-project-workflow/templates/pr-queue-cleanup.md +++ b/skills/llm-project-workflow/templates/pr-queue-cleanup.md @@ -16,7 +16,7 @@ Rules: 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. +- Report Next suggested PR without continuing to it. If the PR queue is empty, look at approvals, or issues next. Operator PR list (optional): Merge authorized for selected PR in this run: diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index a959634..3bff67d 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -28,7 +28,8 @@ Repo name disambiguation (Gitea-Tools blind review hardening): `pr_inventory_trust_gate.status`, trust-gate reasons, corroboration, remote/owner/repo/state filter, and the inventory MCP profile. A recent merge commit is not valid corroboration. Author-bound sessions must not present - reviewer queue inventory as a reviewer decision. + reviewer queue inventory as a reviewer decision. If the PR queue is empty, + look at approvals, or issues next. Load the canonical workflow first: `skills/llm-project-workflow/workflows/review-merge-pr.md` (task mode: review-merge-pr). diff --git a/skills/llm-project-workflow/workflows/pr-queue-cleanup.md b/skills/llm-project-workflow/workflows/pr-queue-cleanup.md index 8a1be98..f43b968 100644 --- a/skills/llm-project-workflow/workflows/pr-queue-cleanup.md +++ b/skills/llm-project-workflow/workflows/pr-queue-cleanup.md @@ -55,6 +55,10 @@ claim. Select exactly one PR according to project queue ordering rules PRs only with live per-PR proof. No multi-PR validation and no batch report may substitute for per-PR proof. +If the open PR queue is empty: +* First, look at **Approvals** next: check if there are open PRs with pending/completed approvals requiring attention or merge. +* Next, look at **Issues** next: check if there are unresolved open issues requiring action/fixes. + ## 4. Terminal mutation chain `pr_queue_cleanup.resolve_cleanup_run_state` is the authority: diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 5a90ea5..95cee0e 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -272,6 +272,10 @@ If queue ordering cannot be proven, stop and produce a recovery handoff. ## 10. Select the next actionable PR using project rules +If the open PR queue is empty: +* First, look at approvals next to see if there are pending approvals or approved PRs that need attention/merge. +* Next, look at issues next to see if there are open issues requiring action/fixes. + Do not review your own PR. Do not review stale, draft, blocked, duplicate, already-owned, dependency-blocked, already-landed, already-requested-changes work unless the rules explicitly allow it. From e4f1a52a8771493358f5d1f7b8c48024e473be75 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 23:12:57 -0400 Subject: [PATCH 6/7] feat: authorized cleanup for reviewer scratch worktrees (Closes #534) Discover branches/review-pr[-submit] worktrees separately from author cleanup, fail closed on dirty trees / open PRs / active reviewer leases, and remove only assessed-safe scratch paths via reconcile. --- gitea_mcp_server.py | 38 ++++ merged_cleanup_reconcile.py | 245 +++++++++++++++++++++++-- tests/test_merged_cleanup_reconcile.py | 201 ++++++++++++++++++++ 3 files changed, 473 insertions(+), 11 deletions(-) diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed650b5..078db67 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -4737,6 +4737,34 @@ def gitea_reconcile_merged_cleanups( ) delete_capability_allowed = not _profile_operation_gate("gitea.branch.delete") + + # #534: discover reviewer scratch trees and active leases for those PRs. + scratch_candidates = merged_cleanup_reconcile.discover_reviewer_scratch_worktrees( + PROJECT_ROOT + ) + active_reviewer_leases: dict[int, bool] = {} + pr_states: dict[int, dict] = {} + for scratch in scratch_candidates: + pr_num = int(scratch["pr_number"]) + if pr_num in active_reviewer_leases: + continue + try: + comments = api_get_all(f"{base}/issues/{pr_num}/comments", auth) + except Exception: + comments = [] + lease = reviewer_pr_lease.find_active_reviewer_lease( + comments, pr_number=pr_num + ) + active_reviewer_leases[pr_num] = bool(lease) + try: + pr_live = api_request("GET", f"{base}/pulls/{pr_num}", auth) + except Exception: + pr_live = {} + pr_states[pr_num] = { + "merged": bool((pr_live or {}).get("merged") or (pr_live or {}).get("merged_at")), + "closed": (pr_live or {}).get("state") == "closed", + } + report = merged_cleanup_reconcile.build_reconciliation_report( project_root=PROJECT_ROOT, closed_prs=merged_closed, @@ -4744,6 +4772,8 @@ def gitea_reconcile_merged_cleanups( remote_branch_exists=remote_branch_exists, head_on_master=head_on_master, delete_capability_allowed=delete_capability_allowed, + active_reviewer_leases=active_reviewer_leases, + pr_states=pr_states, ) if dry_run: @@ -4787,6 +4817,14 @@ def gitea_reconcile_merged_cleanups( ) actions.append({"action": "remove_local_worktree", **result}) + for scratch in report.get("reviewer_scratch_entries") or []: + if not scratch.get("safe_to_remove_worktree"): + continue + result = merged_cleanup_reconcile.remove_reviewer_scratch_worktree( + PROJECT_ROOT, scratch.get("worktree_path") or "" + ) + actions.append({"action": "remove_reviewer_scratch_worktree", **result}) + report["dry_run"] = False report["executed"] = True report["actions"] = actions diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py index 92490ac..e6e7236 100644 --- a/merged_cleanup_reconcile.py +++ b/merged_cleanup_reconcile.py @@ -17,6 +17,11 @@ import issue_lock_store PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) CLOSES_FIXES_RE = re.compile(r"\b(?:closes|fixes)\s+#(\d+)\b", re.IGNORECASE) +# Reviewer scratch folders: review-pr or review-pr-submit (#534). +REVIEWER_SCRATCH_NAME_RE = re.compile( + r"^review-pr(?P\d+)(?:-submit)?$", + re.IGNORECASE, +) def extract_linked_issue(title: str, body: str) -> int | None: @@ -36,8 +41,8 @@ def resolve_worktree_path(project_root: str, branch: str) -> str: return os.path.join(project_root, "branches", branch_worktree_folder(branch)) -def discover_local_worktrees(project_root: str) -> dict[str, str]: - """Return a mapping from branch name to absolute worktree path (#528).""" +def _git_worktree_porcelain(project_root: str) -> list[dict[str, str | None]]: + """Parse ``git worktree list --porcelain`` into structured records.""" res = subprocess.run( ["git", "-C", project_root, "worktree", "list", "--porcelain"], capture_output=True, @@ -45,25 +50,139 @@ def discover_local_worktrees(project_root: str) -> dict[str, str]: check=False, ) if res.returncode != 0: - return {} + return [] - mapping: dict[str, str] = {} - current_worktree = None + records: list[dict[str, str | None]] = [] + current: dict[str, str | None] = {} for line in res.stdout.splitlines(): line = line.strip() if not line: - current_worktree = None + if current.get("worktree"): + records.append(current) + current = {} continue if line.startswith("worktree "): - current_worktree = line[9:].strip() - elif line.startswith("branch ") and current_worktree: + if current.get("worktree"): + records.append(current) + current = {"worktree": line[9:].strip(), "branch": None, "head": None} + elif line.startswith("branch ") and current: ref = line[7:].strip() - if ref.startswith("refs/heads/"): - branch_name = ref[11:] - mapping[branch_name] = current_worktree + current["branch"] = ( + ref[11:] if ref.startswith("refs/heads/") else ref + ) + elif line.startswith("HEAD ") and current: + current["head"] = line[5:].strip() + elif line == "detached" and current: + current["branch"] = None + if current.get("worktree"): + records.append(current) + return records + + +def discover_local_worktrees(project_root: str) -> dict[str, str]: + """Return a mapping from branch name to absolute worktree path (#528).""" + mapping: dict[str, str] = {} + for record in _git_worktree_porcelain(project_root): + branch_name = record.get("branch") + path = record.get("worktree") + if branch_name and path: + mapping[str(branch_name)] = str(path) return mapping +def parse_reviewer_scratch_folder(folder_name: str) -> int | None: + """Return PR number for a reviewer scratch folder name, else None (#534).""" + match = REVIEWER_SCRATCH_NAME_RE.match((folder_name or "").strip()) + if not match: + return None + return int(match.group("pr")) + + +def discover_reviewer_scratch_worktrees(project_root: str) -> list[dict[str, Any]]: + """Discover detached reviewer scratch worktrees under ``branches/`` (#534). + + Matches folder names ``review-pr`` and ``review-pr-submit`` only. + These are never treated as author worktree paths. + """ + root = os.path.realpath(project_root) + branches_root = os.path.realpath(os.path.join(root, "branches")) + found: list[dict[str, Any]] = [] + for record in _git_worktree_porcelain(project_root): + path = record.get("worktree") + if not path: + continue + real = os.path.realpath(path) + parent = os.path.dirname(real) + if parent != branches_root: + continue + folder = os.path.basename(real) + pr_number = parse_reviewer_scratch_folder(folder) + if pr_number is None: + continue + found.append( + { + "pr_number": pr_number, + "worktree_path": real, + "folder_name": folder, + "current_branch": record.get("branch"), + "head_sha": record.get("head"), + "worktree_kind": "reviewer_scratch", + } + ) + return found + + +def assess_reviewer_scratch_cleanup( + *, + pr_number: int, + worktree_path: str, + folder_name: str, + pr_merged: bool, + pr_closed: bool, + worktree_state: dict[str, Any], + active_reviewer_lease: bool, +) -> dict[str, Any]: + """Assess whether a reviewer scratch worktree is safe to remove (#534).""" + reasons: list[str] = [] + exists = bool(worktree_state.get("exists")) + if not (pr_merged or pr_closed): + reasons.append("associated PR is still open") + if not exists: + reasons.append("reviewer scratch worktree not present") + if exists and worktree_state.get("clean") is False: + dirty = worktree_state.get("dirty_files") or [] + reasons.append( + "reviewer scratch worktree has tracked edits" + + (f" ({', '.join(dirty)})" if dirty else "") + ) + if active_reviewer_lease: + reasons.append("active reviewer lease still requires this worktree") + current_branch = worktree_state.get("current_branch") + if current_branch: + # Scratch trees are expected detached; a named branch is ambiguous ownership. + reasons.append( + f"reviewer scratch worktree is on branch '{current_branch}' " + "(expected detached HEAD for review-pr scratch trees)" + ) + + safe = exists and not reasons + return { + "pr_number": pr_number, + "worktree_path": worktree_path, + "folder_name": folder_name, + "worktree_kind": "reviewer_scratch", + "worktree_exists": exists, + "worktree_clean": worktree_state.get("clean"), + "safe_to_remove_worktree": safe, + "block_reasons": reasons, + "recommended_action": ( + "remove_reviewer_scratch_worktree" if safe else "keep_reviewer_scratch_worktree" + ), + # Never treat as author worktree cleanup. + "author_worktree_cleanup": False, + } + + def read_issue_lock(path: str | None = None) -> dict[str, Any] | None: if path: return issue_lock_store.read_lock_file(path.strip()) @@ -310,6 +429,8 @@ def build_reconciliation_report( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, + active_reviewer_leases: dict[int, bool] | None = None, + pr_states: dict[int, dict[str, Any]] | None = None, ) -> dict[str, Any]: open_heads = collect_open_pr_heads(open_prs) worktree_map = discover_local_worktrees(project_root) @@ -334,10 +455,51 @@ def build_reconciliation_report( worktree_map=worktree_map, ) ) + + # #534: reviewer scratch worktrees are reported separately from author cleanup. + lease_map = active_reviewer_leases or {} + state_map = pr_states or {} + open_pr_numbers = { + int(pr["number"]) for pr in (open_prs or []) if pr.get("number") is not None + } + closed_by_number = { + int(pr["number"]): pr for pr in (closed_prs or []) if pr.get("number") is not None + } + scratch_entries: list[dict[str, Any]] = [] + for scratch in discover_reviewer_scratch_worktrees(project_root): + pr_number = int(scratch["pr_number"]) + state_info = state_map.get(pr_number) or {} + closed_pr = closed_by_number.get(pr_number) + if closed_pr is not None: + pr_merged = bool(closed_pr.get("merged_at") or closed_pr.get("merged")) + pr_closed = True + elif pr_number in open_pr_numbers: + pr_merged = False + pr_closed = False + else: + pr_merged = bool(state_info.get("merged")) + pr_closed = bool(state_info.get("closed") or state_info.get("merged")) + worktree_path = scratch["worktree_path"] + worktree_state = read_local_worktree_state(worktree_path) + worktree_state["worktree_path"] = worktree_path + # Prefer live branch from state (git) over porcelain branch field. + assessment = assess_reviewer_scratch_cleanup( + pr_number=pr_number, + worktree_path=worktree_path, + folder_name=scratch["folder_name"], + pr_merged=pr_merged, + pr_closed=pr_closed, + worktree_state=worktree_state, + active_reviewer_lease=bool(lease_map.get(pr_number)), + ) + scratch_entries.append(assessment) + return { "project_root": os.path.realpath(project_root), "merged_pr_count": len(entries), "entries": entries, + "reviewer_scratch_entries": scratch_entries, + "reviewer_scratch_count": len(scratch_entries), "dry_run": True, "executed": False, } @@ -370,4 +532,65 @@ def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: "performed": True, "message": f"removed worktree {worktree_path}", "worktree_path": worktree_path, + } + + +def remove_reviewer_scratch_worktree( + project_root: str, + worktree_path: str, +) -> dict[str, Any]: + """Remove a reviewer scratch worktree by absolute path (#534). + + Fail closed unless the path is a direct ``branches/review-pr*`` child of + *project_root*. Never routes through author branch-name derivation. + """ + root = os.path.realpath(project_root) + branches_root = os.path.realpath(os.path.join(root, "branches")) + real = os.path.realpath((worktree_path or "").strip()) + folder = os.path.basename(real) + if os.path.dirname(real) != branches_root: + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": ( + "reviewer scratch path must be a direct child of " + f"{branches_root}; got {real}" + ), + } + if parse_reviewer_scratch_folder(folder) is None: + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": f"path is not a reviewer scratch folder: {folder}", + } + if not os.path.isdir(real): + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": f"worktree not found: {real}", + } + res = subprocess.run( + ["git", "-C", project_root, "worktree", "remove", real], + capture_output=True, + text=True, + check=False, + ) + if res.returncode != 0: + return { + "success": False, + "performed": False, + "worktree_kind": "reviewer_scratch", + "message": (res.stderr or res.stdout or "worktree remove failed").strip(), + "worktree_path": real, + } + return { + "success": True, + "performed": True, + "worktree_kind": "reviewer_scratch", + "message": f"removed reviewer scratch worktree {real}", + "worktree_path": real, + "author_worktree_cleanup": False, } \ No newline at end of file diff --git a/tests/test_merged_cleanup_reconcile.py b/tests/test_merged_cleanup_reconcile.py index ad4a2ee..e90d5ab 100644 --- a/tests/test_merged_cleanup_reconcile.py +++ b/tests/test_merged_cleanup_reconcile.py @@ -238,5 +238,206 @@ class TestMergedCleanupReport(unittest.TestCase): ) +class TestReviewerScratchCleanup(unittest.TestCase): + """#534: authorized cleanup path for reviewer scratch worktrees.""" + + def test_parse_reviewer_scratch_folder(self): + self.assertEqual(mcr.parse_reviewer_scratch_folder("review-pr512-submit"), 512) + self.assertEqual(mcr.parse_reviewer_scratch_folder("review-pr99"), 99) + self.assertIsNone(mcr.parse_reviewer_scratch_folder("feat-issue-11-x")) + self.assertIsNone(mcr.parse_reviewer_scratch_folder("review-feat-issue-11")) + self.assertIsNone(mcr.parse_reviewer_scratch_folder("review-pr512-extra-tail")) + + def test_assess_safe_clean_detached_after_merge(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=True, + pr_closed=True, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abc", + "dirty_files": [], + }, + active_reviewer_lease=False, + ) + self.assertTrue(assessment["safe_to_remove_worktree"]) + self.assertFalse(assessment["author_worktree_cleanup"]) + self.assertEqual( + assessment["recommended_action"], "remove_reviewer_scratch_worktree" + ) + self.assertEqual(assessment["worktree_kind"], "reviewer_scratch") + + def test_assess_blocks_dirty_scratch(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=True, + pr_closed=True, + worktree_state={ + "exists": True, + "clean": False, + "current_branch": None, + "head_sha": "abc", + "dirty_files": ["foo.py"], + }, + active_reviewer_lease=False, + ) + self.assertFalse(assessment["safe_to_remove_worktree"]) + self.assertTrue( + any("tracked edits" in r for r in assessment["block_reasons"]) + ) + + def test_assess_blocks_active_lease(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=True, + pr_closed=True, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abc", + "dirty_files": [], + }, + active_reviewer_lease=True, + ) + self.assertFalse(assessment["safe_to_remove_worktree"]) + self.assertTrue( + any("active reviewer lease" in r for r in assessment["block_reasons"]) + ) + + def test_assess_blocks_open_pr(self): + assessment = mcr.assess_reviewer_scratch_cleanup( + pr_number=512, + worktree_path="/repo/Gitea-Tools/branches/review-pr512-submit", + folder_name="review-pr512-submit", + pr_merged=False, + pr_closed=False, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abc", + "dirty_files": [], + }, + active_reviewer_lease=False, + ) + self.assertFalse(assessment["safe_to_remove_worktree"]) + self.assertTrue(any("still open" in r for r in assessment["block_reasons"])) + + @patch("subprocess.run") + @patch("merged_cleanup_reconcile.read_local_worktree_state") + def test_report_lists_scratch_separately_from_author(self, mock_state, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/feat-issue-11-x\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD deadbeef\n\n" + "worktree /repo/Gitea-Tools/branches/review-pr512-submit\n" + "HEAD abcdef0\n" + "detached\n\n" + ) + mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + + def _state(path): + if "review-pr512" in path: + return { + "exists": True, + "clean": True, + "current_branch": None, + "head_sha": "abcdef0", + "dirty_files": [], + } + return { + "exists": True, + "clean": True, + "current_branch": "feat/issue-11-x", + "head_sha": "deadbeef", + "dirty_files": [], + } + + mock_state.side_effect = _state + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 11, + "title": "merged author", + "body": "Closes #11", + "head": {"ref": "feat/issue-11-x"}, + "merged_at": "2026-07-06T12:00:00Z", + }, + { + "number": 512, + "title": "merged reviewed", + "body": "Closes #512", + "head": {"ref": "feat/issue-512-x"}, + "merged_at": "2026-07-06T12:00:00Z", + }, + ], + open_prs=[], + remote_branch_exists={ + "feat/issue-11-x": False, + "feat/issue-512-x": False, + }, + head_on_master={11: True, 512: True}, + delete_capability_allowed=True, + active_reviewer_leases={512: False}, + ) + self.assertEqual(report["merged_pr_count"], 2) + self.assertEqual(report["reviewer_scratch_count"], 1) + scratch = report["reviewer_scratch_entries"][0] + self.assertEqual(scratch["pr_number"], 512) + self.assertEqual(scratch["worktree_kind"], "reviewer_scratch") + self.assertFalse(scratch["author_worktree_cleanup"]) + self.assertTrue(scratch["safe_to_remove_worktree"]) + # Author entries must not swallow the scratch path as local_worktree. + author_paths = { + (e.get("local_worktree") or {}).get("worktree_path") + for e in report["entries"] + } + self.assertNotIn( + "/repo/Gitea-Tools/branches/review-pr512-submit", author_paths + ) + + @patch("subprocess.run") + def test_remove_reviewer_scratch_worktree_path_guard(self, mock_run): + mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="") + with patch("os.path.isdir", return_value=True): + bad = mcr.remove_reviewer_scratch_worktree( + "/repo/Gitea-Tools", + "/repo/Gitea-Tools/branches/feat-issue-11-x", + ) + self.assertFalse(bad["performed"]) + good = mcr.remove_reviewer_scratch_worktree( + "/repo/Gitea-Tools", + "/repo/Gitea-Tools/branches/review-pr512-submit", + ) + self.assertTrue(good["success"]) + self.assertTrue(good["performed"]) + self.assertFalse(good["author_worktree_cleanup"]) + mock_run.assert_any_call( + [ + "git", + "-C", + "/repo/Gitea-Tools", + "worktree", + "remove", + "/repo/Gitea-Tools/branches/review-pr512-submit", + ], + capture_output=True, + text=True, + check=False, + ) + + if __name__ == "__main__": unittest.main() \ No newline at end of file From a82d35f8bd87c1d42b6a64f6478ac2f2f90b227a Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 23:33:43 -0400 Subject: [PATCH 7/7] feat: validate Selected PR: none in cleanup mode when queue is empty --- pr_queue_cleanup.py | 16 +++++++++++++++- tests/test_pr_queue_cleanup.py | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/pr_queue_cleanup.py b/pr_queue_cleanup.py index c1237aa..d6091ca 100644 --- a/pr_queue_cleanup.py +++ b/pr_queue_cleanup.py @@ -132,6 +132,13 @@ def check_cleanup_task_allowed(task: str) -> tuple[bool, list[str]]: _SELECTED_PR_RE = re.compile( r"^\s*[-*]?\s*selected pr\s*:\s*#?(\d+)", re.IGNORECASE | re.MULTILINE ) +_SELECTED_PR_NONE_RE = re.compile( + r"^\s*[-*]?\s*selected pr\s*:\s*(?:none|n/a)\b", re.IGNORECASE | re.MULTILINE +) +_EMPTY_QUEUE_RE = re.compile( + r"\b0 open pr|\bno open pr|\bno eligible pr|\bempty (?:review )?queue|\binventory empty|\btrusted_empty\b", + re.IGNORECASE, +) _NEXT_SUGGESTED_RE = re.compile( r"^\s*[-*]?\s*next suggested pr\s*:", re.IGNORECASE | re.MULTILINE ) @@ -176,7 +183,11 @@ def assess_pr_queue_cleanup_report(report_text: str) -> dict: selected = _SELECTED_PR_RE.findall(text) if len(selected) == 0: - reasons.append("report must name exactly one Selected PR") + if _SELECTED_PR_NONE_RE.search(text): + if not _EMPTY_QUEUE_RE.search(text): + reasons.append("Selected PR is 'none' but no valid empty-queue claim found") + else: + reasons.append("report must name exactly one Selected PR") elif len(set(selected)) > 1: reasons.append( "cleanup run selected multiple PRs " @@ -184,6 +195,9 @@ def assess_pr_queue_cleanup_report(report_text: str) -> dict: "PR per run" ) + if len(selected) > 0 and _SELECTED_PR_NONE_RE.search(text): + reasons.append("report contains both a selected PR and a claim of none selected") + terminal = _TERMINAL_MUTATION_RE.findall(text) if len(terminal) > 1: reasons.append( diff --git a/tests/test_pr_queue_cleanup.py b/tests/test_pr_queue_cleanup.py index e26a026..caff6cc 100644 --- a/tests/test_pr_queue_cleanup.py +++ b/tests/test_pr_queue_cleanup.py @@ -265,5 +265,38 @@ class TestCleanupReportVerifier(unittest.TestCase): self.assertEqual(direct["proven"], wrapped["proven"]) +class TestCleanupEmptyQueue(unittest.TestCase): + def test_empty_queue_report_passes(self): + report = _clean_report( + selected="Selected PR: none", + decision="Review decision: comment", + next="Next suggested PR: approvals (queue empty)", + pagination="PR inventory pagination proof: inventory_complete=true, total_count 0", + ) + report += "\npr_inventory_trust_gate.status: trusted_empty" + result = assess_pr_queue_cleanup_report(report) + self.assertTrue(result["proven"], result["reasons"]) + + def test_empty_queue_without_evidence_fails(self): + report = _clean_report( + selected="Selected PR: none", + ) + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("no valid empty-queue claim" in r for r in result["reasons"]), + result["reasons"] + ) + + def test_contradictory_selected_pr_fails(self): + report = _clean_report() + "\nSelected PR: none" + result = assess_pr_queue_cleanup_report(report) + self.assertFalse(result["proven"]) + self.assertTrue( + any("contains both a selected PR and a claim of none selected" in r for r in result["reasons"]), + result["reasons"] + ) + + if __name__ == "__main__": unittest.main()