From 4cc31a42f93e06ce3fe62a9c72ceeff027cdb1e5 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 00:16:42 -0400 Subject: [PATCH 1/4] feat: add merged PR cleanup reconciliation report (#269) Implement gitea_reconcile_merged_cleanups with dry-run reporting for merged PR remote branches and local branches/ worktrees, plus confirmation-gated execute mode. Safety gates cover protected branches, open PR references, active issue locks, dirty worktrees, and delete_branch capability. Closes #269 Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 136 ++++++++++ merged_cleanup_reconcile.py | 333 +++++++++++++++++++++++++ task_capability_map.py | 4 + tests/test_mcp_server.py | 55 ++++ tests/test_merged_cleanup_reconcile.py | 159 ++++++++++++ 5 files changed, 687 insertions(+) create mode 100644 merged_cleanup_reconcile.py create mode 100644 tests/test_merged_cleanup_reconcile.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index dd1480c..021b609 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -374,6 +374,7 @@ import role_namespace_gate # noqa: E402 import task_capability_map # noqa: E402 import review_proofs # noqa: E402 import issue_lock_worktree # noqa: E402 +import merged_cleanup_reconcile # noqa: E402 # Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, @@ -2859,6 +2860,141 @@ def gitea_delete_branch( return {"success": True, "message": f"Remote branch '{branch}' deleted."} +def _remote_branch_exists(h: str, o: str, r: str, auth: str, branch: str) -> bool: + import urllib.parse + + encoded = urllib.parse.quote(branch, safe="") + url = f"{repo_api_url(h, o, r)}/branches/{encoded}" + try: + api_request("GET", url, auth) + return True + except Exception as exc: + message = str(exc).lower() + if "404" in message or "not found" in message: + return False + raise + + +@mcp.tool() +def gitea_reconcile_merged_cleanups( + dry_run: bool = True, + execute_confirmed: bool = False, + limit: int = 50, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Reconcile merged PRs by reporting and optionally cleaning remote branches and local worktrees. + + Args: + dry_run: Defaults to True. When True, only builds the reconciliation report. + execute_confirmed: Must be True when dry_run=False. + limit: Max number of closed PRs to inspect. + remote: Known Gitea instance ('dadeschools' or 'prgs'). + host: Override the Gitea host. + org: Override the owner/organization. + repo: Override the repository name. + + Returns: + dict with reconciliation report and any executed cleanup actions. + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "performed": False, + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + + if not dry_run and not execute_confirmed: + raise ValueError( + "execute_confirmed must be True when dry_run=False (fail closed)" + ) + + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + base = repo_api_url(h, o, r) + closed_prs = api_get_all(f"{base}/pulls?state=closed", auth, limit=limit) + open_prs = api_get_all(f"{base}/pulls?state=open", auth) + + merged_closed: list[dict] = [] + remote_branch_exists: dict[str, bool] = {} + head_on_master: dict[int, bool | None] = {} + master_ref = f"{remote}/master" if remote in REMOTES else "origin/master" + + for pr in closed_prs: + if not (pr.get("merged") or pr.get("merged_at")): + continue + merged_closed.append(pr) + head_ref = (pr.get("head") or {}).get("ref") or "" + head_sha = (pr.get("head") or {}).get("sha") + if head_ref and head_ref not in remote_branch_exists: + remote_branch_exists[head_ref] = _remote_branch_exists( + h, o, r, auth, head_ref + ) + head_on_master[int(pr["number"])] = merged_cleanup_reconcile.is_head_ancestor_of_ref( + PROJECT_ROOT, head_sha, master_ref + ) + + delete_capability_allowed = not _profile_operation_gate("gitea.branch.delete") + report = merged_cleanup_reconcile.build_reconciliation_report( + project_root=PROJECT_ROOT, + closed_prs=merged_closed, + open_prs=open_prs, + remote_branch_exists=remote_branch_exists, + head_on_master=head_on_master, + delete_capability_allowed=delete_capability_allowed, + ) + + if dry_run: + report["dry_run"] = True + report["executed"] = False + return {"success": True, "performed": False, **report} + + verify_preflight_purity(remote) + actions: list[dict] = [] + for entry in report.get("entries") or []: + head_branch = entry.get("head_branch") or "" + remote_assessment = entry.get("remote_branch") or {} + local_assessment = entry.get("local_worktree") or {} + + if remote_assessment.get("safe_to_delete_remote"): + import urllib.parse + + encoded = urllib.parse.quote(head_branch, safe="") + url = f"{base}/branches/{encoded}" + with _audited( + "delete_branch", + host=h, + remote=remote, + org=o, + repo=r, + target_branch=head_branch, + request_metadata={"branch": head_branch, "source": "reconcile_merged_cleanups"}, + ): + api_request("DELETE", url, auth) + actions.append( + { + "action": "delete_remote_branch", + "branch": head_branch, + "success": True, + } + ) + + if local_assessment.get("safe_to_remove_worktree"): + result = merged_cleanup_reconcile.remove_local_worktree( + PROJECT_ROOT, head_branch + ) + actions.append({"action": "remove_local_worktree", **result}) + + report["dry_run"] = False + report["executed"] = True + report["actions"] = actions + return {"success": True, "performed": True, **report} + + @mcp.tool() def gitea_close_issue( issue_number: int, diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py new file mode 100644 index 0000000..60204f6 --- /dev/null +++ b/merged_cleanup_reconcile.py @@ -0,0 +1,333 @@ +"""Merged PR branch and worktree cleanup reconciliation (#269). + +Builds dry-run reports for merged pull requests and optionally executes +remote branch deletion and local worktree removal when every safety gate passes. +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +from typing import Any + +from reviewer_worktree import parse_dirty_tracked_files + +PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) +ISSUE_LOCK_FILE = os.environ.get("GITEA_ISSUE_LOCK_FILE", "/tmp/gitea_issue_lock.json") +CLOSES_FIXES_RE = re.compile(r"\b(?:closes|fixes)\s+#(\d+)\b", re.IGNORECASE) + + +def extract_linked_issue(title: str, body: str) -> int | None: + """Return the first Closes/Fixes issue number from PR metadata.""" + for text in (title or "", body or ""): + match = CLOSES_FIXES_RE.search(text) + if match: + return int(match.group(1)) + return None + + +def branch_worktree_folder(branch: str) -> str: + return (branch or "").replace("/", "-") + + +def resolve_worktree_path(project_root: str, branch: str) -> str: + return os.path.join(project_root, "branches", branch_worktree_folder(branch)) + + +def read_issue_lock(path: str | None = None) -> dict[str, Any] | None: + lock_path = (path or ISSUE_LOCK_FILE).strip() + if not lock_path or not os.path.exists(lock_path): + return None + try: + with open(lock_path, encoding="utf-8") as handle: + data = json.load(handle) + except (OSError, json.JSONDecodeError): + return None + return data if isinstance(data, dict) else None + + +def has_active_issue_lock(branch: str, lock_path: str | None = None) -> bool: + lock = read_issue_lock(lock_path) + if not lock: + return False + return (lock.get("branch_name") or "").strip() == (branch or "").strip() + + +def collect_open_pr_heads(open_prs: list[dict[str, Any]]) -> set[str]: + heads: set[str] = set() + for pr in open_prs or []: + head = pr.get("head") + if isinstance(head, dict): + ref = head.get("ref") + else: + ref = head + if ref: + heads.add(str(ref)) + return heads + + +def read_local_worktree_state(worktree_path: str) -> dict[str, Any]: + path = (worktree_path or "").strip() + if not path or not os.path.isdir(path): + return { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + } + + branch_res = subprocess.run( + ["git", "-C", path, "branch", "--show-current"], + capture_output=True, + text=True, + check=False, + ) + current_branch = (branch_res.stdout or "").strip() or None + + status_res = subprocess.run( + ["git", "-C", path, "status", "--porcelain"], + capture_output=True, + text=True, + check=False, + ) + dirty_files = parse_dirty_tracked_files(status_res.stdout or "") + + head_res = subprocess.run( + ["git", "-C", path, "rev-parse", "HEAD"], + capture_output=True, + text=True, + check=False, + ) + head_sha = (head_res.stdout or "").strip() if head_res.returncode == 0 else None + + return { + "exists": True, + "clean": not dirty_files, + "current_branch": current_branch, + "head_sha": head_sha, + "dirty_files": dirty_files, + } + + +def is_head_ancestor_of_ref(project_root: str, head_sha: str | None, base_ref: str) -> bool | None: + if not head_sha: + return None + res = subprocess.run( + ["git", "-C", project_root, "merge-base", "--is-ancestor", head_sha, base_ref], + capture_output=True, + text=True, + check=False, + ) + if res.returncode == 0: + return True + if res.returncode == 1: + return False + return None + + +def assess_remote_branch_cleanup( + *, + pr_number: int, + head_branch: str, + merged: bool, + remote_branch_exists: bool, + open_pr_heads: set[str], + protected_branches: frozenset[str] | None = None, + head_on_master: bool | None, + delete_capability_allowed: bool, + active_lock: bool, +) -> dict[str, Any]: + protected = protected_branches or PROTECTED_BRANCHES + reasons: list[str] = [] + if not merged: + reasons.append("PR is not merged") + if not remote_branch_exists: + reasons.append("remote branch already absent") + if head_branch in protected: + reasons.append(f"branch '{head_branch}' is protected") + if head_branch in open_pr_heads: + reasons.append("an open PR still references this head branch") + if active_lock: + reasons.append("active issue lock references this branch") + if head_on_master is False: + reasons.append("PR head is not an ancestor of master") + if not delete_capability_allowed: + reasons.append("delete_branch capability is not allowed in the active profile") + + safe = merged and remote_branch_exists and not reasons + return { + "pr_number": pr_number, + "head_branch": head_branch, + "remote_branch_exists": remote_branch_exists, + "safe_to_delete_remote": safe, + "block_reasons": reasons, + "recommended_action": "delete_remote_branch" if safe else "keep_remote_branch", + } + + +def assess_local_worktree_cleanup( + *, + pr_number: int, + head_branch: str, + merged: bool, + worktree_state: dict[str, Any], + active_lock: bool, +) -> dict[str, Any]: + reasons: list[str] = [] + exists = bool(worktree_state.get("exists")) + if not merged: + reasons.append("PR is not merged") + if not exists: + reasons.append("local worktree not present") + if exists and worktree_state.get("clean") is False: + dirty = worktree_state.get("dirty_files") or [] + reasons.append( + "local worktree has tracked edits" + + (f" ({', '.join(dirty)})" if dirty else "") + ) + if exists: + current_branch = worktree_state.get("current_branch") + if current_branch and current_branch != head_branch: + reasons.append( + f"worktree branch '{current_branch}' does not match PR head '{head_branch}'" + ) + if active_lock: + reasons.append("active issue lock references this branch") + + safe = merged and exists and not reasons + return { + "pr_number": pr_number, + "head_branch": head_branch, + "worktree_path": worktree_state.get("worktree_path"), + "worktree_exists": exists, + "worktree_clean": worktree_state.get("clean"), + "safe_to_remove_worktree": safe, + "block_reasons": reasons, + "recommended_action": "remove_local_worktree" if safe else "keep_local_worktree", + } + + +def build_pr_cleanup_entry( + *, + pr: dict[str, Any], + project_root: str, + open_pr_heads: set[str], + remote_branch_exists: bool, + head_on_master: bool | None, + delete_capability_allowed: bool, + issue_lock_path: str | None = None, + protected_branches: frozenset[str] | None = None, +) -> dict[str, Any]: + pr_number = int(pr["number"]) + head_branch = pr.get("head") or "" + if isinstance(head_branch, dict): + head_branch = head_branch.get("ref") or "" + 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) + 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) + + remote = assess_remote_branch_cleanup( + pr_number=pr_number, + head_branch=head_branch, + merged=merged, + remote_branch_exists=remote_branch_exists, + open_pr_heads=open_pr_heads, + protected_branches=protected_branches, + head_on_master=head_on_master, + delete_capability_allowed=delete_capability_allowed, + active_lock=active_lock, + ) + local = assess_local_worktree_cleanup( + pr_number=pr_number, + head_branch=head_branch, + merged=merged, + worktree_state=worktree_state, + active_lock=active_lock, + ) + return { + "pr_number": pr_number, + "issue_number": extract_linked_issue(title, body), + "title": title, + "head_branch": head_branch, + "merge_commit_sha": pr.get("merge_commit_sha"), + "merged_at": pr.get("merged_at"), + "merged": merged, + "remote_branch": remote, + "local_worktree": local, + } + + +def build_reconciliation_report( + *, + project_root: str, + closed_prs: list[dict[str, Any]], + open_prs: list[dict[str, Any]], + remote_branch_exists: dict[str, bool], + head_on_master: dict[int, bool | None], + delete_capability_allowed: bool, + issue_lock_path: str | None = None, + protected_branches: frozenset[str] | None = None, +) -> dict[str, Any]: + open_heads = collect_open_pr_heads(open_prs) + entries: list[dict[str, Any]] = [] + for pr in closed_prs or []: + if not pr.get("merged_at"): + continue + pr_number = int(pr["number"]) + head = pr.get("head") or "" + if isinstance(head, dict): + head = head.get("ref") or "" + entries.append( + build_pr_cleanup_entry( + pr=pr, + project_root=project_root, + open_pr_heads=open_heads, + remote_branch_exists=bool(remote_branch_exists.get(head)), + head_on_master=head_on_master.get(pr_number), + delete_capability_allowed=delete_capability_allowed, + issue_lock_path=issue_lock_path, + protected_branches=protected_branches, + ) + ) + return { + "project_root": os.path.realpath(project_root), + "merged_pr_count": len(entries), + "entries": entries, + "dry_run": True, + "executed": False, + } + + +def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: + worktree_path = resolve_worktree_path(project_root, branch) + if not os.path.isdir(worktree_path): + return { + "success": False, + "performed": False, + "message": f"worktree not found: {worktree_path}", + } + res = subprocess.run( + ["git", "-C", project_root, "worktree", "remove", worktree_path], + capture_output=True, + text=True, + check=False, + ) + if res.returncode != 0: + return { + "success": False, + "performed": False, + "message": (res.stderr or res.stdout or "worktree remove failed").strip(), + } + return { + "success": True, + "performed": True, + "message": f"removed worktree {worktree_path}", + "worktree_path": worktree_path, + } \ No newline at end of file diff --git a/task_capability_map.py b/task_capability_map.py index fd3c344..7607170 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -80,6 +80,10 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.branch.delete", "role": "author", }, + "reconcile_merged_cleanups": { + "permission": "gitea.read", + "role": "author", + }, } # Issue-mutating MCP tools and their resolver task keys. diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 133e515..6b2827f 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -24,6 +24,7 @@ from mcp_server import ( # noqa: E402 gitea_merge_pr, gitea_review_pr, gitea_delete_branch, + gitea_reconcile_merged_cleanups, gitea_edit_pr, gitea_get_file, gitea_commit_files, @@ -990,6 +991,60 @@ class TestDeleteBranch(unittest.TestCase): self.assertIn("feat%2Fbranch", url) +# --------------------------------------------------------------------------- +# Reconcile merged cleanups (#269) +# --------------------------------------------------------------------------- +READ_ENV = { + "GITEA_ALLOWED_OPERATIONS": "gitea.read", +} + + +class TestReconcileMergedCleanups(unittest.TestCase): + + @patch("mcp_server.api_get_all") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_dry_run_builds_report_without_mutations(self, _auth, mock_api_get_all): + mock_api_get_all.side_effect = [ + [ + { + "number": 50, + "title": "merged feature", + "body": "Closes #50", + "merged": True, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "deadbeef", + "head": {"ref": "feat/issue-50-example", "sha": "cafebabe"}, + } + ], + [], + ] + with patch.dict(os.environ, READ_ENV, clear=True): + 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 = gitea_reconcile_merged_cleanups( + dry_run=True, + remote="prgs", + limit=10, + ) + self.assertTrue(result["success"]) + self.assertTrue(result["dry_run"]) + self.assertFalse(result["executed"]) + self.assertEqual(result["merged_pr_count"], 1) + self.assertEqual(result["entries"][0]["issue_number"], 50) + + def test_execute_requires_confirmation(self): + with patch.dict(os.environ, READ_ENV, clear=True): + with self.assertRaises(ValueError) as ctx: + gitea_reconcile_merged_cleanups(dry_run=False, execute_confirmed=False) + self.assertIn("execute_confirmed", str(ctx.exception)) + + # --------------------------------------------------------------------------- # Edit PR # --------------------------------------------------------------------------- diff --git a/tests/test_merged_cleanup_reconcile.py b/tests/test_merged_cleanup_reconcile.py new file mode 100644 index 0000000..779df64 --- /dev/null +++ b/tests/test_merged_cleanup_reconcile.py @@ -0,0 +1,159 @@ +"""Tests for merged PR cleanup reconciliation (#269).""" + +import os +import sys +import tempfile +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import merged_cleanup_reconcile as mcr # noqa: E402 + + +class TestMergedCleanupAssessment(unittest.TestCase): + def test_extract_linked_issue_from_closes(self): + issue = mcr.extract_linked_issue( + "feat: cleanup (Closes #269)", + "No other refs", + ) + self.assertEqual(issue, 269) + + def test_merged_remote_branch_safe_when_gates_pass(self): + result = mcr.assess_remote_branch_cleanup( + pr_number=42, + head_branch="feat/issue-269-merged-pr-cleanup-reconcile", + merged=True, + remote_branch_exists=True, + open_pr_heads=set(), + head_on_master=True, + delete_capability_allowed=True, + active_lock=False, + ) + self.assertTrue(result["safe_to_delete_remote"]) + self.assertEqual(result["block_reasons"], []) + + def test_open_pr_blocks_remote_delete(self): + result = mcr.assess_remote_branch_cleanup( + pr_number=42, + head_branch="feat/issue-9-example", + merged=True, + remote_branch_exists=True, + open_pr_heads={"feat/issue-9-example"}, + head_on_master=True, + delete_capability_allowed=True, + active_lock=False, + ) + self.assertFalse(result["safe_to_delete_remote"]) + self.assertIn("open PR", result["block_reasons"][0]) + + def test_protected_branch_blocks_remote_delete(self): + result = mcr.assess_remote_branch_cleanup( + pr_number=1, + head_branch="master", + merged=True, + remote_branch_exists=True, + open_pr_heads=set(), + head_on_master=True, + delete_capability_allowed=True, + active_lock=False, + ) + self.assertFalse(result["safe_to_delete_remote"]) + self.assertIn("protected", result["block_reasons"][0]) + + def test_active_lock_blocks_remote_and_worktree_cleanup(self): + remote = mcr.assess_remote_branch_cleanup( + pr_number=42, + head_branch="feat/issue-9-example", + merged=True, + remote_branch_exists=True, + open_pr_heads=set(), + head_on_master=True, + delete_capability_allowed=True, + active_lock=True, + ) + local = mcr.assess_local_worktree_cleanup( + pr_number=42, + head_branch="feat/issue-9-example", + merged=True, + worktree_state={ + "exists": True, + "clean": True, + "current_branch": "feat/issue-9-example", + "worktree_path": "/repo/branches/feat-issue-9-example", + }, + active_lock=True, + ) + self.assertFalse(remote["safe_to_delete_remote"]) + self.assertFalse(local["safe_to_remove_worktree"]) + self.assertIn("active issue lock", remote["block_reasons"][0]) + + def test_dirty_worktree_blocks_local_cleanup(self): + result = mcr.assess_local_worktree_cleanup( + pr_number=42, + head_branch="feat/issue-9-example", + merged=True, + worktree_state={ + "exists": True, + "clean": False, + "dirty_files": ["gitea_mcp_server.py"], + "current_branch": "feat/issue-9-example", + "worktree_path": "/repo/branches/feat-issue-9-example", + }, + active_lock=False, + ) + self.assertFalse(result["safe_to_remove_worktree"]) + self.assertIn("tracked edits", result["block_reasons"][0]) + + +class TestMergedCleanupReport(unittest.TestCase): + def test_build_report_skips_unmerged_closed_prs(self): + with tempfile.TemporaryDirectory() as tmp: + report = mcr.build_reconciliation_report( + project_root=tmp, + closed_prs=[ + { + "number": 10, + "title": "closed not merged", + "body": "Closes #10", + "head": {"ref": "feat/issue-10-x"}, + "merged_at": None, + }, + { + "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=False, + ) + self.assertEqual(report["merged_pr_count"], 1) + entry = report["entries"][0] + self.assertEqual(entry["pr_number"], 11) + self.assertEqual(entry["issue_number"], 11) + self.assertFalse(entry["remote_branch"]["safe_to_delete_remote"]) + self.assertIn("delete_branch capability", entry["remote_branch"]["block_reasons"][0]) + + def test_has_active_issue_lock_reads_lock_file(self): + with tempfile.NamedTemporaryFile("w", encoding="utf-8", delete=False) as handle: + handle.write('{"branch_name": "feat/issue-9-example"}') + lock_path = handle.name + try: + self.assertTrue( + mcr.has_active_issue_lock("feat/issue-9-example", lock_path=lock_path) + ) + self.assertFalse( + mcr.has_active_issue_lock("feat/other-branch", lock_path=lock_path) + ) + finally: + os.remove(lock_path) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 82e6d3a3f476146241594ab42f0814c73eb2c004 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 00:17:26 -0400 Subject: [PATCH 2/4] docs: add subagent tool-budget guardrails for MCP tasks (Closes #259) Document default tool-call and wall-time budgets for deterministic MCP work, forbid subagent commit delegation when gitea_commit_files is available, and pin the rules in runbooks, portable skill, operator guide, and doc-contract tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/llm-workflow-runbooks.md | 33 +++++++++ gitea_mcp_server.py | 10 +++ skills/llm-project-workflow/SKILL.md | 25 +++++++ tests/test_operator_guide.py | 3 +- tests/test_subagent_tool_budget_docs.py | 89 +++++++++++++++++++++++++ 5 files changed, 159 insertions(+), 1 deletion(-) create mode 100644 tests/test_subagent_tool_budget_docs.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 1b04cb3..0b84151 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -285,6 +285,39 @@ explicit control-checkout repair. Portable wording: [`skills/llm-project-workflow/SKILL.md`](../skills/llm-project-workflow/SKILL.md). +## Subagent Tool-Budget Guardrails + +General-purpose subagents tasked with **deterministic MCP work** (for example a +single `gitea_commit_files` call) have expanded to 100–122 tool calls, +WebFetch/Playwright fallbacks, and throwaway helper-script generation instead of +calling the native MCP tool once (observed during #152 closure, issue #259). + +**Default budgets** (fail closed when exceeded): + +| Task class | Max tool calls | Max wall time | +|------------|----------------|---------------| +| Single-step MCP mutation (`commit_files`, `create_pr`, `lock_issue`) | 15 | 5 minutes | +| Review / merge queue inspection | 40 | 15 minutes | +| Exploration / codebase search (non-mutating) | 60 | 20 minutes | + +**Required behavior:** + +1. **Main session first.** When the active author profile allows + `gitea.repo.commit` and `gitea_commit_files` is visible, the main session + must call it directly — do not delegate commit authority to a subagent + (see #260). +2. **Native MCP before fallback.** After a shell spawn failure (#258), attempt + the native MCP tool once before any alternate path. Shell unavailability + never authorizes WebFetch, Playwright, or manual base64 encoding. +3. **No retry spirals.** Never resume a failed subagent into a larger retry + loop or spawn a second subagent for the same deterministic step. Stop and + emit a recovery report instead. +4. **Forbidden detours** when `gitea_commit_files` is available: WebFetch, + Playwright/browser automation, manual LLM-generated base64, and ad-hoc + `_encode_*` / `_emit_*` helper scripts left in the repo. + +Doc-contract tests: `tests/test_subagent_tool_budget_docs.py`. + ## Branch worktree isolation All LLM implementation and review work happens in an isolated branch worktree diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index dd1480c..f25fa86 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -3483,6 +3483,16 @@ _GUIDE_RULES = { "small fixes, review fixes, conflicts, emergencies). Main checkout: " "read-only inspect, fetch, create worktrees, post-merge stable " "update, explicit repair only."), + "subagent_tool_budget": ( + "General-purpose subagents on deterministic MCP tasks must stay within " + "budget: single-step mutations (commit_files, create_pr, lock_issue) " + "max 15 tool calls / 5 min; review/merge queue inspection max 40 / " + "15 min. The main session with gitea.repo.commit must call " + "gitea_commit_files directly — no subagent delegation (#260). After " + "shell spawn failure (#258), attempt native MCP once; never authorize " + "WebFetch/Playwright/manual base64 when gitea_commit_files is " + "available. Never resume a failed subagent into a larger retry loop " + "(#259)."), } _COMMON_WORKFLOWS = [ diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index cfb723a..674c87d 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -115,6 +115,31 @@ The main checkout may only be used for read-only inspection, fetching, stable-branch update after merged PRs, creating `branches/` worktrees, or explicit control-checkout repair. +## Subagent Tool-Budget Guardrails + +General-purpose subagents on **single-step MCP tasks** (for example +`gitea_commit_files`) must not expand into 100+ tool-call retry spirals with +WebFetch/Playwright/manual-encoding fallbacks (issue #259). + +Default budgets (stop when exceeded): + +- **Single-step MCP mutation** (`commit_files`, `create_pr`, `lock_issue`): + 15 tool calls, 5 minutes wall time. +- **Review / merge queue inspection**: 40 tool calls, 15 minutes. +- **Non-mutating exploration**: 60 tool calls, 20 minutes. + +Rules: + +1. When the main session has `gitea.repo.commit`, call `gitea_commit_files` + directly — do not delegate commit to a subagent (#260). +2. After shell spawn failure (#258), attempt the native MCP tool once before + any fallback; shell unavailability never authorizes WebFetch/Playwright/ + manual base64. +3. Never resume a failed subagent into a larger retry loop or spawn a second + subagent for the same deterministic step — stop and report. +4. When `gitea_commit_files` is available, forbid WebFetch, Playwright, + manual encoding, and ad-hoc `_encode_*` / `_emit_*` helpers in the repo. + ## B. Isolated worktree rule **Never implement or review in the main checkout** (Global LLM Worktree Rule). diff --git a/tests/test_operator_guide.py b/tests/test_operator_guide.py index 602160a..2a26416 100644 --- a/tests/test_operator_guide.py +++ b/tests/test_operator_guide.py @@ -133,7 +133,8 @@ class TestControlPlaneGuide(GuideTestBase): for key in ("hard_stops", "fail_closed", "head_sha_pinning", "merge_confirmation", "redaction", "separation", "profile_switching", "identity_verification", - "work_selection", "global_worktree"): + "work_selection", "global_worktree", + "subagent_tool_budget"): self.assertIn(key, rules) self.assertIn("MERGE PR", json.dumps(rules["merge_confirmation"])) self.assertTrue(rules["hard_stops"]) diff --git a/tests/test_subagent_tool_budget_docs.py b/tests/test_subagent_tool_budget_docs.py new file mode 100644 index 0000000..8b603ba --- /dev/null +++ b/tests/test_subagent_tool_budget_docs.py @@ -0,0 +1,89 @@ +"""Doc-contract checks for subagent tool-budget guardrails (#259). + +Single-step MCP tasks must not expand into 100+ tool-call retry spirals with +WebFetch/Playwright/manual-encoding fallbacks. These tests pin budget defaults, +native-MCP-first rules, and forbidden detours in the runbooks, portable skill, +and operator guide. +""" +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +RUNBOOK = REPO_ROOT / "docs" / "llm-workflow-runbooks.md" +SKILL = REPO_ROOT / "skills" / "llm-project-workflow" / "SKILL.md" + + +def _normalize(text: str) -> str: + """Collapse whitespace so phrase checks survive markdown line wrapping.""" + return " ".join(text.split()) + + +def _runbook_text() -> str: + return _normalize(RUNBOOK.read_text(encoding="utf-8")) + + +def _skill_text() -> str: + return _normalize(SKILL.read_text(encoding="utf-8")) + + +def test_runbook_has_subagent_tool_budget_section(): + text = _runbook_text() + assert "## Subagent Tool-Budget Guardrails" in text + for phrase in ( + "15", + "5 minutes", + "40", + "15 minutes", + "gitea_commit_files", + "WebFetch", + "Playwright", + "no retry spirals", + ): + assert phrase.lower() in text.lower(), f"runbook missing phrase: {phrase!r}" + + +def test_runbook_forbids_subagent_commit_delegation(): + text = _runbook_text() + for phrase in ( + "do not delegate commit authority to a subagent", + "main session first", + "native MCP before fallback", + ): + assert phrase.lower() in text.lower(), f"runbook missing: {phrase!r}" + + +def test_runbook_rejects_fallback_detours_when_mcp_commit_available(): + lower = _runbook_text().lower() + for forbidden in ("webfetch", "playwright", "manual llm-generated base64"): + assert forbidden in lower, f"runbook must name forbidden detour: {forbidden!r}" + assert "_encode_" in lower + assert "gitea_commit_files" in lower + + +def test_skill_doc_declares_subagent_tool_budget_guardrails(): + text = _skill_text() + assert "## Subagent Tool-Budget Guardrails" in text + for phrase in ( + "15 tool calls", + "5 minutes", + "gitea_commit_files", + "WebFetch", + "never resume a failed subagent", + ): + assert phrase.lower() in text.lower(), f"SKILL.md missing phrase: {phrase!r}" + + +def test_operator_guide_rules_include_subagent_tool_budget(): + import sys + + sys.path.insert(0, str(REPO_ROOT)) + import gitea_mcp_server + + rule = gitea_mcp_server._GUIDE_RULES["subagent_tool_budget"] + for phrase in ( + "15 tool calls", + "gitea_commit_files", + "WebFetch", + "retry loop", + ): + assert phrase in rule, f"operator guide rule missing: {phrase!r}" \ No newline at end of file From 5af1b796ef8406e6d4a6d64051f3bb46b28799d1 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 00:21:18 -0400 Subject: [PATCH 3/4] fix: recompute infra_stop live on every capability resolve (#285) Add assess_infra_stop() to scan git state and conflict markers on each reviewer capability check, return the exact conflict path in responses, and clear stale capability-stop terminal state once the worktree is clean. Closes #285 Co-Authored-By: Claude Opus 4.8 (1M context) --- gitea_mcp_server.py | 58 +++++++++++++++++------------ role_session_router.py | 84 +++++++++++++++++++++++++++++------------- tests/test_health.py | 83 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 172 insertions(+), 53 deletions(-) diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index dd1480c..a9e4d81 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -4905,30 +4905,40 @@ def gitea_resolve_task_capability( required_permission = task_capability_map.required_permission(task) required_role = task_capability_map.required_role(task) - if required_role == "reviewer" and role_session_router.check_mid_merge(): - profile = get_profile() - h = host or (REMOTES.get(remote, {}).get("host") if remote in REMOTES else None) - username = _authenticated_username(h) if h else None - next_safe_action = ( - "infra_stop: Unresolved merge conflict or mid-merge state detected in MCP runtime source. " - "Please resolve all conflicts manually, finish/abort the merge, restart the MCP server, and retry." - ) - return { - "requested_task": task, - "required_operation_permission": required_permission, - "required_role_kind": required_role, - "active_profile": profile["profile_name"], - "active_identity": username, - "active_profile_allowed_operations": profile.get("allowed_operations") or [], - "allowed_in_current_session": False, - "stop_required": True, - "infra_stop": True, - "task_role_guidance": [next_safe_action], - "matching_configured_profile": [], - "runtime_switching_supported": gitea_config.is_runtime_switching_enabled(), - "different_mcp_namespace_required": False, - "exact_safe_next_action": next_safe_action, - } + infra_assessment = role_session_router.assess_infra_stop(PROJECT_ROOT) + if required_role == "reviewer": + if not infra_assessment["infra_stop"]: + capability_stop_terminal.clear() + last_route = role_session_router.last_route() + if last_route and last_route.get("route_result") == role_session_router.ROUTE_INFRA_STOP: + role_session_router.clear_route_state() + elif infra_assessment["infra_stop"]: + profile = get_profile() + h = host or (REMOTES.get(remote, {}).get("host") if remote in REMOTES else None) + username = _authenticated_username(h) if h else None + detail = "; ".join(infra_assessment.get("infra_stop_reasons") or []) + next_safe_action = ( + "infra_stop: Unresolved merge conflict or mid-merge state detected in " + f"MCP runtime source ({detail}). Please resolve all conflicts manually, " + "finish/abort the merge, and retry." + ) + return { + "requested_task": task, + "required_operation_permission": required_permission, + "required_role_kind": required_role, + "active_profile": profile["profile_name"], + "active_identity": username, + "active_profile_allowed_operations": profile.get("allowed_operations") or [], + "allowed_in_current_session": False, + "stop_required": True, + "infra_stop": True, + "infra_stop_assessment": infra_assessment, + "task_role_guidance": [next_safe_action], + "matching_configured_profile": [], + "runtime_switching_supported": gitea_config.is_runtime_switching_enabled(), + "different_mcp_namespace_required": False, + "exact_safe_next_action": next_safe_action, + } record_preflight_check("capability", required_role) diff --git a/role_session_router.py b/role_session_router.py index 742a961..a7db807 100644 --- a/role_session_router.py +++ b/role_session_router.py @@ -134,22 +134,28 @@ def route_task_session( _record_route(result) return result - if required_role == "reviewer" and check_mid_merge(): - result = { - "task_type": task_type, - "required_role": required_role, - "active_role": active_role_kind, - "active_profile": active_profile, - "route_result": ROUTE_INFRA_STOP, - "downstream_allowed": False, - "reasons": [ - "infra_stop: Unresolved merge conflict or mid-merge state detected in MCP runtime source.", - "Please resolve all conflicts manually, finish/abort the merge, restart the MCP server, and retry." - ], - "message": "infra_stop: Unresolved merge conflict or mid-merge state detected in MCP runtime source.", - } - _record_route(result) - return result + if required_role == "reviewer": + infra = assess_infra_stop() + if infra["infra_stop"]: + detail = "; ".join(infra.get("infra_stop_reasons") or []) + message = ( + "infra_stop: Unresolved merge conflict or mid-merge state detected " + f"in MCP runtime source ({detail}). Please resolve all conflicts " + "manually, finish/abort the merge, and retry." + ) + result = { + "task_type": task_type, + "required_role": required_role, + "active_role": active_role_kind, + "active_profile": active_profile, + "route_result": ROUTE_INFRA_STOP, + "downstream_allowed": False, + "reasons": [message], + "message": message, + "infra_stop_assessment": infra, + } + _record_route(result) + return result if allowed_in_current_session: result = { @@ -317,14 +323,40 @@ def first_conflict_marker_path(project_root: str | None = None) -> str | None: return None -def check_mid_merge() -> bool: - """Return True if the repository is mid-merge, mid-rebase, or has conflict markers.""" - project_root = os.path.dirname(os.path.abspath(__file__)) - git_dir = os.path.join(project_root, ".git") - if os.path.exists(git_dir): - if (os.path.exists(os.path.join(git_dir, "MERGE_HEAD")) - or os.path.exists(os.path.join(git_dir, "rebase-merge")) - or os.path.exists(os.path.join(git_dir, "rebase-apply"))): - return True +def _default_project_root() -> str: + override = (os.environ.get("GITEA_MCP_PROJECT_ROOT") or "").strip() + if override: + return os.path.realpath(override) + return os.path.dirname(os.path.abspath(__file__)) - return first_conflict_marker_path(project_root) is not None \ No newline at end of file + +def assess_infra_stop(project_root: str | None = None) -> dict: + """Recompute infra_stop from live git and source scan state (#285).""" + root_dir = os.path.realpath(project_root or _default_project_root()) + reasons: list[str] = [] + mid_merge = False + git_dir = os.path.join(root_dir, ".git") + if os.path.isdir(git_dir): + for marker in ("MERGE_HEAD", "rebase-merge", "rebase-apply"): + marker_path = os.path.join(git_dir, marker) + if os.path.exists(marker_path): + mid_merge = True + reasons.append(f"git state: {marker} present under {git_dir}") + conflict_file = first_conflict_marker_path(root_dir) + if conflict_file: + reasons.append( + f"conflict markers detected in {conflict_file} (project_root={root_dir})" + ) + infra_stop = mid_merge or bool(conflict_file) + return { + "infra_stop": infra_stop, + "project_root": root_dir, + "conflict_file": conflict_file, + "mid_merge": mid_merge, + "infra_stop_reasons": reasons, + } + + +def check_mid_merge(project_root: str | None = None) -> bool: + """Return True if the repository is mid-merge, mid-rebase, or has conflict markers.""" + return assess_infra_stop(project_root)["infra_stop"] \ No newline at end of file diff --git a/tests/test_health.py b/tests/test_health.py index c177ddd..3294e4e 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -1,9 +1,12 @@ import os +import shutil import subprocess import sys +import tempfile import unittest from unittest.mock import patch +import capability_stop_terminal import role_session_router from role_session_router import python_bytes_have_conflict_markers from mcp_server import gitea_route_task_session, gitea_resolve_task_capability @@ -130,7 +133,7 @@ class TestMCPHealth(unittest.TestCase): self.assertEqual(res["route_result"], "infra_stop") self.assertIn("infra_stop", res["message"]) - @patch("role_session_router.check_mid_merge", return_value=True) + @patch("role_session_router.assess_infra_stop") @patch( "mcp_server.get_profile", return_value={ @@ -139,9 +142,83 @@ class TestMCPHealth(unittest.TestCase): }, ) def test_resolve_task_capability_blocks_during_merge( - self, mock_profile, mock_check + self, mock_profile, mock_assess ): + mock_assess.return_value = { + "infra_stop": True, + "project_root": "/repo", + "conflict_file": "/repo/gitea_mcp_server.py", + "mid_merge": False, + "infra_stop_reasons": [ + "conflict markers detected in /repo/gitea_mcp_server.py (project_root=/repo)" + ], + } res = gitea_resolve_task_capability("review_pr") self.assertTrue(res["infra_stop"]) self.assertFalse(res["allowed_in_current_session"]) - self.assertIn("infra_stop", res["exact_safe_next_action"]) \ No newline at end of file + self.assertIn("infra_stop", res["exact_safe_next_action"]) + self.assertEqual( + res["infra_stop_assessment"]["conflict_file"], + "/repo/gitea_mcp_server.py", + ) + + @patch("role_session_router.assess_infra_stop") + @patch("mcp_server._authenticated_username", return_value="reviewer-user") + @patch( + "mcp_server.get_profile", + return_value={ + "profile_name": "prgs-reviewer", + "allowed_operations": ["gitea.pr.review", "gitea.pr.approve"], + }, + ) + def test_resolve_task_capability_clears_stale_terminal_when_infra_clean( + self, mock_profile, _username, mock_assess + ): + mock_assess.return_value = { + "infra_stop": False, + "project_root": "/repo", + "conflict_file": None, + "mid_merge": False, + "infra_stop_reasons": [], + } + capability_stop_terminal.enter_from_capability_result({ + "requested_task": "review_pr", + "required_role_kind": "reviewer", + "stop_required": True, + "active_profile": "prgs-reviewer", + "active_identity": "reviewer-user", + "exact_safe_next_action": "blocked", + }) + self.assertTrue(capability_stop_terminal.is_active()) + res = gitea_resolve_task_capability("review_pr", remote="prgs") + self.assertFalse(capability_stop_terminal.is_active()) + self.assertTrue(res["allowed_in_current_session"]) + + +class TestInfraStopLiveAssessment(unittest.TestCase): + def setUp(self): + self.tempdir = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.tempdir, ignore_errors=True) + + def test_clean_project_root_is_not_infra_stop(self): + infra = role_session_router.assess_infra_stop(self.tempdir) + self.assertFalse(infra["infra_stop"]) + self.assertIsNone(infra["conflict_file"]) + + def test_conflict_present_then_resolved_recomputes_allowed(self): + conflict_path = os.path.join(self.tempdir, "conflicted.py") + with open(conflict_path, "wb") as handle: + handle.write(b"<<<<<<< HEAD\n") + blocked = role_session_router.assess_infra_stop(self.tempdir) + self.assertTrue(blocked["infra_stop"]) + self.assertEqual( + os.path.realpath(blocked["conflict_file"]), + os.path.realpath(conflict_path), + ) + + os.remove(conflict_path) + cleared = role_session_router.assess_infra_stop(self.tempdir) + self.assertFalse(cleared["infra_stop"]) + self.assertIsNone(cleared["conflict_file"]) \ No newline at end of file From e1fbf0112af7cbd7732cf81e40364b710de002fe Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 04:35:26 -0400 Subject: [PATCH 4/4] test: mock assess_infra_stop in route_task_session merge blocker test route_task_session now calls assess_infra_stop() live instead of check_mid_merge(). Update TestMCPHealth.test_route_task_session_blocks_during_merge to mock the infra-stop assessment path and assert infra_stop_assessment is returned when blocked. Part of #285 / PR #286 review fix. --- tests/test_health.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/test_health.py b/tests/test_health.py index 3294e4e..5e92f70 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -120,7 +120,7 @@ class TestMCPHealth(unittest.TestCase): stderr_text, ) - @patch("role_session_router.check_mid_merge", return_value=True) + @patch("role_session_router.assess_infra_stop") @patch( "mcp_server.get_profile", return_value={ @@ -128,10 +128,21 @@ class TestMCPHealth(unittest.TestCase): "allowed_operations": ["gitea.pr.review"], }, ) - def test_route_task_session_blocks_during_merge(self, mock_profile, mock_check): + def test_route_task_session_blocks_during_merge(self, mock_profile, mock_assess): + mock_assess.return_value = { + "infra_stop": True, + "project_root": "/repo", + "conflict_file": None, + "mid_merge": True, + "infra_stop_reasons": [ + "mid-merge markers detected in /repo/.git/MERGE_HEAD (project_root=/repo)" + ], + } res = gitea_route_task_session("review_pr") self.assertEqual(res["route_result"], "infra_stop") self.assertIn("infra_stop", res["message"]) + self.assertTrue(res.get("infra_stop_assessment", {}).get("infra_stop")) + mock_assess.assert_called() @patch("role_session_router.assess_infra_stop") @patch(