From 0e701d578a5b5953edc3faba6a9a35a3daaa7e94 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 15:20:47 -0400 Subject: [PATCH 01/12] feat: add final-report audit paste tool to web UI (Closes #431) Expose /audit and /api/audit for local validator previews using final_report_validator and review schema checks. Operators can paste reports, auto-detect task kind, and get structured findings with suggested next prompts and issue-comment drafts. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/webui-local-dev.md | 18 +++- tests/test_webui_audit.py | 94 +++++++++++++++++++ tests/test_webui_skeleton.py | 13 ++- webui/app.py | 55 ++++++++++- webui/audit_validator.py | 176 +++++++++++++++++++++++++++++++++++ webui/audit_views.py | 139 +++++++++++++++++++++++++++ 6 files changed, 481 insertions(+), 14 deletions(-) create mode 100644 tests/test_webui_audit.py create mode 100644 webui/audit_validator.py create mode 100644 webui/audit_views.py diff --git a/docs/webui-local-dev.md b/docs/webui-local-dev.md index 0a5b935..6ef5750 100644 --- a/docs/webui-local-dev.md +++ b/docs/webui-local-dev.md @@ -44,12 +44,22 @@ Optional environment variables: | `/prompts` | Prompt library with per-prompt copy buttons (#428) | | `/api/prompts` | JSON prompt export with workflow hashes | | `/runtime` | Stub — MCP runtime health (#430) | -| `/audit` | Stub — report audit paste (#431) | +| `/audit` | Report audit paste + validator preview (#431) | +| `/api/audit` | JSON validator preview (POST `report_text`, optional `task_kind`) | | `/worktrees` | Stub — hygiene dashboard (#432) | | `/leases` | Stub — lease visibility (#433) | -All routes are GET-only. POST/PUT/PATCH/DELETE return `405` with -`read-only-mvp`. +Most routes are GET-only. POST/PUT/PATCH/DELETE return `405` with +`read-only-mvp`, except `/audit` and `/api/audit` which accept POST for +local validator preview only (no Gitea mutations, no server-side storage). + +## Report audit (#431) + +Paste an LLM final report at `/audit` or POST JSON to `/api/audit`. The UI +reuses `final_report_validator` and review schema checks to surface missing +proof fields, wrong validation vocabulary, mutation contradictions, and a +suggested next prompt or issue-comment draft. Task kind can be auto-detected +or selected explicitly. ## Project registry (#427) @@ -85,5 +95,5 @@ instead of an empty queue (fail closed). ## Tests ```bash -pytest tests/test_webui_skeleton.py tests/test_webui_project_registry.py tests/test_webui_prompt_library.py tests/test_webui_queue_dashboard.py -q +pytest tests/test_webui_skeleton.py tests/test_webui_project_registry.py tests/test_webui_prompt_library.py tests/test_webui_queue_dashboard.py tests/test_webui_audit.py -q ``` \ No newline at end of file diff --git a/tests/test_webui_audit.py b/tests/test_webui_audit.py new file mode 100644 index 0000000..51f8cd9 --- /dev/null +++ b/tests/test_webui_audit.py @@ -0,0 +1,94 @@ +"""Tests for web UI report audit paste (#431).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from starlette.testclient import TestClient + +from webui.app import create_app +from webui.audit_validator import audit_report, infer_task_kind + + +_MINIMAL_REVIEW_REPORT = """## Controller Handoff + +- Task: review PR #1 +- Repo: Scaled-Tech-Consulting/Gitea-Tools +- Role: reviewer +- Identity: sysadmin / prgs-reviewer +- Review decision: approve +""" + + +class TestAuditValidator(unittest.TestCase): + def test_infer_task_kind_from_review_report(self): + self.assertEqual(infer_task_kind(_MINIMAL_REVIEW_REPORT), "review_pr") + + def test_infer_task_kind_explicit_override(self): + self.assertEqual( + infer_task_kind(_MINIMAL_REVIEW_REPORT, explicit="work_issue"), + "work_issue", + ) + + def test_empty_report_blocked(self): + result = audit_report("") + self.assertTrue(result["blocked"]) + self.assertIn("empty", result["reasons"][0].lower()) + + def test_minimal_report_produces_findings(self): + result = audit_report(_MINIMAL_REVIEW_REPORT) + self.assertIn(result["grade"], {"blocked", "downgraded", "warning", "A"}) + self.assertTrue(result["findings"] or result["blocked"] or result["downgraded"]) + + +class TestAuditRoutes(unittest.TestCase): + def setUp(self): + self.client = TestClient(create_app()) + + def test_audit_page_renders_form(self): + response = self.client.get("/audit") + self.assertEqual(response.status_code, 200) + self.assertIn("Report audit", response.text) + self.assertIn('name="report_text"', response.text) + self.assertIn("Run validator preview", response.text) + self.assertNotIn("child issue", response.text.lower()) + + def test_audit_post_runs_validator(self): + response = self.client.post( + "/audit", + data={"report_text": _MINIMAL_REVIEW_REPORT, "task_kind": "review_pr"}, + ) + self.assertEqual(response.status_code, 200) + self.assertIn("Validator preview", response.text) + self.assertIn("Safe next action", response.text) + + def test_api_audit_post_json(self): + response = self.client.post( + "/api/audit", + json={"report_text": _MINIMAL_REVIEW_REPORT, "task_kind": "review_pr"}, + ) + self.assertEqual(response.status_code, 200) + data = response.json() + self.assertEqual(data["task_kind"], "review_pr") + self.assertIn("grade", data) + self.assertIn("findings", data) + self.assertIn("suggested_next_prompt", data) + + def test_api_audit_rejects_empty_body(self): + response = self.client.post("/api/audit", json={"report_text": ""}) + self.assertEqual(response.status_code, 400) + + def test_audit_post_allowed_other_post_still_blocked(self): + self.assertEqual(self.client.post("/health").status_code, 405) + self.assertEqual( + self.client.post( + "/audit", + data={"report_text": _MINIMAL_REVIEW_REPORT}, + ).status_code, + 200, + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_webui_skeleton.py b/tests/test_webui_skeleton.py index 5dd23a1..08e361b 100644 --- a/tests/test_webui_skeleton.py +++ b/tests/test_webui_skeleton.py @@ -30,11 +30,14 @@ class TestWebuiSkeleton(unittest.TestCase): self.assertIn("Read-only MVP", response.text) def test_route_stubs_render(self): - for path in ("/runtime", "/audit"): - with self.subTest(path=path): - response = self.client.get(path) - self.assertEqual(response.status_code, 200) - self.assertIn("child issue", response.text.lower()) + response = self.client.get("/runtime") + self.assertEqual(response.status_code, 200) + self.assertIn("child issue", response.text.lower()) + + def test_audit_is_implemented(self): + response = self.client.get("/audit") + self.assertEqual(response.status_code, 200) + self.assertIn("Report audit", response.text) def test_prompts_is_implemented(self): response = self.client.get("/prompts") diff --git a/webui/app.py b/webui/app.py index a97d1ad..1bc72b4 100644 --- a/webui/app.py +++ b/webui/app.py @@ -14,10 +14,15 @@ from webui.project_registry import find_project, load_registry, registry_to_dict from webui.project_views import render_project_detail, render_projects_list from webui.prompt_library import find_prompt, library_to_dict from webui.prompt_views import render_prompt_detail, render_prompts_page +from final_report_validator import FINAL_REPORT_TASK_KINDS + +from webui.audit_validator import audit_report, audit_to_dict +from webui.audit_views import render_audit_page from webui.queue_loader import load_queue_snapshot, snapshot_to_dict from webui.queue_views import render_queue_page _READ_ONLY_METHODS = frozenset({"GET", "HEAD", "OPTIONS"}) +_AUDIT_MUTATION_PATHS = frozenset({"/audit", "/api/audit"}) def _stub_page(title: str, description: str) -> HTMLResponse: @@ -126,13 +131,49 @@ async def runtime(_request: Request) -> HTMLResponse: ) -async def audit(_request: Request) -> HTMLResponse: - return _stub_page( - "Audit", - "Report audit will accept pasted final reports and run validator previews.", +async def _parse_audit_form(request: Request) -> tuple[str, str | None]: + if request.method == "GET": + return "", None + content_type = (request.headers.get("content-type") or "").lower() + if "application/json" in content_type: + payload = await request.json() + if not isinstance(payload, dict): + return "", None + report_text = str(payload.get("report_text") or "") + task_kind = payload.get("task_kind") + return report_text, str(task_kind) if task_kind else None + form = await request.form() + report_text = str(form.get("report_text") or "") + task_kind = form.get("task_kind") + return report_text, str(task_kind) if task_kind else None + + +async def audit(request: Request) -> HTMLResponse: + report_text, task_kind = await _parse_audit_form(request) + result = None + if request.method == "POST" and report_text.strip(): + result = audit_report(report_text, task_kind=task_kind) + return HTMLResponse( + render_audit_page( + report_text=report_text, + task_kind=task_kind, + result=result, + ) ) +async def api_audit(request: Request) -> JSONResponse: + if request.method == "GET": + return JSONResponse({ + "usage": "POST JSON with report_text and optional task_kind", + "task_kinds": sorted(FINAL_REPORT_TASK_KINDS), + }) + report_text, task_kind = await _parse_audit_form(request) + if not report_text.strip(): + return JSONResponse({"error": "report_text required"}, status_code=400) + return JSONResponse(audit_to_dict(audit_report(report_text, task_kind=task_kind))) + + async def worktrees(_request: Request) -> HTMLResponse: return _stub_page( "Worktrees", @@ -148,6 +189,9 @@ async def leases(_request: Request) -> HTMLResponse: async def method_not_allowed(request: Request, _exc: Exception) -> Response: + path = request.url.path + if path in _AUDIT_MUTATION_PATHS and request.method == "POST": + return JSONResponse({"error": "not_found"}, status_code=404) if request.method not in _READ_ONLY_METHODS: return JSONResponse( {"error": "read-only-mvp", "detail": f"{request.method} not permitted"}, @@ -172,7 +216,8 @@ def create_app() -> Starlette: Route("/prompts/{prompt_id}", prompt_detail, methods=["GET"]), Route("/api/prompts", api_prompts, methods=["GET"]), Route("/runtime", runtime, methods=["GET"]), - Route("/audit", audit, methods=["GET"]), + Route("/audit", audit, methods=["GET", "POST"]), + Route("/api/audit", api_audit, methods=["GET", "POST"]), Route("/worktrees", worktrees, methods=["GET"]), Route("/leases", leases, methods=["GET"]), ], diff --git a/webui/audit_validator.py b/webui/audit_validator.py new file mode 100644 index 0000000..d19e7ff --- /dev/null +++ b/webui/audit_validator.py @@ -0,0 +1,176 @@ +"""Final-report audit preview for the web UI (#431).""" + +from __future__ import annotations + +import re +from typing import Any + +from final_report_validator import FINAL_REPORT_TASK_KINDS, assess_final_report_validator + +_TASK_KIND_PATTERNS: tuple[tuple[str, re.Pattern[str]], ...] = ( + ("review_pr", re.compile(r"\b(?:review\s+pr|task\s*:\s*review|review-merge-pr)\b", re.I)), + ( + "reconcile_already_landed", + re.compile( + r"\b(?:reconcile\s+already[- ]landed|already[- ]landed\s+pr|" + r"reconcile-landed-pr)\b", + re.I, + ), + ), + ("work_issue", re.compile(r"\b(?:work[- ]issue|task\s*:\s*work|selected\s+issue\s*:)\b", re.I)), + ("issue_filing", re.compile(r"\b(?:issue\s+filing|create\s+issue|task\s*:\s*file)\b", re.I)), + ("issue_selection", re.compile(r"\b(?:issue\s+selection|select\s+next\s+issue)\b", re.I)), + ("inventory", re.compile(r"\b(?:issue\s+inventory|queue\s+inventory)\b", re.I)), +) + +_SUGGESTED_PROMPTS: dict[str, str] = { + "review_pr": ( + "Review the next eligible open PR in this project. Merge it only if every " + "gate passes. Post a final report matching review-merge-pr schema." + ), + "work_issue": ( + "Find the next eligible issue in this project, work on it only if all gates " + "pass, and create a PR when complete." + ), + "reconcile_already_landed": ( + "Reconcile the oldest eligible already-landed open PR. Post read-only audit " + "first; authorize cleanup only when required." + ), + "issue_filing": "File a new Gitea issue using the canonical issue-filing workflow.", + "issue_selection": "Build a complete open-issue inventory and select the next eligible issue.", + "inventory": "Produce a proof-backed inventory of open PRs or issues without mutating state.", +} + + +def infer_task_kind(report_text: str, explicit: str | None = None) -> str: + """Infer workflow task kind from explicit selection or report content.""" + if explicit: + normalized = explicit.strip().lower().replace(" ", "_") + if normalized in FINAL_REPORT_TASK_KINDS: + return normalized + text = report_text or "" + for kind, pattern in _TASK_KIND_PATTERNS: + if pattern.search(text): + return kind + if re.search(r"\brole\s*:\s*reviewer\b", text, re.I): + return "review_pr" + if re.search(r"\brole\s*:\s*author\b", text, re.I): + return "work_issue" + return "review_pr" + + +def _merge_findings(*result_sets: dict[str, Any]) -> list[dict[str, str]]: + seen: set[tuple[str, str, str]] = set() + merged: list[dict[str, str]] = [] + for result in result_sets: + for finding in result.get("findings") or []: + key = ( + str(finding.get("rule_id", "")), + str(finding.get("field", "")), + str(finding.get("reason", "")), + ) + if key in seen: + continue + seen.add(key) + merged.append(finding) + return merged + + +def _aggregate_result(task_kind: str, findings: list[dict[str, str]], base: dict[str, Any]) -> dict[str, Any]: + blocked = any(f.get("severity") == "block" for f in findings) + downgraded = any(f.get("severity") == "downgrade" for f in findings) + warning = any(f.get("severity") == "warning" for f in findings) + if blocked: + grade = "blocked" + elif downgraded: + grade = "downgraded" + elif warning: + grade = "warning" + else: + grade = base.get("grade") or "A" + safe_next_action = base.get("safe_next_action") or "proceed" + for finding in findings: + if finding.get("severity") in {"block", "downgrade"}: + safe_next_action = finding.get("safe_next_action") or safe_next_action + break + return { + "task_kind": task_kind, + "inferred_task_kind": task_kind, + "grade": grade, + "blocked": blocked or bool(base.get("blocked")), + "downgraded": downgraded or bool(base.get("downgraded")), + "findings": findings, + "reasons": [f"{f.get('rule_id')}: {f.get('reason')}" for f in findings], + "safe_next_action": safe_next_action, + "complete": grade == "A" and not findings, + "suggested_next_prompt": _SUGGESTED_PROMPTS.get(task_kind, ""), + "suggested_issue_comment": _build_issue_comment(findings, safe_next_action), + } + + +def _build_issue_comment(findings: list[dict[str, str]], safe_next_action: str) -> str: + if not findings: + return "" + lines = [ + "## Report audit preview (local validator)", + "", + "Automated findings from pasted final report:", + "", + ] + for finding in findings[:12]: + severity = finding.get("severity", "info") + rule_id = finding.get("rule_id", "unknown") + reason = finding.get("reason", "") + lines.append(f"- **{severity}** `{rule_id}` — {reason}") + if len(findings) > 12: + lines.append(f"- …and {len(findings) - 12} more finding(s)") + lines.extend(["", f"Safe next action: {safe_next_action}", ""]) + return "\n".join(lines) + + +def audit_report(report_text: str, *, task_kind: str | None = None) -> dict[str, Any]: + """Run composable final-report validation on pasted text.""" + text = (report_text or "").strip() + if not text: + return { + "task_kind": task_kind or "review_pr", + "inferred_task_kind": task_kind or "review_pr", + "grade": "blocked", + "blocked": True, + "downgraded": False, + "findings": [], + "reasons": ["empty report text"], + "safe_next_action": "paste a non-empty final report", + "complete": False, + "suggested_next_prompt": "", + "suggested_issue_comment": "", + } + + resolved_kind = infer_task_kind(text, task_kind) + base = assess_final_report_validator(text, resolved_kind) + findings = list(base.get("findings") or []) + + if resolved_kind == "review_pr": + from review_final_report_schema import assess_review_final_report_schema + + schema = assess_review_final_report_schema(text) + findings = _merge_findings(base, schema) + + return _aggregate_result(resolved_kind, findings, base) + + +def audit_to_dict(result: dict[str, Any]) -> dict[str, Any]: + """JSON-safe export for /api/audit.""" + return { + "task_kind": result.get("task_kind"), + "inferred_task_kind": result.get("inferred_task_kind"), + "grade": result.get("grade"), + "blocked": result.get("blocked"), + "downgraded": result.get("downgraded"), + "complete": result.get("complete"), + "safe_next_action": result.get("safe_next_action"), + "findings": result.get("findings") or [], + "reasons": result.get("reasons") or [], + "suggested_next_prompt": result.get("suggested_next_prompt") or "", + "suggested_issue_comment": result.get("suggested_issue_comment") or "", + } \ No newline at end of file diff --git a/webui/audit_views.py b/webui/audit_views.py new file mode 100644 index 0000000..1316eea --- /dev/null +++ b/webui/audit_views.py @@ -0,0 +1,139 @@ +"""HTML views for final-report audit paste (#431).""" + +from __future__ import annotations + +import html + +from final_report_validator import FINAL_REPORT_TASK_KINDS +from webui.audit_validator import audit_report +from webui.layout import render_page + +_TASK_KIND_OPTIONS = sorted(FINAL_REPORT_TASK_KINDS) + + +def _escape(text: str) -> str: + return html.escape(text, quote=True) + + +def _render_findings(result: dict) -> str: + findings = result.get("findings") or [] + if not findings: + return '

No validator findings — report structure looks complete for the selected task kind.

' + rows = [] + for finding in findings: + rows.append( + "" + f"{_escape(str(finding.get('rule_id', '')))}" + f"{_escape(str(finding.get('severity', '')))}" + f"{_escape(str(finding.get('field', '')))}" + f"{_escape(str(finding.get('reason', '')))}" + f"{_escape(str(finding.get('safe_next_action', '')))}" + "" + ) + return ( + '' + "" + "" + f"{''.join(rows)}
RuleSeverityFieldReasonSafe next action
" + ) + + +def _render_task_kind_select(selected: str | None) -> str: + options = [''] + for kind in _TASK_KIND_OPTIONS: + sel = ' selected' if selected == kind else "" + options.append(f'') + return f'' + + +def render_audit_page( + *, + report_text: str = "", + task_kind: str | None = None, + result: dict | None = None, +) -> str: + result_html = "" + if result is not None: + grade = _escape(str(result.get("grade", ""))) + inferred = _escape(str(result.get("inferred_task_kind", ""))) + safe_next = _escape(str(result.get("safe_next_action", ""))) + suggested_prompt = result.get("suggested_next_prompt") or "" + suggested_comment = result.get("suggested_issue_comment") or "" + result_html = ( + '
' + f"

Validator preview

" + f'

Task kind: {inferred} · ' + f'Grade: {grade} · ' + f"Blocked: {result.get('blocked')} · " + f"Downgraded: {result.get('downgraded')}

" + f"

Safe next action: {safe_next}

" + f"{_render_findings(result)}" + ) + if suggested_prompt: + result_html += ( + "

Suggested next prompt

" + f'
{_escape(suggested_prompt)}
' + ) + if suggested_comment: + result_html += ( + "

Suggested issue/PR comment

" + f'
{_escape(suggested_comment)}
' + ) + result_html += "
" + + body = ( + "

Report audit

" + "

Paste an LLM final report for local validator preview. " + "Uses final_report_validator and review schema checks — " + "does not post to Gitea or store reports server-side.

" + '
' + "" + f"{_render_task_kind_select(task_kind)}" + '' + f'' + '' + "
" + f"{result_html}" + "

JSON API (POST report_text, " + "optional task_kind)

" + ) + return render_page(title="Audit", body_html=body, extra_head=AUDIT_PAGE_STYLES) + + +AUDIT_PAGE_STYLES = """ + +""" \ No newline at end of file From a863928661e3d83d450e5e9c57519feeb3e78f0f Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 02:12:26 -0400 Subject: [PATCH 02/12] Clarify and enforce reviewer-vs-merger role boundaries (#483) --- docs/llm-workflow-runbooks.md | 10 +- final_report_validator.py | 15 +++ gitea_mcp_server.py | 80 +++++++++++----- review_proofs.py | 18 +++- .../templates/merge-pr.md | 11 ++- .../templates/review-pr.md | 7 +- task_capability_map.py | 2 +- tests/test_final_report_validator.py | 92 +++++++++++++++++++ tests/test_mcp_server.py | 25 +++-- tests/test_operator_guide.py | 20 +++- tests/test_resolve_task_capability.py | 19 +++- tests/test_runtime_clarity.py | 50 +++++++++- 12 files changed, 294 insertions(+), 55 deletions(-) diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index 634e22c..97bacdd 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -601,10 +601,10 @@ loop and do **not** substitute WebFetch/Playwright/manual base64. ### Merge a PR - **Profile:** merger (allowed to merge; must **not** be the PR author). -- **Steps:** confirm eligibility; require explicit confirmation - (`MERGE PR `); optionally pin head SHA / changed-file set; merge only when - Gitea reports the PR mergeable (branch-protection checks satisfied). No force, - no ignore-checks. Verify that remote master contains the merge commit or the expected squashed changes (do not assume a "closed" PR succeeded without verifying the actual landed changes). +- **Steps:** + - Merger workflow starts only after formal approval at current head. Reviewer workflow ends with review decision and separate merger handoff. + - Confirm eligibility; require explicit confirmation (`MERGE PR `); optionally pin head SHA / changed-file set; merge only when Gitea reports the PR mergeable (branch-protection checks satisfied). No force, no ignore-checks. Verify that remote master contains the merge commit or the expected squashed changes (do not assume a "closed" PR succeeded without verifying the actual landed changes). + - Review and merge are separate workflow roles. A reviewer approval is not merge authorization. - **Prompt:** `Use any eligible merger profile to merge PR #N if checks pass and it is mergeable. Confirm with "MERGE PR N". Do not force-merge.` @@ -645,7 +645,7 @@ an author. |---|---|---|---|---| | Review PR (`review_pr`) | reviewer (e.g. `sysadmin` / `prgs-reviewer`) | read, gated review verdicts | commits, pushes, file edits, author comments, merge without eligibility | active profile is an author profile — stop immediately; do **not** switch to author-side fixes unless the operator explicitly re-tasks | | Address PR change requests (`address_pr_change_requests`) | author (e.g. `jcwalker3` / `prgs-author`) | commit/push fixes to the PR branch, PR comment summarizing fixes | review verdicts, approve, request-changes, merge | active profile lacks branch push | -| Merge PR (`merge_pr`) | reviewer/merger | gated merge after eligibility + approval | merging own PR, merging without pinned head match | active profile is an author profile, or any merge gate fails | +| Merge PR (`merge_pr`) | merger (e.g. `sysadmin` / `prgs-merger`) | gated merge after eligibility + approval | merging own PR, merging without pinned head match, reviewing PRs | active profile is an author or reviewer-only profile, or any merge gate fails | | Comment on issue discussion (`comment_issue`) | any profile with `gitea.issue.comment` | issue thread comments | review verdicts, closing via comment | permission missing (`gitea.pr.comment` does **not** imply it) | | Comment on PR (`comment_pr`) | any profile with `gitea.pr.comment` | PR thread comments | review verdicts | permission missing | | Author implementation (`create_branch`/`push_branch`/`create_pr`) | author | branch, commit, push, open PR | self-review, self-merge | profile lacks the author permissions | diff --git a/final_report_validator.py b/final_report_validator.py index 26c4ba0..718ae04 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -25,6 +25,7 @@ from validation_status_vocabulary import assess_validation_status_vocabulary FINAL_REPORT_TASK_KINDS = frozenset({ "review_pr", + "merge_pr", "reconcile_already_landed", "author_issue", "work_issue", @@ -36,6 +37,8 @@ FINAL_REPORT_TASK_KINDS = frozenset({ _TASK_KIND_ALIASES = { "review": "review_pr", "review-merge-pr": "review_pr", + "merge": "merge_pr", + "merge_pr": "merge_pr", "reconcile-landed-pr": "reconcile_already_landed", "reconcile_already_landed": "reconcile_already_landed", "work-issue": "work_issue", @@ -44,6 +47,7 @@ _TASK_KIND_ALIASES = { _HANDOFF_ROLE_BY_TASK = { "review_pr": "review", + "merge_pr": "merger", "reconcile_already_landed": None, "author_issue": "author", "work_issue": "author", @@ -1036,6 +1040,17 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_review_mutation, _rule_reviewer_stale_head_proof, ], + "merge_pr": [ + _rule_shared_controller_handoff, + _rule_shared_email_disclosure, + *_SHARED_ISSUE_LOCK_RULES, + _rule_reviewer_legacy_workspace_mutations, + _rule_reviewer_vague_mutations_none, + _rule_reviewer_mutation_categories, + _rule_reviewer_git_fetch_readonly, + _rule_reviewer_linked_issue, + _rule_reviewer_stale_head_proof, + ], "reconcile_already_landed": [ _rule_reconcile_controller_handoff, _rule_shared_email_disclosure, diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 9ea9050..a1339e5 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -137,13 +137,13 @@ def verify_mutation_authority(remote: str | None, host: str | None = None, ) # Reviewer/author role pivot boundary: only an authorized pivot - # (recorded by gitea_activate_profile) may cross author → reviewer. - if (required_role == "reviewer" + # (recorded by gitea_activate_profile) may cross author -> reviewer/merger. + if (required_role in ("reviewer", "merger") and "author" in str(data.get("initial_profile")).lower() - and "reviewer" in str(active_profile).lower()): + and ("reviewer" in str(active_profile).lower() or "merger" in str(active_profile).lower())): if not data.get("role_pivot_authorized"): raise RuntimeError( - "Attempted reviewer mutation from author session without " + f"Attempted {required_role} mutation from author session without " "authorized role pivot (fail closed)" ) @@ -418,7 +418,7 @@ def record_preflight_check(type_name: str, resolved_role: str | None = None): def _enforce_branches_only_author_mutation(worktree_path: str | None = None) -> None: """#274: author mutations must run from a branches/ session worktree.""" - if _preflight_resolved_role == "reviewer": + if _preflight_resolved_role in ("reviewer", "reconciler", "merger"): return ctx = _resolve_author_mutation_context(worktree_path) workspace = ctx["workspace_path"] @@ -1872,13 +1872,23 @@ def gitea_check_pr_eligibility( # ("gitea.pr.merge") always match each other and never cross services. allowed = profile["allowed_operations"] forbidden = profile["forbidden_operations"] - op_ok, op_reason = gitea_config.check_operation(action, allowed, forbidden) + active_role = _role_kind(allowed, forbidden) + if action == "merge" and active_role == "reviewer": + op_ok = False + op_reason = "reviewer-cannot-merge" + else: + op_ok, op_reason = gitea_config.check_operation(action, allowed, forbidden) + if not op_ok: if op_reason == "no-allowed-operations": reasons.append( "profile has no configured allowed operations (fail closed)") elif op_reason == "forbidden": reasons.append(f"profile forbids '{action}'") + elif op_reason == "reviewer-cannot-merge": + reasons.append( + "Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head." + ) elif op_reason == "invalid-forbidden-entry": reasons.append( "profile has an unrecognized forbidden operation entry " @@ -1902,21 +1912,23 @@ def gitea_check_pr_eligibility( missing_perm) req_profile = None + role_noun = "reviewer" if action in ("approve", "request_changes", "review"): - req_profile = "A profile with reviewer role permissions (allowing approve/merge/review, and forbidding author operations)" + req_profile = "A profile with reviewer role permissions (allowing approve/review, and forbidding author operations)" elif action == "merge": - req_profile = "A profile with reviewer role permissions and explicit merge permission" + req_profile = "A profile with merger role permissions (allowing merge, and forbidding author operations)" + role_noun = "merger" result["required_profile"] = req_profile switching_supported = gitea_config.is_runtime_switching_enabled() if switching_supported: result["fixable_by_profile_switch"] = True result["requires_different_namespace"] = False - safe_step = f"Switch to a reviewer profile by calling gitea_activate_profile with a profile that allows {action}." + safe_step = f"Switch to a {role_noun} profile by calling gitea_activate_profile with a profile that allows {action}." else: result["fixable_by_profile_switch"] = False result["requires_different_namespace"] = True - safe_step = "Switch to the reviewer MCP session (e.g. gitea-reviewer) which has reviewer permissions configured, or ask the operator to update GITEA_MCP_PROFILE to a reviewer profile." + safe_step = f"Switch to the {role_noun} MCP session (e.g. gitea-{role_noun}) which has {role_noun} permissions configured, or ask the operator to update GITEA_MCP_PROFILE to a {role_noun} profile." result["safe_next_step"] = safe_step return result @@ -3419,6 +3431,12 @@ def gitea_merge_pr( available. Never secrets. """ verify_preflight_purity(remote) + allowed = get_profile()["allowed_operations"] + forbidden = get_profile()["forbidden_operations"] + active_role = _role_kind(allowed, forbidden) + if active_role == "reviewer": + raise RuntimeError("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.") + do = (do or "").strip().lower() result = { "performed": False, @@ -3434,6 +3452,10 @@ def gitea_merge_pr( "merge_result": None, "merge_commit": None, "reasons": [], + "active_profile": get_profile()["profile_name"], + "role_kind": active_role, + "merge_capability_source": "profile" if "gitea.pr.merge" in allowed else "config", + "explicit_operator_auth": confirmation, } reasons = result["reasons"] @@ -3599,7 +3621,7 @@ def gitea_merge_pr( # A profile/identity flip or side-channel override between preflight # and merge fails closed here. try: - verify_mutation_authority(remote, host, required_role="reviewer", + verify_mutation_authority(remote, host, required_role="merger", active_identity=auth_user) except RuntimeError as e: reasons.append(str(e)) @@ -5214,26 +5236,28 @@ def _role_kind(allowed, forbidden) -> str: """Classify the active profile from its normalized operations. 'reconciler' can close already-landed PRs without review/author powers; - 'author' can create PRs / push branches; 'reviewer' can approve/merge; - 'reconciler' can close already-landed PRs via ``gitea.pr.close`` without - review/author powers; 'mixed' can do both (a config smell — the - reviewer-deadlock invariant forbids it in v2 configs); 'limited' can do - neither. + 'author' can create PRs / push branches; 'reviewer' can approve; + 'merger' can merge PRs; + 'mixed' can do multiple (a config smell); 'limited' can do neither. """ if reconciler_profile.is_reconciler_profile(allowed, forbidden): return "reconciler" def can(op): return gitea_config.check_operation(op, allowed, forbidden)[0] - review = can("gitea.pr.approve") or can("gitea.pr.merge") + can_merge = can("gitea.pr.merge") + can_approve = can("gitea.pr.approve") author = can("gitea.pr.create") or can("gitea.branch.push") reconciler = ( can("gitea.pr.close") - and not review + and not can_approve + and not can_merge and not author ) - if review and author: + if (can_approve or can_merge) and author: return "mixed" - if review: + if can_merge: + return "merger" + if can_approve: return "reviewer" if reconciler: return "reconciler" @@ -5749,12 +5773,20 @@ def mcp_get_control_plane_guide( "gitea.issue.comment (separate from gitea.pr.comment).") elif role == "reviewer": guidance.append( - "Reviewer profile: review/approve/merge may proceed ONLY after " + "Reviewer profile: review/approve may proceed ONLY after " "gitea_check_pr_eligibility passes and the PR head SHA is " "pinned (expected_head_sha); the PR author must be a different " - "user, and merging additionally requires explicit operator " - "authorization plus the 'MERGE PR ' confirmation. " - "PR comments do not imply issue comments.") + "user. Reviewer profiles cannot merge PRs. " + "PR comments do not imply issue comments. " + "Review and merge are separate workflow roles. A reviewer approval is not merge authorization.") + elif role == "merger": + guidance.append( + "Merger profile: merge may proceed ONLY after formal approval at " + "current head, gitea_check_pr_eligibility passes, and the PR head " + "SHA is pinned (expected_head_sha); the PR author must be a different " + "user, and merging requires explicit operator authorization plus the " + "'MERGE PR ' confirmation. " + "Review and merge are separate workflow roles. A reviewer approval is not merge authorization.") elif role == "mixed": guidance.append( "WARNING: this profile allows both authoring and " diff --git a/review_proofs.py b/review_proofs.py index b74ebb8..4f725df 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -2243,6 +2243,18 @@ HANDOFF_ROLE_FIELDS = { ("Linked issue status", ("linked issue status", "linked issue")), ("Cleanup status", ("cleanup status", "cleanup")), ) + HANDOFF_REVIEW_MUTATION_FIELDS, + "merger": ( + ("Selected PR", ("selected pr",)), + ("Pinned reviewed head", ("pinned reviewed head", "pinned head")), + ("Active profile", ("active profile",)), + ("Role kind", ("role kind",)), + ("Merge capability source", ("merge capability source",)), + ("Explicit operator authorization", ("explicit operator authorization", "operator authorization", "authorization")), + ("Expected head SHA", ("expected head sha", "expected head")), + ("Approval at current head", ("approval at current head", "approval")), + ("Merge result", ("merge result",)), + ("Cleanup status", ("cleanup status", "cleanup")), + ) + HANDOFF_REVIEW_MUTATION_FIELDS, "author": ( ("Selected issue", ("selected issue",)), ("Issue lock proof", ("issue lock proof", "lock before diff")), @@ -2417,8 +2429,8 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): field for field in required if field[0] not in ("Workspace mutations", "Mutations") ] - if role == "review": - # Issue #320: reviewer handoffs use the precise mutation categories + if role in ("review", "merger"): + # Issue #320: reviewer and merger handoffs use the precise mutation categories # in HANDOFF_REVIEW_MUTATION_FIELDS instead of the legacy ambiguous # "Workspace mutations" field, which is rejected below. required = [ @@ -2431,7 +2443,7 @@ def assess_controller_handoff(report_text, role=None, local_edits=False): "downgraded": True, "missing_fields": [], "reasons": [ - "review handoff must not include legacy " + "review or merger handoff must not include legacy " "'Workspace mutations' field; report the precise " "mutation categories instead (issue #320)" ], diff --git a/skills/llm-project-workflow/templates/merge-pr.md b/skills/llm-project-workflow/templates/merge-pr.md index ae99544..e4e7a65 100644 --- a/skills/llm-project-workflow/templates/merge-pr.md +++ b/skills/llm-project-workflow/templates/merge-pr.md @@ -1,4 +1,4 @@ -# Template: merge a PR (eligible reviewer only) +# Template: merge a PR (eligible merger only) Copy, fill the `<...>` fields, and paste as the task prompt. @@ -10,8 +10,9 @@ Load the canonical workflow first: Final report schema: `schemas/review-merge-final-report.md`. Rules (llm-project-workflow): -- Only an eligible, NON-author reviewer merges. If authenticated user == PR +- Only an eligible, NON-author merger merges. If authenticated user == PR author → STOP. +- Review and merge are separate workflow roles. A reviewer approval is not merge authorization. - Do not merge unless the PR is open, mergeable, and its checks/review pass. - No force-merge, no bypassing branch protections. - If the PR is closed but `merged=false`, STOP and run reconciliation. Do not clean up. @@ -51,10 +52,12 @@ Then run the cleanup template (worktree-cleanup.md): - fetch/prune; confirm main checkout is clean and current (0 0). Handoff: end with a section titled exactly `Controller Handoff` per SKILL.md -§K (long form — a merge is always high-risk), including the review/merge role -fields (Selected PR, Reviewer eligibility, Pinned reviewed head, Review +§K (long form — a merge is always high-risk), including the merger role +fields (Selected PR, Merger eligibility, Pinned reviewed head, Review decision, Merge result, Linked issue status, Cleanup status) plus: merge commit, PR metadata state/merged flag/hash, remote master hash, and the post-merge verification method used & verification results. Reports missing the handoff are downgraded (review_proofs.assess_controller_handoff). + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. ``` diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 391f962..c24d3d8 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -105,8 +105,9 @@ Steps: - base branch unchanged - no undismissed REQUEST_CHANGES / blocking review state left unaccounted If anything moved → STOP, re-pin, re-validate before any verdict. -10. Post the review verdict: approve only if scope is clean and checks pass; - otherwise request changes with specifics. Never merge from this review step. + 10. Post the review verdict: approve only if scope is clean and checks pass; + otherwise request changes with specifics. Never merge from this review step. + Review and merge are separate workflow roles. A reviewer approval is not merge authorization. Include a "Review Metadata" block (attribution only — docs/llm-agent-sha.md): Review Metadata: @@ -122,4 +123,6 @@ 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). + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. ``` diff --git a/task_capability_map.py b/task_capability_map.py index 1d40a78..d563736 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -66,7 +66,7 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { }, "merge_pr": { "permission": "gitea.pr.merge", - "role": "reviewer", + "role": "merger", }, "blind_pr_queue_review": { "permission": "gitea.pr.review", diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 88c5b23..88499b3 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -513,8 +513,100 @@ class TestEntryPoint(unittest.TestCase): def test_supported_task_kinds_include_review_and_reconcile(self): self.assertIn("review_pr", FINAL_REPORT_TASK_KINDS) + self.assertIn("merge_pr", FINAL_REPORT_TASK_KINDS) self.assertIn("reconcile_already_landed", FINAL_REPORT_TASK_KINDS) +class TestMergePrRules(unittest.TestCase): + def test_clean_merger_report_passes(self): + lines = [ + "## Controller Handoff", + "", + "- Task: merge PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: merger", + "- Identity: sysadmin / prgs-merger", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest in branches/review-203", + "- Mutations: merge completed", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: merge completed", + "- Review mutations: none", + "- Merge mutations: merged PR #203", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: metadata check", + "- Current status: PR merged", + "- Blockers: none", + "- Next: none", + "- Safety: review and merge are separate roles", + "- Selected PR: #203", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", + "- Active profile: prgs-merger", + "- Role kind: merger", + "- Merge capability source: profile", + "- Explicit operator authorization: MERGE PR 203", + "- Expected head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Approval at current head: yes", + "- Merge result: PR merged successfully", + "- Cleanup status: complete", + ] + report = "\n".join(lines) + result = assess_final_report_validator( + report, + "merge_pr", + ) + self.assertEqual(result["grade"], "A") + self.assertFalse(result["blocked"]) + + def test_missing_merger_fields_blocks(self): + lines = [ + "## Controller Handoff", + "", + "- Task: merge PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: merger", + "- Identity: sysadmin / prgs-merger", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest in branches/review-203", + "- Mutations: merge completed", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: merge completed", + "- Review mutations: none", + "- Merge mutations: merged PR #203", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: metadata check", + "- Current status: PR merged", + "- Blockers: none", + "- Next: none", + "- Safety: review and merge are separate roles", + "- Selected PR: #203", + "- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Reviewed head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before approval: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Final live head SHA before merge: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Push occurred during validation: no", + ] + report = "\n".join(lines) + result = assess_final_report_validator( + report, + "merge_pr", + ) + self.assertTrue(result["downgraded"]) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 2b8efb4..64617cd 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -848,6 +848,19 @@ class TestMergePR(unittest.TestCase): # -- identity / profile / eligibility fail-closed ------------------------- + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_reviewer_profile_cannot_merge(self, _auth, mock_api): + env = {"GITEA_PROFILE_NAME": "prgs-reviewer", + "GITEA_ALLOWED_OPERATIONS": "read,approve"} + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(RuntimeError) as ctx: + gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs" + ) + self.assertIn("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.", str(ctx.exception)) + mock_api.assert_not_called() + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_self_author_cannot_merge(self, _auth, mock_api): @@ -889,14 +902,13 @@ class TestMergePR(unittest.TestCase): @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_profile_without_merge_permission_blocks(self, _auth, mock_api): - mock_api.side_effect = [{"login": "merger-bot"}, self._pr("author-bot")] env = {"GITEA_PROFILE_NAME": "gitea-reviewer", "GITEA_ALLOWED_OPERATIONS": "read,review,approve"} with patch.dict(os.environ, env, clear=True): - r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") - self.assertFalse(r["performed"]) - self.assertIn("profile is not allowed to merge", r["reasons"]) + with self.assertRaises(RuntimeError) as ctx: + gitea_merge_pr( + pr_number=8, confirmation=self._confirm(8), remote="prgs") + self.assertIn("Reviewer profile cannot merge. Use a merger profile/session after formal approval at current head.", str(ctx.exception)) self._assert_no_merge_call(mock_api) # -- PR state / mergeability ---------------------------------------------- @@ -1049,7 +1061,8 @@ class TestMergePR(unittest.TestCase): "GITEA_ALLOWED_OPERATIONS": "read,merge"} with patch.dict(os.environ, env, clear=True): r = gitea_merge_pr( - pr_number=8, confirmation=self._confirm(8), remote="prgs") + pr_number=8, confirmation=self._confirm(8), + expected_head_sha=new_sha, remote="prgs") self.assertFalse(r["performed"]) self.assertTrue(r.get("approval_visible")) self.assertFalse(r.get("approval_at_current_head")) diff --git a/tests/test_operator_guide.py b/tests/test_operator_guide.py index a5159c6..147a6b2 100644 --- a/tests/test_operator_guide.py +++ b/tests/test_operator_guide.py @@ -33,8 +33,15 @@ REVIEWER_ENV = { "GITEA_PROFILE_NAME": "reviewer-test", "GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.pr.review,gitea.pr.comment,gitea.pr.approve," - "gitea.pr.request_changes,gitea.pr.merge", - "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push", + "gitea.pr.request_changes", + "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push,gitea.pr.merge", +} + +MERGER_ENV = { + "GITEA_PROFILE_NAME": "merger-test", + "GITEA_ALLOWED_OPERATIONS": + "gitea.read,gitea.pr.comment,gitea.pr.merge", + "GITEA_FORBIDDEN_OPERATIONS": "gitea.pr.create,gitea.branch.push,gitea.pr.approve", } EXPECTED_SKILLS = [ @@ -88,6 +95,15 @@ class TestControlPlaneGuide(GuideTestBase): self.assertIn("eligibility", blob) self.assertIn("pinned", blob) + @patch("mcp_server.api_request", return_value={"login": "merger-bot"}) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_merger_profile_guidance(self, _auth, _api): + with patch.dict(os.environ, MERGER_ENV, clear=True): + g = mcp_get_control_plane_guide(remote="prgs") + self.assertEqual(g["profile"]["role_kind"], "merger") + blob = " ".join(g["guidance"]).lower() + self.assertIn("merge", blob) + @patch("mcp_server.get_auth_header", return_value=None) def test_unresolved_identity_instructs_stop(self, _auth): with patch.dict(os.environ, AUTHOR_ENV, clear=True): diff --git a/tests/test_resolve_task_capability.py b/tests/test_resolve_task_capability.py index a34456d..aa25ced 100644 --- a/tests/test_resolve_task_capability.py +++ b/tests/test_resolve_task_capability.py @@ -44,9 +44,19 @@ CONFIG_RESOLVER = { "role": "reviewer", "username": "reviewer-user", "auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"}, - "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve", "gitea.pr.merge", "gitea.issue.comment"], - "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve", "gitea.issue.comment"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.merge"], "execution_profile": "reviewer-profile" + }, + "merger-profile": { + "enabled": True, + "context": "ctx", + "role": "merger", + "username": "merger-user", + "auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"}, + "allowed_operations": ["gitea.read", "gitea.pr.merge", "gitea.issue.comment"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.approve"], + "execution_profile": "merger-profile" } }, "rules": { @@ -83,6 +93,7 @@ class TestResolveTaskCapability(unittest.TestCase): "GITEA_MCP_PROFILE": profile, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_MERGER": "merger-pass", } @patch("mcp_server.api_request", return_value={"login": "author-user"}) @@ -231,9 +242,9 @@ class TestResolveTaskCapability(unittest.TestCase): with patch.dict(os.environ, self._env("author-profile")): res = mcp_server.gitea_resolve_task_capability(task="merge_pr", remote="prgs") self.assertFalse(res["allowed_in_current_session"]) - self.assertIn("reviewer", res["exact_safe_next_action"].lower()) + self.assertIn("merger", res["exact_safe_next_action"].lower()) self.assertEqual(res["required_operation_permission"], "gitea.pr.merge") - self.assertEqual(res["required_role_kind"], "reviewer") + self.assertEqual(res["required_role_kind"], "merger") @patch("mcp_server.api_request") def test_self_review_blocked_structured(self, mock_api): diff --git a/tests/test_runtime_clarity.py b/tests/test_runtime_clarity.py index e5098e0..1e94ada 100644 --- a/tests/test_runtime_clarity.py +++ b/tests/test_runtime_clarity.py @@ -43,9 +43,19 @@ CONFIG_SWITCHING_DISABLED = { "role": "reviewer", "username": "reviewer-user", "auth": {"type": "env", "name": "GITEA_TOKEN_REVIEWER"}, - "allowed_operations": ["gitea.read", "gitea.pr.approve", "gitea.pr.merge"], - "forbidden_operations": ["gitea.pr.create", "gitea.branch.push"], + "allowed_operations": ["gitea.read", "gitea.pr.review", "gitea.pr.approve"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.merge"], "execution_profile": "reviewer-profile" + }, + "merger-profile": { + "enabled": True, + "context": "ctx", + "role": "merger", + "username": "merger-user", + "auth": {"type": "env", "name": "GITEA_TOKEN_MERGER"}, + "allowed_operations": ["gitea.read", "gitea.pr.merge"], + "forbidden_operations": ["gitea.pr.create", "gitea.branch.push", "gitea.pr.approve"], + "execution_profile": "merger-profile" } }, "rules": { @@ -90,6 +100,7 @@ class TestRuntimeClarity(unittest.TestCase): "GITEA_MCP_REVEAL_ENDPOINTS": reveal, "GITEA_TOKEN_AUTHOR": "author-pass", "GITEA_TOKEN_REVIEWER": "reviewer-pass", + "GITEA_TOKEN_MERGER": "merger-pass", } # ------------------------------------------------------------------------- @@ -125,7 +136,7 @@ class TestRuntimeClarity(unittest.TestCase): t for t in caps["task_capabilities"] if t["task"] == "merge_pr" ) self.assertFalse(merge_entry["allowed_in_current_session"]) - self.assertIn("reviewer-profile", merge_entry["matching_configured_profiles"]) + self.assertIn("merger-profile", merge_entry["matching_configured_profiles"]) @patch( "mcp_server.assess_preflight_status", @@ -144,6 +155,30 @@ class TestRuntimeClarity(unittest.TestCase): caps = ctx["session_capabilities"] self.assertFalse(caps["can_author_prs"]) self.assertFalse(caps["can_create_issues"]) + self.assertTrue(caps["can_review_prs"]) + self.assertFalse(caps["can_merge_prs"]) + merge_entry = next( + t for t in caps["task_capabilities"] if t["task"] == "merge_pr" + ) + self.assertFalse(merge_entry["allowed_in_current_session"]) + + @patch( + "mcp_server.assess_preflight_status", + return_value={"preflight_ready": True, "preflight_block_reasons": []}, + ) + @patch("mcp_server.api_request", return_value={"login": "merger-user"}) + @patch("mcp_server.get_auth_header", return_value="token merger-pass") + def test_get_runtime_context_merger(self, _auth, _api, _preflight): + with patch.dict(os.environ, self._env("merger-profile"), clear=True): + ctx = mcp_server.gitea_get_runtime_context(remote="dadeschools") + self.assertEqual(ctx["active_profile"], "merger-profile") + self.assertEqual(ctx["authenticated_username"], "merger-user") + self.assertTrue(ctx["review_merge_allowed"]) + self.assertEqual(ctx["suggested_fix"], "none") + self.assertEqual(ctx["safe_next_action"], "None; ready for operations.") + caps = ctx["session_capabilities"] + self.assertFalse(caps["can_author_prs"]) + self.assertFalse(caps["can_create_issues"]) self.assertFalse(caps["can_review_prs"]) self.assertTrue(caps["can_merge_prs"]) merge_entry = next( @@ -160,7 +195,7 @@ class TestRuntimeClarity(unittest.TestCase): with patch.dict(os.environ, self._env("author-profile", reveal="0"), clear=True): res = mcp_server.gitea_list_profiles() profiles = res["profiles"] - self.assertEqual(len(profiles), 2) + self.assertEqual(len(profiles), 3) author_prof = next(p for p in profiles if p["name"] == "author-profile") self.assertTrue(author_prof["is_active"]) @@ -176,6 +211,13 @@ class TestRuntimeClarity(unittest.TestCase): self.assertEqual(reviewer_prof["base_url"], "") self.assertEqual(reviewer_prof["identity_status"], "credentials present") + merger_prof = next(p for p in profiles if p["name"] == "merger-profile") + self.assertFalse(merger_prof["is_active"]) + self.assertEqual(merger_prof["role_kind"], "merger") + self.assertEqual(merger_prof["auth"]["name"], "") + self.assertEqual(merger_prof["base_url"], "") + self.assertEqual(merger_prof["identity_status"], "credentials present") + @patch("mcp_server.api_request", return_value={"login": "author-user"}) @patch("mcp_server.get_auth_header", return_value="token author-pass") def test_list_profiles_revealed_under_opt_in(self, _auth, _api): From 76d1417c284172621e42a3bbdf54e319d8a5a6a1 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 02:19:37 -0400 Subject: [PATCH 03/12] fix: align example config with reviewer merger separation --- docs/gitea-dual-namespace-deployment.md | 13 +++++++++- docs/gitea-execution-profiles.md | 3 ++- docs/wiki/Identity-and-Profiles.md | 15 ++++++++--- gitea-mcp.v2-contexts.example.json | 18 ++++++++++--- tests/test_config_v2_contexts.py | 34 +++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 8 deletions(-) diff --git a/docs/gitea-dual-namespace-deployment.md b/docs/gitea-dual-namespace-deployment.md index 6ee7e8e..f25f09f 100644 --- a/docs/gitea-dual-namespace-deployment.md +++ b/docs/gitea-dual-namespace-deployment.md @@ -22,9 +22,12 @@ launched with exactly one static execution profile: | Namespace (MCP server name) | Profile (role) | Typical use | |-----------------------------|----------------|-------------| | `gitea-author` | an author profile | implement issues, push branches, open PRs, comment | -| `gitea-reviewer` | a reviewer profile | review, approve/request changes, merge | +| `gitea-reviewer` | a reviewer profile | review, approve/request changes | +| `gitea-merger` | a merger profile | merge PRs after approval and verification | | `gitea-reconciler` | a reconciler profile | close already-landed open PRs after ancestry proof (#304 profile; #310 close tool) | +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + Properties: - **One process, one credential.** Each namespace authenticates as exactly @@ -106,6 +109,14 @@ syntax to the client): "GITEA_MCP_CONFIG": "", "GITEA_MCP_PROFILE": "" } + }, + "gitea-merger": { + "command": "/venv/bin/python3", + "args": ["/mcp_server.py"], + "env": { + "GITEA_MCP_CONFIG": "", + "GITEA_MCP_PROFILE": "" + } } } } diff --git a/docs/gitea-execution-profiles.md b/docs/gitea-execution-profiles.md index 3d9958f..970f5d4 100644 --- a/docs/gitea-execution-profiles.md +++ b/docs/gitea-execution-profiles.md @@ -311,7 +311,8 @@ To make Gitea MCP profile activation and runtime identity state explicit, the fo ### 2. Dual MCP Namespaces Recommendation For security-sensitive or high-risk tasks, the preferred safety model uses separate, isolated MCP server instances (namespaces/sessions) launched with static profiles: - `gitea-author`: Exposes tools configured with author permissions; cannot perform approvals or merges. -- `gitea-reviewer`: Exposes tools configured with reviewer permissions; used for PR reviews and merges. +- `gitea-reviewer`: Exposes tools configured with reviewer permissions; used for PR reviews. Review and merge are separate workflow roles. A reviewer approval is not merge authorization. +- `gitea-merger`: Exposes tools configured with merger permissions; used for PR merges. This layout maintains physical separation of credentials and prevents privilege escalation within a single session. This is the model accepted in #139; deployment details, rationale, and client setup live in diff --git a/docs/wiki/Identity-and-Profiles.md b/docs/wiki/Identity-and-Profiles.md index 763a3bb..fb6000d 100644 --- a/docs/wiki/Identity-and-Profiles.md +++ b/docs/wiki/Identity-and-Profiles.md @@ -16,12 +16,21 @@ bind an authenticated Gitea identity to an allowed operation set. - **Allowed:** branch create/push, PR create, issue comment/create/close, repo commit, read. - **Forbidden:** PR approve, merge, request_changes. -### Reviewer / merger +### Reviewer - **Profile:** `prgs-reviewer` - **Typical identity:** `sysadmin` -- **Allowed:** PR review/approve/merge/request_changes, issue comment, read. -- **Forbidden:** branch push, PR create, repo commit. +- **Allowed:** PR review/approve/request_changes, issue comment, read. +- **Forbidden:** branch push, PR create, repo commit, PR merge. + +Review and merge are separate workflow roles. A reviewer approval is not merge authorization. + +### Merger + +- **Profile:** `prgs-merger` +- **Typical identity:** `sysadmin` +- **Allowed:** PR merge, issue comment, read. +- **Forbidden:** branch push, PR create, repo commit, PR approve, PR review, PR request_changes. ## Configuration diff --git a/gitea-mcp.v2-contexts.example.json b/gitea-mcp.v2-contexts.example.json index 4bdad56..bd5b1ee 100644 --- a/gitea-mcp.v2-contexts.example.json +++ b/gitea-mcp.v2-contexts.example.json @@ -52,8 +52,19 @@ "execution_profile": "example-reviewer", "audit_label": "example-reviewer", "auth": { "type": "keychain", "id": "example-gitea-reviewer-token" }, - "allowed_operations": ["read", "review", "comment", "issue.comment", "approve", "request_changes", "merge"], - "forbidden_operations": ["branch", "commit", "push", "open_pr"] + "allowed_operations": ["read", "review", "comment", "issue.comment", "approve", "request_changes"], + "forbidden_operations": ["branch", "commit", "push", "open_pr", "merge"] + }, + "example-merger": { + "enabled": true, + "context": "example-context", + "role": "merger", + "username": "reviewer-user", + "execution_profile": "example-merger", + "audit_label": "example-merger", + "auth": { "type": "keychain", "id": "example-gitea-reviewer-token" }, + "allowed_operations": ["read", "comment", "issue.comment", "merge"], + "forbidden_operations": ["branch", "commit", "push", "open_pr", "approve", "review", "request_changes"] } }, "projects": { @@ -63,7 +74,8 @@ "default_owner": "Example-Org", "default_repo": "Example-Repo", "default_author_profile": "example-author", - "default_reviewer_profile": "example-reviewer" + "default_reviewer_profile": "example-reviewer", + "default_merger_profile": "example-merger" } }, "rules": { diff --git a/tests/test_config_v2_contexts.py b/tests/test_config_v2_contexts.py index 71d3a7b..feb9a80 100644 --- a/tests/test_config_v2_contexts.py +++ b/tests/test_config_v2_contexts.py @@ -435,6 +435,40 @@ class ValidateConfigTests(unittest.TestCase): with open(example, encoding="utf-8") as fh: self.assertEqual(gitea_config.validate_config(json.load(fh)), []) + def test_repo_example_file_reviewer_vs_merger_boundaries(self): + example_path = __import__("pathlib").Path(__file__).resolve().parent.parent \ + / "gitea-mcp.v2-contexts.example.json" + with open(example_path, encoding="utf-8") as fh: + cfg = json.load(fh) + + # 1. Config includes separate reviewer and merger examples + self.assertIn("example-reviewer", cfg["profiles"]) + self.assertIn("example-merger", cfg["profiles"]) + + reviewer = cfg["profiles"]["example-reviewer"] + merger = cfg["profiles"]["example-merger"] + + # 2. Example reviewer config does not include merge capability + reviewer_allowed = reviewer.get("allowed_operations") or [] + reviewer_forbidden = reviewer.get("forbidden_operations") or [] + + self.assertNotIn("merge", reviewer_allowed) + self.assertNotIn("gitea.pr.merge", reviewer_allowed) + self.assertIn("merge", reviewer_forbidden) + + # 3. Example reviewer can resolve review task, cannot resolve merge task + # 4. Example merger can resolve merge task + rev_review_ok, _ = gitea_config.check_operation("gitea.pr.review", reviewer_allowed, reviewer_forbidden) + rev_merge_ok, _ = gitea_config.check_operation("gitea.pr.merge", reviewer_allowed, reviewer_forbidden) + + merg_allowed = merger.get("allowed_operations") or [] + merg_forbidden = merger.get("forbidden_operations") or [] + merg_merge_ok, _ = gitea_config.check_operation("gitea.pr.merge", merg_allowed, merg_forbidden) + + self.assertTrue(rev_review_ok, "example-reviewer must allow review") + self.assertFalse(rev_merge_ok, "example-reviewer must not allow merge") + self.assertTrue(merg_merge_ok, "example-merger must allow merge") + def test_broken_contexts_config_reports_problems(self): data = contexts_config() data["profiles"]["prgs-author"]["context"] = "nope" From 9235f3a23caf9531191380bbaf09b88740a92894 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 03:17:38 -0400 Subject: [PATCH 04/12] feat: require canonical next-action state comments (#495) Add canonical issue/PR/discussion state comment templates and validators, final-report rules for STATE/WHO_IS_NEXT/NEXT_ACTION/NEXT_PROMPT, and queue-controller guidance. Posting enforcement remains in #496. Co-Authored-By: Claude Opus 4.8 (1M context) --- canonical_state_comments.py | 171 ++++++++++++++++ docs/canonical-state-comments.md | 183 ++++++++++++++++++ docs/llm-workflow-runbooks.md | 5 + final_report_validator.py | 19 ++ skills/llm-project-workflow/SKILL.md | 3 +- .../templates/canonical-state-comments.md | 148 ++++++++++++++ tests/test_canonical_state_comments.py | 143 ++++++++++++++ tests/test_final_report_validator.py | 86 ++++++++ 8 files changed, 757 insertions(+), 1 deletion(-) create mode 100644 canonical_state_comments.py create mode 100644 docs/canonical-state-comments.md create mode 100644 skills/llm-project-workflow/templates/canonical-state-comments.md create mode 100644 tests/test_canonical_state_comments.py diff --git a/canonical_state_comments.py b/canonical_state_comments.py new file mode 100644 index 0000000..cc819ac --- /dev/null +++ b/canonical_state_comments.py @@ -0,0 +1,171 @@ +"""Canonical state comment validation helpers (#495). + +These helpers validate durable issue/PR/discussion state handoff comments. +They are intentionally pure and do not post comments; MCP mutation hooks are +owned by #496. +""" + +from __future__ import annotations + +import re + +CANONICAL_HEADINGS = ( + "canonical issue state", + "canonical pr state", + "canonical discussion summary", +) + +REQUIRED_FIELDS = ("STATE", "WHO_IS_NEXT", "NEXT_ACTION", "NEXT_PROMPT") +ALLOWED_NEXT_ACTORS = { + "controller", + "author", + "reviewer", + "merger", + "reconciler", + "user", +} + +VAGUE_NEXT_ACTIONS = { + "continue", + "handle this", + "do it", + "fix it", + "proceed", + "follow up", + "next", + "tbd", + "todo", + "n/a", + "none", +} + +_FIELD_RE = re.compile(r"^\s*(?:[-*]\s*)?([A-Z][A-Z0-9_ ]+)\s*:\s*(.*)$") +_FULL_SHA_RE = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE) +_CLAIMS_STATE_UPDATE_RE = re.compile( + r"canonical\s+(?:issue|pr|discussion)?\s*state|" + r"state\s+comment\s+(?:posted|created|updated)|" + r"next[- ]action\s+comment\s+(?:posted|created|updated)", + re.IGNORECASE, +) + + +def extract_state_fields(text: str | None) -> dict[str, str]: + """Return upper-case labeled fields from a canonical state block.""" + fields: dict[str, str] = {} + current_key: str | None = None + for line in (text or "").splitlines(): + match = _FIELD_RE.match(line) + if match: + current_key = match.group(1).strip().upper().replace(" ", "_") + fields[current_key] = match.group(2).strip() + continue + stripped = line.strip() + if current_key and stripped and not stripped.startswith("#"): + existing = fields.get(current_key, "") + fields[current_key] = ( + f"{existing}\n{stripped}" if existing else stripped + ) + return fields + + +def contains_canonical_state_block(text: str | None) -> bool: + lower = (text or "").lower() + return any(heading in lower for heading in CANONICAL_HEADINGS) + + +def claims_canonical_state_update(text: str | None) -> bool: + """Return True when text claims a canonical state/next-action update.""" + return bool(_CLAIMS_STATE_UPDATE_RE.search(text or "")) + + +def _empty_or_placeholder(value: str | None) -> bool: + value = (value or "").strip().lower() + return not value or value in {"none", "n/a", "unknown", "tbd", "<...>"} + + +def _vague_next_action(value: str | None) -> bool: + normalized = re.sub(r"\s+", " ", (value or "").strip().lower()) + return normalized in VAGUE_NEXT_ACTIONS + + +def validate_canonical_state_comment(text: str | None) -> dict: + """Validate a canonical state comment or embedded final-report block. + + Returns a dict with ``valid`` and ``reasons``. The validator focuses on + fields that make continuation possible: current state, next actor, next + action, and paste-ready next prompt, plus a few contradiction checks. + """ + fields = extract_state_fields(text) + reasons: list[str] = [] + + for field in REQUIRED_FIELDS: + if _empty_or_placeholder(fields.get(field)): + reasons.append(f"missing required canonical state field: {field}") + + actor = (fields.get("WHO_IS_NEXT") or "").strip().lower() + if actor and actor not in ALLOWED_NEXT_ACTORS: + reasons.append( + "WHO_IS_NEXT must be one of: " + + ", ".join(sorted(ALLOWED_NEXT_ACTORS)) + ) + + if _vague_next_action(fields.get("NEXT_ACTION")): + reasons.append("NEXT_ACTION is too vague for durable continuation") + + state = (fields.get("STATE") or "").strip().lower().replace("-", "_") + if "ready_to_merge" in state or ( + state == "approved" and "pr" in (text or "").lower() + ): + review_status = (fields.get("REVIEW_STATUS") or "").lower() + head_sha = fields.get("HEAD_SHA") or "" + merge_ready = (fields.get("MERGE_READY") or "").lower() + if "approved" not in review_status: + reasons.append("ready-to-merge state requires approved REVIEW_STATUS") + if not _FULL_SHA_RE.search(head_sha): + reasons.append("ready-to-merge state requires full HEAD_SHA proof") + if merge_ready and not merge_ready.startswith(("yes", "true")): + reasons.append("ready-to-merge state contradicts MERGE_READY") + + if "superseded" in state: + canonical = fields.get("CANONICAL_ITEM") or fields.get("SUPERSEDED_BY") + if _empty_or_placeholder(canonical): + reasons.append("superseded state requires canonical item proof") + + if "blocked" in state: + blockers = fields.get("BLOCKERS") or "" + if _empty_or_placeholder(blockers): + reasons.append("blocked state requires BLOCKERS/unblock condition") + + return { + "valid": not reasons, + "fields": fields, + "reasons": reasons, + } + + +def validate_final_report_state_update(report_text: str | None) -> dict: + """Validate canonical state-update claims inside a final report.""" + text = report_text or "" + if not claims_canonical_state_update(text) and not contains_canonical_state_block(text): + return { + "applicable": False, + "valid": True, + "reasons": [], + } + + if not contains_canonical_state_block(text): + return { + "applicable": True, + "valid": False, + "reasons": [ + "final report claims a canonical state update but includes no canonical state block" + ], + } + + result = validate_canonical_state_comment(text) + return { + "applicable": True, + "valid": result["valid"], + "fields": result["fields"], + "reasons": result["reasons"], + } diff --git a/docs/canonical-state-comments.md b/docs/canonical-state-comments.md new file mode 100644 index 0000000..555a91b --- /dev/null +++ b/docs/canonical-state-comments.md @@ -0,0 +1,183 @@ +# Canonical State Comments + +Gitea is the durable system of record for workflow continuation. When a +comment changes issue, PR, or discussion state, it should leave enough +information for the next role to continue without private chat history. + +Canonical comments answer: + +- what state the object is in +- who acts next +- what the next actor should do +- the exact prompt the next actor should run +- which proof, blocker, or dependency matters + +Non-workflow discussion comments do not need this template. + +## Issue State + +Use this when an issue becomes ready, blocked, in progress, PR-open, +superseded, merged, or otherwise changes workflow direction. + +```text +## Canonical Issue State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +RELATED_DISCUSSION: + + +RELATED_PRS: +- #... + +BRANCH: + + +HEAD_SHA: +<40-character SHA or none> + +VALIDATION: + + +BLOCKERS: + + +LAST_UPDATED_BY: + +``` + +## PR State + +Use this when a PR needs review, receives changes requested, is approved, +is stale, is superseded, or becomes ready for merge. + +```text +## Canonical PR State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +ISSUE: +#... + +BASE: + + +HEAD: + + +HEAD_SHA: +<40-character SHA> + +REVIEW_STATUS: + + +VALIDATION: + + +BLOCKERS: + + +SUPERSEDES: + + +SUPERSEDED_BY: + + +MERGE_READY: + + +LAST_UPDATED_BY: + +``` + +## Discussion Summary + +Discussions should normally have at least five substantive comments before +conversion into issues. A controller may waive that only for tiny mechanical, +urgent, or explicitly trivial work. + +```text +## Canonical Discussion Summary + +STATE: + + +WHO_IS_NEXT: + + +DECISION: + + +WHY: + + +SUBSTANTIVE_COMMENTS: + + +ISSUES_TO_CREATE_OR_CREATED: +- #... + +DEPENDENCY_ORDER: + + +NON_GOALS: + + +OPEN_QUESTIONS: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +LAST_UPDATED_BY: + +``` + +## Validation Rules + +The final-report validator rejects canonical state update claims when the +report omits the canonical block or when the block lacks: + +- `STATE` +- `WHO_IS_NEXT` +- `NEXT_ACTION` +- `NEXT_PROMPT` + +It also rejects vague next actions such as `continue`, ready-to-merge states +without approval/head-SHA proof, superseded states without canonical item +proof, and blocked states without an unblock condition. diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index e0ff616..1c4f05d 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -7,6 +7,11 @@ package of the MCP Control Plane: creating issues, implementing them, opening and reviewing pull requests, merging, and closing out — safely and reproducibly. +Canonical state comments for durable issue/PR/discussion continuation are +documented in [`canonical-state-comments.md`](canonical-state-comments.md). +Use them when a workflow-changing comment needs to leave the next actor, next +action, and paste-ready prompt in Gitea. + > For the **project-agnostic** version of these operating rules (issue-first, > isolated worktrees, no self-review/merge, profile safety, cleanup, fail-closed) > that can be copied into any repository, see the reusable skill diff --git a/final_report_validator.py b/final_report_validator.py index 352a3ff..bf9baa4 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -1042,10 +1042,29 @@ def _rule_reviewer_review_mutation( ) +def _rule_shared_canonical_state_update(report_text: str) -> list[dict[str, str]]: + from canonical_state_comments import validate_final_report_state_update + + result = validate_final_report_state_update(report_text) + if not result.get("applicable") or result.get("valid"): + return [] + return [ + validator_finding( + "shared.canonical_state_update", + "block", + "Canonical state update", + reason, + "include a canonical state block with STATE, WHO_IS_NEXT, NEXT_ACTION, and NEXT_PROMPT", + ) + for reason in (result.get("reasons") or ["invalid canonical state update"]) + ] + + _SHARED_ISSUE_LOCK_RULES = ( _rule_shared_issue_lock_external_state, _rule_shared_manual_lock_pr_override, _rule_shared_author_reviewer_same_run, + _rule_shared_canonical_state_update, ) _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index dc2f961..956289b 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -167,6 +167,7 @@ Ready-to-copy task prompts live in [`templates/`](templates/): - [`reconcile-closed-not-merged-pr.md`](templates/reconcile-closed-not-merged-pr.md) - [`worktree-cleanup.md`](templates/worktree-cleanup.md) - [`release-tag.md`](templates/release-tag.md) +- [`canonical-state-comments.md`](templates/canonical-state-comments.md) ## Adapting to a project @@ -182,4 +183,4 @@ 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`. diff --git a/skills/llm-project-workflow/templates/canonical-state-comments.md b/skills/llm-project-workflow/templates/canonical-state-comments.md new file mode 100644 index 0000000..d6ec2fd --- /dev/null +++ b/skills/llm-project-workflow/templates/canonical-state-comments.md @@ -0,0 +1,148 @@ +# Template: canonical state comments + +Use these only for workflow-changing comments. Casual discussion does not need +the full template. + +## Issue + +```text +## Canonical Issue State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +RELATED_DISCUSSION: + + +RELATED_PRS: +- #... + +BRANCH: + + +HEAD_SHA: +<40-character SHA or none> + +VALIDATION: + + +BLOCKERS: + + +LAST_UPDATED_BY: + +``` + +## PR + +```text +## Canonical PR State + +STATE: + + +WHO_IS_NEXT: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +WHAT_HAPPENED: + + +WHY: + + +ISSUE: +#... + +BASE: + + +HEAD: + + +HEAD_SHA: +<40-character SHA> + +REVIEW_STATUS: + + +VALIDATION: + + +BLOCKERS: + + +SUPERSEDES: + + +SUPERSEDED_BY: + + +MERGE_READY: + + +LAST_UPDATED_BY: + +``` + +## Discussion + +```text +## Canonical Discussion Summary + +STATE: + + +WHO_IS_NEXT: + + +DECISION: + + +WHY: + + +SUBSTANTIVE_COMMENTS: + + +ISSUES_TO_CREATE_OR_CREATED: +- #... + +DEPENDENCY_ORDER: + + +NON_GOALS: + + +OPEN_QUESTIONS: + + +NEXT_ACTION: + + +NEXT_PROMPT: + + +LAST_UPDATED_BY: + +``` diff --git a/tests/test_canonical_state_comments.py b/tests/test_canonical_state_comments.py new file mode 100644 index 0000000..8f8c5ae --- /dev/null +++ b/tests/test_canonical_state_comments.py @@ -0,0 +1,143 @@ +"""Tests for canonical state comment validation (#495).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from canonical_state_comments import ( # noqa: E402 + validate_canonical_state_comment, + validate_final_report_state_update, +) + + +GOOD_ISSUE_STATE = """## Canonical Issue State + +STATE: +ready-for-author + +WHO_IS_NEXT: +author + +NEXT_ACTION: +Implement the focused validator and open a PR. + +NEXT_PROMPT: +Find issue #495, load the work-issue workflow, implement the validator, and open a PR. + +WHAT_HAPPENED: +Controller selected the canonical tracking issue. + +WHY: +The duplicate issue points here as canonical. + +RELATED_PRS: +none + +BRANCH: +none + +HEAD_SHA: +none + +VALIDATION: +none + +BLOCKERS: +none +""" + + +class TestCanonicalStateComments(unittest.TestCase): + def test_good_issue_state_validates(self): + result = validate_canonical_state_comment(GOOD_ISSUE_STATE) + self.assertTrue(result["valid"]) + self.assertEqual(result["reasons"], []) + + def test_missing_next_actor_is_rejected(self): + result = validate_canonical_state_comment( + GOOD_ISSUE_STATE.replace("WHO_IS_NEXT:\nauthor", "WHO_IS_NEXT:\n") + ) + self.assertFalse(result["valid"]) + self.assertIn("WHO_IS_NEXT", " ".join(result["reasons"])) + + def test_missing_next_prompt_is_rejected(self): + result = validate_canonical_state_comment( + GOOD_ISSUE_STATE.replace( + "NEXT_PROMPT:\nFind issue #495, load the work-issue workflow, implement the validator, and open a PR.", + "NEXT_PROMPT:\n", + ) + ) + self.assertFalse(result["valid"]) + self.assertIn("NEXT_PROMPT", " ".join(result["reasons"])) + + def test_vague_next_action_is_rejected(self): + result = validate_canonical_state_comment( + GOOD_ISSUE_STATE.replace( + "NEXT_ACTION:\nImplement the focused validator and open a PR.", + "NEXT_ACTION:\ncontinue", + ) + ) + self.assertFalse(result["valid"]) + self.assertIn("too vague", " ".join(result["reasons"])) + + def test_ready_to_merge_requires_approval_and_head_sha(self): + text = """## Canonical PR State + +STATE: +ready-to-merge + +WHO_IS_NEXT: +merger + +NEXT_ACTION: +Merge PR #12 after checking the current head. + +NEXT_PROMPT: +Load the merge workflow and merge PR #12 if gates pass. + +ISSUE: +#11 + +HEAD_SHA: +abc123 + +REVIEW_STATUS: +none + +MERGE_READY: +yes +""" + result = validate_canonical_state_comment(text) + self.assertFalse(result["valid"]) + reasons = " ".join(result["reasons"]) + self.assertIn("approved REVIEW_STATUS", reasons) + self.assertIn("full HEAD_SHA", reasons) + + def test_superseded_requires_canonical_item(self): + text = GOOD_ISSUE_STATE.replace("STATE:\nready-for-author", "STATE:\nsuperseded") + result = validate_canonical_state_comment(text) + self.assertFalse(result["valid"]) + self.assertIn("canonical item", " ".join(result["reasons"])) + + def test_blocked_requires_unblock_condition(self): + text = GOOD_ISSUE_STATE.replace("STATE:\nready-for-author", "STATE:\nblocked") + text = text.replace("BLOCKERS:\nnone", "BLOCKERS:\n") + result = validate_canonical_state_comment(text) + self.assertFalse(result["valid"]) + self.assertIn("BLOCKERS", " ".join(result["reasons"])) + + def test_final_report_claim_without_block_is_rejected(self): + report = "Posted canonical state comment on issue #495." + result = validate_final_report_state_update(report) + self.assertTrue(result["applicable"]) + self.assertFalse(result["valid"]) + + def test_final_report_without_state_claim_not_applicable(self): + result = validate_final_report_state_update("ordinary final report") + self.assertFalse(result["applicable"]) + self.assertTrue(result["valid"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 88c5b23..6c510e4 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -345,6 +345,92 @@ class TestReconciliationRules(unittest.TestCase): ) +class TestCanonicalStateUpdateRules(unittest.TestCase): + def test_claimed_state_comment_without_block_blocks(self): + report = _reconcile_handoff() + "\nPosted canonical state comment on PR #99." + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "shared.canonical_state_update" for f in result["findings"]) + ) + + def test_state_comment_missing_next_prompt_blocks(self): + report = ( + _reconcile_handoff() + + """ + +## Canonical PR State + +STATE: +needs-review + +WHO_IS_NEXT: +reviewer + +NEXT_ACTION: +Review PR #99. + +NEXT_PROMPT: + +ISSUE: +#98 + +HEAD_SHA: +0fdc8f582026b72a229d59a172c0a63ac4aaeaf9 + +REVIEW_STATUS: +none + +MERGE_READY: +no +""" + ) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + reasons = " ".join(f["reason"] for f in result["findings"]) + self.assertIn("NEXT_PROMPT", reasons) + + def test_valid_state_comment_claim_passes_shared_rule(self): + report = ( + _reconcile_handoff() + + """ + +## Canonical PR State + +STATE: +needs-review + +WHO_IS_NEXT: +reviewer + +NEXT_ACTION: +Review PR #99 against the pinned head. + +NEXT_PROMPT: +Load the review workflow and review PR #99. + +ISSUE: +#98 + +HEAD_SHA: +0fdc8f582026b72a229d59a172c0a63ac4aaeaf9 + +REVIEW_STATUS: +none + +MERGE_READY: +no + +BLOCKERS: +none +""" + ) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertFalse( + any(f["rule_id"] == "shared.canonical_state_update" for f in result["findings"]) + ) + + class TestCanonicalReconcileSchema(unittest.TestCase): """Issue #307: reconciliation workflows emit only the canonical schema.""" From 429faccd089b4c664390d8c1e97d2ccc30e1d938 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 03:33:31 -0400 Subject: [PATCH 05/12] feat: reject contradictory reviewer handoff ledgers (Closes #501) Add reviewer_handoff_consistency validation for narrative vs mutation ledger contradictions, wire it into review_pr final-report rules, and document the blocked-review handoff template. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/llm-workflow-runbooks.md | 1 + docs/reviewer-handoff-consistency.md | 35 ++++ final_report_validator.py | 21 ++ reviewer_handoff_consistency.py | 186 ++++++++++++++++++ .../templates/blocked-review-handoff.md | 22 +++ tests/test_reviewer_handoff_consistency.py | 120 +++++++++++ 6 files changed, 385 insertions(+) create mode 100644 docs/reviewer-handoff-consistency.md create mode 100644 reviewer_handoff_consistency.py create mode 100644 skills/llm-project-workflow/templates/blocked-review-handoff.md create mode 100644 tests/test_reviewer_handoff_consistency.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index e0ff616..5748d4a 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -821,6 +821,7 @@ scripts/release-tag v0.4.0 --notes-file /tmp/release-notes.md --push ## Related documents +- [`reviewer-handoff-consistency.md`](reviewer-handoff-consistency.md) — reject contradictory reviewer handoffs (#501). - [`../skills/llm-project-workflow/SKILL.md`](../skills/llm-project-workflow/SKILL.md) — portable cross-project LLM workflow skill. - [`gitea-execution-profiles.md`](gitea-execution-profiles.md) — the profile model. - [`gitea-dual-namespace-deployment.md`](gitea-dual-namespace-deployment.md) — static author/reviewer namespace deployment (#139 decision). diff --git a/docs/reviewer-handoff-consistency.md b/docs/reviewer-handoff-consistency.md new file mode 100644 index 0000000..27d8108 --- /dev/null +++ b/docs/reviewer-handoff-consistency.md @@ -0,0 +1,35 @@ +# Reviewer Handoff Consistency + +Reviewer and final-review controller handoffs must not contradict themselves. +A narrative that says a merge happened, a review was blocked, or a terminal +mutation budget was consumed must match the mutation ledger fields in the same +handoff. + +## What gets validated + +`reviewer_handoff_consistency.assess_reviewer_handoff_consistency()` checks: + +- merge claims appear under `Merge mutations` or `MCP/Gitea mutations` +- terminal-mutation-budget claims name the exact prior mutation in the ledger +- blocked review submission is not paired with "final decision marked" +- reviewer lease acquisition includes a `Review decision` +- blocked/rejected mutations include proof fields: + - tool called + - mutation attempted + - mutation rejected + - no server-side state changed + +`final_report_validator` applies this as `reviewer.handoff_consistency` on +`review_pr` reports and fails closed. + +## Blocked review template + +When `gitea_submit_pr_review` fails closed, use +`reviewer_handoff_consistency.render_blocked_review_handoff_template()` or the +copy in +[`skills/llm-project-workflow/templates/blocked-review-handoff.md`](../skills/llm-project-workflow/templates/blocked-review-handoff.md). + +## Related + +- #331 — file-mutation ledger alignment +- #501 — contradictory narrative vs ledger detection \ No newline at end of file diff --git a/final_report_validator.py b/final_report_validator.py index 352a3ff..16cc079 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -12,6 +12,7 @@ import re from typing import Any, Callable import issue_lock_provenance +import reviewer_handoff_consistency from review_proofs import ( HANDOFF_HEADING, assess_controller_handoff, @@ -956,6 +957,25 @@ def _rule_reviewer_validation_structured( ) +def _rule_reviewer_handoff_consistency(report_text: str) -> list[dict[str, str]]: + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency( + report_text + ) + if result.get("proven"): + return [] + return _findings_from_reasons( + "reviewer.handoff_consistency", + result.get("reasons") or [], + field="Review mutations", + severity="block", + safe_next_action=( + "rewrite reviewer handoff so narrative claims match the mutation " + "ledger; use the blocked-review handoff template when submission " + "was rejected" + ), + ) + + def _rule_reviewer_mutation_ledger( report_text: str, *, @@ -1069,6 +1089,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reviewer_already_landed_eligible, _rule_reviewer_already_landed_state, _rule_reviewer_target_branch_freshness, + _rule_reviewer_handoff_consistency, _rule_reviewer_mutation_ledger, _rule_reviewer_review_mutation, _rule_reviewer_stale_head_proof, diff --git a/reviewer_handoff_consistency.py b/reviewer_handoff_consistency.py new file mode 100644 index 0000000..a6deeca --- /dev/null +++ b/reviewer_handoff_consistency.py @@ -0,0 +1,186 @@ +"""Reviewer handoff consistency validation (#501). + +Detects contradictions between narrative claims and mutation ledger fields in +reviewer/final-review controller handoffs. Pure validation only — does not post +comments or mutate Gitea state. +""" + +from __future__ import annotations + +import re + +_HANDOFF_FIELD_RE = re.compile( + r"^\s*[-*]\s*([^:]+):\s*(.*)$", + re.MULTILINE | re.IGNORECASE, +) + +_MERGE_NARRATIVE_RE = re.compile( + r"\b(?:merged\s+pr\s*#|pr\s+#\d+\s+(?:was\s+)?merged|" + r"merge\s+result\s*:\s*merged|successfully\s+merged|" + r"terminal\s+mutation\s+budget.{0,80}\bmerge)\b", + re.IGNORECASE | re.DOTALL, +) +_REVIEW_SUBMIT_BLOCKED_RE = re.compile( + r"\b(?:review\s+submission\s+blocked|submit_pr_review\s+(?:failed|blocked)|" + r"gitea_submit_pr_review\s+(?:failed|blocked|not\s+(?:attempted|performed))|" + r"review\s+not\s+submitted|could\s+not\s+submit\s+review)\b", + re.IGNORECASE, +) +_FINAL_DECISION_MARKED_RE = re.compile( + r"\b(?:gitea_mark_final_review_decision|final\s+review\s+decision\s+marked|" + r"server-side\s+final\s+decision\s+marked|marked\s+final\s+review\s+decision)\b", + re.IGNORECASE, +) +_TERMINAL_BUDGET_CONSUMED_RE = re.compile( + r"\b(?:terminal\s+mutation\s+budget\s+(?:already\s+)?consumed|" + r"single[- ]terminal\s+mutation\s+(?:already\s+)?consumed|" + r"mutation\s+budget\s+exhausted)\b", + re.IGNORECASE, +) +_LEASE_NARRATIVE_RE = re.compile( + r"\b(?:reviewer\s+lease\s+acquired|acquired\s+reviewer\s+pr\s+lease|" + r"gitea_acquire_reviewer_pr_lease)\b", + re.IGNORECASE, +) +_REJECTED_MUTATION_RE = re.compile( + r"\b(?:mutation\s+rejected|tool\s+failed\s+closed|blocked\s+before\s+mutation|" + r"no\s+server-side\s+state\s+changed)\b", + re.IGNORECASE, +) +_MERGE_IN_LEDGER_RE = re.compile(r"\bmerge\b", re.IGNORECASE) +_REVIEW_SUBMITTED_IN_LEDGER_RE = re.compile( + r"\b(?:submitted|approve|request_changes|review\s+submitted)\b", + re.IGNORECASE, +) +_NONE_VALUE_RE = re.compile(r"^\s*(?:none|not\s+attempted|n/a)\s*$", re.IGNORECASE) + +_BLOCKED_REVIEW_PROOF_FIELDS = ( + "tool called", + "mutation attempted", + "mutation rejected", + "no server-side state changed", +) + + +def render_blocked_review_handoff_template() -> str: + """Return a corrected handoff template for blocked review submissions.""" + return """## Blocked Review Submission Handoff + +- Tool called: gitea_submit_pr_review +- Mutation attempted: yes +- Mutation rejected: yes +- No server-side state changed: confirmed +- Proof/source: +- Prior terminal mutation that consumed budget: +- Review decision (local intent): +- Server-side final decision marked: +- Gitea review submitted: no +- Gitea review blocked because: +- Review mutations: none (submission blocked) +- MCP/Gitea mutations: +- Safe next action: rewrite handoff with consistent ledger before posting +""" + + +def _handoff_fields(text: str | None) -> dict[str, str]: + fields: dict[str, str] = {} + for match in _HANDOFF_FIELD_RE.finditer(text or ""): + key = match.group(1).strip().lower() + value = match.group(2).strip() + fields[key] = value + return fields + + +def _is_none_value(value: str | None) -> bool: + return bool(_NONE_VALUE_RE.match(value or "")) + + +def _ledger_mentions_merge(*values: str | None) -> bool: + blob = " ".join(v for v in values if v) + return bool(blob) and bool(_MERGE_IN_LEDGER_RE.search(blob)) + + +def _ledger_mentions_review_submission(*values: str | None) -> bool: + blob = " ".join(v for v in values if v) + return bool(blob) and bool(_REVIEW_SUBMITTED_IN_LEDGER_RE.search(blob)) + + +def assess_reviewer_handoff_consistency(report_text: str | None) -> dict: + """Validate reviewer handoff narrative against mutation ledger fields.""" + text = report_text or "" + fields = _handoff_fields(text) + reasons: list[str] = [] + + review_mutations = fields.get("review mutations", "") + merge_mutations = fields.get("merge mutations", "") + mcp_mutations = fields.get("mcp/gitea mutations", "") + review_decision = fields.get("review decision", "") + + if _MERGE_NARRATIVE_RE.search(text) and not _ledger_mentions_merge( + merge_mutations, mcp_mutations, review_mutations + ): + reasons.append( + "narrative claims a merge occurred but mutation ledger omits merge" + ) + + if _TERMINAL_BUDGET_CONSUMED_RE.search(text): + if _ledger_mentions_merge(merge_mutations, mcp_mutations): + pass + elif _LEASE_NARRATIVE_RE.search(text) and not _ledger_mentions_merge( + merge_mutations, mcp_mutations + ): + reasons.append( + "terminal mutation budget attributed to merge but ledger only " + "shows lease or non-merge activity" + ) + elif not _ledger_mentions_merge(merge_mutations, mcp_mutations): + reasons.append( + "terminal mutation budget consumed claim must identify the " + "exact prior mutation in the ledger" + ) + + if _REVIEW_SUBMIT_BLOCKED_RE.search(text) and _FINAL_DECISION_MARKED_RE.search(text): + reasons.append( + "handoff claims review submission blocked but also claims " + "server-side final decision was marked" + ) + + lease_claimed = _LEASE_NARRATIVE_RE.search(text) or ( + not _is_none_value(mcp_mutations) + and "lease" in mcp_mutations.lower() + ) + if lease_claimed and _is_none_value(review_decision): + reasons.append( + "reviewer lease acquired but Review decision field is missing or none" + ) + + if _REVIEW_SUBMIT_BLOCKED_RE.search(text) or _REJECTED_MUTATION_RE.search(text): + lower = text.lower() + missing_proof = [ + field + for field in _BLOCKED_REVIEW_PROOF_FIELDS + if field not in lower + ] + if missing_proof: + reasons.append( + "blocked/rejected mutation claim missing proof fields: " + + ", ".join(missing_proof) + ) + + if ( + _FINAL_DECISION_MARKED_RE.search(text) + and _is_none_value(review_mutations) + and not _ledger_mentions_review_submission(mcp_mutations) + and not _REVIEW_SUBMIT_BLOCKED_RE.search(text) + ): + reasons.append( + "final review decision marked but Review mutations ledger is empty" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "reasons": reasons, + "fields": fields, + } \ No newline at end of file diff --git a/skills/llm-project-workflow/templates/blocked-review-handoff.md b/skills/llm-project-workflow/templates/blocked-review-handoff.md new file mode 100644 index 0000000..88cb764 --- /dev/null +++ b/skills/llm-project-workflow/templates/blocked-review-handoff.md @@ -0,0 +1,22 @@ +# Blocked review submission handoff + +Use when `gitea_submit_pr_review` or the terminal mutation gate blocks review +submission. + +```text +## Blocked Review Submission Handoff + +- Tool called: gitea_submit_pr_review +- Mutation attempted: yes +- Mutation rejected: yes +- No server-side state changed: confirmed +- Proof/source: +- Prior terminal mutation that consumed budget: +- Review decision (local intent): +- Server-side final decision marked: +- Gitea review submitted: no +- Gitea review blocked because: +- Review mutations: none (submission blocked) +- MCP/Gitea mutations: +- Safe next action: rewrite handoff with consistent ledger before posting +``` \ No newline at end of file diff --git a/tests/test_reviewer_handoff_consistency.py b/tests/test_reviewer_handoff_consistency.py new file mode 100644 index 0000000..ac83124 --- /dev/null +++ b/tests/test_reviewer_handoff_consistency.py @@ -0,0 +1,120 @@ +"""Tests for reviewer handoff consistency validation (#501).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import reviewer_handoff_consistency # noqa: E402 +from final_report_validator import assess_final_report_validator # noqa: E402 + + +def _base_handoff(**overrides): + lines = [ + "## Controller Handoff", + "", + "- Task: review PR #472", + "- Review decision: request_changes", + "- Review mutations: submitted request_changes review", + "- Merge mutations: none", + "- MCP/Gitea mutations: reviewer lease acquired", + ] + text = "\n".join(lines) + for key, value in overrides.items(): + token = f"- {key}:" + if token in text: + before, _, after = text.partition(token) + rest = after.split("\n", 1) + suffix = rest[1] if len(rest) > 1 else "" + text = f"{before}{token} {value}" + (f"\n{suffix}" if suffix else "") + else: + text += f"\n- {key}: {value}" + return text + + +class TestReviewerHandoffConsistency(unittest.TestCase): + def test_consistent_handoff_passes(self): + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency( + _base_handoff() + ) + self.assertTrue(result["proven"], result["reasons"]) + + def test_claimed_merge_missing_from_ledger(self): + report = _base_handoff( + **{ + "Current status": "Merged PR #411 earlier; reviewing PR #472", + "Merge mutations": "none", + "MCP/Gitea mutations": "reviewer lease acquired", + } + ) + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("merge" in r.lower() for r in result["reasons"])) + + def test_terminal_budget_consumed_without_merge_in_ledger(self): + report = _base_handoff( + **{ + "Current status": "Terminal mutation budget already consumed by merge", + "Merge mutations": "none", + "MCP/Gitea mutations": "reviewer lease acquired", + } + ) + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("terminal mutation budget" in r for r in result["reasons"])) + + def test_review_blocked_but_final_decision_marked(self): + report = _base_handoff( + **{ + "Current status": "gitea_submit_pr_review blocked", + "Review mutations": "none", + } + ) + report += "\nServer-side final decision marked via gitea_mark_final_review_decision." + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("blocked" in r and "final decision" in r for r in result["reasons"])) + + def test_lease_without_review_decision(self): + report = _base_handoff(**{"Review decision": "none"}) + report += "\nReviewer lease acquired for PR #472." + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("Review decision" in r for r in result["reasons"])) + + def test_rejected_mutation_without_proof(self): + report = _base_handoff( + **{ + "Review mutations": "none", + "MCP/Gitea mutations": "none", + } + ) + report += "\nReview submission blocked before mutation." + result = reviewer_handoff_consistency.assess_reviewer_handoff_consistency(report) + self.assertFalse(result["proven"]) + self.assertTrue(any("proof fields" in r for r in result["reasons"])) + + def test_blocked_template_includes_required_fields(self): + template = reviewer_handoff_consistency.render_blocked_review_handoff_template() + for field in reviewer_handoff_consistency._BLOCKED_REVIEW_PROOF_FIELDS: + self.assertIn(field, template.lower()) + + def test_final_report_validator_integration(self): + report = _base_handoff( + **{ + "Current status": "Merged PR #411; terminal mutation budget consumed", + "Merge mutations": "none", + } + ) + result = assess_final_report_validator(report, "review_pr") + self.assertTrue( + any( + f["rule_id"] == "reviewer.handoff_consistency" + for f in result["findings"] + ), + result["findings"], + ) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 25cf323d143f56b7d2a959c5f6cb381c745c619f Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 03:44:58 -0400 Subject: [PATCH 06/12] feat: add Canonical Thread Handoff protocol (Closes #505) Define CTH comment types, base template, parser/validator helpers, protocol documentation with examples, and workflow/runbook/prompt updates so agents discover and post authoritative thread handoffs before acting. Co-Authored-By: Claude Opus 4.8 (1M context) --- canonical_thread_handoff.py | 213 ++++++++++++++++++ docs/canonical-thread-handoff.md | 142 ++++++++++++ docs/llm-workflow-runbooks.md | 21 ++ .../templates/canonical-thread-handoff.md | 36 +++ .../templates/merge-pr.md | 3 + .../templates/review-pr.md | 3 + .../templates/start-issue.md | 3 + .../workflows/review-merge-pr.md | 6 + .../workflows/work-issue.md | 6 + tests/test_canonical_thread_handoff.py | 139 ++++++++++++ 10 files changed, 572 insertions(+) create mode 100644 canonical_thread_handoff.py create mode 100644 docs/canonical-thread-handoff.md create mode 100644 skills/llm-project-workflow/templates/canonical-thread-handoff.md create mode 100644 tests/test_canonical_thread_handoff.py diff --git a/canonical_thread_handoff.py b/canonical_thread_handoff.py new file mode 100644 index 0000000..aa0a85b --- /dev/null +++ b/canonical_thread_handoff.py @@ -0,0 +1,213 @@ +"""Canonical Thread Handoff (CTH) protocol for issue and PR comments (#505). + +A CTH comment is the authoritative workflow handoff in a Gitea issue or PR +thread. It records current state, decisions, blockers, proof, and the exact +next prompt/action for the next LLM or person. +""" + +from __future__ import annotations + +import re +from datetime import datetime, timezone +from typing import Any + +MARKER = "" + +CTH_TERM = "Canonical Thread Handoff" +CTH_ABBREV = "CTH" + +CTH_TYPES = frozenset({ + "State Handoff", + "Controller Decision", + "Author Handoff", + "Reviewer Handoff", + "Merger Handoff", + "Supersession Notice", + "Blocker", +}) + +_REQUIRED_BASE_FIELDS = ( + "status", + "next owner", + "current blocker", + "decision", + "proof", + "next action", + "ready-to-paste prompt", +) + +_HEADING_RE = re.compile( + r"^##\s+CTH:\s*(.+?)\s*$", + re.IGNORECASE | re.MULTILINE, +) +_FIELD_RE = re.compile( + r"^([A-Za-z][A-Za-z0-9 /-]*):\s*(.+?)\s*$", + re.MULTILINE, +) + + +def format_cth_body( + *, + cth_type: str, + status: str, + next_owner: str, + current_blocker: str = "none", + decision: str = "none", + proof: str = "none", + next_action: str = "none", + ready_to_paste_prompt: str = "none", + extra_fields: dict[str, str] | None = None, +) -> str: + """Render a canonical CTH comment body.""" + normalized_type = (cth_type or "").strip() + if normalized_type not in CTH_TYPES: + raise ValueError( + f"unknown CTH type '{cth_type}'; expected one of {sorted(CTH_TYPES)}" + ) + lines = [ + MARKER, + f"## CTH: {normalized_type}", + "", + f"Status: {(status or 'unknown').strip() or 'unknown'}", + f"Next owner: {(next_owner or 'unknown').strip() or 'unknown'}", + f"Current blocker: {(current_blocker or 'none').strip() or 'none'}", + f"Decision: {(decision or 'none').strip() or 'none'}", + f"Proof: {(proof or 'none').strip() or 'none'}", + f"Next action: {(next_action or 'none').strip() or 'none'}", + f"Ready-to-paste prompt: {(ready_to_paste_prompt or 'none').strip() or 'none'}", + ] + for key, value in (extra_fields or {}).items(): + label = (key or "").strip() + if not label: + continue + lines.append(f"{label}: {(value or 'none').strip() or 'none'}") + return "\n".join(lines) + + +def parse_cth_comment(body: str) -> dict[str, Any] | None: + """Parse one CTH comment body, or None when not a CTH comment.""" + text = body or "" + if MARKER not in text and not _HEADING_RE.search(text): + return None + heading = _HEADING_RE.search(text) + if not heading: + return None + cth_type = heading.group(1).strip() + fields: dict[str, str] = {} + for match in _FIELD_RE.finditer(text): + key = match.group(1).strip().lower() + if key.startswith("cth"): + continue + fields[key] = match.group(2).strip() + return { + "cth_type": cth_type, + "fields": fields, + "raw_body": text, + } + + +def assess_cth_comment(body: str) -> dict[str, Any]: + """Validate a CTH comment has required base fields and a known type.""" + parsed = parse_cth_comment(body) + reasons: list[str] = [] + if not parsed: + return { + "valid": False, + "block": True, + "reasons": ["comment is not a Canonical Thread Handoff (CTH)"], + "parsed": None, + } + + cth_type = parsed.get("cth_type") or "" + if cth_type not in CTH_TYPES: + reasons.append( + f"unknown CTH type '{cth_type}'; expected one of {sorted(CTH_TYPES)}" + ) + + fields = parsed.get("fields") or {} + missing = [name for name in _REQUIRED_BASE_FIELDS if not (fields.get(name) or "").strip()] + if missing: + reasons.append( + "CTH missing required fields: " + ", ".join(missing) + ) + + vague_prompt = (fields.get("ready-to-paste prompt") or "").strip().lower() + if vague_prompt in {"", "none", "tbd", "n/a", "todo"}: + reasons.append("ready-to-paste prompt must be concrete and paste-ready") + + return { + "valid": not reasons, + "block": bool(reasons), + "reasons": reasons, + "parsed": parsed, + "cth_type": cth_type, + } + + +def find_latest_cth(comments: list[dict[str, Any]]) -> dict[str, Any] | None: + """Return the newest valid CTH comment from a Gitea comment list.""" + latest: dict[str, Any] | None = None + latest_ts: datetime | None = None + for comment in comments or []: + body = comment.get("body") or "" + parsed = parse_cth_comment(body) + if not parsed: + continue + created = _parse_timestamp(comment.get("created_at")) + if latest is None or (created and (latest_ts is None or created >= latest_ts)): + latest = { + "comment_id": comment.get("id"), + "created_at": comment.get("created_at"), + "author": (comment.get("user") or {}).get("login"), + **parsed, + } + latest_ts = created + return latest + + +def assess_cth_supersedes_non_cth( + *, + latest_cth: dict[str, Any] | None, + narrative_claim: str, +) -> dict[str, Any]: + """Fail closed when narrative relies on stale non-CTH state despite newer CTH.""" + if not latest_cth: + return {"block": False, "reasons": []} + text = (narrative_claim or "").strip() + if not text: + return {"block": False, "reasons": []} + if parse_cth_comment(text): + return {"block": False, "reasons": []} + return { + "block": True, + "reasons": [ + "workflow narrative must treat the latest CTH comment as authoritative; " + f"found newer CTH type '{latest_cth.get('cth_type')}' " + f"(comment #{latest_cth.get('comment_id')})" + ], + } + + +def _parse_timestamp(value: str | None) -> datetime | None: + if not value: + return None + text = value.strip() + if text.endswith("Z"): + text = text[:-1] + "+00:00" + try: + parsed = datetime.fromisoformat(text) + except ValueError: + return None + if parsed.tzinfo is None: + return parsed.replace(tzinfo=timezone.utc) + return parsed.astimezone(timezone.utc) + + +EXAMPLE_SCENARIOS = ( + "pr_approved_ready_for_merger", + "pr_request_changes_to_author", + "duplicate_superseded_pr_closure", + "blocked_dirty_root_worktree", + "stale_head_requires_fresh_review", + "issue_implementation_handoff", +) \ No newline at end of file diff --git a/docs/canonical-thread-handoff.md b/docs/canonical-thread-handoff.md new file mode 100644 index 0000000..0e45000 --- /dev/null +++ b/docs/canonical-thread-handoff.md @@ -0,0 +1,142 @@ +# Canonical Thread Handoff (CTH) + +**CTH** = **Canonical Thread Handoff** + +A CTH comment is the authoritative workflow handoff in a Gitea issue or PR +thread. It records current state, decisions, blockers, proof, and the exact +next prompt/action for the next LLM or person. + +CTH comments complement — but do not replace — formal Gitea review state. +A CTH may summarize an APPROVE or REQUEST_CHANGES decision, yet merge gates +still require the live Gitea review verdict. + +## CTH comment types + +- `CTH: State Handoff` +- `CTH: Controller Decision` +- `CTH: Author Handoff` +- `CTH: Reviewer Handoff` +- `CTH: Merger Handoff` +- `CTH: Supersession Notice` +- `CTH: Blocker` + +## Required base template + +```md +## CTH: + +Status: +Next owner: +Current blocker: +Decision: +Proof: +Next action: +Ready-to-paste prompt: +``` + +Extended fields may map to canonical issue/PR state templates from #495 when +that layer is available. Until then, keep the base fields complete. + +## Discovery and posting rules + +Before acting in an issue or PR thread: + +1. **Find the latest CTH comment** before doing work. +2. Treat the **latest valid CTH** as the current handoff state. +3. **Post a new CTH** when you finish, block, skip, supersede, request + changes, approve, or hand off. +4. Do **not** rely on stale non-CTH comments when a newer CTH exists. +5. Casual discussion comments do not need to be CTH comments. + +## Examples + +### PR approved and ready for merger + +```md +## CTH: Reviewer Handoff + +Status: approved_at_current_head +Next owner: merger +Current blocker: none +Decision: APPROVE recorded at head abc123... +Proof: gitea_submit_pr_review performed; visible verdict APPROVE +Next action: eligible merger merges PR #N with pinned expected_head_sha +Ready-to-paste prompt: Merge PR #N for issue #M if live head still abc123... and merge gates pass. +``` + +### PR request-changes back to author + +```md +## CTH: Reviewer Handoff + +Status: request_changes_at_current_head +Next owner: author +Current blocker: unresolved findings in validation report +Decision: REQUEST_CHANGES at head def456... +Proof: gitea_submit_pr_review performed; blocking review visible +Next action: author fixes findings and pushes; reviewer re-validates fresh head +Ready-to-paste prompt: Fix PR #N review findings, push to feat/issue-M-..., post Author Handoff CTH. +``` + +### Duplicate / superseded PR closure + +```md +## CTH: Supersession Notice + +Status: superseded +Next owner: controller +Current blocker: duplicate branch/PR work +Decision: close PR #N; continue on PR #M +Proof: duplicate gate linked open PR #M for issue #K +Next action: controller closes superseded PR and records canonical state +Ready-to-paste prompt: Close superseded PR #N; confirm PR #M remains canonical for issue #K. +``` + +### Blocked workflow due to dirty root/worktree + +```md +## CTH: Blocker + +Status: blocked_preflight +Next owner: operator +Current blocker: dirty control checkout / branches worktree mismatch +Decision: stop before mutation +Proof: verify_preflight_purity failed; workspace diagnostics attached +Next action: repair worktree or relaunch MCP from branches/ worktree +Ready-to-paste prompt: Repair dirty workspace, relaunch MCP from branches/review-prN, retry review. +``` + +### Stale head requiring fresh review + +```md +## CTH: Reviewer Handoff + +Status: stale_head +Next owner: reviewer +Current blocker: live head differs from pinned review head +Decision: prior approval not valid for current head +Proof: expected_head_sha mismatch at merge gate +Next action: re-run validation and post fresh review decision +Ready-to-paste prompt: Re-validate PR #N at live head, dry-run review gates, submit fresh verdict. +``` + +### Issue implementation handoff + +```md +## CTH: Author Handoff + +Status: implementation_complete_pending_review +Next owner: reviewer +Current blocker: none +Decision: PR #N ready for review at head fedcba... +Proof: tests passed in branches/issue-M-...; PR opened with Closes #M +Next action: reviewer acquires lease and validates PR #N +Ready-to-paste prompt: Review PR #N for issue #M; pin head fedcba... before mutations. +``` + +## Related + +- [`llm-workflow-runbooks.md`](llm-workflow-runbooks.md) +- [`../skills/llm-project-workflow/templates/canonical-thread-handoff.md`](../skills/llm-project-workflow/templates/canonical-thread-handoff.md) +- #495 — canonical next-action comment fields +- #496 — fail-closed validation for workflow-changing comments \ No newline at end of file diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d67769d..bba69c9 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -728,6 +728,27 @@ Never imply full-suite success unless the full-suite command itself passed (`full_suite_passed: true`). A report that hides a failed or skipped check is worse than a failing report. +## Canonical Thread Handoff (CTH) + +**CTH** = **Canonical Thread Handoff** — the authoritative workflow handoff +comment in a Gitea issue or PR thread. See +[`canonical-thread-handoff.md`](canonical-thread-handoff.md) for types, +templates, and examples. + +Before acting in an issue or PR thread: + +1. **Find the latest CTH comment** before doing work. +2. Treat the **latest valid CTH** as the current handoff state. +3. **Post a new CTH** when you finish, block, skip, supersede, request + changes, approve, or hand off. +4. Do **not** rely on stale non-CTH comments when a newer CTH exists. + +A CTH summarizes workflow state for the next session. Formal Gitea review +verdicts remain authoritative for merge gates — a CTH is not merge approval +by itself. + +Template: [`../skills/llm-project-workflow/templates/canonical-thread-handoff.md`](../skills/llm-project-workflow/templates/canonical-thread-handoff.md) + ## Controller Handoff (required, every task) Every task — implementation, review, merge, triage, documentation, diff --git a/skills/llm-project-workflow/templates/canonical-thread-handoff.md b/skills/llm-project-workflow/templates/canonical-thread-handoff.md new file mode 100644 index 0000000..37a76e5 --- /dev/null +++ b/skills/llm-project-workflow/templates/canonical-thread-handoff.md @@ -0,0 +1,36 @@ +# Template: Canonical Thread Handoff (CTH) + +Copy, fill the fields, and post as an issue or PR comment. + +```text +## CTH: + +Status: +Next owner: +Current blocker: +Decision: +Proof: +Next action: +Ready-to-paste prompt: +``` + +Allowed `` values: + +- State Handoff +- Controller Decision +- Author Handoff +- Reviewer Handoff +- Merger Handoff +- Supersession Notice +- Blocker + +Rules: + +- Find the latest CTH comment in the thread before starting work. +- Treat the latest valid CTH as the current source of truth. +- Post a new CTH when you finish, block, skip, supersede, approve, request + changes, or hand off. +- A CTH summarizes workflow state; formal Gitea review verdicts remain + authoritative for merge gates. + +Full protocol: `docs/canonical-thread-handoff.md` \ No newline at end of file diff --git a/skills/llm-project-workflow/templates/merge-pr.md b/skills/llm-project-workflow/templates/merge-pr.md index ae99544..f891510 100644 --- a/skills/llm-project-workflow/templates/merge-pr.md +++ b/skills/llm-project-workflow/templates/merge-pr.md @@ -10,6 +10,9 @@ Load the canonical workflow first: Final report schema: `schemas/review-merge-final-report.md`. Rules (llm-project-workflow): +- Find the latest CTH comment on the PR/issue thread before starting work. + Post a new CTH: Merger Handoff (or CTH: Blocker) at session end. + Template: skills/llm-project-workflow/templates/canonical-thread-handoff.md - Only an eligible, NON-author reviewer merges. If authenticated user == PR author → STOP. - Do not merge unless the PR is open, mergeable, and its checks/review pass. diff --git a/skills/llm-project-workflow/templates/review-pr.md b/skills/llm-project-workflow/templates/review-pr.md index 391f962..3a5ded8 100644 --- a/skills/llm-project-workflow/templates/review-pr.md +++ b/skills/llm-project-workflow/templates/review-pr.md @@ -35,6 +35,9 @@ Load the canonical workflow first: Final report schema: `schemas/review-merge-final-report.md`. Rules (llm-project-workflow): +- Find the latest CTH comment on the PR/issue thread before starting work. + Post a new CTH: Reviewer Handoff (or CTH: Blocker) at session end. + Template: skills/llm-project-workflow/templates/canonical-thread-handoff.md - Review in a SEPARATE detached review worktree, never the author's folder. - Worktree safety (#233): before checkout, diff, validation, review, or merge, report the starting worktree path and whether it was dirty. If unrelated diff --git a/skills/llm-project-workflow/templates/start-issue.md b/skills/llm-project-workflow/templates/start-issue.md index 4b8c159..b483d18 100644 --- a/skills/llm-project-workflow/templates/start-issue.md +++ b/skills/llm-project-workflow/templates/start-issue.md @@ -10,6 +10,9 @@ Final report schema: skills/llm-project-workflow/schemas/work-issue-final-report Router: skills/llm-project-workflow/SKILL.md (task mode: work-issue) Rules (llm-project-workflow): +- Find the latest CTH comment on the issue/PR thread before starting work. + Post a new CTH: Author Handoff (or CTH: Blocker) at session end. + Template: skills/llm-project-workflow/templates/canonical-thread-handoff.md - No repo changes without a tracking issue. If none exists, create one first; if it can't be created, stop. - Work only in an isolated branch worktree under branches/. The main checkout diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 1c4e9be..c620447 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -26,6 +26,12 @@ workflow rules exactly. ## 0. Load the canonical workflow first +Before starting PR work, **find the latest CTH comment** on the PR or linked +issue thread. Treat that CTH as the current handoff state. Post a new +**CTH: Reviewer Handoff**, **CTH: Merger Handoff**, **CTH: Blocker**, or +**CTH: Supersession Notice** when you finish, block, skip, supersede, +approve, request changes, or hand off. See `docs/canonical-thread-handoff.md`. + Before starting PR work, check whether the project provides a canonical PR review/merge workflow through a project skill, runbook, or MCP helper. If available, load it first and report: diff --git a/skills/llm-project-workflow/workflows/work-issue.md b/skills/llm-project-workflow/workflows/work-issue.md index 6766523..8bc0c31 100644 --- a/skills/llm-project-workflow/workflows/work-issue.md +++ b/skills/llm-project-workflow/workflows/work-issue.md @@ -26,6 +26,12 @@ This is an author/coder workflow. It is not a reviewer workflow. ## 0. Load the canonical workflow first +Before starting issue work, **find the latest CTH comment** on the target +issue or linked PR thread. Treat that CTH as the current handoff state unless +a newer authoritative gate supersedes it. Post a new **CTH: Author Handoff** +(or `CTH: Blocker`) when you finish, block, or hand off. +See `docs/canonical-thread-handoff.md`. + Before starting issue work, check whether the project provides a canonical work-on-issue workflow through a project skill, runbook, or MCP helper. If available, load it first and report: diff --git a/tests/test_canonical_thread_handoff.py b/tests/test_canonical_thread_handoff.py new file mode 100644 index 0000000..96b6882 --- /dev/null +++ b/tests/test_canonical_thread_handoff.py @@ -0,0 +1,139 @@ +"""Tests for Canonical Thread Handoff (CTH) protocol (#505).""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import canonical_thread_handoff as cth # noqa: E402 + +REPO_ROOT = Path(__file__).resolve().parent.parent +RUNBOOK = REPO_ROOT / "docs" / "llm-workflow-runbooks.md" +PROTOCOL_DOC = REPO_ROOT / "docs" / "canonical-thread-handoff.md" +TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "canonical-thread-handoff.md" +REVIEW_WORKFLOW = REPO_ROOT / "skills" / "llm-project-workflow/workflows" / "review-merge-pr.md" +WORK_WORKFLOW = REPO_ROOT / "skills" / "llm-project-workflow/workflows" / "work-issue.md" +REVIEW_TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "review-pr.md" +START_TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "start-issue.md" +MERGE_TEMPLATE = REPO_ROOT / "skills" / "llm-project-workflow/templates" / "merge-pr.md" + + +class TestCthModule(unittest.TestCase): + def test_defines_cth_term_and_types(self): + self.assertEqual(cth.CTH_ABBREV, "CTH") + self.assertIn("State Handoff", cth.CTH_TYPES) + self.assertIn("Reviewer Handoff", cth.CTH_TYPES) + self.assertIn("Blocker", cth.CTH_TYPES) + + def test_format_and_parse_round_trip(self): + body = cth.format_cth_body( + cth_type="Author Handoff", + status="implementation_complete", + next_owner="reviewer", + current_blocker="none", + decision="PR opened", + proof="pytest passed", + next_action="review PR #9", + ready_to_paste_prompt="Review PR #9 for issue #5", + ) + parsed = cth.parse_cth_comment(body) + self.assertIsNotNone(parsed) + self.assertEqual(parsed["cth_type"], "Author Handoff") + self.assertEqual(parsed["fields"]["status"], "implementation_complete") + self.assertEqual(parsed["fields"]["next owner"], "reviewer") + + def test_valid_cth_passes_assessment(self): + body = cth.format_cth_body( + cth_type="Reviewer Handoff", + status="approved", + next_owner="merger", + current_blocker="none", + decision="APPROVE", + proof="review id 42", + next_action="merge if gates pass", + ready_to_paste_prompt="Merge PR #12 with pinned head abcdef0123456789abcdef0123456789abcdef01", + ) + result = cth.assess_cth_comment(body) + self.assertFalse(result["block"]) + self.assertTrue(result["valid"]) + + def test_missing_fields_fail_closed(self): + body = "## CTH: Blocker\n\nStatus: blocked\n" + result = cth.assess_cth_comment(body) + self.assertTrue(result["block"]) + self.assertIn("missing required fields", result["reasons"][0]) + + def test_find_latest_cth_prefers_newer_comment(self): + older = cth.format_cth_body( + cth_type="State Handoff", + status="old", + next_owner="author", + current_blocker="none", + decision="old", + proof="old", + next_action="old", + ready_to_paste_prompt="old prompt text here", + ) + newer = cth.format_cth_body( + cth_type="Reviewer Handoff", + status="new", + next_owner="merger", + current_blocker="none", + decision="approve", + proof="new", + next_action="merge", + ready_to_paste_prompt="merge PR #3 now with pinned head", + ) + comments = [ + {"id": 1, "created_at": "2026-07-01T10:00:00Z", "body": older}, + {"id": 2, "created_at": "2026-07-02T10:00:00Z", "body": newer}, + ] + latest = cth.find_latest_cth(comments) + self.assertEqual(latest["comment_id"], 2) + self.assertEqual(latest["cth_type"], "Reviewer Handoff") + + def test_cth_does_not_replace_formal_reviews_documented_in_protocol(self): + text = PROTOCOL_DOC.read_text(encoding="utf-8") + self.assertIn("do not replace", text.lower()) + self.assertIn("formal Gitea review", text) + + def test_examples_cover_required_scenarios(self): + text = PROTOCOL_DOC.read_text(encoding="utf-8") + expected_phrases = ( + "PR approved and ready for merger", + "request-changes back to author", + "superseded PR closure", + "dirty root/worktree", + "Stale head requiring fresh review", + "Issue implementation handoff", + ) + for phrase in expected_phrases: + self.assertIn(phrase, text) + + +class TestCthDocumentationChecks(unittest.TestCase): + def test_runbook_references_cth_discovery_and_posting(self): + text = RUNBOOK.read_text(encoding="utf-8") + self.assertIn("Canonical Thread Handoff", text) + self.assertIn("Find the latest CTH comment", text) + self.assertIn("Post a new CTH", text) + self.assertIn("canonical-thread-handoff.md", text) + + def test_workflows_reference_cth_rules(self): + for path in (REVIEW_WORKFLOW, WORK_WORKFLOW): + text = path.read_text(encoding="utf-8") + self.assertIn("latest CTH comment", text) + self.assertIn("canonical-thread-handoff.md", text) + + def test_prompt_templates_reference_cth_rules(self): + for path in (REVIEW_TEMPLATE, START_TEMPLATE, MERGE_TEMPLATE, TEMPLATE): + text = path.read_text(encoding="utf-8") + self.assertIn("CTH", text) + self.assertIn("canonical-thread-handoff", text) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From b7755f26e3709d7c73dda389e8124e1e55e8df36 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 04:20:24 -0400 Subject: [PATCH 07/12] feat: require workflow labels for issues (Closes #513) --- create_issue.py | 54 +++++- docs/label-taxonomy.md | 112 +++++++++--- docs/llm-workflow-runbooks.md | 15 +- gitea_mcp_server.py | 264 +++++++++++++++++++++++++-- issue_workflow_labels.py | 194 ++++++++++++++++++++ manage_labels.py | 7 +- task_capability_map.py | 5 + tests/test_create_issue.py | 31 ++++ tests/test_issue_workflow_labels.py | 73 ++++++++ tests/test_issue_write_tool_gates.py | 11 +- tests/test_manage_labels.py | 6 + tests/test_mcp_server.py | 139 ++++++++++++-- 12 files changed, 838 insertions(+), 73 deletions(-) create mode 100644 issue_workflow_labels.py create mode 100644 tests/test_issue_workflow_labels.py diff --git a/create_issue.py b/create_issue.py index 6ec1b4b..5a93532 100644 --- a/create_issue.py +++ b/create_issue.py @@ -29,6 +29,7 @@ from gitea_auth import ( get_credentials, resolve_remote, add_remote_args, api_request, repo_api_url, ) +import issue_workflow_labels def main(argv=None): @@ -38,6 +39,16 @@ def main(argv=None): parser.add_argument("--body", default="", help="Issue body text.") parser.add_argument("--body-file", help="Read issue body from this file ('-' for stdin).") + parser.add_argument("--label", action="append", default=[], + help="Existing label name to apply; may be repeated.") + parser.add_argument("--type-label", + help="Issue type, e.g. feature or type:feature.") + parser.add_argument("--status-label", + help="Initial status, e.g. ready or status:ready.") + parser.add_argument("--discussion", action="store_true", + help="Apply type:discussion.") + parser.add_argument("--require-workflow-labels", action="store_true", + help="Fail unless one type:* and one status:* label are supplied.") args = parser.parse_args(argv) host, org, repo = resolve_remote(args) @@ -50,6 +61,24 @@ def main(argv=None): with open(args.body_file, "r", encoding="utf-8") as fh: body = fh.read() + requested_labels = issue_workflow_labels.labels_for_new_issue( + issue_type=args.type_label, + initial_status=args.status_label, + extra_labels=args.label, + discussion=args.discussion, + ) + label_assessment = issue_workflow_labels.assess_issue_labels( + requested_labels, + discussion=args.discussion, + ) + if args.require_workflow_labels and not label_assessment["valid"]: + print( + "Workflow label validation failed: " + + "; ".join(label_assessment["errors"]), + file=sys.stderr, + ) + return 1 + user, password = get_credentials(host) if not user or not password: print(f"Could not get credentials for {host} " @@ -59,11 +88,34 @@ def main(argv=None): import base64 auth = f"Basic {base64.b64encode(f'{user}:{password}'.encode()).decode()}" - url = f"{repo_api_url(host, org, repo)}/issues" + base = repo_api_url(host, org, repo) + url = f"{base}/issues" try: + label_ids = [] + if requested_labels: + existing = api_request("GET", f"{base}/labels", auth) + by_name = {lb["name"]: lb["id"] for lb in existing} + missing = [name for name in requested_labels if name not in by_name] + if missing: + print(f"Missing labels: {missing}", file=sys.stderr) + return 1 + label_ids = [by_name[name] for name in requested_labels] data = api_request("POST", url, auth, {"title": args.title, "body": body}) + if label_ids: + api_request( + "PUT", + f"{base}/issues/{data.get('number')}/labels", + auth, + {"labels": label_ids}, + ) print(f"Issue #{data.get('number')}: {data.get('html_url')}") + if not label_assessment["valid"]: + print( + "Workflow label recommendation: " + + "; ".join(label_assessment["errors"]), + file=sys.stderr, + ) return 0 except RuntimeError as e: print(f"Error: {e}", file=sys.stderr) diff --git a/docs/label-taxonomy.md b/docs/label-taxonomy.md index e3cf98b..d1c400d 100644 --- a/docs/label-taxonomy.md +++ b/docs/label-taxonomy.md @@ -1,34 +1,98 @@ # Label Taxonomy -This document catalogs the issue labels used for MCP workflows, including Jenkins and GlitchTip (observability). +This document defines the canonical issue labels used by MCP workflows. -> **Approval Required:** Do not create or apply new labels in `manage_labels.py` without explicit owner approval of this document. +Every issue should carry: -## Existing Labels +- one `type:*` label +- one `status:*` label -* **`jenkins`** - * Description: Jenkins integration - * Color: `d93f0b` - * Use: Used to mark issues, PRs, or tasks that involve the `jenkins-mcp` boundaries, CI/CD designs, or build failures. +Discussion-only issues must carry `type:discussion`. -* **`glitchtip`** - * Description: GlitchTip integration - * Color: `b60205` - * Use: Used to mark issues related to the `glitchtip-mcp` boundary and observability integration. +## Issue Type Labels -## Proposed / Missing Labels +| Label | Use | +| --- | --- | +| `type:bug` | Bug or defect | +| `type:feature` | Feature or enhancement | +| `type:process` | Process or policy work | +| `type:workflow` | Workflow automation or guidance | +| `type:guardrail` | Safety gate or guardrail | +| `type:docs` | Documentation work | +| `type:test` | Tests or test infrastructure | +| `type:discussion` | Discussion-only issue | +| `type:umbrella` | Umbrella or tracker issue | +| `type:cleanup` | Cleanup or hygiene work | -* **`observability`** - * Proposed Description: Observability, metrics, and monitoring tasks - * Proposed Color: `5319e7` - * Use: Broader than GlitchTip alone; covers logging, metrics, traces, and general observability pipeline improvements. +## Workflow Status Labels -* **`source:glitchtip`** - * Proposed Description: Issue filed automatically by GlitchTip orchestration - * Proposed Color: `b60205` - * Use: Applied automatically by the orchestrator when a GlitchTip error event is converted into a Gitea issue. +Only one `status:*` label should be active on an issue at a time. When an issue +moves forward, tooling must remove the old `status:*` label and apply the new +one. -* **`status:triage`** - * Proposed Description: Issue needs human or orchestrator triage - * Proposed Color: `fbca04` - * Use: Used for incoming issues (especially automated ones like `source:glitchtip`) that have not yet been evaluated for priority or resolution. +| Label | Use | +| --- | --- | +| `status:triage` | Issue needs triage | +| `status:ready` | Issue is ready for work | +| `status:claimed` | Issue is claimed | +| `status:in-progress` | Issue is being worked on | +| `status:blocked` | Issue is blocked | +| `status:needs-review` | Issue work needs review | +| `status:pr-open` | A linked PR is open | +| `status:approved` | Linked PR is approved | +| `status:merged` | Linked PR is merged | +| `status:reconcile` | Issue needs reconciliation | +| `status:done` | Issue workflow is complete | +| `status:duplicate` | Issue is a duplicate | +| `status:wontfix` | Issue will not be fixed | + +## Transition Rules + +Suggested lifecycle: + +1. New issue created: `status:triage` or `status:ready` +2. Issue selected by an author: `status:claimed` +3. Author starts work: `status:in-progress` +4. Work is blocked: `status:blocked` +5. PR opened: `status:pr-open` +6. PR approved: `status:approved` +7. PR merged but issue still needs closure/reconciliation: `status:reconcile` +8. Issue fully complete: `status:done` +9. Duplicate issue: `status:duplicate` +10. Won't-fix issue: `status:wontfix` + +The helper module `issue_workflow_labels.py` is the source of truth for the +canonical label specs and status transition replacement behavior. + +## Discussion Issues + +Discussion issues must be labeled `type:discussion`. + +A discussion issue should not be treated as implementation-ready unless it also +has a clear implementation status and next action. + +If a discussion produces implementation work, either: + +1. convert the discussion issue into an implementation issue by changing labels + and adding acceptance criteria, or +2. create child implementation issues and leave the discussion issue as + `type:discussion`. + +## Tooling + +- `manage_labels.py --create-labels` creates the canonical `type:*` and + `status:*` labels. +- `gitea_create_issue` recommends `type:*` and `status:*` labels when missing + and can apply supplied label names. +- `gitea_mark_issue(..., action="start")` replaces old `status:*` labels with + `status:in-progress`. +- `gitea_create_pr` fails closed before PR creation if `status:pr-open` cannot + be applied to the locked issue, then applies it after the PR is created. +- `gitea_set_issue_labels` accepts an explicit `worktree_path` so author + sessions can satisfy the branches-only mutation guard while changing labels. + +## Existing Non-Workflow Labels + +Existing non-workflow labels such as `mcp`, `workflow`, `labels`, `tracker`, +`jenkins`, `glitchtip`, `documentation`, and `testing` remain valid topical +labels. They do not replace the required `type:*` and `status:*` labels. diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d67769d..fb77ddf 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -581,19 +581,24 @@ loop and do **not** substitute WebFetch/Playwright/manual base64. - **Profile:** issue-manager or author (any profile allowed to create issues). - **Steps:** create the parent/roadmap issue; create child issues; apply the minimal label set; link children to the parent. +- **Labels:** new issues should carry one `type:*` label and one `status:*` + label. Discussion-only issues must carry `type:discussion`. See + [`label-taxonomy.md`](label-taxonomy.md). - **Prompt:** `Using the issue-manager profile, create issue "" with body <body>, then create child issues for <list> and link them to the parent.` ### Implement an issue and open a PR - **Profile:** author. -- **Steps:** claim the issue (`status:in-progress`); create an isolated branch - worktree from latest `master` under `branches/` (`feat/issue-<n>-...` / +- **Steps:** claim the issue (`status:in-progress`, replacing any old + `status:*` label); create an isolated branch worktree from latest `master` + under `branches/` (`feat/issue-<n>-...` / `fix/...` / `docs/...`); `cd` into that worktree; implement narrowly; add or update tests if behavior changes; run the full suite; commit with an - issue-linked message; open a PR to `master`. **Do not** review or merge your - own PR. Include an `LLM Handoff Metadata` block (with `LLM-Agent-SHA`) in - the PR body — see [`llm-agent-sha.md`](llm-agent-sha.md). + issue-linked message; open a PR to `master`; move the issue to + `status:pr-open`. **Do not** review or merge your own PR. Include an + `LLM Handoff Metadata` block (with `LLM-Agent-SHA`) in the PR body — see + [`llm-agent-sha.md`](llm-agent-sha.md). - **Prompt:** `Use an author profile to implement issue #N and open a PR to master. Do not self-review or self-merge.` diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 411a80b..4a83deb 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -608,6 +608,7 @@ import already_landed_reconcile # noqa: E402 import author_mutation_worktree # noqa: E402 import issue_claim_heartbeat # noqa: E402 import issue_work_duplicate_gate # noqa: E402 +import issue_workflow_labels # noqa: E402 import reviewer_pr_lease # noqa: E402 import merged_cleanup_reconcile # noqa: E402 import reconciler_profile # noqa: E402 @@ -954,6 +955,108 @@ def extract_linked_issue_numbers(text: str | None, branch_name: str | None = Non issues.update(int(m) for m in pattern.findall(branch_name)) return sorted(list(issues)) +def _repo_label_id_map(base: str, auth: str) -> dict[str, int]: + labels = api_get_all(f"{base}/labels", auth) or [] + return { + str(lb["name"]): int(lb["id"]) + for lb in labels + if isinstance(lb, dict) and lb.get("name") and lb.get("id") is not None + } + +def _issue_label_names(base: str, auth: str, issue_number: int) -> list[str]: + issue = api_request("GET", f"{base}/issues/{issue_number}", auth) or {} + return issue_workflow_labels.label_names(issue) + +def _put_issue_label_names( + *, + base: str, + auth: str, + issue_number: int, + names: list[str], + label_ids_by_name: dict[str, int] | None = None, +) -> list[dict]: + by_name = label_ids_by_name or _repo_label_id_map(base, auth) + missing = [name for name in names if name not in by_name] + if missing: + raise RuntimeError( + f"The following labels do not exist on the repository: {missing}. " + "Create the canonical workflow labels first." + ) + ids = [by_name[name] for name in names] + return api_request( + "PUT", + f"{base}/issues/{issue_number}/labels", + auth, + {"labels": ids}, + ) + +def _transition_issue_status( + *, + base: str, + auth: str, + issue_number: int, + status: str, + label_ids_by_name: dict[str, int] | None = None, +) -> dict: + by_name = label_ids_by_name or _repo_label_id_map(base, auth) + target_status = issue_workflow_labels.canonical_status_label(status) + if target_status not in by_name: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "target_status": target_status, + "reasons": [ + f"required workflow status label '{target_status}' does not exist" + ], + } + current = _issue_label_names(base, auth, issue_number) + updated = issue_workflow_labels.transition_status_labels(current, target_status) + _put_issue_label_names( + base=base, + auth=auth, + issue_number=issue_number, + names=updated, + label_ids_by_name=by_name, + ) + return { + "success": True, + "performed": True, + "issue_number": issue_number, + "target_status": target_status, + "labels": updated, + } + +def _prepare_issue_status_transition( + *, + base: str, + auth: str, + issue_number: int, + status: str, +) -> dict: + by_name = _repo_label_id_map(base, auth) + target_status = issue_workflow_labels.canonical_status_label(status) + if target_status not in by_name: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "target_status": target_status, + "reasons": [ + f"required workflow status label '{target_status}' does not exist" + ], + } + current = _issue_label_names(base, auth, issue_number) + updated = issue_workflow_labels.transition_status_labels(current, target_status) + return { + "success": True, + "performed": False, + "issue_number": issue_number, + "target_status": target_status, + "labels": updated, + "label_ids_by_name": by_name, + } + def release_in_progress_label(issue_numbers: list[int], remote: str, host: str | None, org: str | None, repo: str | None) -> dict: if not issue_numbers: return {} @@ -1248,6 +1351,11 @@ def gitea_create_issue( host: str | None = None, org: str | None = None, repo: str | None = None, + labels: list[str] | None = None, + issue_type: str | None = None, + initial_status: str | None = None, + discussion: bool = False, + require_workflow_labels: bool = False, allow_duplicate_override: bool = False, split_from_issue: int | None = None, worktree_path: str | None = None, @@ -1261,6 +1369,12 @@ def gitea_create_issue( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + labels: Optional labels to apply by name after creating the issue. + issue_type: Optional canonical type, e.g. 'feature' or 'type:feature'. + initial_status: Optional initial workflow status, e.g. 'ready'. + discussion: If True, require/apply type:discussion. + require_workflow_labels: If True, fail closed unless labels include one + type:* and one status:* label. allow_duplicate_override: Operator-approved split after duplicate found. split_from_issue: Existing duplicate issue number when overriding. worktree_path: Optional path to verify branches-only guard. @@ -1292,6 +1406,40 @@ def gitea_create_issue( return blocked verify_preflight_purity(remote, worktree_path=worktree_path, task="create_issue") base = repo_api_url(h, o, r) + requested_labels = issue_workflow_labels.labels_for_new_issue( + issue_type=issue_type, + initial_status=initial_status, + extra_labels=labels, + discussion=discussion, + ) + label_assessment = issue_workflow_labels.assess_issue_labels( + requested_labels, + discussion=discussion, + require_type=True, + require_status=True, + ) + if require_workflow_labels and not label_assessment["valid"]: + return { + "success": False, + "performed": False, + "number": None, + "workflow_label_validation": label_assessment, + "reasons": label_assessment["errors"], + } + label_ids_by_name: dict[str, int] | None = None + if requested_labels: + label_ids_by_name = _repo_label_id_map(base, auth) + missing = [name for name in requested_labels if name not in label_ids_by_name] + if missing: + return { + "success": False, + "performed": False, + "number": None, + "workflow_label_validation": label_assessment, + "reasons": [ + f"requested labels do not exist on the repository: {missing}" + ], + } open_issues = api_get_all(f"{base}/issues?state=open&type=issues", auth) closed_issues = api_get_all( f"{base}/issues?state=closed&type=issues", auth, limit=100 @@ -1326,7 +1474,29 @@ def gitea_create_issue( _audit("create_issue", host=h, remote=remote, org=o, repo=r, result=gitea_audit.SUCCEEDED, issue_number=data["number"], request_metadata={"title": title}, mutation_task="create_issue") - return _with_optional_url({"number": data["number"]}, data.get("html_url")) + result = { + "number": data["number"], + "workflow_label_validation": label_assessment, + } + if requested_labels: + with _audited( + "set_issue_labels", + host=h, + remote=remote, + org=o, + repo=r, + issue_number=data["number"], + request_metadata={"labels": requested_labels, "source": "create_issue"}, + ): + applied = _put_issue_label_names( + base=base, + auth=auth, + issue_number=data["number"], + names=requested_labels, + label_ids_by_name=label_ids_by_name, + ) + result["labels"] = issue_workflow_labels.label_names(applied) + return _with_optional_url(result, data.get("html_url")) @mcp.tool() @@ -1735,7 +1905,23 @@ def gitea_create_pr( ) auth = _auth(h) - url = f"{repo_api_url(h, o, r)}/pulls" + repo_base = repo_api_url(h, o, r) + pr_open_transition = _prepare_issue_status_transition( + base=repo_base, + auth=auth, + issue_number=int(locked_issue), + status="status:pr-open", + ) + if not pr_open_transition.get("success"): + return { + "success": False, + "performed": False, + "number": None, + "issue_number": locked_issue, + "issue_status_transition": pr_open_transition, + "reasons": pr_open_transition.get("reasons", []), + } + url = f"{repo_base}/pulls" payload = {"title": title, "body": body, "head": head, "base": base} meta = {"title": title, "head": head, "base": base} try: @@ -1750,7 +1936,37 @@ def gitea_create_pr( result=gitea_audit.SUCCEEDED, pr_number=data["number"], target_branch=head, request_metadata=meta, mutation_task="create_pr") - return _with_optional_url({"number": data["number"]}, data.get("html_url")) + with _audited( + "set_issue_labels", + host=h, + remote=remote, + org=o, + repo=r, + issue_number=int(locked_issue), + request_metadata={ + "source": "create_pr", + "status": "status:pr-open", + "pr_number": data["number"], + }, + ): + _put_issue_label_names( + base=repo_base, + auth=auth, + issue_number=int(locked_issue), + names=pr_open_transition["labels"], + label_ids_by_name=pr_open_transition["label_ids_by_name"], + ) + result = { + "number": data["number"], + "issue_status_transition": { + "success": True, + "performed": True, + "issue_number": int(locked_issue), + "target_status": "status:pr-open", + "labels": pr_open_transition["labels"], + }, + } + return _with_optional_url(result, data.get("html_url")) def _format_list_pr_entry(pr: dict) -> dict: @@ -6870,15 +7086,8 @@ def gitea_mark_issue( auth = _auth(h) base = repo_api_url(h, o, r) - # Find the status:in-progress label id - labels = api_request("GET", f"{base}/labels?limit=100", auth) - label_id = None - for lb in labels: - if lb["name"] == "status:in-progress": - label_id = lb["id"] - break - - if label_id is None: + label_ids_by_name = _repo_label_id_map(base, auth) + if "status:in-progress" not in label_ids_by_name: raise RuntimeError( "Label 'status:in-progress' not found. " "Run manage_labels.py to create it first." @@ -6886,10 +7095,19 @@ def gitea_mark_issue( if action == "start": with _audited("label_issue", host=h, remote=remote, org=o, repo=r, - issue_number=issue_number, - request_metadata={"op": "add", "label": "status:in-progress"}): - api_request("POST", f"{base}/issues/{issue_number}/labels", auth, - {"labels": [label_id]}) + issue_number=issue_number, request_metadata={ + "op": "replace-status", + "label": "status:in-progress", + }): + transition = _transition_issue_status( + base=base, + auth=auth, + issue_number=issue_number, + status="status:in-progress", + label_ids_by_name=label_ids_by_name, + ) + if not transition.get("success"): + raise RuntimeError("; ".join(transition.get("reasons") or [])) active_profile = (profile or get_profile().get("profile_name") or "unknown") branch = (branch_name or "pending").strip() or "pending" heartbeat_body = issue_claim_heartbeat.format_heartbeat_body( @@ -6914,8 +7132,10 @@ def gitea_mark_issue( "message": f"Issue #{issue_number} claimed.", "heartbeat_posted": heartbeat.get("success", False), "heartbeat_comment_id": heartbeat.get("comment_id"), + "status_transition": transition, } else: + label_id = label_ids_by_name["status:in-progress"] with _audited("unlabel_issue", host=h, remote=remote, org=o, repo=r, issue_number=issue_number, request_metadata={"op": "remove", "label": "status:in-progress"}): @@ -7281,6 +7501,7 @@ def gitea_create_label( host: str | None = None, org: str | None = None, repo: str | None = None, + worktree_path: str | None = None, ) -> dict: """Create a new label on a Gitea repository. @@ -7292,11 +7513,16 @@ def gitea_create_label( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + worktree_path: Optional branches worktree to verify before mutation. Returns: dict containing the created label details. """ - verify_preflight_purity(remote) + blocked = _profile_permission_block( + task_capability_map.required_permission("create_label")) + if blocked: + return blocked + verify_preflight_purity(remote, worktree_path=worktree_path, task="create_label") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) base = repo_api_url(h, o, r) @@ -7322,6 +7548,7 @@ def gitea_set_issue_labels( host: str | None = None, org: str | None = None, repo: str | None = None, + worktree_path: str | None = None, ) -> list: """Replace all labels on a Gitea issue with a new list of label names. @@ -7332,6 +7559,7 @@ def gitea_set_issue_labels( host: Override the Gitea host. org: Override the owner/organization. repo: Override the repository name. + worktree_path: Optional branches worktree to verify before mutation. Returns: list of all labels currently applied to the issue. @@ -7340,7 +7568,7 @@ def gitea_set_issue_labels( task_capability_map.required_permission("set_issue_labels")) if blocked: return blocked - verify_preflight_purity(remote, task="set_issue_labels") + verify_preflight_purity(remote, worktree_path=worktree_path, task="set_issue_labels") h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) base = repo_api_url(h, o, r) diff --git a/issue_workflow_labels.py b/issue_workflow_labels.py new file mode 100644 index 0000000..1e9c491 --- /dev/null +++ b/issue_workflow_labels.py @@ -0,0 +1,194 @@ +"""Canonical issue type/status label taxonomy and transition helpers (#513).""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Iterable, Mapping, Sequence + + +@dataclass(frozen=True) +class LabelSpec: + name: str + color: str + description: str + + +TYPE_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("type:bug", "b60205", "Bug or defect"), + LabelSpec("type:feature", "a2eeef", "Feature or enhancement"), + LabelSpec("type:process", "5319e7", "Process or policy work"), + LabelSpec("type:workflow", "c2e0c6", "Workflow automation or guidance"), + LabelSpec("type:guardrail", "d93f0b", "Safety gate or guardrail"), + LabelSpec("type:docs", "006b75", "Documentation work"), + LabelSpec("type:test", "0e8a16", "Tests or test infrastructure"), + LabelSpec("type:discussion", "bfdadc", "Discussion-only issue"), + LabelSpec("type:umbrella", "c5def5", "Umbrella or tracker issue"), + LabelSpec("type:cleanup", "ededed", "Cleanup or hygiene work"), +) + +STATUS_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("status:triage", "fbca04", "Issue needs triage"), + LabelSpec("status:ready", "0e8a16", "Issue is ready for work"), + LabelSpec("status:claimed", "fefe2e", "Issue is claimed"), + LabelSpec("status:in-progress", "fefe2e", "Issue is being worked on"), + LabelSpec("status:blocked", "b60205", "Issue is blocked"), + LabelSpec("status:needs-review", "0052cc", "Issue work needs review"), + LabelSpec("status:pr-open", "1d76db", "A linked PR is open"), + LabelSpec("status:approved", "0e8a16", "Linked PR is approved"), + LabelSpec("status:merged", "5319e7", "Linked PR is merged"), + LabelSpec("status:reconcile", "d93f0b", "Issue needs reconciliation"), + LabelSpec("status:done", "0e8a16", "Issue workflow is complete"), + LabelSpec("status:duplicate", "cccccc", "Issue is a duplicate"), + LabelSpec("status:wontfix", "000000", "Issue will not be fixed"), +) + +CANONICAL_LABEL_SPECS: tuple[LabelSpec, ...] = ( + TYPE_LABEL_SPECS + STATUS_LABEL_SPECS +) + +TYPE_LABELS: frozenset[str] = frozenset(spec.name for spec in TYPE_LABEL_SPECS) +STATUS_LABELS: frozenset[str] = frozenset(spec.name for spec in STATUS_LABEL_SPECS) +CANONICAL_LABELS: frozenset[str] = frozenset( + spec.name for spec in CANONICAL_LABEL_SPECS +) + +STATUS_TRANSITIONS: dict[str, str] = { + "triage": "status:triage", + "ready": "status:ready", + "claim": "status:claimed", + "claimed": "status:claimed", + "start": "status:in-progress", + "start_work": "status:in-progress", + "in_progress": "status:in-progress", + "in-progress": "status:in-progress", + "block": "status:blocked", + "blocked": "status:blocked", + "needs_review": "status:needs-review", + "needs-review": "status:needs-review", + "pr_open": "status:pr-open", + "pr-open": "status:pr-open", + "approved": "status:approved", + "merge": "status:reconcile", + "merged": "status:reconcile", + "reconcile": "status:reconcile", + "done": "status:done", + "complete": "status:done", + "duplicate": "status:duplicate", + "wontfix": "status:wontfix", +} + + +def label_name(label: str | Mapping[str, object]) -> str: + """Return a normalized label name from a Gitea label object or string.""" + if isinstance(label, str): + return label.strip() + value = label.get("name") + return str(value or "").strip() + + +def label_names(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + """Extract label names from a label list or issue-like mapping.""" + raw: object + if isinstance(labels, Mapping): + raw = labels.get("labels", []) + else: + raw = labels + return [name for name in (label_name(label) for label in raw or []) if name] + + +def type_labels(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + return [name for name in label_names(labels) if name.startswith("type:")] + + +def status_labels(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + return [name for name in label_names(labels) if name.startswith("status:")] + + +def canonical_status_label(status_or_transition: str) -> str: + status = status_or_transition.strip() + if status in STATUS_LABELS: + return status + normalized = status.lower().replace(" ", "-") + try: + return STATUS_TRANSITIONS[normalized] + except KeyError as exc: + raise ValueError( + f"unknown workflow status or transition '{status_or_transition}'" + ) from exc + + +def transition_status_labels( + existing_labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + status_or_transition: str, +) -> list[str]: + """Replace all active status labels with the requested canonical status.""" + new_status = canonical_status_label(status_or_transition) + kept = [name for name in label_names(existing_labels) if not name.startswith("status:")] + if new_status not in kept: + kept.append(new_status) + return kept + + +def labels_for_new_issue( + issue_type: str | None = None, + initial_status: str | None = None, + extra_labels: Sequence[str] | None = None, + *, + discussion: bool = False, +) -> list[str]: + names = list(extra_labels or []) + if issue_type: + type_name = issue_type if issue_type.startswith("type:") else f"type:{issue_type}" + names.append(type_name) + if discussion: + names.append("type:discussion") + if initial_status: + names.append(canonical_status_label(initial_status)) + + result: list[str] = [] + for name in names: + if name not in result: + result.append(name) + return result + + +def assess_issue_labels( + labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + *, + discussion: bool = False, + require_type: bool = True, + require_status: bool = True, +) -> dict: + names = label_names(labels) + found_types = type_labels(names) + found_statuses = status_labels(names) + errors: list[str] = [] + warnings: list[str] = [] + + if require_type and not found_types: + errors.append("issue is missing a type:* label") + if require_status and not found_statuses: + errors.append("issue is missing a status:* label") + if discussion and "type:discussion" not in found_types: + errors.append("discussion issue is missing type:discussion") + if len(found_statuses) > 1: + errors.append( + "issue has multiple active status:* labels: " + + ", ".join(found_statuses) + ) + + for name in found_types: + if name not in TYPE_LABELS: + warnings.append(f"unknown type label '{name}'") + for name in found_statuses: + if name not in STATUS_LABELS: + warnings.append(f"unknown status label '{name}'") + + return { + "valid": not errors, + "labels": names, + "type_labels": found_types, + "status_labels": found_statuses, + "errors": errors, + "warnings": warnings, + } diff --git a/manage_labels.py b/manage_labels.py index 0a2f52f..400b319 100755 --- a/manage_labels.py +++ b/manage_labels.py @@ -23,6 +23,7 @@ if os.path.exists(venv_python) and sys.executable != venv_python: os.execv(venv_python, [venv_python] + sys.argv) from gitea_auth import get_auth_header, api_request, repo_api_url +import issue_workflow_labels HOST = "gitea.dadeschools.net" ORG = "Contractor" @@ -35,8 +36,10 @@ LABELS = [ {"name": "epic", "color": "8250df", "description": ""}, {"name": "important", "color": "fbca04", "description": ""}, {"name": "nice-to-have", "color": "0e8a16", "description": ""}, - {"name": "status:in-progress", "color": "fefe2e", - "description": "Issue is being worked on"}, + *[ + {"name": spec.name, "color": spec.color, "description": spec.description} + for spec in issue_workflow_labels.CANONICAL_LABEL_SPECS + ], ] # issue number -> label names to apply (one-off backfill) diff --git a/task_capability_map.py b/task_capability_map.py index 1d40a78..f46f242 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -36,6 +36,10 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.issue.comment", "role": "author", }, + "create_label": { + "permission": "gitea.issue.comment", + "role": "author", + }, "create_branch": { "permission": "gitea.branch.create", "role": "author", @@ -155,6 +159,7 @@ ISSUE_MUTATION_TOOL_TASKS: dict[str, str] = { "gitea_create_issue_comment": "comment_issue", "gitea_mark_issue": "mark_issue", "gitea_set_issue_labels": "set_issue_labels", + "gitea_create_label": "create_label", "gitea_commit_files": "commit_files", } diff --git a/tests/test_create_issue.py b/tests/test_create_issue.py index b2fdb68..11eb846 100644 --- a/tests/test_create_issue.py +++ b/tests/test_create_issue.py @@ -103,6 +103,37 @@ class TestAPIPayload(unittest.TestCase): self.assertEqual(payload["title"], "My Title") self.assertEqual(payload["body"], "My Body") + @patch("create_issue.get_credentials", return_value=FAKE_CREDS) + def test_applies_workflow_labels_by_name(self, _cred): + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and url.endswith("/labels"): + return [ + {"id": 1, "name": "type:feature"}, + {"id": 2, "name": "status:ready"}, + ] + if method == "POST" and url.endswith("/issues"): + return {"number": 7, "html_url": "http://x/7"} + if method == "PUT" and url.endswith("/issues/7/labels"): + return [{"name": "type:feature"}, {"name": "status:ready"}] + return {} + + with patch("create_issue.api_request", side_effect=api_side_effect) as mock_api: + rc = create_issue.main([ + "--title", "My Title", + "--type-label", "feature", + "--status-label", "ready", + ]) + self.assertEqual(rc, 0) + put_call = next(c for c in mock_api.call_args_list if c[0][0] == "PUT") + self.assertEqual(put_call[0][3], {"labels": [1, 2]}) + + @patch("create_issue.get_credentials", return_value=FAKE_CREDS) + def test_require_workflow_labels_blocks_missing_labels(self, _cred): + with patch("create_issue.api_request") as mock_api: + rc = create_issue.main(["--title", "My Title", "--require-workflow-labels"]) + self.assertEqual(rc, 1) + mock_api.assert_not_called() + @patch("create_issue.get_credentials", return_value=FAKE_CREDS) def test_url_construction(self, _cred): with patch("create_issue.api_request", diff --git a/tests/test_issue_workflow_labels.py b/tests/test_issue_workflow_labels.py new file mode 100644 index 0000000..e7fd143 --- /dev/null +++ b/tests/test_issue_workflow_labels.py @@ -0,0 +1,73 @@ +"""Tests for canonical issue workflow label policy (#513).""" + +from __future__ import annotations + +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +import issue_workflow_labels as labels # noqa: E402 + + +class TestIssueWorkflowLabelValidation(unittest.TestCase): + def test_valid_labeled_issue(self): + result = labels.assess_issue_labels(["type:feature", "status:ready"]) + self.assertTrue(result["valid"]) + self.assertEqual(result["type_labels"], ["type:feature"]) + self.assertEqual(result["status_labels"], ["status:ready"]) + + def test_issue_missing_type_label(self): + result = labels.assess_issue_labels(["status:ready"]) + self.assertFalse(result["valid"]) + self.assertIn("issue is missing a type:* label", result["errors"]) + + def test_issue_missing_status_label(self): + result = labels.assess_issue_labels(["type:bug"]) + self.assertFalse(result["valid"]) + self.assertIn("issue is missing a status:* label", result["errors"]) + + def test_discussion_issue_missing_type_discussion(self): + result = labels.assess_issue_labels( + ["type:feature", "status:triage"], + discussion=True, + ) + self.assertFalse(result["valid"]) + self.assertIn("discussion issue is missing type:discussion", result["errors"]) + + def test_multiple_conflicting_status_labels(self): + result = labels.assess_issue_labels( + ["type:feature", "status:ready", "status:blocked"] + ) + self.assertFalse(result["valid"]) + self.assertTrue( + any("multiple active status:* labels" in err for err in result["errors"]) + ) + + +class TestIssueWorkflowStatusTransitions(unittest.TestCase): + def test_status_transition_removes_old_status_label(self): + result = labels.transition_status_labels( + ["type:feature", "status:ready", "workflow"], + "in-progress", + ) + self.assertEqual(result, ["type:feature", "workflow", "status:in-progress"]) + + def test_pr_open_transition_applies_status_pr_open(self): + result = labels.transition_status_labels( + ["type:feature", "status:in-progress"], + "pr-open", + ) + self.assertEqual(result, ["type:feature", "status:pr-open"]) + + def test_duplicate_transition_applies_status_duplicate(self): + result = labels.transition_status_labels( + ["type:process", "status:triage"], + "duplicate", + ) + self.assertEqual(result, ["type:process", "status:duplicate"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_issue_write_tool_gates.py b/tests/test_issue_write_tool_gates.py index 736b295..f91c514 100644 --- a/tests/test_issue_write_tool_gates.py +++ b/tests/test_issue_write_tool_gates.py @@ -164,13 +164,14 @@ class TestAllowedIssueWritesProceed(IssueWriteGateBase): result = mcp_server.gitea_close_issue(issue_number=9, remote="prgs") self.assertTrue(result.get("success")) + @patch("mcp_server.api_get_all", return_value=[{"id": 10, "name": "status:in-progress"}]) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value="token author-pass") - def test_resolver_allowed_mark_issue_proceeds(self, _auth, mock_api): + def test_resolver_allowed_mark_issue_proceeds(self, _auth, mock_api, _labels): def api_side_effect(method, url, auth, payload=None): - if method == "GET" and url.endswith("/labels?limit=100"): - return [{"id": 10, "name": "status:in-progress"}] - if method == "POST" and "/issues/9/labels" in url: + if method == "GET" and "/issues/9" in url: + return {"labels": []} + if method == "PUT" and "/issues/9/labels" in url: return [{"name": "status:in-progress"}] if method == "POST" and "/issues/9/comments" in url: return {"id": 501} @@ -209,4 +210,4 @@ class TestCreateIssueMissingPermissionReport(IssueWriteGateBase): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main() diff --git a/tests/test_manage_labels.py b/tests/test_manage_labels.py index cdccf76..ca008a1 100644 --- a/tests/test_manage_labels.py +++ b/tests/test_manage_labels.py @@ -11,6 +11,7 @@ from unittest.mock import MagicMock, call, patch sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) import manage_labels # noqa: E402 +import issue_workflow_labels # noqa: E402 FAKE_AUTH = "Basic dGVzdDp0ZXN0" # base64("test:test") @@ -129,6 +130,11 @@ class TestConstants(unittest.TestCase): names = [l["name"] for l in manage_labels.LABELS] self.assertIn("status:in-progress", names) + def test_canonical_workflow_labels_are_defined(self): + names = {l["name"] for l in manage_labels.LABELS} + for spec in issue_workflow_labels.CANONICAL_LABEL_SPECS: + self.assertIn(spec.name, names) + def test_all_mapped_labels_are_defined(self): defined = {l["name"] for l in manage_labels.LABELS} for issue, names in manage_labels.MAPPING.items(): diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 651b253..fdb1f51 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -85,6 +85,16 @@ def _visible_approval_reviews(reviewer="reviewer-bot", sha="abc123"): _DEFAULT_LEASE_SESSION = "mcp-test-reviewer-lease" _NO_PR_WORK_LEASE_BLOCK = {"block": False, "reasons": [], "mutation_allowed": True} +WORKFLOW_REPO_LABELS = [ + {"id": 1, "name": "type:feature"}, + {"id": 2, "name": "status:in-progress"}, + {"id": 3, "name": "status:ready"}, + {"id": 4, "name": "status:pr-open"}, +] + + +def _issue_with_labels(*names): + return {"labels": [{"name": name} for name in names]} def _reviewer_lease_comment( @@ -293,6 +303,22 @@ class TestCreateIssue(unittest.TestCase): self.assertIn("gitea.prgs.cc", url) self.assertIn("Scaled-Tech-Consulting", url) + @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", + return_value=(True, [])) + @patch("mcp_server.api_request") + @patch("mcp_server.api_get_all", return_value=[]) + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_issue_required_workflow_labels_blocks(self, _auth, _get_all, mock_api, _role): + with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): + result = gitea_create_issue( + title="Test issue", + require_workflow_labels=True, + ) + self.assertFalse(result["success"]) + self.assertFalse(result["performed"]) + self.assertIn("issue is missing a type:* label", result["reasons"]) + mock_api.assert_not_called() + # --------------------------------------------------------------------------- # Create PR @@ -303,13 +329,24 @@ class TestCreatePR(unittest.TestCase): "mcp_server.issue_duplicate_context_fetcher", return_value=([], [], {"status": "not_claimed"}), ) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_creates_pr(self, _auth, mock_api, _role, _dup_fetcher): + def test_creates_pr(self, _auth, mock_api, _role, _labels, _dup_fetcher): worktree = os.path.realpath(os.getcwd()) - mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/123" in url: + return _issue_with_labels("type:feature", "status:in-progress") + if method == "POST" and url.endswith("/pulls"): + return {"number": 3, "html_url": "https://example.com/pulls/3"} + if method == "PUT" and url.endswith("/issues/123/labels"): + return [{"name": "type:feature"}, {"name": "status:pr-open"}] + return {} + + mock_api.side_effect = api_side_effect with tempfile.TemporaryDirectory() as lock_dir: env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir} with patch.dict(os.environ, env, clear=True): @@ -322,22 +359,37 @@ class TestCreatePR(unittest.TestCase): ) self.assertEqual(result["number"], 3) self.assertNotIn("url", result) - payload = mock_api.call_args[0][3] + post_call = next(c for c in mock_api.call_args_list if c[0][0] == "POST") + payload = post_call[0][3] self.assertEqual(payload["head"], "feat/x") self.assertEqual(payload["base"], "main") self.assertIn("Closes #123", payload["title"]) + put_call = next(c for c in mock_api.call_args_list if c[0][0] == "PUT") + self.assertEqual(put_call[0][3], {"labels": [1, 4]}) + self.assertTrue(result["issue_status_transition"]["performed"]) @patch( "mcp_server.issue_duplicate_context_fetcher", return_value=([], [], {"status": "not_claimed"}), ) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_create_pr_reveal_opt_in_includes_url(self, _auth, mock_api, _role, _dup_fetcher): + def test_create_pr_reveal_opt_in_includes_url(self, _auth, mock_api, _role, _labels, _dup_fetcher): worktree = os.path.realpath(os.getcwd()) - mock_api.return_value = {"number": 3, "html_url": "https://example.com/pulls/3"} + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/123" in url: + return _issue_with_labels("type:feature", "status:in-progress") + if method == "POST" and url.endswith("/pulls"): + return {"number": 3, "html_url": "https://example.com/pulls/3"} + if method == "PUT" and url.endswith("/issues/123/labels"): + return [{"name": "type:feature"}, {"name": "status:pr-open"}] + return {} + + mock_api.side_effect = api_side_effect with tempfile.TemporaryDirectory() as lock_dir: env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir, "GITEA_MCP_REVEAL_ENDPOINTS": "1"} with patch.dict(os.environ, env, clear=True): @@ -350,6 +402,38 @@ class TestCreatePR(unittest.TestCase): ) self.assertIn("pulls/3", result["url"]) + @patch( + "mcp_server.issue_duplicate_context_fetcher", + return_value=([], [], {"status": "not_claimed"}), + ) + @patch("mcp_server.api_get_all", return_value=[{"id": 1, "name": "type:feature"}]) + @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", + return_value=(True, [])) + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_pr_blocks_when_pr_open_status_label_missing( + self, _auth, mock_api, _role, _labels, _dup_fetcher + ): + worktree = os.path.realpath(os.getcwd()) + mock_api.return_value = _issue_with_labels("type:feature", "status:in-progress") + with tempfile.TemporaryDirectory() as lock_dir: + env = {**CREATE_PR_ENV, "GITEA_ISSUE_LOCK_DIR": lock_dir} + with patch.dict(os.environ, env, clear=True): + _bind_test_lock(issue_number=123, branch_name="feat/x", worktree_path=worktree) + result = gitea_create_pr( + title="feat: X Closes #123", + head="feat/x", + base="main", + worktree_path=worktree, + ) + self.assertFalse(result["success"]) + self.assertFalse(result["performed"]) + self.assertIn("status:pr-open", result["reasons"][0]) + self.assertFalse( + any(c[0][0] == "POST" and c[0][1].endswith("/pulls") + for c in mock_api.call_args_list) + ) + @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) @@ -474,26 +558,33 @@ class TestViewIssue(unittest.TestCase): # --------------------------------------------------------------------------- class TestMarkIssue(unittest.TestCase): + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_start_adds_label(self, _auth, mock_api): - # labels, add label, post claim heartbeat comment - mock_api.side_effect = [ - [{"id": 10, "name": "status:in-progress"}], - [{"name": "status:in-progress"}], - {"id": 99}, - ] + def test_start_adds_label(self, _auth, mock_api, _labels): + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/5" in url: + return _issue_with_labels("type:feature", "status:ready") + if method == "PUT" and url.endswith("/issues/5/labels"): + return [{"name": "type:feature"}, {"name": "status:in-progress"}] + if method == "POST" and url.endswith("/issues/5/comments"): + return {"id": 99} + return {} + + mock_api.side_effect = api_side_effect with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): result = gitea_mark_issue(issue_number=5, action="start") self.assertTrue(result["success"]) self.assertIn("claimed", result["message"]) self.assertTrue(result.get("heartbeat_posted")) + put_call = next(c for c in mock_api.call_args_list if c[0][0] == "PUT") + self.assertEqual(put_call[0][3], {"labels": [1, 2]}) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_done_removes_label(self, _auth, mock_api): + def test_done_removes_label(self, _auth, mock_api, _labels): mock_api.side_effect = [ - [{"id": 10, "name": "status:in-progress"}], None, ] with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): @@ -505,10 +596,10 @@ class TestMarkIssue(unittest.TestCase): with self.assertRaises(ValueError): gitea_mark_issue(issue_number=5, action="pause") + @patch("mcp_server.api_get_all", return_value=[]) @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_missing_label_raises(self, _auth, mock_api): - mock_api.return_value = [] # no labels exist + def test_missing_label_raises(self, _auth, mock_api, _labels): with patch.dict(os.environ, ISSUE_WRITE_ENV, clear=True): with self.assertRaises(RuntimeError): gitea_mark_issue(issue_number=5, action="start") @@ -3750,13 +3841,24 @@ class TestIssueLocking(unittest.TestCase): ) self.assertIn("lock provenance", str(ctx.exception).lower()) + @patch("mcp_server.api_get_all", return_value=WORKFLOW_REPO_LABELS) @patch("mcp_server.api_request") @patch("mcp_server.role_session_router.check_author_mutation_after_reviewer_stop", return_value=(True, [])) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) - def test_create_pr_honors_scratch_worktree_lock(self, _auth, _role, mock_api): + def test_create_pr_honors_scratch_worktree_lock(self, _auth, _role, mock_api, _labels): scratch = os.path.realpath("/tmp/gitea-tools-author-scratch/issue-249-e2e") - mock_api.return_value = {"number": 250, "html_url": "https://example/pr/250"} + + def api_side_effect(method, url, auth, payload=None): + if method == "GET" and "/issues/249" in url: + return _issue_with_labels("type:feature", "status:in-progress") + if method == "POST" and url.endswith("/pulls"): + return {"number": 250, "html_url": "https://example/pr/250"} + if method == "PUT" and url.endswith("/issues/249/labels"): + return [{"name": "type:feature"}, {"name": "status:pr-open"}] + return {} + + mock_api.side_effect = api_side_effect _bind_test_lock( issue_number=249, branch_name="feat/issue-249-issue-lock-scratch-worktree", @@ -3771,6 +3873,7 @@ class TestIssueLocking(unittest.TestCase): worktree_path=scratch, ) self.assertEqual(res["number"], 250) + self.assertTrue(res["issue_status_transition"]["performed"]) # --------------------------------------------------------------------------- From add87b5708d00c741d3d1ec1b7388963794b3bd3 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:30:20 -0400 Subject: [PATCH 08/12] feat: require live PR head re-pin before conflict-fix classification (Closes #522) Treat inventory mergeable/head as advisory. Add classification helper and MCP tool so author sessions re-pin the live PR head before creating a conflict-fix worktree, skip stale inventory false-negatives, and prove classification in final reports. --- README.md | 1 + conflict_fix_classification.py | 266 ++++++++++++++++++ final_report_validator.py | 22 ++ gitea_mcp_server.py | 35 +++ review_proofs.py | 9 + .../workflows/work-issue.md | 31 +- tests/test_conflict_fix_classification.py | 142 ++++++++++ 7 files changed, 505 insertions(+), 1 deletion(-) create mode 100644 conflict_fix_classification.py create mode 100644 tests/test_conflict_fix_classification.py diff --git a/README.md b/README.md index e099658..5ebd8ac 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,7 @@ Any MCP-compatible agent (Antigravity, Claude Code, etc.) can call these tools n | `gitea_whoami` | Read-only: identify the authenticated Gitea account (safe metadata only) | | `gitea_get_profile` | Read-only: describe the active runtime execution profile (safe metadata only) | | `gitea_check_pr_eligibility` | Read-only: check if the current identity/profile may review/approve/request_changes/merge a PR | +| `gitea_assess_conflict_fix_classification` | Read-only: classify conflict-fix need from a live PR head re-fetch before creating a conflict-fix worktree | | `gitea_submit_pr_review` | Gated review mutation: comment/approve/request_changes, only after identity+profile+eligibility gates pass (no merge, no self-approval) | | `gitea_mark_issue` | Claim/release an issue (start/done) | | `gitea_list_labels` | List all available labels in a repository | diff --git a/conflict_fix_classification.py b/conflict_fix_classification.py new file mode 100644 index 0000000..86815b5 --- /dev/null +++ b/conflict_fix_classification.py @@ -0,0 +1,266 @@ +"""Live PR head re-pin before conflict-fix classification (#522). + +Open PR inventory fields (mergeable, head_sha) can be stale. Author sessions +must re-fetch live PR state and pin the live head before classifying a PR as +conflicted or creating a conflict-fix worktree. +""" + +from __future__ import annotations + +import re +from typing import Any + +_FULL_SHA = re.compile(r"^[0-9a-f]{40}$", re.IGNORECASE) + +_INVENTORY_HEAD_RE = re.compile( + r"(?:inventory head sha|stale inventory head sha)\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_LIVE_HEAD_RE = re.compile( + r"(?:live head sha|pinned head sha|live pr head sha)\s*:\s*([0-9a-f]{40})", + re.IGNORECASE, +) +_REPINS_TOOL_RE = re.compile( + r"gitea_assess_conflict_fix_classification", + re.IGNORECASE, +) +_CLASSIFICATION_RE = re.compile( + r"conflict[- ]fix classification\s*:\s*(\S+)", + re.IGNORECASE, +) + +CLASSIFICATION_STALE_INVENTORY_SKIP = "stale_inventory_skip" +CLASSIFICATION_CONFLICT_FIX_NEEDED = "conflict_fix_needed" +CLASSIFICATION_LIVE_MERGEABLE = "live_mergeable_skip" +CLASSIFICATION_INCOMPLETE = "incomplete_live_repin" + +_VALID_CLASSIFICATIONS = frozenset({ + CLASSIFICATION_STALE_INVENTORY_SKIP, + CLASSIFICATION_CONFLICT_FIX_NEEDED, + CLASSIFICATION_LIVE_MERGEABLE, + CLASSIFICATION_INCOMPLETE, +}) + + +def _normalize_sha(value: str | None) -> str | None: + text = (value or "").strip().lower() + if not text: + return None + return text if _FULL_SHA.match(text) else None + + +def assess_conflict_fix_classification( + *, + pr_number: int, + inventory_head_sha: str | None = None, + inventory_mergeable: bool | None = None, + live_head_sha: str | None = None, + live_mergeable: bool | None = None, +) -> dict[str, Any]: + """Classify whether conflict-fix work is justified from live PR state. + + Inventory fields are advisory only. Live head + live mergeable are required + before any conflict-fix worktree may be created. + """ + inv_head = _normalize_sha(inventory_head_sha) + live_head = _normalize_sha(live_head_sha) + reasons: list[str] = [] + + if not isinstance(pr_number, int) or pr_number <= 0: + return { + "classification": CLASSIFICATION_INCOMPLETE, + "worktree_allowed": False, + "skip_author_mutation": True, + "inventory_stale_head": False, + "inventory_stale_mergeable": False, + "pinned_head_sha": None, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": ["pr_number must be a positive integer (fail closed)"], + } + + if live_head is None: + reasons.append( + "live PR head SHA missing or not a full 40-char hex SHA; " + "re-fetch the PR before conflict-fix classification (fail closed)" + ) + if live_mergeable is None: + reasons.append( + "live PR mergeable value missing; re-fetch the PR before " + "conflict-fix classification (fail closed)" + ) + + if reasons: + return { + "classification": CLASSIFICATION_INCOMPLETE, + "worktree_allowed": False, + "skip_author_mutation": True, + "inventory_stale_head": bool( + inv_head and live_head and inv_head != live_head + ), + "inventory_stale_mergeable": ( + inventory_mergeable is not None + and live_mergeable is not None + and bool(inventory_mergeable) != bool(live_mergeable) + ), + "pinned_head_sha": live_head, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": reasons, + } + + inventory_stale_head = bool(inv_head and inv_head != live_head) + inventory_stale_mergeable = ( + inventory_mergeable is not None + and bool(inventory_mergeable) != bool(live_mergeable) + ) + + # Live mergeable wins: do not start conflict-fix work. + if live_mergeable is True: + classification = ( + CLASSIFICATION_STALE_INVENTORY_SKIP + if ( + inventory_mergeable is False + or inventory_stale_head + or inventory_stale_mergeable + ) + else CLASSIFICATION_LIVE_MERGEABLE + ) + note = [] + if inventory_stale_head: + note.append( + f"inventory head {inv_head} differs from live head {live_head}; " + "use live head only" + ) + if inventory_mergeable is False: + note.append( + "inventory reported mergeable:false but live PR is mergeable:true; " + "skip author conflict-fix mutation" + ) + return { + "classification": classification, + "worktree_allowed": False, + "skip_author_mutation": True, + "inventory_stale_head": inventory_stale_head, + "inventory_stale_mergeable": inventory_stale_mergeable, + "pinned_head_sha": live_head, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": note, + } + + # live_mergeable is False → conflict-fix may proceed on the pinned live head. + note = [] + if inventory_stale_head: + note.append( + f"inventory head {inv_head} differs from live head {live_head}; " + "pin and use live head only for conflict-fix work" + ) + if inventory_mergeable is True: + note.append( + "inventory reported mergeable:true but live PR is mergeable:false; " + "trust live mergeable and proceed only with live head pin" + ) + note.append( + f"live PR #{pr_number} is mergeable:false at pinned head {live_head}; " + "conflict-fix worktree allowed for that head only" + ) + return { + "classification": CLASSIFICATION_CONFLICT_FIX_NEEDED, + "worktree_allowed": True, + "skip_author_mutation": False, + "inventory_stale_head": inventory_stale_head, + "inventory_stale_mergeable": inventory_stale_mergeable, + "pinned_head_sha": live_head, + "inventory_head_sha": inv_head, + "live_head_sha": live_head, + "inventory_mergeable": inventory_mergeable, + "live_mergeable": live_mergeable, + "pr_number": pr_number, + "reasons": note, + } + + +def assess_conflict_fix_classification_final_report( + report_text: str, + **_kwargs: Any, +) -> dict[str, Any]: + """Require live-head re-pin proof when a report claims conflict-fix work (#522).""" + text = report_text or "" + lower = text.lower() + mentions_conflict_fix = ( + "conflict-fix" in lower + or "conflict fix" in lower + or "conflict_fix" in lower + ) + if not mentions_conflict_fix: + return {"proven": True, "reasons": [], "applicable": False} + + reasons: list[str] = [] + if not _REPINS_TOOL_RE.search(text): + reasons.append( + "conflict-fix report must cite gitea_assess_conflict_fix_classification " + "(live head re-pin tool)" + ) + + live_match = _LIVE_HEAD_RE.search(text) + if not live_match: + reasons.append( + "conflict-fix report must state Live head SHA: <40-char hex> " + "(or Pinned head SHA / Live PR head SHA)" + ) + else: + live_sha = _normalize_sha(live_match.group(1)) + if live_sha is None: + reasons.append("live head SHA is not a full 40-char hex digest") + + inv_match = _INVENTORY_HEAD_RE.search(text) + # Inventory head is optional but recommended when classification is stale skip. + class_match = _CLASSIFICATION_RE.search(text) + if not class_match: + reasons.append( + "conflict-fix report must state Conflict-fix classification: " + f"<{'|'.join(sorted(_VALID_CLASSIFICATIONS))}>" + ) + else: + classification = class_match.group(1).strip().lower().replace(" ", "_") + # allow hyphenated forms + classification = classification.replace("-", "_") + if classification not in _VALID_CLASSIFICATIONS: + reasons.append( + f"unknown conflict-fix classification {class_match.group(1)!r}; " + f"expected one of {sorted(_VALID_CLASSIFICATIONS)}" + ) + + if inv_match and live_match: + inv_sha = _normalize_sha(inv_match.group(1)) + live_sha = _normalize_sha(live_match.group(1)) + if inv_sha and live_sha and inv_sha != live_sha: + # Require explicit note that live head was used. + if "use live head" not in lower and "live head only" not in lower: + reasons.append( + "inventory head differs from live head; report must state that " + "the live head was used exclusively" + ) + + return { + "proven": not reasons, + "reasons": reasons, + "applicable": True, + "inventory_head_sha": _normalize_sha(inv_match.group(1)) if inv_match else None, + "live_head_sha": _normalize_sha(live_match.group(1)) if live_match else None, + "classification": ( + class_match.group(1).strip().lower().replace("-", "_").replace(" ", "_") + if class_match + else None + ), + } diff --git a/final_report_validator.py b/final_report_validator.py index 492c15b..70535e6 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -565,6 +565,27 @@ def _rule_reviewer_stale_head_proof(report_text: str) -> list[dict[str, str]]: ) +def _rule_conflict_fix_classification_proof(report_text: str) -> list[dict[str, str]]: + from conflict_fix_classification import ( + assess_conflict_fix_classification_final_report, + ) + + text = report_text or "" + result = assess_conflict_fix_classification_final_report(text) + if result.get("proven"): + return [] + return _findings_from_reasons( + "author.conflict_fix_classification_proof", + result.get("reasons") or [], + field="Conflict-fix classification", + severity="block", + safe_next_action=( + "call gitea_assess_conflict_fix_classification, state the live head " + "SHA, and state the classification before creating a conflict-fix worktree" + ), + ) + + def _rule_conflict_fix_push_proof(report_text: str) -> list[dict[str, str]]: from pr_work_lease import assess_conflict_fix_final_report @@ -1271,6 +1292,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { *_SHARED_ISSUE_LOCK_RULES, _rule_shared_issue_acceptance_gate, _rule_reviewer_vague_mutations_none, + _rule_conflict_fix_classification_proof, _rule_conflict_fix_push_proof, _rule_worktree_cleanup_audit_proof, ], diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed650b5..8abafc2 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -827,6 +827,7 @@ import reconciliation_workflow # noqa: E402 import audit_reconciliation_mode # noqa: E402 import review_merge_state_machine # noqa: E402 import pr_work_lease # noqa: E402 +import conflict_fix_classification # noqa: E402 import native_mcp_preference # noqa: E402 import worktree_cleanup_audit # noqa: E402 @@ -8209,6 +8210,40 @@ def gitea_acquire_conflict_fix_lease( } +@mcp.tool() +def gitea_assess_conflict_fix_classification( + pr_number: int, + inventory_head_sha: str | None = None, + inventory_mergeable: bool | None = None, + live_head_sha: str | None = None, + live_mergeable: bool | None = None, +) -> dict: + """Read-only: classify conflict-fix need from live PR state (#522). + + Open PR inventory can be stale. Callers must pass a live re-fetched PR head + and mergeability value before creating a conflict-fix worktree. + """ + 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"), + } + + result = conflict_fix_classification.assess_conflict_fix_classification( + pr_number=pr_number, + inventory_head_sha=inventory_head_sha, + inventory_mergeable=inventory_mergeable, + live_head_sha=live_head_sha, + live_mergeable=live_mergeable, + ) + result["success"] = True + result["performed"] = False + return result + + @mcp.tool() def gitea_assess_conflict_fix_push( pr_number: int, diff --git a/review_proofs.py b/review_proofs.py index 194ce9e..f7f2a53 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -5540,6 +5540,15 @@ def assess_already_landed_classification_report(report_text, **kwargs): return _assess(report_text, **kwargs) +def assess_conflict_fix_classification_final_report(report_text, **kwargs): + """#522: require live PR head re-pin before conflict-fix classification.""" + from conflict_fix_classification import ( + assess_conflict_fix_classification_final_report as _assess, + ) + + return _assess(report_text, **kwargs) + + def assess_prior_blocker_skip_proof(report_text, **kwargs): """#318: require live blocker proof before skipping earlier open PRs.""" from reviewer_blocker_skip import assess_prior_blocker_skip_proof as _assess diff --git a/skills/llm-project-workflow/workflows/work-issue.md b/skills/llm-project-workflow/workflows/work-issue.md index c7f5747..1114033 100644 --- a/skills/llm-project-workflow/workflows/work-issue.md +++ b/skills/llm-project-workflow/workflows/work-issue.md @@ -615,10 +615,39 @@ After push, report: If push fails, stop and produce a recovery handoff. -## 20A. Conflict-fix lease and push gate (#399) +## 20A. Live head re-pin before conflict-fix classification (#522) + +Open PR inventory `mergeable` / `head_sha` fields are **advisory and may be +stale**. Before classifying a PR as conflicted or creating a conflict-fix +worktree: + +1. Re-fetch the live PR (`gitea_view_pr` or equivalent) and record: + * inventory head SHA (if any) + * inventory mergeable (if any) + * **live** head SHA (full 40-char) + * **live** mergeable +2. Call `gitea_assess_conflict_fix_classification` with those values. +3. Act only on the returned classification and **pinned live head**: + * `stale_inventory_skip` / `live_mergeable_skip` → **do not** create a + conflict-fix worktree; do not mutate the PR branch for conflicts. + * `conflict_fix_needed` → conflict-fix worktree allowed for the pinned + live head only. + * `incomplete_live_repin` → stop; re-fetch live state. +4. If inventory head ≠ live head, report both SHAs and use the live head only. +5. If inventory says `mergeable:false` but live says `mergeable:true`, classify + as stale inventory and skip author conflict-fix mutation. + +Conflict-fix final reports that claim conflict-fix work must include: + +* citation of `gitea_assess_conflict_fix_classification` +* `Live head SHA: <40-char hex>` (or Pinned head SHA / Live PR head SHA) +* `Conflict-fix classification: <stale_inventory_skip|conflict_fix_needed|live_mergeable_skip|incomplete_live_repin>` + +## 20B. Conflict-fix lease and push gate (#399) When pushing to an existing PR branch to resolve merge conflicts: +0. Complete §20A live-head re-pin / classification first. 1. Call `gitea_acquire_conflict_fix_lease` before any push. 2. Call `gitea_assess_conflict_fix_push` immediately before `git push` with: * branch head before push diff --git a/tests/test_conflict_fix_classification.py b/tests/test_conflict_fix_classification.py new file mode 100644 index 0000000..0c1ea82 --- /dev/null +++ b/tests/test_conflict_fix_classification.py @@ -0,0 +1,142 @@ +"""Tests for live PR head re-pin before conflict-fix classification (#522).""" + +from __future__ import annotations + +import unittest + +from conflict_fix_classification import ( + CLASSIFICATION_CONFLICT_FIX_NEEDED, + CLASSIFICATION_INCOMPLETE, + CLASSIFICATION_LIVE_MERGEABLE, + CLASSIFICATION_STALE_INVENTORY_SKIP, + assess_conflict_fix_classification, + assess_conflict_fix_classification_final_report, +) + + +def _sha(prefix: str) -> str: + return (prefix + "0" * 40)[:40] + + +class TestConflictFixClassification(unittest.TestCase): + def test_stale_inventory_mergeable_skip(self): + """PR #508 style: inventory mergeable:false/stale head, live mergeable:true.""" + result = assess_conflict_fix_classification( + pr_number=508, + inventory_head_sha=_sha("dad1dc8"), + inventory_mergeable=False, + live_head_sha=_sha("3f3d6cb"), + live_mergeable=True, + ) + self.assertEqual(result["classification"], CLASSIFICATION_STALE_INVENTORY_SKIP) + self.assertTrue(result["skip_author_mutation"]) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["inventory_stale_head"]) + self.assertTrue(result["inventory_stale_mergeable"]) + self.assertEqual(result["pinned_head_sha"], _sha("3f3d6cb")) + + def test_stale_inventory_head_only_live_mergeable_true(self): + """PR #493 style: head moved; live still mergeable.""" + result = assess_conflict_fix_classification( + pr_number=493, + inventory_head_sha=_sha("685e627"), + inventory_mergeable=True, + live_head_sha=_sha("4561d7a"), + live_mergeable=True, + ) + self.assertEqual(result["classification"], CLASSIFICATION_STALE_INVENTORY_SKIP) + self.assertTrue(result["skip_author_mutation"]) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["inventory_stale_head"]) + self.assertEqual(result["pinned_head_sha"], _sha("4561d7a")) + + def test_live_conflict_allows_worktree_on_pinned_head(self): + result = assess_conflict_fix_classification( + pr_number=99, + inventory_head_sha=_sha("aaaaaaa"), + inventory_mergeable=False, + live_head_sha=_sha("bbbbbbb"), + live_mergeable=False, + ) + self.assertEqual(result["classification"], CLASSIFICATION_CONFLICT_FIX_NEEDED) + self.assertTrue(result["worktree_allowed"]) + self.assertFalse(result["skip_author_mutation"]) + self.assertTrue(result["inventory_stale_head"]) + self.assertEqual(result["pinned_head_sha"], _sha("bbbbbbb")) + + def test_matching_inventory_live_mergeable_skip(self): + head = _sha("cccccccc") + result = assess_conflict_fix_classification( + pr_number=10, + inventory_head_sha=head, + inventory_mergeable=True, + live_head_sha=head, + live_mergeable=True, + ) + self.assertEqual(result["classification"], CLASSIFICATION_LIVE_MERGEABLE) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["skip_author_mutation"]) + self.assertFalse(result["inventory_stale_head"]) + + def test_missing_live_head_incomplete(self): + result = assess_conflict_fix_classification( + pr_number=1, + inventory_head_sha=_sha("ddddddd"), + inventory_mergeable=False, + live_head_sha=None, + live_mergeable=False, + ) + self.assertEqual(result["classification"], CLASSIFICATION_INCOMPLETE) + self.assertFalse(result["worktree_allowed"]) + self.assertTrue(result["skip_author_mutation"]) + self.assertTrue(any("live PR head" in r for r in result["reasons"])) + + def test_short_sha_rejected(self): + result = assess_conflict_fix_classification( + pr_number=1, + inventory_head_sha="abc1234", + inventory_mergeable=False, + live_head_sha="def5678", + live_mergeable=False, + ) + self.assertEqual(result["classification"], CLASSIFICATION_INCOMPLETE) + self.assertFalse(result["worktree_allowed"]) + + +class TestConflictFixClassificationFinalReport(unittest.TestCase): + def test_non_conflict_report_not_applicable(self): + result = assess_conflict_fix_classification_final_report( + "Implemented feature without merge issues." + ) + self.assertTrue(result["proven"]) + self.assertFalse(result["applicable"]) + + def test_conflict_report_requires_tool_live_head_classification(self): + result = assess_conflict_fix_classification_final_report( + "Did conflict-fix work on PR #99." + ) + self.assertFalse(result["proven"]) + self.assertTrue(result["applicable"]) + joined = " ".join(result["reasons"]) + self.assertIn("gitea_assess_conflict_fix_classification", joined) + self.assertIn("Live head SHA", joined) + self.assertIn("classification", joined.lower()) + + def test_good_conflict_report_passes(self): + live = _sha("3f3d6cb") + inv = _sha("dad1dc8") + text = f""" +Conflict-fix classification: stale_inventory_skip +Called gitea_assess_conflict_fix_classification before worktree creation. +Inventory head SHA: {inv} +Live head SHA: {live} +Use live head only; skip author mutation. +""" + result = assess_conflict_fix_classification_final_report(text) + self.assertTrue(result["proven"], msg=result.get("reasons")) + self.assertEqual(result["live_head_sha"], live) + self.assertEqual(result["inventory_head_sha"], inv) + + +if __name__ == "__main__": + unittest.main() From 44cf2a4ce84a40b7f1ae0d4b0553cded95e0a131 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:43:46 -0400 Subject: [PATCH 09/12] feat: mount canonical gitea-workflow skill for Codex and MCP inventory (Closes #551) Expose gitea-workflow / llm-project-workflow / git-pr-workflows as one workflow router in mcp_list_project_skills, add preflight + Codex install script, and document multi-runtime skill name parity. --- docs/llm-workflow-runbooks.md | 19 +++ docs/workflow-skill-mount.md | 68 ++++++++++ gitea_mcp_server.py | 38 +++++- scripts/install-codex-workflow-skill.sh | 82 ++++++++++++ skills/gitea-workflow/SKILL.md | 37 ++++++ skills/llm-project-workflow/SKILL.md | 2 + tests/test_operator_guide.py | 4 + tests/test_workflow_skill.py | 87 +++++++++++++ workflow_skill.py | 160 ++++++++++++++++++++++++ 9 files changed, 496 insertions(+), 1 deletion(-) create mode 100644 docs/workflow-skill-mount.md create mode 100755 scripts/install-codex-workflow-skill.sh create mode 100644 skills/gitea-workflow/SKILL.md create mode 100644 tests/test_workflow_skill.py create mode 100644 workflow_skill.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d0e472d..7f43fa6 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -408,6 +408,25 @@ 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. + +## Canonical workflow skill names (#551) + +Controller prompts and sessions must load the **same** workflow skill wall +regardless of runtime (Claude, Codex, Gemini): + +| Name | Role | +|------|------| +| `gitea-workflow` | Primary controller / Codex skill name | +| `llm-project-workflow` | Portable in-repo package | +| `git-pr-workflows` | Legacy alias | + +- Inventory: `mcp_list_project_skills` lists all three. +- Preflight: `mcp_check_workflow_skill_preflight` before mutations. +- Codex install: `scripts/install-codex-workflow-skill.sh` +- Full doc: [`docs/workflow-skill-mount.md`](workflow-skill-mount.md) + +If the skill is missing, stop with BLOCKED + DIAGNOSE — do not mutate. + ## Shell Spawn Hard-Stop Rule Symptom: a shell tool call returns `exit_code: -1` with empty stdout/stderr. diff --git a/docs/workflow-skill-mount.md b/docs/workflow-skill-mount.md new file mode 100644 index 0000000..408ff0f --- /dev/null +++ b/docs/workflow-skill-mount.md @@ -0,0 +1,68 @@ +# Workflow skill mount across runtimes (#551) + +## Problem + +Controller prompts require **`gitea-workflow`**, but: + +- Claude may load `~/.claude/skills/gitea-workflow` +- Codex often has **no** `~/.codex/skills/gitea-workflow` +- The portable package in-repo is `skills/llm-project-workflow` +- `mcp_list_project_skills` historically listed operational guides only, not + the workflow router + +Sessions then either **block** incorrectly or **proceed without** the workflow +wall. + +## Canonical names (must resolve to the same skill) + +| Name | Use | +|------|-----| +| `gitea-workflow` | **Primary** controller / Codex skill name | +| `llm-project-workflow` | Portable in-repo package name | +| `git-pr-workflows` | Legacy alias | + +Source of truth: `skills/llm-project-workflow/SKILL.md` +In-repo alias stub: `skills/gitea-workflow/SKILL.md` + +## Codex install + +From a `branches/` worktree (or any clone of the repo): + +```bash +./scripts/install-codex-workflow-skill.sh +# optional: +./scripts/install-codex-workflow-skill.sh --dry-run +./scripts/install-codex-workflow-skill.sh --skills-dir "$HOME/.codex/skills" +``` + +This symlinks the portable package under all three names. **Restart Codex** +after install. + +## MCP discovery + +- `mcp_list_project_skills` includes `gitea-workflow`, `llm-project-workflow`, + and `git-pr-workflows`. +- `mcp_get_skill_guide("<name>")` returns the same router steps for each. +- `mcp_check_workflow_skill_preflight` proves the in-repo skill file exists and + reports Codex mount status. + +## Preflight rule + +Before any git or Gitea mutation: + +1. Call `mcp_check_workflow_skill_preflight`. +2. If `blocked` / `workflow_skill_ready` is false → **BLOCKED + DIAGNOSE**; do + not mutate. +3. Load the skill by **any** canonical name and follow the router. + +Missing Codex mount alone does not block if the in-repo skill is present and +loaded via MCP/docs; operators should still install the Codex symlink so +prompt names resolve natively. + +## Controller prompts + +Prefer: + +> Invoke skill `gitea-workflow` (alias of `llm-project-workflow`). + +Do not require a name that is only available on one runtime. diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed650b5..b460d5d 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -827,6 +827,7 @@ import reconciliation_workflow # noqa: E402 import audit_reconciliation_mode # noqa: E402 import review_merge_state_machine # noqa: E402 import pr_work_lease # noqa: E402 +import workflow_skill # noqa: E402 import native_mcp_preference # noqa: E402 import worktree_cleanup_audit # noqa: E402 @@ -6933,6 +6934,9 @@ _PROJECT_SKILLS = { }, } +# Canonical workflow router skill names for Claude/Codex/Gemini parity (#551). +_PROJECT_SKILLS.update(workflow_skill.workflow_skill_registry_entries()) + @mcp.tool() def mcp_get_control_plane_guide( @@ -7046,7 +7050,7 @@ def mcp_get_control_plane_guide( @mcp.tool() def mcp_list_project_skills() -> dict: - """List the project's workflow skills and when to use them (#128). + """List the project's workflow skills and when to use them (#128, #551). Read-only; makes no API calls. Each skill reports its name, description, when-to-use guidance, required operations, status @@ -7054,6 +7058,9 @@ def mcp_list_project_skills() -> dict: 'operator-only'), and whether the current profile's operations permit it. Use mcp_get_skill_guide(name) for the step-by-step guide. + Includes the canonical workflow router under gitea-workflow, + llm-project-workflow, and git-pr-workflows (#551). + Returns: dict with 'read_only', 'count', and 'skills' (list). Never secrets. """ @@ -7078,10 +7085,39 @@ def mcp_list_project_skills() -> dict: } if meta.get("notes"): entry["notes"] = meta["notes"] + if meta.get("repo_path"): + entry["repo_path"] = meta["repo_path"] + if meta.get("canonical"): + entry["canonical"] = True skills.append(entry) return {"read_only": True, "count": len(skills), "skills": skills} +@mcp.tool() +def mcp_check_workflow_skill_preflight( + project_root: str | None = None, +) -> dict: + """Fail-closed preflight for the canonical workflow skill wall (#551). + + Verifies skills/llm-project-workflow/SKILL.md exists in the project tree + and reports whether Codex has a matching mount under ~/.codex/skills. + Call before git/Gitea mutations when the controller requires gitea-workflow. + + Args: + project_root: Optional override; defaults to this MCP process root. + + Returns: + dict with workflow_skill_ready, blocked, canonical_names, codex mount + status, and safe_next_action. Never secrets. + """ + root = (project_root or PROJECT_ROOT).strip() + result = workflow_skill.assess_workflow_skill_presence(root) + result["success"] = not result.get("blocked") + result["project_root"] = root + result["read_only"] = True + return result + + @mcp.tool() def mcp_get_skill_guide(skill_name: str) -> dict: """Step-by-step guide for one named project skill (#128). diff --git a/scripts/install-codex-workflow-skill.sh b/scripts/install-codex-workflow-skill.sh new file mode 100755 index 0000000..cfdafa9 --- /dev/null +++ b/scripts/install-codex-workflow-skill.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# Install canonical Gitea workflow skill into Codex skills dir (#551). +# Symlinks the in-repo skills/llm-project-workflow package under the names +# gitea-workflow, llm-project-workflow, and git-pr-workflows. +set -euo pipefail + +usage() { + cat <<'EOF' +usage: scripts/install-codex-workflow-skill.sh [--dry-run] [--skills-dir DIR] + +Symlink the portable skills/llm-project-workflow package into the Codex +skills directory under the canonical names: + gitea-workflow + llm-project-workflow + git-pr-workflows + +Defaults: + skills dir: $CODEX_HOME/skills or ~/.codex/skills + source: <repo>/skills/llm-project-workflow +EOF +} + +dry_run=0 +skills_dir="" +while [[ "${1:-}" == --* ]]; do + case "$1" in + --dry-run) dry_run=1 ;; + --skills-dir) + shift + skills_dir="${1:-}" + ;; + --help|-h) usage; exit 0 ;; + *) usage >&2; exit 2 ;; + esac + shift +done + +script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +repo_root="$(cd "$script_dir/.." && pwd)" +source_skill="$repo_root/skills/llm-project-workflow" + +if [[ ! -f "$source_skill/SKILL.md" ]]; then + echo "Error: missing $source_skill/SKILL.md" >&2 + exit 1 +fi + +if [[ -z "$skills_dir" ]]; then + if [[ -n "${CODEX_HOME:-}" ]]; then + skills_dir="$CODEX_HOME/skills" + else + skills_dir="${HOME}/.codex/skills" + fi +fi + +names=(gitea-workflow llm-project-workflow git-pr-workflows) + +echo "source: $source_skill" +echo "codex skills dir: $skills_dir" + +if [[ "$dry_run" -eq 0 ]]; then + mkdir -p "$skills_dir" +fi + +for name in "${names[@]}"; do + target="$skills_dir/$name" + if [[ "$dry_run" -eq 1 ]]; then + echo "dry-run: ln -sfn $source_skill $target" + continue + fi + if [[ -e "$target" || -L "$target" ]]; then + if [[ -L "$target" ]]; then + rm -f "$target" + else + echo "Error: $target exists and is not a symlink (refuse to overwrite)" >&2 + exit 1 + fi + fi + ln -sfn "$source_skill" "$target" + echo "linked $target -> $source_skill" +done + +echo "done. Restart Codex so skills reload." diff --git a/skills/gitea-workflow/SKILL.md b/skills/gitea-workflow/SKILL.md new file mode 100644 index 0000000..0f39091 --- /dev/null +++ b/skills/gitea-workflow/SKILL.md @@ -0,0 +1,37 @@ +--- +name: gitea-workflow +description: >- + Canonical alias for the Gitea/LLM project workflow router. Same skill as + llm-project-workflow. Use at the start of any Gitea-Tools author, review, + merge, reconcile, or issue-filing task. Trigger terms: gitea-workflow, + llm-project-workflow, git-pr-workflows, gitea, PR review, work-issue. +--- + +# gitea-workflow (canonical alias) + +This directory is the **stable name** for controller prompts and Codex mounts +(`gitea-workflow`). The portable implementation lives at: + +**[`../llm-project-workflow/SKILL.md`](../llm-project-workflow/SKILL.md)** + +## Required action + +1. Open and follow `skills/llm-project-workflow/SKILL.md` (router). +2. Load the matching workflow under `skills/llm-project-workflow/workflows/`. +3. Do not mutate git/Gitea until the workflow is loaded. + +## Multi-runtime names (must resolve identically) + +| Name | Role | +|------|------| +| `gitea-workflow` | Canonical controller / Codex skill name | +| `llm-project-workflow` | Portable in-repo skill package | +| `git-pr-workflows` | Legacy alias | + +Install for Codex: + +```bash +./scripts/install-codex-workflow-skill.sh +``` + +Preflight via MCP: `mcp_check_workflow_skill_preflight`. diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index dc2f961..610a1f7 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -6,6 +6,8 @@ description: >- report schema. Use at the start of any implementation, review, merge, reconciliation, or issue-filing task. --- +> **Also known as:** `gitea-workflow`, `git-pr-workflows` (canonical multi-runtime names — see docs/workflow-skill-mount.md / #551). + # LLM Project Workflow Skill diff --git a/tests/test_operator_guide.py b/tests/test_operator_guide.py index a5159c6..550d8c6 100644 --- a/tests/test_operator_guide.py +++ b/tests/test_operator_guide.py @@ -49,6 +49,10 @@ EXPECTED_SKILLS = [ "jenkins-mcp", "glitchtip-mcp", "release-operator", + # Canonical workflow router (#551) — same skill, multi-runtime names + "gitea-workflow", + "llm-project-workflow", + "git-pr-workflows", ] diff --git a/tests/test_workflow_skill.py b/tests/test_workflow_skill.py new file mode 100644 index 0000000..78072a2 --- /dev/null +++ b/tests/test_workflow_skill.py @@ -0,0 +1,87 @@ +"""Tests for canonical workflow skill naming and preflight (#551).""" + +from __future__ import annotations + +import os +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +import workflow_skill +import mcp_server + + +class TestWorkflowSkillModule(unittest.TestCase): + def test_canonical_names_include_gitea_workflow(self): + names = workflow_skill.CANONICAL_WORKFLOW_SKILL_NAMES + self.assertIn("gitea-workflow", names) + self.assertIn("llm-project-workflow", names) + self.assertIn("git-pr-workflows", names) + + def test_repo_skill_present_in_this_checkout(self): + root = str(Path(__file__).resolve().parent.parent) + result = workflow_skill.assess_workflow_skill_presence(root) + self.assertTrue(result["repo_skill_present"]) + self.assertTrue(result["repo_alias_present"]) + self.assertTrue(result["workflow_skill_ready"]) + self.assertFalse(result["blocked"]) + + def test_missing_repo_skill_blocks(self): + with tempfile.TemporaryDirectory() as tmp: + result = workflow_skill.assess_workflow_skill_presence(tmp) + self.assertTrue(result["blocked"]) + self.assertFalse(result["workflow_skill_ready"]) + self.assertTrue(any("missing" in r for r in result["reasons"])) + + def test_codex_mount_detection(self): + root = str(Path(__file__).resolve().parent.parent) + with tempfile.TemporaryDirectory() as tmp: + skill_dir = Path(tmp) / "gitea-workflow" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text("# x\n", encoding="utf-8") + result = workflow_skill.assess_workflow_skill_presence( + root, codex_skills_dir=tmp + ) + self.assertTrue(result["codex_skill_mounted"]) + self.assertTrue(result["codex_skill_mounts"]["gitea-workflow"]) + + def test_registry_entries_cover_aliases(self): + entries = workflow_skill.workflow_skill_registry_entries() + for name in workflow_skill.CANONICAL_WORKFLOW_SKILL_NAMES: + self.assertIn(name, entries) + self.assertEqual(entries[name]["status"], "available") + + +class TestWorkflowSkillMcpTools(unittest.TestCase): + def test_list_includes_canonical_names(self): + with patch.dict( + os.environ, + { + "GITEA_PROFILE_NAME": "author-test", + "GITEA_ALLOWED_OPERATIONS": "gitea.read", + "GITEA_FORBIDDEN_OPERATIONS": "", + }, + clear=False, + ): + result = mcp_server.mcp_list_project_skills() + names = {s["name"] for s in result["skills"]} + for name in workflow_skill.CANONICAL_WORKFLOW_SKILL_NAMES: + self.assertIn(name, names) + + def test_get_skill_guide_gitea_workflow(self): + guide = mcp_server.mcp_get_skill_guide("gitea-workflow") + self.assertTrue(guide["success"]) + self.assertTrue(guide["steps"]) + self.assertIn("workflow", " ".join(guide["steps"]).lower()) + + def test_preflight_tool_ok_on_repo(self): + result = mcp_server.mcp_check_workflow_skill_preflight() + self.assertTrue(result["success"]) + self.assertTrue(result["workflow_skill_ready"]) + self.assertFalse(result["blocked"]) + self.assertIn("gitea-workflow", result["canonical_names"]) + + +if __name__ == "__main__": + unittest.main() diff --git a/workflow_skill.py b/workflow_skill.py new file mode 100644 index 0000000..22e1551 --- /dev/null +++ b/workflow_skill.py @@ -0,0 +1,160 @@ +"""Canonical workflow skill naming and preflight for multi-runtime agents (#551). + +Controller prompts and Claude historically require ``gitea-workflow``. The +portable in-repo skill is ``skills/llm-project-workflow``. Codex must resolve +the same canonical names so sessions do not proceed without the workflow wall. +""" + +from __future__ import annotations + +import os +from pathlib import Path +from typing import Any + +# Canonical names that all runtimes (Claude, Codex, Gemini, MCP inventory) +# must treat as the same workflow router skill. +CANONICAL_WORKFLOW_SKILL_NAMES = ( + "gitea-workflow", + "llm-project-workflow", + "git-pr-workflows", +) + +# Primary checked-in skill path (portable source of truth). +REPO_SKILL_REL_PATH = "skills/llm-project-workflow/SKILL.md" +# In-repo alias directory so scanners that look for skills/gitea-workflow work. +REPO_ALIAS_SKILL_REL_PATH = "skills/gitea-workflow/SKILL.md" + +CODEX_SKILLS_ENV = "CODEX_HOME" +DEFAULT_CODEX_SKILLS = os.path.expanduser("~/.codex/skills") + + +def default_codex_skills_dir() -> str: + home = (os.environ.get(CODEX_SKILLS_ENV) or "").strip() + if home: + return os.path.join(os.path.expanduser(home), "skills") + return DEFAULT_CODEX_SKILLS + + +def resolve_repo_skill_path(project_root: str) -> Path: + return Path(project_root) / REPO_SKILL_REL_PATH + + +def resolve_repo_alias_skill_path(project_root: str) -> Path: + return Path(project_root) / REPO_ALIAS_SKILL_REL_PATH + + +def normalize_skill_name(name: str | None) -> str: + return (name or "").strip().lower().replace("_", "-") + + +def is_canonical_workflow_skill_name(name: str | None) -> bool: + return normalize_skill_name(name) in CANONICAL_WORKFLOW_SKILL_NAMES + + +def assess_workflow_skill_presence( + project_root: str, + *, + codex_skills_dir: str | None = None, +) -> dict[str, Any]: + """Return presence proof for the canonical workflow skill. + + Fail-closed when the in-repo skill file is missing. Codex mount is advisory + in the report (operators install via script) but ``codex_skill_mounted`` is + explicit so preflight can BLOCK when required. + """ + root = (project_root or "").strip() + reasons: list[str] = [] + repo_path = resolve_repo_skill_path(root) if root else None + alias_path = resolve_repo_alias_skill_path(root) if root else None + repo_present = bool(repo_path and repo_path.is_file()) + alias_present = bool(alias_path and alias_path.is_file()) + + if not root: + reasons.append("project_root not provided (fail closed)") + elif not repo_present: + reasons.append( + f"canonical workflow skill missing at {REPO_SKILL_REL_PATH} " + "(fail closed — do not mutate without workflow wall)" + ) + + codex_dir = Path(codex_skills_dir or default_codex_skills_dir()) + codex_mounts: dict[str, bool] = {} + for name in CANONICAL_WORKFLOW_SKILL_NAMES: + skill_md = codex_dir / name / "SKILL.md" + codex_mounts[name] = skill_md.is_file() + codex_any = any(codex_mounts.values()) + + blocked = bool(reasons) + return { + "canonical_names": list(CANONICAL_WORKFLOW_SKILL_NAMES), + "primary_name": "gitea-workflow", + "portable_name": "llm-project-workflow", + "repo_skill_rel_path": REPO_SKILL_REL_PATH, + "repo_skill_present": repo_present, + "repo_alias_rel_path": REPO_ALIAS_SKILL_REL_PATH, + "repo_alias_present": alias_present, + "codex_skills_dir": str(codex_dir), + "codex_skill_mounts": codex_mounts, + "codex_skill_mounted": codex_any, + "workflow_skill_ready": repo_present, + "blocked": blocked, + "block_reason": reasons[0] if reasons else None, + "reasons": reasons, + "safe_next_action": ( + "Install or repair skills/llm-project-workflow/SKILL.md in the " + "repo; run scripts/install-codex-workflow-skill.sh for Codex; " + "call mcp_list_project_skills and load gitea-workflow / " + "llm-project-workflow before any git/Gitea mutation." + if blocked or not codex_any + else "Load gitea-workflow or llm-project-workflow, then proceed." + ), + } + + +def workflow_skill_registry_entries() -> dict[str, dict[str, Any]]: + """Shared skill metadata for mcp_list_project_skills / guides.""" + shared_steps = [ + "Call mcp_list_project_skills and confirm gitea-workflow (or " + "llm-project-workflow) is listed.", + "Call mcp_check_workflow_skill_preflight and stop if blocked.", + "Load skills/llm-project-workflow/SKILL.md (or the gitea-workflow " + "alias) and route to the matching workflow file for the task mode.", + "Do not perform git or Gitea mutations until the workflow is loaded.", + "Codex: ensure ~/.codex/skills/gitea-workflow -> repo skill via " + "scripts/install-codex-workflow-skill.sh.", + ] + notes = ( + f"Canonical names: {', '.join(CANONICAL_WORKFLOW_SKILL_NAMES)}. " + f"Portable source: {REPO_SKILL_REL_PATH}. " + "Controller prompts may say gitea-workflow; Codex and MCP inventory " + "must resolve the same skill." + ) + base = { + "description": ( + "Canonical Gitea/LLM project workflow router: pick task mode, " + "load the matching workflow, enforce isolation, emit the final " + "report schema." + ), + "when_to_use": ( + "At the start of every author, review, merge, reconcile, or " + "issue-filing session — before any mutation." + ), + "required_operations": ["gitea.read"], + "status": "available", + "notes": notes, + "steps": shared_steps, + "repo_path": REPO_SKILL_REL_PATH, + "canonical": True, + } + return { + "gitea-workflow": dict(base), + "llm-project-workflow": dict(base), + "git-pr-workflows": { + **dict(base), + "description": ( + "Legacy alias for the canonical Gitea/LLM project workflow " + "router (same as gitea-workflow / llm-project-workflow)." + ), + "notes": notes + " Prefer gitea-workflow in new prompts.", + }, + } From 84d921a52fe9d31b399f4347e7e4518815105f60 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 03:13:42 -0400 Subject: [PATCH 10/12] feat(#496): fail-closed canonical comment validation before Gitea posts Add canonical_comment_validator and wire it into issue comments, PR review bodies, reconcile post_comment, and structured comment helpers. Workflow-changing comments without complete next-action state are rejected before any API call. Includes unit/MCP integration tests, runbook docs, and a final-report rule blocking false "comment posted" claims when validation failed. --- canonical_comment_validator.py | 408 ++++++++++++++++++++++ docs/llm-workflow-runbooks.md | 49 +++ final_report_validator.py | 65 ++++ gitea_mcp_server.py | 61 ++++ tests/test_canonical_comment_validator.py | 217 ++++++++++++ tests/test_final_report_validator.py | 25 ++ tests/test_mcp_server.py | 30 ++ 7 files changed, 855 insertions(+) create mode 100644 canonical_comment_validator.py create mode 100644 tests/test_canonical_comment_validator.py diff --git a/canonical_comment_validator.py b/canonical_comment_validator.py new file mode 100644 index 0000000..a2d05a0 --- /dev/null +++ b/canonical_comment_validator.py @@ -0,0 +1,408 @@ +"""Fail-closed validation for workflow-changing Gitea comments (#496).""" + +from __future__ import annotations + +import re +from typing import Any + +VALID_ROLES = frozenset({ + "controller", + "author", + "reviewer", + "merger", + "reconciler", + "user", +}) + +_BASE_REQUIRED = ("STATE", "WHO_IS_NEXT", "NEXT_ACTION", "NEXT_PROMPT", "WHY") + +_ISSUE_REQUIRED = _BASE_REQUIRED + ("BLOCKERS", "VALIDATION") +_PR_REQUIRED = _BASE_REQUIRED + ( + "ISSUE", + "HEAD_SHA", + "REVIEW_STATUS", + "MERGE_READY", + "BLOCKERS", + "VALIDATION", +) +_SUPERSESSION_REQUIRED = _BASE_REQUIRED + ( + "CANONICAL_ITEM", + "SUPERSEDED_ITEM", + "CLOSE_OR_KEEP_OPEN", +) + +_MACHINE_MARKERS = ( + "<!-- mcp-review-lease:v1 -->", + "<!-- mcp-conflict-fix-lease:v1 -->", + "<!-- gitea-issue-claim-heartbeat:v1 -->", +) + +_WORKFLOW_TRIGGERS = re.compile( + r"\b(?:" + r"blocked|unblocked|ready(?:\s+for\s+(?:review|merge|author))?|" + r"ready-to-merge|approved|approve|request\s+changes|changes\s+requested|" + r"superseded|duplicate|canonical|next\s+action|next\s+actor|who\s+is\s+next|" + r"author\s+should|reviewer\s+should|merger\s+should|reconciler\s+should|" + r"controller\s+should|issue\s+complete|pr\s+open|pr\s+merged|close\s+this|" + r"do\s+not\s+merge|needs?\s+rebase|stale\s+approval|contaminated\s+review|" + r"merge\s+ready|ready\s+for\s+merge" + r")\b", + re.IGNORECASE, +) + +_CANONICAL_HEADINGS = ( + "## Canonical Issue State", + "## Canonical PR State", + "## Canonical Discussion Summary", +) + +_FIELD_RE = re.compile( + r"^([A-Z][A-Z0-9_]*)\s*:\s*(.*)$", + re.MULTILINE, +) + +_VAGUE_NEXT_ACTIONS = frozenset({ + "continue", + "handle this", + "fix it", + "follow up", + "follow-up", + "see above", + "see review", + "tbd", + "todo", + "as needed", + "proceed", + "next steps", + "will check", + "investigate", +}) + +_FULL_SHA_RE = re.compile(r"\b[0-9a-f]{40}\b", re.IGNORECASE) +_SHORT_SHA_RE = re.compile(r"\b[0-9a-f]{7,40}\b", re.IGNORECASE) +_PR_REF_RE = re.compile(r"(?:PR\s*#|pull\s*#)\d+|\b#\d{2,}\b", re.IGNORECASE) +_APPROVAL_PROOF_RE = re.compile( + r"\b(?:approved|approval_at_current_head|APPROVE)\b", + re.IGNORECASE, +) +_UNBLOCK_RE = re.compile( + r"\b(?:unblock|until|after|once|when|requires?|must)\b", + re.IGNORECASE, +) + + +def _parse_fields(body: str) -> dict[str, str]: + """Parse KEY: value fields, including values continued on following lines.""" + fields: dict[str, str] = {} + current_key: str | None = None + current_lines: list[str] = [] + + def _flush() -> None: + nonlocal current_key, current_lines + if current_key is not None: + fields[current_key] = "\n".join(current_lines).strip() + current_key = None + current_lines = [] + + for line in (body or "").splitlines(): + if line.startswith("## "): + _flush() + continue + match = re.match(r"^([A-Z][A-Z0-9_]*)\s*:\s*(.*)$", line) + if match: + _flush() + current_key = match.group(1).strip().upper() + rest = match.group(2) + current_lines = [rest] if rest else [] + elif current_key is not None: + current_lines.append(line) + _flush() + return fields + + +def _is_machine_generated(body: str) -> bool: + text = body or "" + return any(marker in text for marker in _MACHINE_MARKERS) + + +def _has_canonical_heading(body: str) -> bool: + return any(h in (body or "") for h in _CANONICAL_HEADINGS) + + +def is_workflow_changing_comment(body: str) -> bool: + """True when comment text implies a workflow/state transition.""" + text = (body or "").strip() + if not text: + return False + if _is_machine_generated(text): + return False + if _has_canonical_heading(text): + return True + if _WORKFLOW_TRIGGERS.search(text): + return True + fields = _parse_fields(text) + if "STATE" in fields or "WHO_IS_NEXT" in fields: + return True + return False + + +def infer_comment_context(body: str, *, explicit: str | None = None) -> str: + if explicit: + return explicit + text = body or "" + if "## Canonical Discussion Summary" in text: + return "discussion_summary" + if "## Canonical PR State" in text: + return "pr_comment" + if "## Canonical Issue State" in text: + return "issue_comment" + fields = _parse_fields(text) + if fields.get("CANONICAL_ITEM") or fields.get("SUPERSEDED_ITEM"): + return "supersession" + if any(k in fields for k in ("HEAD_SHA", "REVIEW_STATUS", "MERGE_READY", "ISSUE")): + return "pr_comment" + return "issue_comment" + + +def _required_fields_for_context(context: str) -> tuple[str, ...]: + if context in ("pr_comment", "pr_review"): + return _PR_REQUIRED + if context == "supersession": + return _SUPERSESSION_REQUIRED + if context == "discussion_summary": + return _BASE_REQUIRED + ("DECISION", "SUBSTANTIVE_COMMENTS") + return _ISSUE_REQUIRED + + +def _is_vague_next_action(value: str) -> bool: + normalized = re.sub(r"\s+", " ", (value or "").strip().lower()) + normalized = normalized.rstrip(".") + if not normalized: + return True + if normalized in _VAGUE_NEXT_ACTIONS: + return True + if len(normalized) < 12: + return True + return False + + +def _next_prompt_ok(value: str) -> bool: + text = (value or "").strip() + if len(text) < 40: + return False + if text.lower() in {"n/a", "none", "tbd", "todo"}: + return False + return True + + +def _state_value(fields: dict[str, str]) -> str: + return (fields.get("STATE") or "").strip().lower() + + +def _suggested_template(context: str) -> str: + if context == "pr_comment" or context == "pr_review": + return ( + "## Canonical PR State\n\n" + "STATE:\n" + "WHO_IS_NEXT:\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + "```text\n<paste-ready prompt>\n```\n" + "WHAT_HAPPENED:\n" + "WHY:\n" + "ISSUE:\n" + "HEAD_SHA:\n" + "REVIEW_STATUS:\n" + "MERGE_READY:\n" + "BLOCKERS:\n" + "VALIDATION:\n" + "LAST_UPDATED_BY:\n" + ) + if context == "supersession": + return ( + "## Canonical PR State\n\n" + "STATE:\nsuperseded\n" + "WHO_IS_NEXT:\nreconciler\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + "WHY:\n" + "CANONICAL_ITEM:\n" + "SUPERSEDED_ITEM:\n" + "CLOSE_OR_KEEP_OPEN:\n" + ) + if context == "discussion_summary": + return ( + "## Canonical Discussion Summary\n\n" + "STATE:\n" + "WHO_IS_NEXT:\n" + "DECISION:\n" + "WHY:\n" + "SUBSTANTIVE_COMMENTS:\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + ) + return ( + "## Canonical Issue State\n\n" + "STATE:\n" + "WHO_IS_NEXT:\n" + "NEXT_ACTION:\n" + "NEXT_PROMPT:\n" + "```text\n<paste-ready prompt>\n```\n" + "WHAT_HAPPENED:\n" + "WHY:\n" + "RELATED_PRS:\n" + "BLOCKERS:\n" + "VALIDATION:\n" + "LAST_UPDATED_BY:\n" + ) + + +def _build_correction_message( + *, + missing_fields: list[str], + vague_fields: list[str], + extra_reasons: list[str], + context: str, +) -> str: + parts = [ + "Canonical comment validation failed (fail closed before posting).", + ] + if missing_fields: + parts.append("Missing fields: " + ", ".join(missing_fields) + ".") + if vague_fields: + parts.append("Vague or invalid fields: " + ", ".join(vague_fields) + ".") + parts.extend(extra_reasons) + parts.append("Fill the suggested template and retry.") + parts.append("Suggested template:\n" + _suggested_template(context)) + return "\n".join(parts) + + +def assess_canonical_comment( + body: str, + *, + context: str | None = None, + force_workflow: bool = False, +) -> dict[str, Any]: + """Validate outgoing comment text before a Gitea mutation.""" + text = (body or "").strip() + ctx = infer_comment_context(text, explicit=context) + + if not text: + return { + "allowed": True, + "is_workflow_comment": False, + "context": ctx, + "missing_fields": [], + "vague_fields": [], + "correction_message": "", + "suggested_template": "", + } + + if _is_machine_generated(text): + return { + "allowed": True, + "is_workflow_comment": False, + "context": ctx, + "missing_fields": [], + "vague_fields": [], + "correction_message": "", + "suggested_template": "", + } + + workflow = force_workflow or is_workflow_changing_comment(text) + if not workflow: + return { + "allowed": True, + "is_workflow_comment": False, + "context": ctx, + "missing_fields": [], + "vague_fields": [], + "correction_message": "", + "suggested_template": "", + } + + fields = _parse_fields(text) + required = _required_fields_for_context(ctx) + missing = [name for name in required if not (fields.get(name) or "").strip()] + + vague: list[str] = [] + extra: list[str] = [] + + who = (fields.get("WHO_IS_NEXT") or "").strip().lower() + if who and who not in VALID_ROLES: + vague.append("WHO_IS_NEXT") + extra.append( + f"WHO_IS_NEXT must be one of: {', '.join(sorted(VALID_ROLES))}." + ) + + next_action = fields.get("NEXT_ACTION") or "" + if next_action and _is_vague_next_action(next_action): + vague.append("NEXT_ACTION") + + next_prompt = fields.get("NEXT_PROMPT") or "" + if not _next_prompt_ok(next_prompt): + if "NEXT_PROMPT" not in missing: + vague.append("NEXT_PROMPT") + + state = _state_value(fields) + blockers = (fields.get("BLOCKERS") or "").strip() + if "blocked" in state: + if not blockers or blockers.lower() in {"none", "n/a"}: + missing.append("BLOCKERS (unblock condition)") + elif not _UNBLOCK_RE.search(blockers): + vague.append("BLOCKERS") + extra.append("BLOCKED state requires an explicit unblock condition in BLOCKERS.") + + if "superseded" in state: + canon = (fields.get("CANONICAL_ITEM") or fields.get("SUPERSEDED_BY") or "").strip() + superseded = (fields.get("SUPERSEDED_ITEM") or fields.get("SUPERSEDES") or "").strip() + if not canon and "CANONICAL_ITEM" not in missing: + missing.append("CANONICAL_ITEM") + if not superseded and "SUPERSEDED_ITEM" not in missing: + missing.append("SUPERSEDED_ITEM") + + if "ready-to-merge" in state or "ready to merge" in state: + head_sha = fields.get("HEAD_SHA") or "" + merge_ready = fields.get("MERGE_READY") or "" + review_status = fields.get("REVIEW_STATUS") or "" + validation = fields.get("VALIDATION") or "" + proof_blob = " ".join((head_sha, merge_ready, review_status, validation)) + has_sha = bool(_FULL_SHA_RE.search(proof_blob) or _SHORT_SHA_RE.search(proof_blob)) + has_approval = bool(_APPROVAL_PROOF_RE.search(proof_blob)) + if not has_sha or not has_approval: + extra.append( + "ready-to-merge STATE requires approval proof and HEAD_SHA in " + "REVIEW_STATUS, MERGE_READY, HEAD_SHA, or VALIDATION." + ) + + if ctx in ("pr_comment", "pr_review"): + head_sha = (fields.get("HEAD_SHA") or "").strip() + if not head_sha or not _SHORT_SHA_RE.search(head_sha): + if "HEAD_SHA" not in missing: + missing.append("HEAD_SHA") + + if ctx == "issue_comment" and _PR_REF_RE.search(text): + related = (fields.get("RELATED_PRS") or "").strip() + if not related or related.lower() in {"none", "n/a", "-"}: + missing.append("RELATED_PRS") + + allowed = not missing and not vague and not extra + correction = "" + if not allowed: + correction = _build_correction_message( + missing_fields=missing, + vague_fields=vague, + extra_reasons=extra, + context=ctx, + ) + + return { + "allowed": allowed, + "is_workflow_comment": True, + "context": ctx, + "missing_fields": missing, + "vague_fields": vague, + "extra_reasons": extra, + "correction_message": correction, + "suggested_template": _suggested_template(ctx) if not allowed else "", + } \ No newline at end of file diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d0e472d..912b460 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -818,6 +818,55 @@ touched release state names the exact tag/commit and why. Design debates belong in **discussion/RFC issues** (e.g. #100 `profiles.json v2`) — comment on the issue, create no branches/PRs, and end the comment with this handoff. +## Canonical comment validation (#496) + +Workflow-changing issue/PR/review comments must carry durable next-action +state. Casual discussion is still allowed. + +The MCP server runs `canonical_comment_validator.assess_canonical_comment` +**before** posting through: + +- `gitea_create_issue_comment` +- `gitea_submit_pr_review` / `gitea_dry_run_pr_review` (non-empty review bodies) +- `gitea_reconcile_already_landed_pr` when `post_comment=True` +- internal structured comment helpers (machine lease/heartbeat markers stay exempt) + +Detection examples: + +- **Allowed:** `Thanks, I will check this.` +- **Rejected:** `Blocked, author should fix.` (workflow trigger without canonical fields) + +When validation fails, the tool returns `canonical_comment_validation` with +`allowed: false`, `missing_fields`, `vague_fields`, `correction_message`, and +`suggested_template`. **No Gitea API call is made.** + +Minimum workflow comment fields: + +```text +STATE: +WHO_IS_NEXT: +NEXT_ACTION: +NEXT_PROMPT: +WHY: +``` + +`WHO_IS_NEXT` must be one of: `controller`, `author`, `reviewer`, `merger`, +`reconciler`, `user`. + +Issue comments also require `RELATED_PRS`, `BLOCKERS`, and `VALIDATION` when +they mention PR work. PR comments/reviews also require `ISSUE`, `HEAD_SHA`, +`REVIEW_STATUS`, `MERGE_READY`, `BLOCKERS`, and `VALIDATION`. + +Special states: + +- `STATE: blocked` — `BLOCKERS` must name an explicit unblock condition. +- `STATE: superseded` — requires `CANONICAL_ITEM` and `SUPERSEDED_ITEM`. +- `STATE: ready-to-merge` — requires approval proof and head SHA in + `HEAD_SHA`, `REVIEW_STATUS`, `MERGE_READY`, or `VALIDATION`. + +Final reports must not claim a comment was posted when +`canonical_comment_validation.allowed` is false (#496 AC14). + ## Fail-closed behavior Before any mutating action the workflow verifies identity, active profile, diff --git a/final_report_validator.py b/final_report_validator.py index a807ff9..c5198fe 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -194,6 +194,23 @@ _PAGINATION_PROOF_RE = re.compile( re.IGNORECASE, ) _GIT_FETCH_RE = re.compile(r"\bgit\s+fetch\b", re.IGNORECASE) +_CANONICAL_VALIDATION_REJECTED_RE = re.compile( + r"canonical comment validation failed|" + r"canonical_comment_validation|" + r'"allowed"\s*:\s*false', + re.IGNORECASE, +) +_COMMENT_POSTED_CLAIM_RE = re.compile( + r"(?:issue comments posted\s*:\s+(?!none\b)\S|" + r"pr comments posted\s*:\s+(?!none\b)\S|" + r"comment_id\s*[:=]\s*\d+|" + r"gitea comment (?:was )?posted|" + r"posted (?:issue|pr|canonical) (?:state )?comment|" + r"comment posted successfully|" + r"mcp/gitea mutations\s*:\s*[^;\n]*comment posted|" + r"reconciliation mutations\s*:\s*[^;\n]*comment posted)", + re.IGNORECASE, +) _READONLY_DIAG_RE = re.compile( r"read[- ]only diagnostics\s*:\s*(.+)$", re.IGNORECASE | re.MULTILINE, @@ -348,6 +365,43 @@ def _rule_shared_email_disclosure(report_text: str) -> list[dict[str, str]]: ) +def _rule_shared_canonical_comment_post_claim( + report_text: str, + *, + action_log: list[dict] | None = None, +) -> list[dict[str, str]]: + """#496: reports must not claim comment posted when validator rejected.""" + text = report_text or "" + rejected_in_report = bool(_CANONICAL_VALIDATION_REJECTED_RE.search(text)) + rejected_in_log = False + if action_log: + for entry in action_log: + validation = entry.get("canonical_comment_validation") or {} + if validation.get("allowed") is False: + rejected_in_log = True + break + result = entry.get("result") or {} + nested = result.get("canonical_comment_validation") or {} + if nested.get("allowed") is False: + rejected_in_log = True + break + if not (rejected_in_report or rejected_in_log): + return [] + if not _COMMENT_POSTED_CLAIM_RE.search(text): + return [] + return [ + validator_finding( + "shared.canonical_comment_post_claim", + "block", + "MCP/Gitea mutations", + "report claims a Gitea comment was posted while canonical comment " + "validation rejected the workflow comment", + "do not claim comment posted when validator fail-closed; repair " + "the canonical comment body and retry posting", + ) + ] + + def _rule_reviewer_legacy_workspace_mutations( report_text: str, *, @@ -1279,10 +1333,15 @@ _SHARED_CLEANUP_PROOF_RULES = ( _rule_shared_mcp_native_cleanup_proof, ) +_SHARED_CANONICAL_COMMENT_RULES = ( + _rule_shared_canonical_comment_post_claim, +) + _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "review_pr": [ _rule_shared_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_legacy_workspace_mutations, _rule_reviewer_vague_mutations_none, @@ -1311,6 +1370,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "reconcile_already_landed": [ _rule_reconcile_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, *_SHARED_CLEANUP_PROOF_RULES, _rule_reconcile_stale_author_fields, @@ -1326,12 +1386,14 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "author_issue": [ _rule_shared_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_reviewer_vague_mutations_none, ], "work_issue": [ _rule_shared_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_shared_issue_acceptance_gate, _rule_reviewer_vague_mutations_none, @@ -1341,17 +1403,20 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { "issue_filing": [ _rule_shared_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, ], "inventory": [ _rule_shared_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, _rule_reconcile_pagination_proof, ], "issue_selection": [ _rule_shared_controller_handoff, _rule_shared_email_disclosure, + *_SHARED_CANONICAL_COMMENT_RULES, *_SHARED_ISSUE_LOCK_RULES, ], } diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed650b5..83c134b 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -829,6 +829,7 @@ import review_merge_state_machine # noqa: E402 import pr_work_lease # noqa: E402 import native_mcp_preference # noqa: E402 import worktree_cleanup_audit # noqa: E402 +import canonical_comment_validator as ccv # noqa: E402 # Keyed issue-lock storage (#443): per remote/org/repo/issue files under @@ -3268,6 +3269,13 @@ def _evaluate_pr_review_submission( result["pr_work_lease"] = lease_block return result + if (body or "").strip(): + gate = _canonical_comment_gate(body, context="pr_review") + if gate["blocked"]: + reasons.extend(gate["reasons"]) + result["canonical_comment_validation"] = gate["canonical_comment_validation"] + return result + result["would_perform"] = True if not live: reasons.append( @@ -5207,6 +5215,12 @@ def gitea_reconcile_already_landed_pr( "gitea.pr.comment" ) return result + gate = _canonical_comment_gate(comment_body) + if gate["blocked"]: + result["success"] = False + result["reasons"].extend(gate["reasons"]) + result["canonical_comment_validation"] = gate["canonical_comment_validation"] + return result comment_url = f"{base}/issues/{pr_number}/comments" with _audited( "comment_pr", @@ -6444,6 +6458,15 @@ def gitea_create_issue_comment( blocked["permission_report"] = _permission_block_report( "gitea.issue.comment") return blocked + canon_gate = _canonical_comment_gate(body, context="issue_comment") + if canon_gate["blocked"]: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "reasons": canon_gate["reasons"], + "canonical_comment_validation": canon_gate["canonical_comment_validation"], + } h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) api = f"{repo_api_url(h, o, r)}/issues/{issue_number}/comments" @@ -7973,6 +7996,34 @@ def gitea_audit_config() -> dict: return report +def _canonical_comment_gate( + body: str, + *, + context: str | None = None, + force_workflow: bool = False, +) -> dict: + """Fail-closed canonical state validation before comment/review POST (#496).""" + assessment = ccv.assess_canonical_comment( + body, + context=context, + force_workflow=force_workflow, + ) + if assessment.get("allowed"): + return { + "blocked": False, + "canonical_comment_validation": assessment, + "reasons": [], + } + correction = (assessment.get("correction_message") or "").strip() + return { + "blocked": True, + "canonical_comment_validation": assessment, + "reasons": [ + correction or "canonical comment validation failed (fail closed before posting)" + ], + } + + def _post_structured_issue_comment( *, issue_number: int, @@ -7982,8 +8033,18 @@ def _post_structured_issue_comment( org: str | None, repo: str | None, audit_op: str = "create_issue_comment", + comment_context: str | None = None, ) -> dict: """Post an issue-thread comment after permission gates already passed.""" + gate = _canonical_comment_gate(body, context=comment_context) + if gate["blocked"]: + return { + "success": False, + "performed": False, + "issue_number": issue_number, + "reasons": gate["reasons"], + "canonical_comment_validation": gate["canonical_comment_validation"], + } h, o, r = _resolve(remote, host, org, repo) auth = _auth(h) api = f"{repo_api_url(h, o, r)}/issues/{issue_number}/comments" diff --git a/tests/test_canonical_comment_validator.py b/tests/test_canonical_comment_validator.py new file mode 100644 index 0000000..47e75d8 --- /dev/null +++ b/tests/test_canonical_comment_validator.py @@ -0,0 +1,217 @@ +"""Unit tests for canonical_comment_validator (#496).""" + +import unittest + +import canonical_comment_validator as ccv + +FULL_SHA = "a" * 40 + +_VALID_ISSUE = f"""## Canonical Issue State + +STATE: ready-for-author +WHO_IS_NEXT: author +NEXT_ACTION: Implement canonical comment validator and wire MCP gates +NEXT_PROMPT: +```text +You are the author LLM for issue #496. Add canonical_comment_validator.py +and wire fail-closed gates into gitea_create_issue_comment. +``` +WHAT_HAPPENED: Issue filed for MCP validation wall +WHY: Workflow comments must carry durable next-action state +RELATED_PRS: none +BLOCKERS: none +VALIDATION: unit tests not yet run +LAST_UPDATED_BY: prgs-author +""" + +_VALID_PR = f"""## Canonical PR State + +STATE: ready-for-review +WHO_IS_NEXT: reviewer +NEXT_ACTION: Run focused pytest and review the validator wiring diff +NEXT_PROMPT: +```text +Review PR for issue #496. Confirm casual comments still post and workflow +comments without canonical fields are rejected before the Gitea API call. +``` +WHAT_HAPPENED: Author opened PR with validator module +WHY: Fail-closed enforcement belongs at the MCP boundary +ISSUE: #496 +HEAD_SHA: {FULL_SHA} +REVIEW_STATUS: pending +MERGE_READY: false +BLOCKERS: none +VALIDATION: not yet run +LAST_UPDATED_BY: prgs-author +""" + +_VALID_SUPERSEDED = """## Canonical PR State + +STATE: superseded +WHO_IS_NEXT: reconciler +NEXT_ACTION: Close superseded issue after canonical issue #495 lands +NEXT_PROMPT: +```text +Close issue #494 as superseded by #495 once templates and docs are merged. +``` +WHY: Duplicate tracking issue replaced by durable canonical template work +CANONICAL_ITEM: issue #495 +SUPERSEDED_ITEM: issue #494 +CLOSE_OR_KEEP_OPEN: close superseded issue #494 after #495 merges +""" + +_VALID_BLOCKED = """## Canonical Issue State + +STATE: blocked +WHO_IS_NEXT: author +NEXT_ACTION: Fix failing validator unit tests before requesting re-review +NEXT_PROMPT: +```text +Run pytest tests/test_canonical_comment_validator.py and repair failures. +``` +WHAT_HAPPENED: Validator tests failed in CI +WHY: Blocked until test suite is green +RELATED_PRS: PR #500 +BLOCKERS: pytest failures; unblock after all validator tests pass +VALIDATION: pytest failed +LAST_UPDATED_BY: prgs-author +""" + +_VALID_READY_TO_MERGE = f"""## Canonical PR State + +STATE: ready-to-merge +WHO_IS_NEXT: merger +NEXT_ACTION: Merge PR after confirming approval_at_current_head +NEXT_PROMPT: +```text +Merge PR #500 for issue #496 after live mergeable check passes. +``` +WHAT_HAPPENED: Reviewer approved at current head +WHY: All gates passed and head SHA is current +ISSUE: #496 +HEAD_SHA: {FULL_SHA} +REVIEW_STATUS: approved / approval_at_current_head +MERGE_READY: true +BLOCKERS: none +VALIDATION: pytest passed; reviewer approved at head {FULL_SHA} +LAST_UPDATED_BY: prgs-reviewer +""" + + +class TestCanonicalCommentDetection(unittest.TestCase): + def test_casual_comment_not_workflow(self): + self.assertFalse(ccv.is_workflow_changing_comment("Thanks, I will check this.")) + + def test_workflow_trigger_detected(self): + self.assertTrue(ccv.is_workflow_changing_comment("Blocked, author should fix.")) + + def test_machine_marker_exempt(self): + body = "<!-- mcp-review-lease:v1 -->\nphase: claimed" + self.assertFalse(ccv.is_workflow_changing_comment(body)) + + +class TestCanonicalCommentValidation(unittest.TestCase): + def test_casual_comment_allowed(self): + result = ccv.assess_canonical_comment("Thanks, I will check this.") + self.assertTrue(result["allowed"]) + self.assertFalse(result["is_workflow_comment"]) + + def test_workflow_comment_missing_fields_rejected(self): + result = ccv.assess_canonical_comment("Blocked, author should fix.") + self.assertFalse(result["allowed"]) + self.assertTrue(result["is_workflow_comment"]) + self.assertIn("WHO_IS_NEXT", result["missing_fields"]) + self.assertIn("correction_message", result) + self.assertIn("Suggested template", result["correction_message"]) + + def test_missing_next_prompt_rejected(self): + body = """STATE: blocked +WHO_IS_NEXT: author +NEXT_ACTION: Repair the failing validator unit tests in CI +WHY: Tests must pass before merge +BLOCKERS: pytest failed; unblock after tests pass +VALIDATION: failed +""" + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("NEXT_PROMPT", result["missing_fields"] + result["vague_fields"]) + + def test_vague_next_action_rejected(self): + body = _VALID_ISSUE.replace( + "Implement canonical comment validator and wire MCP gates", + "continue", + ) + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("NEXT_ACTION", result["vague_fields"]) + + def test_invalid_who_is_next_rejected(self): + body = _VALID_ISSUE.replace("WHO_IS_NEXT: author", "WHO_IS_NEXT: llm") + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("WHO_IS_NEXT", result["vague_fields"]) + + def test_valid_issue_comment_allowed(self): + result = ccv.assess_canonical_comment(_VALID_ISSUE) + self.assertTrue(result["allowed"]) + + def test_issue_comment_mentioning_pr_requires_related_prs(self): + body = _VALID_ISSUE.replace("RELATED_PRS: none", "RELATED_PRS:") + body = body.replace("Issue filed", "Issue filed; see PR #500") + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("RELATED_PRS", result["missing_fields"]) + + def test_pr_review_requires_head_sha(self): + body = _VALID_PR.replace(f"HEAD_SHA: {FULL_SHA}", "HEAD_SHA:") + result = ccv.assess_canonical_comment(body, context="pr_review") + self.assertFalse(result["allowed"]) + self.assertIn("HEAD_SHA", result["missing_fields"]) + + def test_ready_to_merge_requires_approval_and_sha_proof(self): + body = _VALID_READY_TO_MERGE.replace("approval_at_current_head", "pending") + body = body.replace(f"HEAD_SHA: {FULL_SHA}", "HEAD_SHA: pending") + body = body.replace("reviewer approved at head", "reviewer pending at head") + body = body.replace("approved /", "pending /") + result = ccv.assess_canonical_comment(body, context="pr_comment") + self.assertFalse(result["allowed"]) + self.assertTrue(result["extra_reasons"]) + + def test_ready_to_merge_valid_allowed(self): + result = ccv.assess_canonical_comment(_VALID_READY_TO_MERGE, context="pr_comment") + self.assertTrue(result["allowed"]) + + def test_superseded_requires_canonical_and_superseded_items(self): + body = _VALID_SUPERSEDED.replace("CANONICAL_ITEM: issue #495", "CANONICAL_ITEM:") + result = ccv.assess_canonical_comment(body, context="supersession") + self.assertFalse(result["allowed"]) + self.assertIn("CANONICAL_ITEM", result["missing_fields"]) + + def test_superseded_valid_allowed(self): + result = ccv.assess_canonical_comment(_VALID_SUPERSEDED, context="supersession") + self.assertTrue(result["allowed"]) + + def test_blocked_requires_unblock_condition(self): + body = _VALID_BLOCKED.replace( + "BLOCKERS: pytest failures; unblock after all validator tests pass", + "BLOCKERS: pytest failures", + ) + result = ccv.assess_canonical_comment(body) + self.assertFalse(result["allowed"]) + self.assertIn("BLOCKERS", result["vague_fields"]) + + def test_blocked_valid_allowed(self): + result = ccv.assess_canonical_comment(_VALID_BLOCKED) + self.assertTrue(result["allowed"]) + + def test_correction_lists_missing_fields(self): + result = ccv.assess_canonical_comment("ready for merge") + self.assertFalse(result["allowed"]) + message = result["correction_message"] + self.assertIn("Missing fields", message) + self.assertIn("WHO_IS_NEXT", message) + self.assertIn("Suggested template", message) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 70121f4..5d9e526 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -573,6 +573,31 @@ class TestReconcilerCloseProof(unittest.TestCase): ) +class TestCanonicalCommentPostClaim(unittest.TestCase): + def test_blocks_post_claim_when_validator_rejected_in_report(self): + report = "\n".join([ + "## Controller Handoff", + "- MCP/Gitea mutations: issue comment posted", + "- Issue comments posted: 1 canonical state comment", + "canonical comment validation failed (fail closed before posting).", + '- canonical_comment_validation: {"allowed": false}', + ]) + result = assess_final_report_validator(report, "work_issue") + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("shared.canonical_comment_post_claim", rule_ids) + self.assertTrue(result["blocked"]) + + def test_allows_none_when_validator_rejected_without_post_claim(self): + report = "\n".join([ + "## Controller Handoff", + "- Issue comments posted: none", + "canonical comment validation failed (fail closed before posting).", + ]) + result = assess_final_report_validator(report, "work_issue") + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertNotIn("shared.canonical_comment_post_claim", rule_ids) + + class TestEntryPoint(unittest.TestCase): def test_unknown_task_kind_blocks(self): result = assess_final_report_validator("report", "unknown_mode") diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 503e879..dd1a20d 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3102,6 +3102,36 @@ class TestIssueCommentTools(unittest.TestCase): self.assertFalse(result["success"]) mock_api.assert_not_called() + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_casual_comment_allowed(self, _auth, mock_api): + mock_api.return_value = self._comment(556, "gitea-author", "casual") + with patch.dict(os.environ, self.AUTHOR_ENV, clear=True): + result = gitea_create_issue_comment( + issue_number=9, + body="Thanks, I will check this.", + remote="prgs", + ) + self.assertTrue(result["success"]) + self.assertTrue(result["performed"]) + mock_api.assert_called_once() + + @patch("mcp_server.api_request") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_create_workflow_comment_missing_fields_blocked(self, _auth, mock_api): + with patch.dict(os.environ, self.AUTHOR_ENV, clear=True): + result = gitea_create_issue_comment( + issue_number=9, + body="Blocked, author should fix.", + remote="prgs", + ) + self.assertFalse(result["success"]) + self.assertFalse(result["performed"]) + self.assertIn("canonical_comment_validation", result) + self.assertFalse(result["canonical_comment_validation"]["allowed"]) + self.assertTrue(result["canonical_comment_validation"]["is_workflow_comment"]) + mock_api.assert_not_called() + @patch("mcp_server.api_request") @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_create_missing_issue_error_is_redacted(self, _auth, mock_api): From 8ff19240474020898b6c835ff41e3fd0ef16f485 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:49:03 -0400 Subject: [PATCH 11/12] feat: block direct MCP imports and unsanctioned keychain access (Closes #558) Require a sanctioned MCP daemon (or pytest) before get_auth_header and keychain credential fill so raw shell imports cannot bypass preflight gates. --- docs/llm-workflow-runbooks.md | 8 ++++ docs/mcp-daemon-import-guard.md | 23 +++++++++ gitea_auth.py | 11 +++++ gitea_mcp_server.py | 5 ++ mcp_daemon_guard.py | 84 +++++++++++++++++++++++++++++++++ mcp_server.py | 11 +++++ tests/test_mcp_daemon_guard.py | 67 ++++++++++++++++++++++++++ 7 files changed, 209 insertions(+) create mode 100644 docs/mcp-daemon-import-guard.md create mode 100644 mcp_daemon_guard.py create mode 100644 tests/test_mcp_daemon_guard.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d0e472d..7471b75 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -408,6 +408,14 @@ 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. + +## No direct-import mutation path (#558) + +Never `import gitea_mcp_server` or call `gitea_auth.get_auth_header` / +keychain fill from a raw shell to bypass MCP preflight. + +Use the official MCP daemon only. See [`docs/mcp-daemon-import-guard.md`](mcp-daemon-import-guard.md). + ## Shell Spawn Hard-Stop Rule Symptom: a shell tool call returns `exit_code: -1` with empty stdout/stderr. diff --git a/docs/mcp-daemon-import-guard.md b/docs/mcp-daemon-import-guard.md new file mode 100644 index 0000000..8378dbd --- /dev/null +++ b/docs/mcp-daemon-import-guard.md @@ -0,0 +1,23 @@ +# MCP daemon import and keychain guard (#558) + +## Problem + +During deadlock debugging, agents imported `gitea_mcp_server` / ran credential +helpers from a raw shell, bypassing preflight purity and role gates. + +## Rule + +Mutation auth and keychain fill require a **sanctioned MCP daemon** process. + +| Context | Allowed | +|---------|---------| +| Official MCP entrypoint (`mcp_server.py` / `gitea_mcp_server` `__main__`) sets `GITEA_MCP_SANCTIONED_DAEMON=1` | yes | +| pytest | yes | +| `GITEA_ALLOW_DIRECT_MCP_IMPORT=1` (operator/tests only) | yes | +| bare `python -c 'import gitea_auth; get_auth_header(...)'` | **no** | +| keychain fill without daemon | **no** unless `GITEA_ALLOW_KEYCHAIN_CLI=1` | + +## Operator note + +LLM sessions must never set the allow-direct-import or allow-keychain-cli +overrides. Those are human-only escape hatches. diff --git a/gitea_auth.py b/gitea_auth.py index 5e30213..167357b 100644 --- a/gitea_auth.py +++ b/gitea_auth.py @@ -104,6 +104,10 @@ def get_credentials(host): # 3. Optional fallback to macOS Keychain via git credential fill if not user and not password and os.environ.get("GITEA_USE_KEYCHAIN") == "1": + # #558: block raw keychain dumps outside the sanctioned MCP daemon. + import mcp_daemon_guard + + mcp_daemon_guard.assert_keychain_access_allowed() cmd_parts = ["git", "creden" + "tial", "fi" + "ll"] try: p = subprocess.Popen( @@ -116,6 +120,8 @@ def get_credentials(host): user = line.split("=", 1)[1] elif line.startswith("password="): password = line.split("=", 1)[1] + except mcp_daemon_guard.UnsanctionedRuntimeError: + raise except Exception: pass @@ -124,6 +130,11 @@ def get_credentials(host): def get_auth_header(host): """Return an ``Authorization`` header value for *host*.""" + # #558: resolving credentials for API mutation must not happen via ad-hoc + # direct imports that bypass the MCP daemon preflight wall. + import mcp_daemon_guard + + mcp_daemon_guard.assert_sanctioned_mutation_runtime("get_auth_header") host_key = host.lower().strip() # 1. Try Token-based auth from dynamic configs diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed650b5..9838ba5 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -9079,6 +9079,11 @@ def gitea_capability_stop_terminal_report() -> dict: # ── Entry point ─────────────────────────────────────────────────────────────── if __name__ == "__main__": + # #558: mark this process as the official MCP daemon before any tool + # dispatch so direct shell imports cannot reuse mutation/auth paths. + import mcp_daemon_guard + + mcp_daemon_guard.mark_sanctioned_daemon() # Lock this session's launch profile into the environment so child CLI # processes (e.g. review_pr.py) can detect and refuse profile # side-channel overrides (#199). diff --git a/mcp_daemon_guard.py b/mcp_daemon_guard.py new file mode 100644 index 0000000..241c68b --- /dev/null +++ b/mcp_daemon_guard.py @@ -0,0 +1,84 @@ +"""Sanctioned MCP daemon guards for imports and credential access (#558). + +Direct ``import gitea_mcp_server`` / ``import gitea_auth`` from a shell, plus +raw keychain dumps, bypass preflight purity and role gates. Mutation helpers +and keychain fallbacks therefore require an explicit sanctioned runtime. + +Sanctioned contexts (any one): +- ``GITEA_MCP_SANCTIONED_DAEMON=1`` (set by the official MCP entrypoint) +- pytest (``PYTEST_CURRENT_TEST`` present) +- explicit operator opt-in ``GITEA_ALLOW_DIRECT_MCP_IMPORT=1`` (tests/tools only) + +Credential keychain fill additionally allows: +- ``GITEA_ALLOW_KEYCHAIN_CLI=1`` for operator-only non-MCP scripts that must + use git-credential (never the default for LLM shells). +""" + +from __future__ import annotations + +import os +from typing import Any + +SANCTIONED_DAEMON_ENV = "GITEA_MCP_SANCTIONED_DAEMON" +ALLOW_DIRECT_IMPORT_ENV = "GITEA_ALLOW_DIRECT_MCP_IMPORT" +ALLOW_KEYCHAIN_CLI_ENV = "GITEA_ALLOW_KEYCHAIN_CLI" + + +class UnsanctionedRuntimeError(RuntimeError): + """Raised when mutation/credential code runs outside a sanctioned MCP daemon.""" + + +def is_pytest_runtime() -> bool: + return bool((os.environ.get("PYTEST_CURRENT_TEST") or "").strip()) + + +def is_sanctioned_mcp_daemon() -> bool: + if (os.environ.get(SANCTIONED_DAEMON_ENV) or "").strip() in {"1", "true", "yes"}: + return True + if (os.environ.get(ALLOW_DIRECT_IMPORT_ENV) or "").strip() in {"1", "true", "yes"}: + return True + if is_pytest_runtime(): + return True + return False + + +def mark_sanctioned_daemon() -> None: + """Call from the official MCP server entrypoint before serving tools.""" + os.environ[SANCTIONED_DAEMON_ENV] = "1" + + +def assert_sanctioned_mutation_runtime(context: str = "mutation") -> None: + """Fail closed when server mutation code is used outside the MCP daemon.""" + if is_sanctioned_mcp_daemon(): + return + raise UnsanctionedRuntimeError( + f"Unsanctioned runtime blocked {context} (#558). " + "Do not import gitea_mcp_server / call mutation helpers from a raw " + "shell or ad-hoc script. Use the official MCP daemon entrypoint " + f"(sets {SANCTIONED_DAEMON_ENV}=1), or run under pytest. " + f"Operator-only override: {ALLOW_DIRECT_IMPORT_ENV}=1 (not for LLM sessions)." + ) + + +def assert_keychain_access_allowed() -> None: + """Fail closed for git-credential keychain fill outside sanctioned contexts.""" + if is_sanctioned_mcp_daemon(): + return + if (os.environ.get(ALLOW_KEYCHAIN_CLI_ENV) or "").strip() in {"1", "true", "yes"}: + return + raise UnsanctionedRuntimeError( + "Unsanctioned keychain/credential fill blocked (#558). " + "Token extraction via git-credential is only allowed inside the " + f"official MCP daemon ({SANCTIONED_DAEMON_ENV}=1), pytest, or with " + f"explicit operator opt-in {ALLOW_KEYCHAIN_CLI_ENV}=1." + ) + + +def runtime_status() -> dict[str, Any]: + return { + "sanctioned_daemon": is_sanctioned_mcp_daemon(), + "pytest": is_pytest_runtime(), + "sanctioned_env": SANCTIONED_DAEMON_ENV, + "allow_direct_import_env": ALLOW_DIRECT_IMPORT_ENV, + "allow_keychain_cli_env": ALLOW_KEYCHAIN_CLI_ENV, + } diff --git a/mcp_server.py b/mcp_server.py index dbe5f1b..479273b 100644 --- a/mcp_server.py +++ b/mcp_server.py @@ -37,6 +37,17 @@ def check_conflict_markers(): check_conflict_markers() +# #558: official entrypoint marks the process as the sanctioned MCP daemon +# before loading mutation modules (blocks raw shell import bypasses). +try: + import mcp_daemon_guard + + mcp_daemon_guard.mark_sanctioned_daemon() +except Exception: + # Guard import failures must not hide conflict-marker infra_stop above; + # gitea_mcp_server main also marks sanctioned when run as __main__. + pass + # Execute the actual server logic via exec in this namespace. impl_path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "gitea_mcp_server.py") try: diff --git a/tests/test_mcp_daemon_guard.py b/tests/test_mcp_daemon_guard.py new file mode 100644 index 0000000..45007a2 --- /dev/null +++ b/tests/test_mcp_daemon_guard.py @@ -0,0 +1,67 @@ +"""Tests for sanctioned MCP daemon guards (#558).""" + +from __future__ import annotations + +import os +import unittest +from unittest.mock import patch + +import mcp_daemon_guard +import gitea_auth + + +class TestMcpDaemonGuard(unittest.TestCase): + def test_unsanctioned_blocks_mutation_runtime(self): + env = {k: v for k, v in os.environ.items() if k not in { + mcp_daemon_guard.SANCTIONED_DAEMON_ENV, + mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, + "PYTEST_CURRENT_TEST", + }} + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError): + mcp_daemon_guard.assert_sanctioned_mutation_runtime("test") + + def test_pytest_allows_runtime(self): + # Running under pytest already sets PYTEST_CURRENT_TEST. + mcp_daemon_guard.assert_sanctioned_mutation_runtime("pytest") + + def test_mark_sanctioned_allows(self): + env = {k: v for k, v in os.environ.items() if k != "PYTEST_CURRENT_TEST"} + env[mcp_daemon_guard.SANCTIONED_DAEMON_ENV] = "1" + with patch.dict(os.environ, env, clear=True): + mcp_daemon_guard.assert_sanctioned_mutation_runtime("daemon") + + def test_keychain_blocked_without_sanction(self): + env = { + k: v + for k, v in os.environ.items() + if k + not in { + mcp_daemon_guard.SANCTIONED_DAEMON_ENV, + mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, + mcp_daemon_guard.ALLOW_KEYCHAIN_CLI_ENV, + "PYTEST_CURRENT_TEST", + } + } + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError): + mcp_daemon_guard.assert_keychain_access_allowed() + + def test_get_auth_header_blocked_when_unsanctioned(self): + env = { + k: v + for k, v in os.environ.items() + if k + not in { + mcp_daemon_guard.SANCTIONED_DAEMON_ENV, + mcp_daemon_guard.ALLOW_DIRECT_IMPORT_ENV, + "PYTEST_CURRENT_TEST", + } + } + with patch.dict(os.environ, env, clear=True): + with self.assertRaises(mcp_daemon_guard.UnsanctionedRuntimeError): + gitea_auth.get_auth_header("gitea.prgs.cc") + + +if __name__ == "__main__": + unittest.main() From 0a202970fdf6da16b319f79a2051888798a7a4e1 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 23:04:57 -0400 Subject: [PATCH 12/12] feat: discover merged cleanup worktree aliases --- gitea_mcp_server.py | 5 +- merged_cleanup_reconcile.py | 219 ++++++++++++++++++++---- tests/test_merged_cleanup_reconcile.py | 228 +++++++++++++++++++++---- 3 files changed, 385 insertions(+), 67 deletions(-) diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index ed650b5..d9bdbf1 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -4744,6 +4744,7 @@ def gitea_reconcile_merged_cleanups( remote_branch_exists=remote_branch_exists, head_on_master=head_on_master, delete_capability_allowed=delete_capability_allowed, + target_ref=master_ref, ) if dry_run: @@ -4783,7 +4784,9 @@ def gitea_reconcile_merged_cleanups( if local_assessment.get("safe_to_remove_worktree"): result = merged_cleanup_reconcile.remove_local_worktree( - PROJECT_ROOT, head_branch + PROJECT_ROOT, + head_branch, + worktree_path=local_assessment.get("worktree_path"), ) actions.append({"action": "remove_local_worktree", **result}) diff --git a/merged_cleanup_reconcile.py b/merged_cleanup_reconcile.py index 92490ac..b2abad9 100644 --- a/merged_cleanup_reconcile.py +++ b/merged_cleanup_reconcile.py @@ -17,6 +17,7 @@ import issue_lock_store PROTECTED_BRANCHES = frozenset({"master", "main", "dev"}) CLOSES_FIXES_RE = re.compile(r"\b(?:closes|fixes)\s+#(\d+)\b", re.IGNORECASE) +ISSUE_MARKER_RE = re.compile(r"(?:^|[-_/])issue-(\d+)(?:[-_/]|$)", re.IGNORECASE) def extract_linked_issue(title: str, body: str) -> int | None: @@ -36,8 +37,17 @@ 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 extract_issue_marker(*values: str | None) -> int | None: + """Return the first issue marker found in branch/path-like values.""" + for value in values: + match = ISSUE_MARKER_RE.search(value or "") + if match: + return int(match.group(1)) + return None + + +def list_local_worktrees(project_root: str) -> list[dict[str, Any]]: + """Parse ``git worktree list --porcelain`` into structured entries.""" res = subprocess.run( ["git", "-C", project_root, "worktree", "list", "--porcelain"], capture_output=True, @@ -45,25 +55,153 @@ 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 - for line in res.stdout.splitlines(): - line = line.strip() + entries: list[dict[str, Any]] = [] + current: dict[str, Any] | None = None + + def flush() -> None: + nonlocal current + if current: + entries.append(current) + current = None + + for raw_line in (res.stdout or "").splitlines(): + line = raw_line.strip() if not line: - current_worktree = None + flush() continue if line.startswith("worktree "): - current_worktree = line[9:].strip() - elif line.startswith("branch ") and current_worktree: + flush() + current = {"path": line[9:].strip()} + elif current is not None and line.startswith("HEAD "): + current["head_sha"] = line[5:].strip() + elif current is not None and line.startswith("branch "): ref = line[7:].strip() - if ref.startswith("refs/heads/"): - branch_name = ref[11:] - mapping[branch_name] = current_worktree + current["branch_ref"] = ref + current["branch"] = ( + ref[len("refs/heads/"):] + if ref.startswith("refs/heads/") + else ref + ) + elif current is not None and line == "detached": + current["detached"] = True + flush() + return entries + + +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 entry in list_local_worktrees(project_root): + branch = entry.get("branch") + path = entry.get("path") + if branch and path: + mapping[str(branch)] = str(path) return mapping +def _is_under_branches(project_root: str, path: str) -> bool: + branches_root = os.path.realpath(os.path.join(project_root, "branches")) + real_path = os.path.realpath(path) + return real_path == branches_root or real_path.startswith(branches_root + os.sep) + + +def _head_is_safe_for_cleanup( + *, + project_root: str, + candidate_head: str | None, + pr_head_sha: str | None, + target_ref: str | None, +) -> bool: + if not candidate_head: + return False + if pr_head_sha and candidate_head == pr_head_sha: + return True + if target_ref: + return is_head_ancestor_of_ref(project_root, candidate_head, target_ref) is True + return False + + +def resolve_cleanup_worktree_state( + *, + project_root: str, + head_branch: str, + issue_number: int | None, + pr_head_sha: str | None = None, + target_ref: str | None = None, +) -> dict[str, Any]: + """Find the exact or safe alias worktree used for local cleanup (#532).""" + expected_path = resolve_worktree_path(project_root, head_branch) + state = read_local_worktree_state(expected_path) + state["worktree_path"] = expected_path + state["expected_worktree_path"] = expected_path + state["discovered_worktree_path"] = expected_path if state.get("exists") else None + state["match_type"] = "derived_path" if state.get("exists") else "none" + state["candidate_paths"] = [] + state["alias_block_reasons"] = [] + if state.get("exists"): + return state + + branch_issue = extract_issue_marker(head_branch) + wanted_issue = issue_number or branch_issue + candidates: list[dict[str, Any]] = [] + unsafe_matches: list[str] = [] + + for entry in list_local_worktrees(project_root): + path = entry.get("path") or "" + if not path or not _is_under_branches(project_root, path): + continue + branch = entry.get("branch") or "" + path_issue = extract_issue_marker(os.path.basename(path), path) + entry_issue = extract_issue_marker(branch) or path_issue + branch_match = branch == head_branch + issue_match = wanted_issue is not None and entry_issue == wanted_issue + if not (branch_match or issue_match): + continue + safe_head = _head_is_safe_for_cleanup( + project_root=project_root, + candidate_head=entry.get("head_sha"), + pr_head_sha=pr_head_sha, + target_ref=target_ref, + ) + if not safe_head and branch_match and not pr_head_sha and not target_ref: + safe_head = True + if not safe_head: + unsafe_matches.append(path) + continue + candidates.append(entry) + + state["candidate_paths"] = [entry.get("path") for entry in candidates] + if len(candidates) == 1: + path = candidates[0]["path"] + alias_state = read_local_worktree_state(path) + alias_state["worktree_path"] = path + alias_state["expected_worktree_path"] = expected_path + alias_state["discovered_worktree_path"] = path + alias_state["match_type"] = ( + "branch" if candidates[0].get("branch") == head_branch else "alias" + ) + alias_state["candidate_paths"] = [path] + alias_state["alias_block_reasons"] = [] + alias_state["alias_verified"] = True + return alias_state + if len(candidates) > 1: + state["alias_block_reasons"].append( + "ambiguous local worktree aliases: " + ", ".join( + sorted(str(entry.get("path")) for entry in candidates) + ) + ) + state["match_type"] = "ambiguous_alias" + elif unsafe_matches: + state["alias_block_reasons"].append( + "matching local worktree aliases did not point to the PR head or target branch: " + + ", ".join(sorted(unsafe_matches)) + ) + state["match_type"] = "unsafe_alias" + return state + + def read_issue_lock(path: str | None = None) -> dict[str, Any] | None: if path: return issue_lock_store.read_lock_file(path.strip()) @@ -204,6 +342,7 @@ def assess_local_worktree_cleanup( exists = bool(worktree_state.get("exists")) if not merged: reasons.append("PR is not merged") + reasons.extend(worktree_state.get("alias_block_reasons") or []) if not exists: reasons.append("local worktree not present") if exists and worktree_state.get("clean") is False: @@ -214,7 +353,11 @@ def assess_local_worktree_cleanup( ) if exists: current_branch = worktree_state.get("current_branch") - if current_branch and current_branch != head_branch: + if ( + current_branch + and current_branch != head_branch + and not worktree_state.get("alias_verified") + ): reasons.append( f"worktree branch '{current_branch}' does not match PR head '{head_branch}'" ) @@ -226,6 +369,10 @@ def assess_local_worktree_cleanup( "pr_number": pr_number, "head_branch": head_branch, "worktree_path": worktree_state.get("worktree_path"), + "expected_worktree_path": worktree_state.get("expected_worktree_path"), + "discovered_worktree_path": worktree_state.get("discovered_worktree_path"), + "match_type": worktree_state.get("match_type"), + "candidate_paths": worktree_state.get("candidate_paths") or [], "worktree_exists": exists, "worktree_clean": worktree_state.get("clean"), "safe_to_remove_worktree": safe, @@ -244,7 +391,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, + target_ref: str | None = None, ) -> dict[str, Any]: pr_number = int(pr["number"]) head_branch = pr.get("head") or "" @@ -253,18 +400,16 @@ def build_pr_cleanup_entry( title = pr.get("title") or "" body = pr.get("body") or "" merged = bool(pr.get("merged_at")) - - 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 + issue_number = extract_linked_issue(title, body) or extract_issue_marker(head_branch) + head_payload = pr.get("head") if isinstance(pr.get("head"), dict) else {} + pr_head_sha = head_payload.get("sha") if isinstance(head_payload, dict) else None + worktree_state = resolve_cleanup_worktree_state( + project_root=project_root, + head_branch=head_branch, + issue_number=issue_number, + pr_head_sha=pr_head_sha, + target_ref=target_ref, + ) active_lock = has_active_issue_lock(head_branch, issue_lock_path) remote = assess_remote_branch_cleanup( @@ -285,11 +430,9 @@ 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), + "issue_number": issue_number, "title": title, "head_branch": head_branch, "merge_commit_sha": pr.get("merge_commit_sha"), @@ -310,9 +453,9 @@ def build_reconciliation_report( delete_capability_allowed: bool, issue_lock_path: str | None = None, protected_branches: frozenset[str] | None = None, + target_ref: 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"): @@ -331,7 +474,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, + target_ref=target_ref, ) ) return { @@ -343,10 +486,12 @@ def build_reconciliation_report( } -def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: - worktree_map = discover_local_worktrees(project_root) - discovered_path = worktree_map.get(branch) - worktree_path = discovered_path or resolve_worktree_path(project_root, branch) +def remove_local_worktree( + project_root: str, + branch: str, + worktree_path: str | None = None, +) -> dict[str, Any]: + worktree_path = worktree_path or resolve_worktree_path(project_root, branch) if not os.path.isdir(worktree_path): return { "success": False, @@ -370,4 +515,4 @@ def remove_local_worktree(project_root: str, branch: str) -> dict[str, Any]: "performed": True, "message": f"removed worktree {worktree_path}", "worktree_path": worktree_path, - } \ No newline at end of file + } diff --git a/tests/test_merged_cleanup_reconcile.py b/tests/test_merged_cleanup_reconcile.py index ad4a2ee..19f7f30 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, MagicMock +from unittest.mock import Mock, patch sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) @@ -163,7 +163,7 @@ class TestMergedCleanupReport(unittest.TestCase): "branch refs/heads/feat/issue-11-x\n" "HEAD cafebabe\n\n" ) - mock_run.return_value = MagicMock(returncode=0, stdout=mock_output) + mock_run.return_value = Mock(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"}) @@ -177,14 +177,23 @@ class TestMergedCleanupReport(unittest.TestCase): "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": [], - } + mock_run.return_value = Mock(returncode=0, stdout=mock_output) + mock_state.side_effect = [ + { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + }, + { + "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", @@ -209,29 +218,190 @@ class TestMergedCleanupReport(unittest.TestCase): self.assertEqual(local["worktree_path"], "/repo/Gitea-Tools/branches/custom-name") self.assertEqual(local["match_type"], "branch") + @patch("merged_cleanup_reconcile.read_local_worktree_state") @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" + def test_build_report_discovers_issue_alias_worktree(self, mock_run, mock_state): + mock_run.return_value = Mock( + returncode=0, + stdout=( + "worktree /repo/Gitea-Tools\n" + "HEAD aaaa\n" + "branch refs/heads/master\n\n" + "worktree /repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation\n" + "HEAD cafebabe\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\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=""), + mock_state.side_effect = [ + { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + }, + { + "exists": True, + "clean": True, + "current_branch": "feat/issue-510-workspace-binding-isolation", + "head_sha": "cafebabe", + "dirty_files": [], + }, ] - with patch("os.path.isdir", return_value=True): - result = mcr.remove_local_worktree("/repo/Gitea-Tools", "feat/issue-11-x") + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 512, + "title": "merged cleanup", + "body": "Closes #510", + "head": { + "ref": "feat/issue-510-workspace-binding-isolation", + "sha": "cafebabe", + }, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "abc123", + } + ], + open_prs=[], + remote_branch_exists={"feat/issue-510-workspace-binding-isolation": True}, + head_on_master={512: True}, + delete_capability_allowed=True, + target_ref="prgs/master", + ) + + local = report["entries"][0]["local_worktree"] + self.assertTrue(local["safe_to_remove_worktree"]) + self.assertEqual(local["match_type"], "branch") + self.assertEqual( + local["expected_worktree_path"], + "/repo/Gitea-Tools/branches/feat-issue-510-workspace-binding-isolation", + ) + self.assertEqual( + local["discovered_worktree_path"], + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + + @patch("merged_cleanup_reconcile.read_local_worktree_state") + @patch("subprocess.run") + def test_ambiguous_issue_alias_worktrees_fail_closed(self, mock_run, mock_state): + mock_run.return_value = Mock( + returncode=0, + stdout=( + "worktree /repo/Gitea-Tools/branches/issue-510-one\n" + "HEAD cafebabe\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + "worktree /repo/Gitea-Tools/branches/issue-510-two\n" + "HEAD cafebabe\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + ), + ) + mock_state.return_value = { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + } + + report = mcr.build_reconciliation_report( + project_root="/repo/Gitea-Tools", + closed_prs=[ + { + "number": 512, + "title": "merged cleanup", + "body": "Closes #510", + "head": { + "ref": "feat/issue-510-workspace-binding-isolation", + "sha": "cafebabe", + }, + "merged_at": "2026-07-06T12:00:00Z", + "merge_commit_sha": "abc123", + } + ], + open_prs=[], + remote_branch_exists={"feat/issue-510-workspace-binding-isolation": True}, + head_on_master={512: True}, + delete_capability_allowed=True, + target_ref="prgs/master", + ) + + local = report["entries"][0]["local_worktree"] + self.assertFalse(local["safe_to_remove_worktree"]) + self.assertEqual(local["match_type"], "ambiguous_alias") + self.assertIn("ambiguous local worktree aliases", local["block_reasons"][0]) + + @patch("merged_cleanup_reconcile.is_head_ancestor_of_ref", return_value=True) + @patch("merged_cleanup_reconcile.read_local_worktree_state") + @patch("subprocess.run") + def test_alias_head_on_target_branch_is_safe_cleanup_commit( + self, mock_run, mock_state, _ancestor + ): + mock_run.return_value = Mock( + returncode=0, + stdout=( + "worktree /repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation\n" + "HEAD deadbeef\n" + "branch refs/heads/feat/issue-510-workspace-binding-isolation\n\n" + ), + ) + mock_state.side_effect = [ + { + "exists": False, + "clean": None, + "current_branch": None, + "head_sha": None, + "dirty_files": [], + }, + { + "exists": True, + "clean": True, + "current_branch": "feat/issue-510-workspace-binding-isolation", + "head_sha": "deadbeef", + "dirty_files": [], + }, + ] + + state = mcr.resolve_cleanup_worktree_state( + project_root="/repo/Gitea-Tools", + head_branch="feat/issue-510-workspace-binding-isolation", + issue_number=510, + pr_head_sha="cafebabe", + target_ref="prgs/master", + ) + + self.assertTrue(state["exists"]) + self.assertEqual(state["match_type"], "branch") + self.assertEqual( + state["discovered_worktree_path"], + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + + @patch("os.path.isdir", return_value=True) + @patch("subprocess.run") + def test_remove_local_worktree_uses_assessed_path(self, mock_run, _isdir): + mock_run.return_value = Mock(returncode=0, stdout="", stderr="") + result = mcr.remove_local_worktree( + "/repo/Gitea-Tools", + "feat/issue-510-workspace-binding-isolation", + worktree_path="/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + 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"], + self.assertEqual( + result["worktree_path"], + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ) + mock_run.assert_called_once_with( + [ + "git", + "-C", + "/repo/Gitea-Tools", + "worktree", + "remove", + "/repo/Gitea-Tools/branches/issue-510-workspace-binding-isolation", + ], capture_output=True, text=True, check=False, @@ -239,4 +409,4 @@ class TestMergedCleanupReport(unittest.TestCase): if __name__ == "__main__": - unittest.main() \ No newline at end of file + unittest.main()