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 01/12] 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 02/12] 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 03/12] 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 565363d5f45a4c43362b064bd75c93122c7efb82 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:36:10 -0400 Subject: [PATCH 04/12] docs: define bootstrap review path for self-hosted MCP fixes (Closes #557) Document the narrow controller-authorized path for landing MCP workflow fixes when live daemons cannot canonically review themselves, with required audits, allowed/forbidden actions, and post-land restart steps. --- docs/bootstrap-review-path.md | 190 +++++++++++++++++++++++ docs/llm-workflow-runbooks.md | 22 +++ skills/llm-project-workflow/SKILL.md | 15 +- tests/test_bootstrap_review_path_docs.py | 81 ++++++++++ 4 files changed, 307 insertions(+), 1 deletion(-) create mode 100644 docs/bootstrap-review-path.md create mode 100644 tests/test_bootstrap_review_path_docs.py diff --git a/docs/bootstrap-review-path.md b/docs/bootstrap-review-path.md new file mode 100644 index 0000000..33293d2 --- /dev/null +++ b/docs/bootstrap-review-path.md @@ -0,0 +1,190 @@ +# Bootstrap Review Path for Self-Hosted MCP Workflow Fixes (#557) + +## Purpose + +Gitea-Tools MCP workflow fixes can create a **bootstrap deadlock**: the live +MCP daemons still run the broken code from `master`, so canonical reviewer / +merger tools cannot complete the review that would land the fix. + +This document defines a **narrow, controller-authorized bootstrap path** so +such fixes can land without weakening normal review/merge gates. + +It is **not** a general bypass. Normal PRs must still use the full +reviewer → merger MCP workflow. + +## When this path applies + +All of the following must be true: + +1. The PR changes **Gitea-Tools MCP workflow / preflight / lease / gate** code + that the live daemon must load to review itself (self-hosted control plane). +2. Canonical review or merge tools **fail closed** because of that defect + (documented tool errors, not “inconvenience”). +3. A **controller** records an explicit bootstrap authorization on the PR or + linked issue (see below). +4. The PR source is clean, testable, and free of unrelated scope. + +Example (historical): PR #553 / Issue #546 (reviewer preflight capability/lease +deadlock). Live daemons on unpatched `master` could not complete canonical +review of the fix PR. + +## Durable authorization (required) + +**No manual remote merge, force-merge, or API merge of a bootstrap PR is +allowed** unless a controller has posted a durable authorization record on +Gitea. + +### Authorization record (issue or PR comment) + +The controller comment **must** include a machine-readable marker and the +fields below: + +```text +## BOOTSTRAP REVIEW AUTHORIZATION (#557) + +Status: APPROVED +PR: +Issue: +Controller: +Reason: live MCP runtime cannot canonically review this self-hosted workflow fix +Scope: narrow bootstrap only — does not weaken normal PR gates + +Validation evidence: +- git diff --check: clean +- targeted pytest: → pass +- full suite attribution (if non-zero): baseline compared to prgs/master + +Duplicate-PR audit: +Mutation ledger audit:

+Contamination audit: + +Allowed verification actions used: +Forbidden actions NOT used: root checkout edits; raw curl mutation; direct +module import of gitea_mcp_server for mutations; in-memory gate restoration + +Post-land plan: +- restart MCP daemons for author/reviewer/merger/reconciler profiles +- re-verify with canonical tools (see Post-bootstrap steps) +``` + +Without this record, bootstrap merge is **forbidden**. Agents must stop with +`BLOCKED + DIAGNOSE` rather than improvise a merge. + +## Review gates and proof (still required) + +Bootstrap does **not** skip technical review. It only allows verification and +landing when the **live MCP mutation path** cannot complete the review. + +Before authorization, the controller (or a delegated read-only verifier in an +isolated worktree) must have: + +| Gate | Requirement | +|------|-------------| +| Diff hygiene | `git diff --check` clean on the PR tip vs `prgs/master` | +| Tests | Targeted tests for the fix pass; full suite either green or failures attributed to baseline `prgs/master` | +| Duplicate PR | Open-PR inventory shows no competing open PR for the same issue (or supersession is documented) | +| Mutation ledger | Any claimed mutations are ledger-consistent; no silent root edits | +| Contamination audit | PR/issue comments and leases classified as clean vs workflow-contaminated | +| Scope | Diff limited to the bootstrap issue; no drive-by refactors | + +### Contamination audit (comments / leases) + +Comments or leases created via **raw curl**, **direct Python import of MCP +modules**, or **token extraction** are **workflow-contaminated**. They must be +listed and must **not** be treated as canonical review proof. + +Only comments posted via **sanctioned MCP reviewer tools** (after the fix is +landed and daemons restarted, or during a clean path that did not bypass gates) +count as canonical review state. + +Historical example on PR #553: + +| Comment | Disposition | +|---------|-------------| +| #7247 test / raw API | contaminated | +| #7251 lease metadata / raw API | contaminated | +| #7252 lease metadata / direct import | contaminated | +| #7265 lease metadata / MCP reviewer tools | workflow-clean | + +## Allowed verification actions + +These may run **outside** the broken live mutation path to establish facts: + +- Read-only MCP tools (`gitea_view_pr`, `gitea_list_prs`, `gitea_whoami`, + `gitea_get_profile`, inventory, eligibility **reads**). +- Isolated `branches/` worktree checkout of the PR head (never root). +- `git fetch`, `git log`, `git diff`, `git merge-tree` (read-only analysis). +- Targeted `pytest` / `py_compile` inside that worktree. +- Controller posting of the bootstrap authorization comment via a healthy + MCP profile **if available**; if comment mutation is also deadlocked, a + **human operator** posts the authorization in the Gitea UI (still durable on + the thread). + +## Forbidden actions (always) + +Even under bootstrap: + +- Editing or committing in the **project root / control checkout**. +- Treating root as a worktree for implementation or review mutations. +- Raw `curl` / REST mutation with extracted tokens as a normal workflow. +- `import gitea_mcp_server` (or sibling modules) from a shell to call mutation + helpers and bypass preflight. +- In-memory restoration of capability / lease / decision state to “finish” + a blocked chain. +- Force-push, history rewrite, or deleting evidence comments. +- Weakening normal gates in code “temporarily” without a tracked issue/PR. +- Self-review or self-merge by the PR author identity. + +## Landing procedure (after APPROVED authorization) + +1. Confirm the authorization record is present and complete on the PR or issue. +2. Merge **only** with an independent merger identity when possible. + - Preferred: restart/replace the MCP merger daemon with a build that includes + the fix (or a one-shot process started from the PR worktree binary) so + `gitea_merge_pr` can run under normal gates. + - If the live merger daemon still cannot load the fix, a **human operator** + may merge via the Gitea UI **only** after the authorization record exists. +3. Never merge from contaminated proof alone. + +## Post-bootstrap steps + +### 1. Restart MCP daemons + +After the fix lands on `prgs/master`: + +1. Stop author / reviewer / merger / reconciler MCP server processes for this + repo (IDE MCP pool + any long-lived terminals). +2. Confirm no stale PID still serves the old code. +3. Restart each profile so it loads the updated `master` tree (or installed + package path). +4. Call `gitea_whoami` / `gitea_get_profile` on each namespace and record + profile name + identity. + +### 2. Re-verify with canonical tools + +Example pattern after PR #553-class fixes (lease release / #550-style follow-up): + +1. From a **reviewer** MCP session: acquire/release or inspect the relevant + lease with canonical tools only. +2. Confirm no direct-import or raw-API path is required. +3. Post a short controller or reconciler note: bootstrap complete; normal gates + restored. + +If verification still requires a bypass, open a new issue — do **not** extend +this bootstrap authorization silently. + +## Explicit non-goals + +- This path does **not** authorize skipping tests, duplicate audits, or + contamination audits. +- This path does **not** authorize root checkout implementation work. +- This path does **not** replace BLOCKED + DIAGNOSE when a non-bootstrap + failure occurs (#552). +- This path does **not** grant permanent “operator exception” culture. + +## Related + +- Root checkout policy: `docs/llm-workflow-runbooks.md` (Global LLM Worktree Rule, #475) +- BLOCKED + DIAGNOSE: issue #552 / skill workflows +- Reviewer lease / preflight deadlock class: issues #546, #548, #550 +- Durable session proofs across daemon pools: issue #559 diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d0e472d..1e0366b 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -408,6 +408,28 @@ The guard blocks when the **control checkout** (repository root) is: `branches/...` directories are disposable role worktrees; the root checkout is the stable orchestration surface only. +## Bootstrap Review Path for self-hosted MCP fixes (#557) + +When a PR fixes Gitea-Tools MCP workflow code that the **live daemon** still +runs from broken `master`, canonical review can deadlock on itself. + +Do **not** improvise raw API, direct imports, root edits, or gate bypasses. + +Use the narrow controller-authorized path documented in: + +- [`docs/bootstrap-review-path.md`](bootstrap-review-path.md) + +Hard rules: + +- No merge without a durable `BOOTSTRAP REVIEW AUTHORIZATION (#557)` record on + the PR or linked issue. +- Validation, duplicate-PR, mutation-ledger, and contamination audits remain + mandatory. +- Root checkout stays read-only orchestration only; verification runs in + `branches/` worktrees. +- After land: restart MCP daemons and re-verify with canonical tools only. +- This never weakens normal reviewer/merger gates for ordinary PRs. + ## Shell Spawn Hard-Stop Rule Symptom: a shell tool call returns `exit_code: -1` with empty stdout/stderr. diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index dc2f961..73b9f8d 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -182,4 +182,17 @@ Ready-to-copy task prompts live in [`templates/`](templates/): Releases follow SemVer from remote `master` only, after full test suite passes. See [`templates/release-tag.md`](templates/release-tag.md) and -`scripts/release-tag`. \ No newline at end of file +`scripts/release-tag`. + +## Bootstrap Review Path (#557) + +Self-hosted MCP workflow fixes can deadlock live review daemons. Do not bypass +gates with raw API, direct imports, or root checkout edits. + +If and only if a controller posts a durable `BOOTSTRAP REVIEW AUTHORIZATION +(#557)` record, follow: + +`docs/bootstrap-review-path.md` + +Otherwise stop with BLOCKED + DIAGNOSE. Bootstrap never weakens normal PR gates. + diff --git a/tests/test_bootstrap_review_path_docs.py b/tests/test_bootstrap_review_path_docs.py new file mode 100644 index 0000000..71c5c14 --- /dev/null +++ b/tests/test_bootstrap_review_path_docs.py @@ -0,0 +1,81 @@ +"""Doc-contract checks for the bootstrap review path (#557). + +Self-hosted MCP workflow fixes can deadlock live review daemons. These tests +pin the narrow controller-authorized bootstrap path so it cannot silently +regress into a general gate bypass. +""" + +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +DOC = REPO_ROOT / "docs" / "bootstrap-review-path.md" +RUNBOOK = REPO_ROOT / "docs" / "llm-workflow-runbooks.md" +SKILL = REPO_ROOT / "skills" / "llm-project-workflow" / "SKILL.md" + + +def _normalize(text: str) -> str: + return " ".join(text.split()) + + +def test_bootstrap_doc_exists_and_names_authorization_marker(): + text = DOC.read_text(encoding="utf-8") + assert "BOOTSTRAP REVIEW AUTHORIZATION (#557)" in text + assert "bootstrap deadlock" in text.lower() or "Bootstrap deadlock" in text + + +def test_bootstrap_doc_requires_validation_and_audits(): + text = _normalize(DOC.read_text(encoding="utf-8")) + for phrase in ( + "git diff --check", + "Duplicate-PR audit", + "Mutation ledger audit", + "Contamination audit", + "workflow-contaminated", + ): + assert phrase in text, f"bootstrap doc missing: {phrase!r}" + + +def test_bootstrap_doc_lists_allowed_and_forbidden_actions(): + text = _normalize(DOC.read_text(encoding="utf-8")) + lower = text.lower() + for phrase in ( + "Allowed verification actions", + "Forbidden actions", + "project root", + "direct Python import", + "branches/", + ): + assert phrase in text, f"bootstrap doc missing: {phrase!r}" + assert "curl" in lower + assert "raw" in lower + + +def test_bootstrap_doc_forbids_general_gate_weakening(): + text = _normalize(DOC.read_text(encoding="utf-8")).lower() + assert "never weakens normal" in text or "does not weaken normal" in text + assert "general bypass" in text or "general gate" in text + + +def test_bootstrap_doc_post_land_restart_and_verify(): + text = _normalize(DOC.read_text(encoding="utf-8")) + for phrase in ( + "Restart MCP daemons", + "canonical tools", + "gitea_whoami", + ): + assert phrase in text, f"post-bootstrap guidance missing: {phrase!r}" + + +def test_runbook_links_bootstrap_path(): + text = _normalize(RUNBOOK.read_text(encoding="utf-8")) + assert "Bootstrap Review Path for self-hosted MCP fixes (#557)" in text + assert "bootstrap-review-path.md" in text + assert "BOOTSTRAP REVIEW AUTHORIZATION (#557)" in text + + +def test_skill_links_bootstrap_path(): + text = _normalize(SKILL.read_text(encoding="utf-8")) + assert "Bootstrap Review Path (#557)" in text + assert "bootstrap-review-path.md" in text + assert "BLOCKED + DIAGNOSE" in text or "BLOCKED" in text From 32fc1719aa0a63a41c03deddc5583875e9623e93 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:49:19 -0400 Subject: [PATCH 05/12] feat: allow reconciler profile to run merged cleanup directly (Closes #523) --- task_capability_map.py | 4 +- tests/test_audit_reconciliation_mode.py | 2 +- tests/test_reconciler_cleanup_integration.py | 112 +++++++++++++++++++ webui/lease_loader.py | 3 +- 4 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 tests/test_reconciler_cleanup_integration.py diff --git a/task_capability_map.py b/task_capability_map.py index c86ce2a..8d3daf2 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -106,11 +106,11 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { }, "reconcile_merged_cleanups": { "permission": "gitea.read", - "role": "author", + "role": "reconciler", }, "reconciliation_cleanup": { "permission": "gitea.branch.delete", - "role": "author", + "role": "reconciler", }, "work_issue": { "permission": "gitea.pr.create", diff --git a/tests/test_audit_reconciliation_mode.py b/tests/test_audit_reconciliation_mode.py index a117fab..e429ddf 100644 --- a/tests/test_audit_reconciliation_mode.py +++ b/tests/test_audit_reconciliation_mode.py @@ -284,7 +284,7 @@ class TestTaskCapabilityMap(unittest.TestCase): required_permission("reconciliation_cleanup"), "gitea.branch.delete", ) - self.assertEqual(required_role("reconciliation_cleanup"), "author") + self.assertEqual(required_role("reconciliation_cleanup"), "reconciler") if __name__ == "__main__": diff --git a/tests/test_reconciler_cleanup_integration.py b/tests/test_reconciler_cleanup_integration.py new file mode 100644 index 0000000..7b3ef4a --- /dev/null +++ b/tests/test_reconciler_cleanup_integration.py @@ -0,0 +1,112 @@ +"""Integration tests for reconciler merged cleanup (#523).""" +import os +import sys +import unittest +from unittest.mock import patch, MagicMock + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import mcp_server +import task_capability_map +from audit_reconciliation_mode import clear_phase + +RECONCILER_PROFILE = { + "profile_name": "prgs-reconciler", + "context": "prgs", + "role": "reconciler", + "username": "sysadmin", + "base_url": "https://gitea.prgs.cc", + "allowed_operations": ["gitea.read", "gitea.pr.close", "gitea.pr.comment", "gitea.issue.comment"], + "forbidden_operations": [], +} + + +class TestReconcilerCleanupIntegration(unittest.TestCase): + def setUp(self): + clear_phase() + mcp_server._preflight_whoami_called = False + mcp_server._preflight_capability_called = False + mcp_server._preflight_resolved_role = None + mcp_server._preflight_resolved_task = None + mcp_server._preflight_whoami_violation = False + mcp_server._preflight_capability_violation = False + mcp_server._preflight_whoami_violation_files = [] + mcp_server._preflight_capability_violation_files = [] + self.mock_api = patch("gitea_auth.api_request").start() + self.mock_auth = patch( + "mcp_server.get_auth_header", return_value="token test" + ).start() + + def tearDown(self): + patch.stopall() + clear_phase() + mcp_server._IDENTITY_CACHE.clear() + + @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-reconciler"}, clear=True) + @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) + def test_reconciler_can_run_reconcile_merged_cleanups_directly(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check("capability", resolved_role="reconciler", resolved_task="reconcile_merged_cleanups") + + # Mock closed PRs and open PRs returned by API + self.mock_api.side_effect = [ + [ # closed pulls + { + "number": 100, + "title": "merged feature", + "body": "Closes #100", + "merged": True, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "deadbeef", + "head": {"ref": "feat/issue-100", "sha": "cafebabe"}, + } + ], + [], # open pulls + ] + + with patch("mcp_server._remote_branch_exists", return_value=True): + with patch("mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", return_value=True): + result = mcp_server.gitea_reconcile_merged_cleanups( + dry_run=True, + remote="prgs", + ) + self.assertTrue(result["success"]) + self.assertTrue(result["dry_run"]) + self.assertEqual(result["entries"][0]["issue_number"], 100) + + @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-reconciler"}, clear=True) + @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) + def test_reconciler_role_bypasses_branches_only_mutation_guard(self, _profile): + # We simulate verify_preflight_purity under reconciler role. + # It should succeed without raising RuntimeError even if CWD/workspace is the process root. + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check("capability", resolved_role="reconciler", resolved_task="reconcile_merged_cleanups") + + # Calling verify_preflight_purity directly + try: + mcp_server.verify_preflight_purity(remote="prgs", task="reconcile_merged_cleanups") + except RuntimeError as e: + self.fail(f"verify_preflight_purity raised RuntimeError unexpectedly: {e}") + + @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-author", "GITEA_TEST_PORCELAIN": ""}, clear=True) + @patch("mcp_server.get_profile", return_value={ + "profile_name": "prgs-author", + "context": "prgs", + "role": "author", + "username": "jcwalker3", + "base_url": "https://gitea.prgs.cc", + "allowed_operations": ["gitea.read", "gitea.pr.create", "gitea.branch.create", "gitea.branch.push", "gitea.repo.commit"], + "forbidden_operations": [], + }) + def test_author_role_remains_blocked_on_root_checkout(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check("capability", resolved_role="author", resolved_task="reconcile_merged_cleanups") + + with patch("mcp_server.PROJECT_ROOT", "/Users/jasonwalker/Development/Gitea-Tools"): + with self.assertRaises(RuntimeError) as ctx: + mcp_server.verify_preflight_purity(remote="prgs", task="reconcile_merged_cleanups") + self.assertIn("author mutation blocked: workspace is the stable control checkout", str(ctx.exception)) + + +if __name__ == "__main__": + unittest.main() diff --git a/webui/lease_loader.py b/webui/lease_loader.py index 717feef..548aec3 100644 --- a/webui/lease_loader.py +++ b/webui/lease_loader.py @@ -11,7 +11,8 @@ from urllib.parse import urlparse from gitea_auth import api_fetch_page, get_auth_header, repo_api_url from issue_claim_heartbeat import build_claim_inventory -from merged_cleanup_reconcile import ISSUE_LOCK_FILE, read_issue_lock +from merged_cleanup_reconcile import read_issue_lock +from issue_lock_provenance import ISSUE_LOCK_FILE from webui.project_registry import ProjectRecord, load_registry from webui.queue_loader import _extract_linked_issue, _fetch_issues, _fetch_prs From 7a0de4a8e9b98ad80f84b18ece6c2b211a2c4034 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:53:34 -0400 Subject: [PATCH 06/12] feat: discover non-canonical worktrees by branch matching (Closes #528) --- merged_cleanup_reconcile.py | 48 ++++++++++++++- tests/test_merged_cleanup_reconcile.py | 85 +++++++++++++++++++++++++- webui/lease_loader.py | 3 +- 3 files changed, 132 insertions(+), 4 deletions(-) diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py index bf81258..92490ac 100644 --- a/merged_cleanup_reconcile.py +++ b/merged_cleanup_reconcile.py @@ -36,6 +36,34 @@ 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).""" + res = subprocess.run( + ["git", "-C", project_root, "worktree", "list", "--porcelain"], + capture_output=True, + text=True, + check=False, + ) + if res.returncode != 0: + return {} + + mapping: dict[str, str] = {} + current_worktree = None + for line in res.stdout.splitlines(): + line = line.strip() + if not line: + current_worktree = None + continue + if line.startswith("worktree "): + current_worktree = line[9:].strip() + elif line.startswith("branch ") and current_worktree: + ref = line[7:].strip() + if ref.startswith("refs/heads/"): + branch_name = ref[11:] + mapping[branch_name] = current_worktree + return mapping + + def read_issue_lock(path: str | None = None) -> dict[str, Any] | None: if path: return issue_lock_store.read_lock_file(path.strip()) @@ -216,6 +244,7 @@ def build_pr_cleanup_entry( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, + worktree_map: dict[str, str] | None = None, ) -> dict[str, Any]: pr_number = int(pr["number"]) head_branch = pr.get("head") or "" @@ -224,7 +253,16 @@ def build_pr_cleanup_entry( title = pr.get("title") or "" body = pr.get("body") or "" merged = bool(pr.get("merged_at")) - worktree_path = resolve_worktree_path(project_root, head_branch) + + discovered_path = worktree_map.get(head_branch) if worktree_map else None + derived_path = resolve_worktree_path(project_root, head_branch) + if discovered_path: + worktree_path = discovered_path + match_type = "branch" + else: + worktree_path = derived_path + match_type = "path" + worktree_state = read_local_worktree_state(worktree_path) worktree_state["worktree_path"] = worktree_path active_lock = has_active_issue_lock(head_branch, issue_lock_path) @@ -247,6 +285,8 @@ def build_pr_cleanup_entry( worktree_state=worktree_state, active_lock=active_lock, ) + local["match_type"] = match_type + return { "pr_number": pr_number, "issue_number": extract_linked_issue(title, body), @@ -272,6 +312,7 @@ def build_reconciliation_report( protected_branches: frozenset[str] | None = None, ) -> dict[str, Any]: open_heads = collect_open_pr_heads(open_prs) + worktree_map = discover_local_worktrees(project_root) entries: list[dict[str, Any]] = [] for pr in closed_prs or []: if not pr.get("merged_at"): @@ -290,6 +331,7 @@ def build_reconciliation_report( delete_capability_allowed=delete_capability_allowed, issue_lock_path=issue_lock_path, protected_branches=protected_branches, + worktree_map=worktree_map, ) ) return { @@ -302,7 +344,9 @@ def build_reconciliation_report( def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: - worktree_path = resolve_worktree_path(project_root, branch) + worktree_map = discover_local_worktrees(project_root) + discovered_path = worktree_map.get(branch) + worktree_path = discovered_path or resolve_worktree_path(project_root, branch) if not os.path.isdir(worktree_path): return { "success": False, diff --git a/tests/test_merged_cleanup_reconcile.py b/tests/test_merged_cleanup_reconcile.py index 779df64..ad4a2ee 100644 --- a/tests/test_merged_cleanup_reconcile.py +++ b/tests/test_merged_cleanup_reconcile.py @@ -4,7 +4,7 @@ import os import sys import tempfile import unittest -from unittest.mock import patch +from unittest.mock import patch, MagicMock sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) @@ -154,6 +154,89 @@ class TestMergedCleanupReport(unittest.TestCase): finally: os.remove(lock_path) + @patch("subprocess.run") + def test_discover_local_worktrees(self, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/custom-name\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD cafebabe\n\n" + ) + mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + res = mcr.discover_local_worktrees("/repo/Gitea-Tools") + self.assertEqual(res, {"feat/issue-11-x": "/repo/Gitea-Tools/branches/custom-name"}) + + @patch("subprocess.run") + @patch("merged_cleanup_reconcile.read_local_worktree_state") + def test_build_report_discovers_non_canonical_worktree_by_branch(self, mock_state, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/custom-name\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD cafebabe\n\n" + ) + mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + mock_state.return_value = { + "exists": True, + "clean": True, + "current_branch": "feat/issue-11-x", + "head_sha": "cafebabe", + "dirty_files": [], + } + + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 11, + "title": "merged", + "body": "Closes #11", + "head": {"ref": "feat/issue-11-x"}, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "abc123", + } + ], + open_prs=[], + remote_branch_exists={"feat/issue-11-x": True}, + head_on_master={11: True}, + delete_capability_allowed=True, + ) + self.assertEqual(report["merged_pr_count"], 1) + entry = report["entries"][0] + local = entry["local_worktree"] + self.assertEqual(local["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") + self.assertEqual(local["match_type"], "branch") + + @patch("subprocess.run") + def test_remove_local_worktree_uses_discovered_path(self, mock_run): + mock_output = ( + "worktree /repo/Gitea-Tools\n" + "bare\n\n" + "worktree /repo/Gitea-Tools/branches/custom-name\n" + "branch refs/heads/feat/issue-11-x\n" + "HEAD cafebabe\n\n" + ) + # First call is discover_local_worktrees, second is git worktree remove + mock_run.side_effect = [ + MagicMock(returncode=0, stdout=mock_output), + MagicMock(returncode=0, stdout="", stderr=""), + ] + + with patch("os.path.isdir", return_value=True): + result = mcr.remove_local_worktree("/repo/Gitea-Tools", "feat/issue-11-x") + self.assertTrue(result["success"]) + self.assertTrue(result["performed"]) + self.assertEqual(result["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") + # Assert git worktree remove was called with the custom path + mock_run.assert_any_call( + ["git", "-C", "/repo/Gitea-Tools", "worktree", "remove", "/repo/Gitea-Tools/branches/custom-name"], + capture_output=True, + text=True, + check=False, + ) + if __name__ == "__main__": unittest.main() \ No newline at end of file diff --git a/webui/lease_loader.py b/webui/lease_loader.py index 717feef..548aec3 100644 --- a/webui/lease_loader.py +++ b/webui/lease_loader.py @@ -11,7 +11,8 @@ from urllib.parse import urlparse from gitea_auth import api_fetch_page, get_auth_header, repo_api_url from issue_claim_heartbeat import build_claim_inventory -from merged_cleanup_reconcile import ISSUE_LOCK_FILE, read_issue_lock +from merged_cleanup_reconcile import read_issue_lock +from issue_lock_provenance import ISSUE_LOCK_FILE from webui.project_registry import ProjectRecord, load_registry from webui.queue_loader import _extract_linked_issue, _fetch_issues, _fetch_prs From 0ee89fe347b07c694bca769f0398cda0381dfb81 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:56:55 -0400 Subject: [PATCH 07/12] fix: resolve broken ISSUE_LOCK_FILE import in webui/lease_loader.py (Closes #569) --- webui/lease_loader.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/webui/lease_loader.py b/webui/lease_loader.py index 717feef..548aec3 100644 --- a/webui/lease_loader.py +++ b/webui/lease_loader.py @@ -11,7 +11,8 @@ from urllib.parse import urlparse from gitea_auth import api_fetch_page, get_auth_header, repo_api_url from issue_claim_heartbeat import build_claim_inventory -from merged_cleanup_reconcile import ISSUE_LOCK_FILE, read_issue_lock +from merged_cleanup_reconcile import read_issue_lock +from issue_lock_provenance import ISSUE_LOCK_FILE from webui.project_registry import ProjectRecord, load_registry from webui.queue_loader import _extract_linked_issue, _fetch_issues, _fetch_prs 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 08/12] 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 09/12] 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 10/12] 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 11/12] 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() From e5bf8c96474686b5759a2fded0b42159e9d096fb Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Thu, 9 Jul 2026 00:09:47 -0400 Subject: [PATCH 12/12] fix: strengthen reconciler merged cleanup tests and runbook (#523) Make the preflight bypass test actually exercise purity (not test short-circuit), cover unmerged/open-head fail-closed cleanup, and document post-merge cleanup ownership for prgs-reconciler so authors are not required for that path. --- docs/llm-workflow-runbooks.md | 21 ++ tests/test_reconciler_cleanup_integration.py | 225 ++++++++++++++++--- 2 files changed, 209 insertions(+), 37 deletions(-) diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index ed1660f..ce42f39 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -738,6 +738,27 @@ merger handoff. - **Prompt (normal):** `After verifying master contains the merge of PR #N using post-merge file-presence verification, close issue #M and delete the merged branch. Include verification details in the report.` - **Prompt (reconcile):** `Reconcile closed-not-merged PR #N by verifying if its content landed on master.` +### Post-merge merged cleanup ownership (#523) + +Post-merge **local worktree / remote branch cleanup** is **reconciler** work, not +author work. Do not switch from `prgs-reconciler` to `prgs-author` only to run +`gitea_reconcile_merged_cleanups`. + +- **Profile:** `prgs-reconciler` (task `reconcile_merged_cleanups` / + `reconciliation_cleanup`). +- **Namespace:** reconciler MCP server; stable control checkout is allowed for + this role (branches-only author guard does not apply). +- **Steps:** + 1. `gitea_whoami` + `gitea_resolve_task_capability(task="reconcile_merged_cleanups")`. + 2. Dry-run first: `gitea_reconcile_merged_cleanups(dry_run=True)`. + 3. Execute only after audit/authorization gates when remote branch delete or + worktree removal is required (`dry_run=False`, `execute_confirmed=True`, + and `gitea.branch.delete` when deleting remotes). +- **Fail closed:** unmerged/open heads, mismatched worktrees, and non-merged + closed PRs must not be cleaned. +- **Reports:** label cleanup actions as reconciler cleanup (not author mutation). +- **Prompt:** `As prgs-reconciler, dry-run then execute gitea_reconcile_merged_cleanups for recently merged PRs without switching to prgs-author.` + ### Stop on blocker - **Any profile.** If a required gate cannot be satisfied — identity diff --git a/tests/test_reconciler_cleanup_integration.py b/tests/test_reconciler_cleanup_integration.py index 7b3ef4a..7637602 100644 --- a/tests/test_reconciler_cleanup_integration.py +++ b/tests/test_reconciler_cleanup_integration.py @@ -2,13 +2,14 @@ import os import sys import unittest -from unittest.mock import patch, MagicMock +from unittest.mock import patch sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) import mcp_server import task_capability_map from audit_reconciliation_mode import clear_phase +from task_capability_map import required_role RECONCILER_PROFILE = { "profile_name": "prgs-reconciler", @@ -16,10 +17,33 @@ RECONCILER_PROFILE = { "role": "reconciler", "username": "sysadmin", "base_url": "https://gitea.prgs.cc", - "allowed_operations": ["gitea.read", "gitea.pr.close", "gitea.pr.comment", "gitea.issue.comment"], + "allowed_operations": [ + "gitea.read", + "gitea.pr.close", + "gitea.pr.comment", + "gitea.issue.comment", + ], "forbidden_operations": [], } +AUTHOR_PROFILE = { + "profile_name": "prgs-author", + "context": "prgs", + "role": "author", + "username": "jcwalker3", + "base_url": "https://gitea.prgs.cc", + "allowed_operations": [ + "gitea.read", + "gitea.pr.create", + "gitea.branch.create", + "gitea.branch.push", + "gitea.repo.commit", + ], + "forbidden_operations": [], +} + +CONTROL_ROOT = "/Users/jasonwalker/Development/Gitea-Tools" + class TestReconcilerCleanupIntegration(unittest.TestCase): def setUp(self): @@ -32,6 +56,7 @@ class TestReconcilerCleanupIntegration(unittest.TestCase): mcp_server._preflight_capability_violation = False mcp_server._preflight_whoami_violation_files = [] mcp_server._preflight_capability_violation_files = [] + mcp_server._preflight_capability_baseline_porcelain = "" self.mock_api = patch("gitea_auth.api_request").start() self.mock_auth = patch( "mcp_server.get_auth_header", return_value="token test" @@ -42,15 +67,26 @@ class TestReconcilerCleanupIntegration(unittest.TestCase): clear_phase() mcp_server._IDENTITY_CACHE.clear() + def test_capability_map_routes_merged_cleanup_to_reconciler(self): + self.assertEqual(required_role("reconcile_merged_cleanups"), "reconciler") + self.assertEqual(required_role("reconciliation_cleanup"), "reconciler") + self.assertEqual( + task_capability_map.required_permission("reconcile_merged_cleanups"), + "gitea.read", + ) + @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-reconciler"}, clear=True) @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) def test_reconciler_can_run_reconcile_merged_cleanups_directly(self, _profile): mcp_server.record_preflight_check("whoami") - mcp_server.record_preflight_check("capability", resolved_role="reconciler", resolved_task="reconcile_merged_cleanups") - - # Mock closed PRs and open PRs returned by API + mcp_server.record_preflight_check( + "capability", + resolved_role="reconciler", + resolved_task="reconcile_merged_cleanups", + ) + self.mock_api.side_effect = [ - [ # closed pulls + [ # closed pulls page { "number": 100, "title": "merged feature", @@ -59,53 +95,168 @@ class TestReconcilerCleanupIntegration(unittest.TestCase): "merged_at": "2026-07-06T12:00:00Z", "merge_commit_sha": "deadbeef", "head": {"ref": "feat/issue-100", "sha": "cafebabe"}, - } + }, + { + # closed but NOT merged — must not become a cleanup entry + "number": 101, + "title": "abandoned", + "body": "Closes #101", + "merged": False, + "merged_at": None, + "head": {"ref": "feat/issue-101", "sha": "badcafe"}, + }, ], [], # open pulls ] with patch("mcp_server._remote_branch_exists", return_value=True): - with patch("mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", return_value=True): + with patch( + "mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", + return_value=True, + ): result = mcp_server.gitea_reconcile_merged_cleanups( dry_run=True, remote="prgs", ) self.assertTrue(result["success"]) self.assertTrue(result["dry_run"]) - self.assertEqual(result["entries"][0]["issue_number"], 100) + entry_numbers = [e["issue_number"] for e in result["entries"]] + self.assertEqual(entry_numbers, [100]) + self.assertNotIn(101, entry_numbers) + + @patch.dict( + os.environ, + { + "GITEA_PROFILE_NAME": "prgs-reconciler", + # Force preflight purity to evaluate (disable test short-circuit). + "GITEA_TEST_PORCELAIN": "", + }, + clear=True, + ) + @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) + def test_reconciler_role_bypasses_branches_only_mutation_guard(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check( + "capability", + resolved_role="reconciler", + resolved_task="reconcile_merged_cleanups", + ) + + with patch("mcp_server.PROJECT_ROOT", CONTROL_ROOT): + with patch( + "mcp_server.root_checkout_guard.resolve_remote_master_sha", + return_value="abc123", + ): + with patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value={ + "current_branch": "master", + "head_sha": "abc123", + "porcelain_status": "", + }, + ): + try: + mcp_server.verify_preflight_purity( + remote="prgs", + task="reconcile_merged_cleanups", + ) + except RuntimeError as e: + self.fail( + "verify_preflight_purity raised RuntimeError " + f"unexpectedly for reconciler: {e}" + ) + + @patch.dict( + os.environ, + {"GITEA_PROFILE_NAME": "prgs-author", "GITEA_TEST_PORCELAIN": ""}, + clear=True, + ) + @patch("mcp_server.get_profile", return_value=AUTHOR_PROFILE) + def test_author_role_remains_blocked_on_root_checkout(self, _profile): + mcp_server.record_preflight_check("whoami") + mcp_server.record_preflight_check( + "capability", + resolved_role="author", + resolved_task="reconcile_merged_cleanups", + ) + + with patch("mcp_server.PROJECT_ROOT", CONTROL_ROOT): + with patch( + "mcp_server.root_checkout_guard.resolve_remote_master_sha", + return_value="abc123", + ): + with patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value={ + "current_branch": "master", + "head_sha": "abc123", + "porcelain_status": "", + }, + ): + with self.assertRaises(RuntimeError) as ctx: + mcp_server.verify_preflight_purity( + remote="prgs", + task="reconcile_merged_cleanups", + ) + message = str(ctx.exception) + self.assertTrue( + "stable control checkout" in message + or "author mutation blocked" in message + or "branches/" in message, + msg=message, + ) @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-reconciler"}, clear=True) @patch("mcp_server.get_profile", return_value=RECONCILER_PROFILE) - def test_reconciler_role_bypasses_branches_only_mutation_guard(self, _profile): - # We simulate verify_preflight_purity under reconciler role. - # It should succeed without raising RuntimeError even if CWD/workspace is the process root. + def test_open_unmerged_pr_heads_are_not_safe_cleanup_targets(self, _profile): + """AC5: active/unmerged author work must remain blocked from cleanup.""" mcp_server.record_preflight_check("whoami") - mcp_server.record_preflight_check("capability", resolved_role="reconciler", resolved_task="reconcile_merged_cleanups") - - # Calling verify_preflight_purity directly - try: - mcp_server.verify_preflight_purity(remote="prgs", task="reconcile_merged_cleanups") - except RuntimeError as e: - self.fail(f"verify_preflight_purity raised RuntimeError unexpectedly: {e}") + mcp_server.record_preflight_check( + "capability", + resolved_role="reconciler", + resolved_task="reconcile_merged_cleanups", + ) - @patch.dict(os.environ, {"GITEA_PROFILE_NAME": "prgs-author", "GITEA_TEST_PORCELAIN": ""}, clear=True) - @patch("mcp_server.get_profile", return_value={ - "profile_name": "prgs-author", - "context": "prgs", - "role": "author", - "username": "jcwalker3", - "base_url": "https://gitea.prgs.cc", - "allowed_operations": ["gitea.read", "gitea.pr.create", "gitea.branch.create", "gitea.branch.push", "gitea.repo.commit"], - "forbidden_operations": [], - }) - def test_author_role_remains_blocked_on_root_checkout(self, _profile): - mcp_server.record_preflight_check("whoami") - mcp_server.record_preflight_check("capability", resolved_role="author", resolved_task="reconcile_merged_cleanups") - - with patch("mcp_server.PROJECT_ROOT", "/Users/jasonwalker/Development/Gitea-Tools"): - with self.assertRaises(RuntimeError) as ctx: - mcp_server.verify_preflight_purity(remote="prgs", task="reconcile_merged_cleanups") - self.assertIn("author mutation blocked: workspace is the stable control checkout", str(ctx.exception)) + open_pr = { + "number": 200, + "title": "active work", + "body": "Closes #200", + "merged": False, + "state": "open", + "head": {"ref": "feat/issue-200", "sha": "livehead1"}, + } + # Same head branch still open on another PR while a closed/merged PR + # used the same branch name historically — open head must block delete. + closed_merged = { + "number": 199, + "title": "older merge same branch name", + "body": "Closes #199", + "merged": True, + "merged_at": "2026-07-01T12:00:00Z", + "merge_commit_sha": "deadbeef", + "head": {"ref": "feat/issue-200", "sha": "oldhead99"}, + } + self.mock_api.side_effect = [ + [closed_merged], # closed + [open_pr], # open + ] + + with patch("mcp_server._remote_branch_exists", return_value=True): + with patch( + "mcp_server.merged_cleanup_reconcile.is_head_ancestor_of_ref", + return_value=True, + ): + result = mcp_server.gitea_reconcile_merged_cleanups( + dry_run=True, + remote="prgs", + ) + self.assertTrue(result["success"]) + self.assertEqual(len(result["entries"]), 1) + remote_assessment = result["entries"][0].get("remote_branch") or {} + self.assertFalse( + remote_assessment.get("safe_to_delete_remote"), + msg=f"open head must block remote delete: {remote_assessment}", + ) if __name__ == "__main__":