From b9e5cd0f573a0ced3a1a84ee2ce8e3a255a20846 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 13:03:23 -0400 Subject: [PATCH 1/2] feat: enforce reconciler PR-close proof fields in final reports (Closes #306) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #306. The dedicated reconciler close path (profile, gated close tool, ancestor proof, tests) already shipped across #301/#304/#309/#310, but the final-report validator did not require a reconciler close to prove itself. A handoff could report `PRs closed: #N` while omitting the close capability, ancestor proof, or linked-issue result and still grade A. This adds the missing report-time enforcement. Changes: - final_report_validator.py — new `reconcile.close_proof_fields` rule (`_rule_reconcile_close_proof`). Fires only when a PR close is reported (via the `PRs closed` field or a `reconciler_close_lock`), then blocks unless the handoff carries close capability proof (gitea.pr.close), ancestor proof, PR close result, and linked-issue result. Comment-only and blocked reconciliations are unaffected. New optional `reconciler_close_lock` kwarg on `assess_final_report_validator`. - tests/test_final_report_validator.py — TestReconcilerCloseProof (6 cases): full-proof close passes; missing capability / ancestor / linked-issue result each block; session close lock requires proof; comment-only needs no close proof. - reconcile-landed-pr.md — §18A documents the enforced gate. Validation: venv/bin/python -m pytest tests/test_final_report_validator.py \ tests/test_reconciler_close_gate.py tests/test_already_landed_reconcile.py \ tests/test_reconcile_already_landed_pr_tool.py \ tests/test_reconciliation_workflow.py tests/test_review_proofs.py -q 289 passed. Worktree: branches/issue-306-reconciler-close-proof Co-Authored-By: Claude Opus 4.8 (1M context) --- final_report_validator.py | 66 ++++++++++++++++ .../workflows/reconcile-landed-pr.md | 18 +++++ tests/test_final_report_validator.py | 75 +++++++++++++++++++ 3 files changed, 159 insertions(+) diff --git a/final_report_validator.py b/final_report_validator.py index 3fbac0f..99a1d18 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -816,6 +816,69 @@ def _rule_reconcile_linked_issue_live( return [] +_PR_CLOSE_NEGATIVE = frozenset({"", "none", "n/a", "na", "0", "no", "not closed", "not performed"}) +_ANCESTOR_AFFIRMATIVE_RE = re.compile( + r"ancestor|passed|true|verified|confirmed", re.IGNORECASE +) + + +def _reconciler_pr_close_performed(fields: dict[str, str], lock: dict) -> bool: + """True when the report/session indicates a reconciler PR close happened.""" + if lock.get("pr_closed") is True: + return True + value = (fields.get("prs closed", "") or "").strip().lower() + if value in _PR_CLOSE_NEGATIVE: + return False + # A closed PR is reported by number (e.g. "#99") or an affirmative result. + return bool(re.search(r"#\s*\d+|\b(?:closed|success|done)\b", value)) + + +def _rule_reconcile_close_proof( + report_text: str, + *, + reconciler_close_lock: dict | None = None, +) -> list[dict[str, str]]: + """#306: a reconciler PR close must carry exact proof fields. + + Read-only/comment-only reconciliations are untouched. Once a PR close is + reported (via the ``PRs closed`` field or a session close lock), the + handoff must prove the close capability, ancestor landing, PR close + result, and the linked-issue result — narrative alone fails closed. + """ + fields = _handoff_fields(report_text) + lock = reconciler_close_lock or {} + if not _reconciler_pr_close_performed(fields, lock): + return [] + + missing: list[str] = [] + capabilities = fields.get("capabilities proven", "") + if "gitea.pr.close" not in capabilities.lower(): + missing.append("close capability proof (gitea.pr.close)") + ancestor = fields.get("ancestor proof", "") + if not _ANCESTOR_AFFIRMATIVE_RE.search(ancestor): + missing.append("ancestor proof") + prs_closed = (fields.get("prs closed", "") or "").strip().lower() + if prs_closed in _PR_CLOSE_NEGATIVE and lock.get("pr_closed") is True: + missing.append("PR close result") + linked = fields.get("linked issue live status", "") or fields.get("issues closed", "") + if not linked.strip(): + missing.append("linked issue result") + + if not missing: + return [] + return [ + validator_finding( + "reconcile.close_proof_fields", + "block", + "Reconciler close proof", + "reconciler PR close reported without required proof field(s): " + + ", ".join(missing), + "include close capability proof, ancestor proof, PR close result, " + "and linked issue result in the handoff", + ) + ] + + def _rule_reconcile_pagination_proof(report_text: str) -> list[dict[str, str]]: text = report_text or "" if not _INVENTORY_COMPLETE_RE.search(text): @@ -916,6 +979,7 @@ _RULES_BY_TASK: dict[str, list[Callable[..., list[dict[str, str]]]]] = { _rule_reconcile_stale_author_fields, _rule_reconcile_eligible_reviewed, _rule_reconcile_linked_issue_live, + _rule_reconcile_close_proof, _rule_reconcile_pagination_proof, _rule_reviewer_git_fetch_readonly, _rule_reviewer_legacy_workspace_mutations, @@ -1006,6 +1070,7 @@ def assess_final_report_validator( issue_filing_lock: dict | None = None, session_pr_opened: bool = False, validation_session: dict | None = None, + reconciler_close_lock: dict | None = None, ) -> dict[str, Any]: """Validate final-report text against task-specific proof rules (#327). @@ -1062,6 +1127,7 @@ def assess_final_report_validator( "local_edits": local_edits, "session_pr_opened": session_pr_opened, "validation_session": validation_session, + "reconciler_close_lock": reconciler_close_lock, } for rule in _RULES_BY_TASK.get(normalized_kind, ()): diff --git a/skills/llm-project-workflow/workflows/reconcile-landed-pr.md b/skills/llm-project-workflow/workflows/reconcile-landed-pr.md index 10ba60d..53b20b3 100644 --- a/skills/llm-project-workflow/workflows/reconcile-landed-pr.md +++ b/skills/llm-project-workflow/workflows/reconcile-landed-pr.md @@ -341,6 +341,24 @@ Include: * confirmation that no normal review, approval, request-changes, or merge was performed +## 18A. Reconciler close proof is enforced (#306) + +When a reconciler run closes a PR, the final-report validator +(`final_report_validator` rule `reconcile.close_proof_fields`) **blocks** the +handoff unless it carries all four close proofs. The prompt is guidance; the +MCP validator is the authority. + +A close is detected from `PRs closed: #` (or a session close lock). Once a +close is reported, the handoff must include: + +* `Capabilities proven:` naming `gitea.pr.close` — the exact close capability +* `Ancestor proof:` — the landed/ancestor proof for the closed PR +* `PRs closed:` — the PR close result (the closed PR number) +* `Linked issue live status:` (or `Issues closed:`) — the linked-issue result + +Comment-only and blocked reconciliations (no PR close) are unaffected: the rule +returns no finding when nothing was closed. + ## 19. Local artifact and report consistency rule Do not create local walkthrough, notes, markdown, JSON, or report artifacts diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index 672d82e..b747873 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -494,6 +494,81 @@ class TestCanonicalReconcileSchema(unittest.TestCase): ) +class TestReconcilerCloseProof(unittest.TestCase): + """Issue #306: a reconciler PR close must carry its proof fields.""" + + def _closed_report(self, **kwargs): + # A report that claims PR #99 was closed via the reconciler path. + report = ( + _reconcile_handoff(**kwargs) + .replace( + "- Capabilities proven: gitea.read, gitea.pr.comment", + "- Capabilities proven: gitea.read, gitea.pr.comment, gitea.pr.close", + ) + .replace("- Missing capabilities: gitea.pr.close", "- Missing capabilities: none") + .replace("- PRs closed: none", "- PRs closed: #99") + .replace( + "- Blocker: missing gitea.pr.close capability", "- Blocker: none" + ) + ) + return report + + def test_full_proof_close_passes(self): + result = assess_final_report_validator( + self._closed_report(), "reconcile_already_landed" + ) + self.assertEqual(result["grade"], "A") + self.assertFalse( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_without_capability_proof_blocks(self): + # Claims PRs closed: #99 but never proves gitea.pr.close capability. + report = _reconcile_handoff().replace("- PRs closed: none", "- PRs closed: #99") + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_without_ancestor_proof_blocks(self): + report = self._closed_report(drop=("Ancestor proof",)) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_without_linked_issue_result_blocks(self): + report = self._closed_report(drop=("Linked issue live status", "Issues closed")) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_close_lock_requires_proof_even_if_text_says_none(self): + # Session lock proves a PR was closed; the report omits close proof. + result = assess_final_report_validator( + _reconcile_handoff(), + "reconcile_already_landed", + reconciler_close_lock={"pr_closed": True}, + ) + self.assertTrue(result["blocked"]) + self.assertTrue( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + def test_comment_only_reconcile_needs_no_close_proof(self): + result = assess_final_report_validator( + _reconcile_handoff(), "reconcile_already_landed" + ) + self.assertEqual(result["grade"], "A") + self.assertFalse( + any(f["rule_id"] == "reconcile.close_proof_fields" for f in result["findings"]) + ) + + class TestEntryPoint(unittest.TestCase): def test_unknown_task_kind_blocks(self): result = assess_final_report_validator("report", "unknown_mode") From 565363d5f45a4c43362b064bd75c93122c7efb82 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 22:36:10 -0400 Subject: [PATCH 2/2] docs: define bootstrap review path for self-hosted MCP fixes (Closes #557) Document the narrow controller-authorized path for landing MCP workflow fixes when live daemons cannot canonically review themselves, with required audits, allowed/forbidden actions, and post-land restart steps. --- docs/bootstrap-review-path.md | 190 +++++++++++++++++++++++ docs/llm-workflow-runbooks.md | 22 +++ skills/llm-project-workflow/SKILL.md | 15 +- tests/test_bootstrap_review_path_docs.py | 81 ++++++++++ 4 files changed, 307 insertions(+), 1 deletion(-) create mode 100644 docs/bootstrap-review-path.md create mode 100644 tests/test_bootstrap_review_path_docs.py diff --git a/docs/bootstrap-review-path.md b/docs/bootstrap-review-path.md new file mode 100644 index 0000000..33293d2 --- /dev/null +++ b/docs/bootstrap-review-path.md @@ -0,0 +1,190 @@ +# Bootstrap Review Path for Self-Hosted MCP Workflow Fixes (#557) + +## Purpose + +Gitea-Tools MCP workflow fixes can create a **bootstrap deadlock**: the live +MCP daemons still run the broken code from `master`, so canonical reviewer / +merger tools cannot complete the review that would land the fix. + +This document defines a **narrow, controller-authorized bootstrap path** so +such fixes can land without weakening normal review/merge gates. + +It is **not** a general bypass. Normal PRs must still use the full +reviewer → merger MCP workflow. + +## When this path applies + +All of the following must be true: + +1. The PR changes **Gitea-Tools MCP workflow / preflight / lease / gate** code + that the live daemon must load to review itself (self-hosted control plane). +2. Canonical review or merge tools **fail closed** because of that defect + (documented tool errors, not “inconvenience”). +3. A **controller** records an explicit bootstrap authorization on the PR or + linked issue (see below). +4. The PR source is clean, testable, and free of unrelated scope. + +Example (historical): PR #553 / Issue #546 (reviewer preflight capability/lease +deadlock). Live daemons on unpatched `master` could not complete canonical +review of the fix PR. + +## Durable authorization (required) + +**No manual remote merge, force-merge, or API merge of a bootstrap PR is +allowed** unless a controller has posted a durable authorization record on +Gitea. + +### Authorization record (issue or PR comment) + +The controller comment **must** include a machine-readable marker and the +fields below: + +```text +## BOOTSTRAP REVIEW AUTHORIZATION (#557) + +Status: APPROVED +PR: +Issue: +Controller: +Reason: live MCP runtime cannot canonically review this self-hosted workflow fix +Scope: narrow bootstrap only — does not weaken normal PR gates + +Validation evidence: +- git diff --check: clean +- targeted pytest: → pass +- full suite attribution (if non-zero): baseline compared to prgs/master + +Duplicate-PR audit: +Mutation ledger audit: +Contamination audit: + +Allowed verification actions used: +Forbidden actions NOT used: root checkout edits; raw curl mutation; direct +module import of gitea_mcp_server for mutations; in-memory gate restoration + +Post-land plan: +- restart MCP daemons for author/reviewer/merger/reconciler profiles +- re-verify with canonical tools (see Post-bootstrap steps) +``` + +Without this record, bootstrap merge is **forbidden**. Agents must stop with +`BLOCKED + DIAGNOSE` rather than improvise a merge. + +## Review gates and proof (still required) + +Bootstrap does **not** skip technical review. It only allows verification and +landing when the **live MCP mutation path** cannot complete the review. + +Before authorization, the controller (or a delegated read-only verifier in an +isolated worktree) must have: + +| Gate | Requirement | +|------|-------------| +| Diff hygiene | `git diff --check` clean on the PR tip vs `prgs/master` | +| Tests | Targeted tests for the fix pass; full suite either green or failures attributed to baseline `prgs/master` | +| Duplicate PR | Open-PR inventory shows no competing open PR for the same issue (or supersession is documented) | +| Mutation ledger | Any claimed mutations are ledger-consistent; no silent root edits | +| Contamination audit | PR/issue comments and leases classified as clean vs workflow-contaminated | +| Scope | Diff limited to the bootstrap issue; no drive-by refactors | + +### Contamination audit (comments / leases) + +Comments or leases created via **raw curl**, **direct Python import of MCP +modules**, or **token extraction** are **workflow-contaminated**. They must be +listed and must **not** be treated as canonical review proof. + +Only comments posted via **sanctioned MCP reviewer tools** (after the fix is +landed and daemons restarted, or during a clean path that did not bypass gates) +count as canonical review state. + +Historical example on PR #553: + +| Comment | Disposition | +|---------|-------------| +| #7247 test / raw API | contaminated | +| #7251 lease metadata / raw API | contaminated | +| #7252 lease metadata / direct import | contaminated | +| #7265 lease metadata / MCP reviewer tools | workflow-clean | + +## Allowed verification actions + +These may run **outside** the broken live mutation path to establish facts: + +- Read-only MCP tools (`gitea_view_pr`, `gitea_list_prs`, `gitea_whoami`, + `gitea_get_profile`, inventory, eligibility **reads**). +- Isolated `branches/` worktree checkout of the PR head (never root). +- `git fetch`, `git log`, `git diff`, `git merge-tree` (read-only analysis). +- Targeted `pytest` / `py_compile` inside that worktree. +- Controller posting of the bootstrap authorization comment via a healthy + MCP profile **if available**; if comment mutation is also deadlocked, a + **human operator** posts the authorization in the Gitea UI (still durable on + the thread). + +## Forbidden actions (always) + +Even under bootstrap: + +- Editing or committing in the **project root / control checkout**. +- Treating root as a worktree for implementation or review mutations. +- Raw `curl` / REST mutation with extracted tokens as a normal workflow. +- `import gitea_mcp_server` (or sibling modules) from a shell to call mutation + helpers and bypass preflight. +- In-memory restoration of capability / lease / decision state to “finish” + a blocked chain. +- Force-push, history rewrite, or deleting evidence comments. +- Weakening normal gates in code “temporarily” without a tracked issue/PR. +- Self-review or self-merge by the PR author identity. + +## Landing procedure (after APPROVED authorization) + +1. Confirm the authorization record is present and complete on the PR or issue. +2. Merge **only** with an independent merger identity when possible. + - Preferred: restart/replace the MCP merger daemon with a build that includes + the fix (or a one-shot process started from the PR worktree binary) so + `gitea_merge_pr` can run under normal gates. + - If the live merger daemon still cannot load the fix, a **human operator** + may merge via the Gitea UI **only** after the authorization record exists. +3. Never merge from contaminated proof alone. + +## Post-bootstrap steps + +### 1. Restart MCP daemons + +After the fix lands on `prgs/master`: + +1. Stop author / reviewer / merger / reconciler MCP server processes for this + repo (IDE MCP pool + any long-lived terminals). +2. Confirm no stale PID still serves the old code. +3. Restart each profile so it loads the updated `master` tree (or installed + package path). +4. Call `gitea_whoami` / `gitea_get_profile` on each namespace and record + profile name + identity. + +### 2. Re-verify with canonical tools + +Example pattern after PR #553-class fixes (lease release / #550-style follow-up): + +1. From a **reviewer** MCP session: acquire/release or inspect the relevant + lease with canonical tools only. +2. Confirm no direct-import or raw-API path is required. +3. Post a short controller or reconciler note: bootstrap complete; normal gates + restored. + +If verification still requires a bypass, open a new issue — do **not** extend +this bootstrap authorization silently. + +## Explicit non-goals + +- This path does **not** authorize skipping tests, duplicate audits, or + contamination audits. +- This path does **not** authorize root checkout implementation work. +- This path does **not** replace BLOCKED + DIAGNOSE when a non-bootstrap + failure occurs (#552). +- This path does **not** grant permanent “operator exception” culture. + +## Related + +- Root checkout policy: `docs/llm-workflow-runbooks.md` (Global LLM Worktree Rule, #475) +- BLOCKED + DIAGNOSE: issue #552 / skill workflows +- Reviewer lease / preflight deadlock class: issues #546, #548, #550 +- Durable session proofs across daemon pools: issue #559 diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index d0e472d..1e0366b 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -408,6 +408,28 @@ The guard blocks when the **control checkout** (repository root) is: `branches/...` directories are disposable role worktrees; the root checkout is the stable orchestration surface only. +## Bootstrap Review Path for self-hosted MCP fixes (#557) + +When a PR fixes Gitea-Tools MCP workflow code that the **live daemon** still +runs from broken `master`, canonical review can deadlock on itself. + +Do **not** improvise raw API, direct imports, root edits, or gate bypasses. + +Use the narrow controller-authorized path documented in: + +- [`docs/bootstrap-review-path.md`](bootstrap-review-path.md) + +Hard rules: + +- No merge without a durable `BOOTSTRAP REVIEW AUTHORIZATION (#557)` record on + the PR or linked issue. +- Validation, duplicate-PR, mutation-ledger, and contamination audits remain + mandatory. +- Root checkout stays read-only orchestration only; verification runs in + `branches/` worktrees. +- After land: restart MCP daemons and re-verify with canonical tools only. +- This never weakens normal reviewer/merger gates for ordinary PRs. + ## Shell Spawn Hard-Stop Rule Symptom: a shell tool call returns `exit_code: -1` with empty stdout/stderr. diff --git a/skills/llm-project-workflow/SKILL.md b/skills/llm-project-workflow/SKILL.md index dc2f961..73b9f8d 100644 --- a/skills/llm-project-workflow/SKILL.md +++ b/skills/llm-project-workflow/SKILL.md @@ -182,4 +182,17 @@ Ready-to-copy task prompts live in [`templates/`](templates/): Releases follow SemVer from remote `master` only, after full test suite passes. See [`templates/release-tag.md`](templates/release-tag.md) and -`scripts/release-tag`. \ No newline at end of file +`scripts/release-tag`. + +## Bootstrap Review Path (#557) + +Self-hosted MCP workflow fixes can deadlock live review daemons. Do not bypass +gates with raw API, direct imports, or root checkout edits. + +If and only if a controller posts a durable `BOOTSTRAP REVIEW AUTHORIZATION +(#557)` record, follow: + +`docs/bootstrap-review-path.md` + +Otherwise stop with BLOCKED + DIAGNOSE. Bootstrap never weakens normal PR gates. + diff --git a/tests/test_bootstrap_review_path_docs.py b/tests/test_bootstrap_review_path_docs.py new file mode 100644 index 0000000..71c5c14 --- /dev/null +++ b/tests/test_bootstrap_review_path_docs.py @@ -0,0 +1,81 @@ +"""Doc-contract checks for the bootstrap review path (#557). + +Self-hosted MCP workflow fixes can deadlock live review daemons. These tests +pin the narrow controller-authorized bootstrap path so it cannot silently +regress into a general gate bypass. +""" + +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent + +DOC = REPO_ROOT / "docs" / "bootstrap-review-path.md" +RUNBOOK = REPO_ROOT / "docs" / "llm-workflow-runbooks.md" +SKILL = REPO_ROOT / "skills" / "llm-project-workflow" / "SKILL.md" + + +def _normalize(text: str) -> str: + return " ".join(text.split()) + + +def test_bootstrap_doc_exists_and_names_authorization_marker(): + text = DOC.read_text(encoding="utf-8") + assert "BOOTSTRAP REVIEW AUTHORIZATION (#557)" in text + assert "bootstrap deadlock" in text.lower() or "Bootstrap deadlock" in text + + +def test_bootstrap_doc_requires_validation_and_audits(): + text = _normalize(DOC.read_text(encoding="utf-8")) + for phrase in ( + "git diff --check", + "Duplicate-PR audit", + "Mutation ledger audit", + "Contamination audit", + "workflow-contaminated", + ): + assert phrase in text, f"bootstrap doc missing: {phrase!r}" + + +def test_bootstrap_doc_lists_allowed_and_forbidden_actions(): + text = _normalize(DOC.read_text(encoding="utf-8")) + lower = text.lower() + for phrase in ( + "Allowed verification actions", + "Forbidden actions", + "project root", + "direct Python import", + "branches/", + ): + assert phrase in text, f"bootstrap doc missing: {phrase!r}" + assert "curl" in lower + assert "raw" in lower + + +def test_bootstrap_doc_forbids_general_gate_weakening(): + text = _normalize(DOC.read_text(encoding="utf-8")).lower() + assert "never weakens normal" in text or "does not weaken normal" in text + assert "general bypass" in text or "general gate" in text + + +def test_bootstrap_doc_post_land_restart_and_verify(): + text = _normalize(DOC.read_text(encoding="utf-8")) + for phrase in ( + "Restart MCP daemons", + "canonical tools", + "gitea_whoami", + ): + assert phrase in text, f"post-bootstrap guidance missing: {phrase!r}" + + +def test_runbook_links_bootstrap_path(): + text = _normalize(RUNBOOK.read_text(encoding="utf-8")) + assert "Bootstrap Review Path for self-hosted MCP fixes (#557)" in text + assert "bootstrap-review-path.md" in text + assert "BOOTSTRAP REVIEW AUTHORIZATION (#557)" in text + + +def test_skill_links_bootstrap_path(): + text = _normalize(SKILL.read_text(encoding="utf-8")) + assert "Bootstrap Review Path (#557)" in text + assert "bootstrap-review-path.md" in text + assert "BLOCKED + DIAGNOSE" in text or "BLOCKED" in text