From 0cc3ed8868442fc7b3298676f76c18224b9f9dcd Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 05:35:44 -0400 Subject: [PATCH 1/5] feat: enforce author work leases (Closes #267) --- docs/llm-workflow-runbooks.md | 15 +++- docs/wiki/Workflow.md | 6 +- gitea_mcp_server.py | 158 ++++++++++++++++++++++++++++++++++ tests/test_mcp_server.py | 53 ++++++++++++ 4 files changed, 230 insertions(+), 2 deletions(-) diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index a5a51b2..74ca190 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -254,7 +254,20 @@ Never create a parallel branch or PR for the same issue unless the old branch is proven abandoned and the takeover is recorded. Gitea-Tools lease gates: `gitea_lock_issue` (fail-closed before author -mutations), `status:in-progress`, and claim comments. Full portable wording: +mutations), `status:in-progress`, and claim comments. `gitea_lock_issue` +records an `author_issue_work` lease in the issue-lock payload with issue +number, optional PR number, branch, worktree path, claimant identity/profile, +created timestamp, expiry timestamp, and last heartbeat timestamp. An active +same-issue/same-operation lease blocks duplicate work. An expired lease still +blocks takeover until a recovery review records why the prior work is abandoned, +completed, or unsafe to continue. + +Remote branches matching the issue number are also treated as active work unless +the recovery review proves the branch is abandoned or superseded. Never delete +or clean up a branch when it has an active lease, dirty worktree, open PR, or is +the only copy of unmerged work. + +Full portable wording: [`skills/llm-project-workflow/SKILL.md`](../skills/llm-project-workflow/SKILL.md). ## Global LLM Worktree Rule diff --git a/docs/wiki/Workflow.md b/docs/wiki/Workflow.md index 2514dda..2e05526 100644 --- a/docs/wiki/Workflow.md +++ b/docs/wiki/Workflow.md @@ -17,6 +17,10 @@ 0. Work Selection Rule — verify a work lease before any mutations (open PRs, issue-linked PRs, branches, worktrees, dirty worktrees, active leases/ handoffs, merged-PR completion). Stop if another session owns the lease. + `gitea_lock_issue` records an operation-scoped `author_issue_work` lease + with issue, branch, worktree, claimant, created, expiry, and heartbeat + fields; active or expired same-operation leases require recovery review + before takeover. 0b. Global LLM Worktree Rule — main checkout on `master`/`main`/`dev` only; mutate only from a `branches/` worktree after proving root, cwd, branch, stable main-checkout branch, and session worktree path (no exceptions). @@ -31,4 +35,4 @@ Wiki publication, credential provisioning, and cross-repo mirror operations are operator-confirmed actions — see [Runbooks](Runbooks.md). -Full Gitea-specific runbooks: `docs/llm-workflow-runbooks.md` in the repository. \ No newline at end of file +Full Gitea-specific runbooks: `docs/llm-workflow-runbooks.md` in the repository. diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index b5415eb..5652e24 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -21,6 +21,7 @@ import json import functools import contextlib import subprocess +from datetime import datetime, timedelta, timezone # Mutation-authority record (#199, refs #194). Deliberately in-process, NOT a @@ -482,6 +483,132 @@ import merged_cleanup_reconcile # noqa: E402 # Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, # consumed by gitea_create_pr and scripts/worktree-start. ISSUE_LOCK_FILE = "/tmp/gitea_issue_lock.json" +WORK_LEASE_TTL_HOURS = 4 +AUTHOR_ISSUE_WORK_LEASE = "author_issue_work" +VALID_WORK_LEASE_OPERATIONS = frozenset({ + AUTHOR_ISSUE_WORK_LEASE, + "review_pr_work", + "fix_pr_changes", + "cleanup_branch_work", + "issue_filing_work", + "recovery_work", +}) + + +def _work_lease_now() -> datetime: + return datetime.now(timezone.utc) + + +def _work_lease_timestamp(value: datetime) -> str: + return value.astimezone(timezone.utc).replace(microsecond=0).isoformat().replace("+00:00", "Z") + + +def _parse_work_lease_timestamp(value: str | None) -> datetime | None: + text = (value or "").strip() + if not text: + return None + try: + return datetime.fromisoformat(text.replace("Z", "+00:00")).astimezone(timezone.utc) + except ValueError: + return None + + +def _load_existing_issue_lock() -> dict | None: + if not os.path.exists(ISSUE_LOCK_FILE): + return None + try: + with open(ISSUE_LOCK_FILE, encoding="utf-8") as f: + data = json.load(f) + return data if isinstance(data, dict) else None + except Exception: + return None + + +def _work_lease_claimant(host: str | None) -> dict: + profile = get_profile() + username = _IDENTITY_CACHE.get(host) if host else None + if username is None and host and not _preflight_in_test_mode(): + username = _authenticated_username(host) + return { + "username": username, + "profile": profile.get("profile_name"), + } + + +def _build_author_issue_work_lease( + *, + issue_number: int, + branch_name: str, + worktree_path: str, + host: str | None, +) -> dict: + created = _work_lease_now() + expires = created + timedelta(hours=WORK_LEASE_TTL_HOURS) + return { + "operation_type": AUTHOR_ISSUE_WORK_LEASE, + "issue_number": issue_number, + "pr_number": None, + "branch": branch_name, + "worktree_path": worktree_path, + "claimant": _work_lease_claimant(host), + "created_at": _work_lease_timestamp(created), + "expires_at": _work_lease_timestamp(expires), + "last_heartbeat_at": _work_lease_timestamp(created), + } + + +def _active_work_lease_block( + existing_lock: dict | None, + *, + issue_number: int, + branch_name: str, + worktree_path: str, + operation_type: str, +) -> str | None: + if not existing_lock: + return None + existing_issue = existing_lock.get("issue_number") + existing_branch = existing_lock.get("branch_name") + existing_worktree = existing_lock.get("worktree_path") + lease = existing_lock.get("work_lease") + existing_operation = ( + lease.get("operation_type") + if isinstance(lease, dict) + else AUTHOR_ISSUE_WORK_LEASE + ) + if existing_issue != issue_number or existing_operation != operation_type: + return None + + same_owner = ( + existing_branch == branch_name + and os.path.realpath(str(existing_worktree or "")) == os.path.realpath(worktree_path) + ) + expires_at = ( + _parse_work_lease_timestamp(lease.get("expires_at")) + if isinstance(lease, dict) + else None + ) + if expires_at and expires_at <= _work_lease_now(): + return ( + f"Issue #{issue_number} has an expired {operation_type} lease on " + f"branch '{existing_branch}' from worktree '{existing_worktree}'. " + "Recovery review is required before takeover (fail closed)" + ) + if same_owner: + return None + return ( + f"Issue #{issue_number} already has an active {operation_type} lease on " + f"branch '{existing_branch}' from worktree '{existing_worktree}' " + "(fail closed)" + ) + + +def _branch_entry_name(branch: dict | str) -> str: + if isinstance(branch, str): + return branch + if not isinstance(branch, dict): + return "" + return str(branch.get("name") or branch.get("ref") or "") def _reveal_endpoints() -> bool: @@ -929,6 +1056,16 @@ def gitea_lock_issue( resolved_worktree = issue_lock_worktree.resolve_author_worktree_path( worktree_path, PROJECT_ROOT ) + active_lease_block = _active_work_lease_block( + _load_existing_issue_lock(), + issue_number=issue_number, + branch_name=branch_name, + worktree_path=resolved_worktree, + operation_type=AUTHOR_ISSUE_WORK_LEASE, + ) + if active_lease_block: + raise RuntimeError(active_lease_block) + git_state = issue_lock_worktree.read_worktree_git_state(resolved_worktree) verify_preflight_purity(remote, worktree_path=resolved_worktree) lock_assessment = issue_lock_worktree.assess_issue_lock_worktree( @@ -974,6 +1111,25 @@ def gitea_lock_issue( f"Issue #{issue_number} is already tied to an open PR (PR #{pr.get('number')}) via Closes/Fixes reference (fail closed)" ) + branch_url = f"{repo_api_url(h, o, r)}/branches" + try: + branches = api_get_all(branch_url, auth) + except Exception as e: + raise RuntimeError(f"Could not list branches to verify issue lock: {e}") + for branch in branches: + name = _branch_entry_name(branch) + if expected_pattern in name: + raise ValueError( + f"Issue #{issue_number} already has matching branch '{name}' " + "(fail closed)" + ) + + work_lease = _build_author_issue_work_lease( + issue_number=issue_number, + branch_name=branch_name, + worktree_path=resolved_worktree, + host=h, + ) data = { "issue_number": issue_number, "branch_name": branch_name, @@ -981,6 +1137,7 @@ def gitea_lock_issue( "org": o, "repo": r, "worktree_path": resolved_worktree, + "work_lease": work_lease, } try: @@ -1001,6 +1158,7 @@ def gitea_lock_issue( "issue_number": issue_number, "branch_name": branch_name, "worktree_path": resolved_worktree, + "work_lease": work_lease, } if agent_artifacts: result["warnings"] = [ diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 394ab03..8e1aeac 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3048,10 +3048,18 @@ class TestIssueLocking(unittest.TestCase): mock_api.return_value = [] # no open PRs res = gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") self.assertTrue(res["success"]) + self.assertEqual(res["work_lease"]["operation_type"], "author_issue_work") + self.assertEqual(res["work_lease"]["issue_number"], 196) + self.assertEqual(res["work_lease"]["branch"], "feat/issue-196-mutations") + self.assertIn("created_at", res["work_lease"]) + self.assertIn("expires_at", res["work_lease"]) + self.assertIn("last_heartbeat_at", res["work_lease"]) + self.assertEqual(res["work_lease"]["claimant"]["profile"], "gitea-default") self.assertTrue(os.path.exists(ISSUE_LOCK_FILE)) with open(ISSUE_LOCK_FILE, encoding="utf-8") as f: lock = json.load(f) self.assertIn("worktree_path", lock) + self.assertIn("work_lease", lock) def test_lock_issue_mismatch_branch_fails(self): with self.assertRaises(ValueError) as ctx: @@ -3092,6 +3100,51 @@ class TestIssueLocking(unittest.TestCase): gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") self.assertIn("already tied to an open PR", str(ctx.exception)) + @patch( + "mcp_server.issue_lock_worktree.read_worktree_git_state", + return_value=_clean_master_git_state_for_lock(), + ) + @patch("mcp_server.api_get_all") + @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) + def test_lock_issue_reused_by_remote_branch(self, _auth, mock_api, _git_state): + mock_api.side_effect = [ + [], + [{"name": "feat/issue-196-existing-work"}], + ] + with self.assertRaises(ValueError) as ctx: + gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") + self.assertIn("already has matching branch", str(ctx.exception)) + + def test_lock_issue_blocks_active_same_operation_lease(self): + with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: + json.dump({ + "issue_number": 196, + "branch_name": "feat/issue-196-other-work", + "worktree_path": "/tmp/other-worktree", + "work_lease": { + "operation_type": "author_issue_work", + "expires_at": "2999-01-01T00:00:00Z", + }, + }, f) + with self.assertRaises(RuntimeError) as ctx: + gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") + self.assertIn("already has an active author_issue_work lease", str(ctx.exception)) + + def test_lock_issue_blocks_expired_same_operation_lease_for_recovery(self): + with open(ISSUE_LOCK_FILE, "w", encoding="utf-8") as f: + json.dump({ + "issue_number": 196, + "branch_name": "feat/issue-196-other-work", + "worktree_path": "/tmp/other-worktree", + "work_lease": { + "operation_type": "author_issue_work", + "expires_at": "2000-01-01T00:00:00Z", + }, + }, f) + with self.assertRaises(RuntimeError) as ctx: + gitea_lock_issue(issue_number=196, branch_name="feat/issue-196-mutations", remote="prgs") + self.assertIn("Recovery review is required before takeover", str(ctx.exception)) + @patch("mcp_server.api_get_all", return_value=[]) @patch("mcp_server.get_auth_header", return_value=FAKE_AUTH) def test_lock_from_clean_scratch_worktree(self, _auth, _api): From f89019f804e1a1d5b944553b5f4625226358830e Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:23:50 -0400 Subject: [PATCH 2/5] feat: classify already-landed PRs as reconciliation-only (Closes #295) Add reviewer_already_landed_classification verifier to block oldest/next eligible PR wording, review-eligible claims, and pinned reviewed head usage when a PR is already landed. Wire the check into build_final_report and tighten final_report_validator regex coverage. Co-Authored-By: Claude Opus 4.8 (1M context) --- final_report_validator.py | 3 +- review_proofs.py | 31 ++++ reviewer_already_landed_classification.py | 143 ++++++++++++++++++ tests/test_final_report_validator.py | 14 ++ tests/test_llm_workflow_split.py | 6 + ..._reviewer_already_landed_classification.py | 81 ++++++++++ 6 files changed, 277 insertions(+), 1 deletion(-) create mode 100644 reviewer_already_landed_classification.py create mode 100644 tests/test_reviewer_already_landed_classification.py diff --git a/final_report_validator.py b/final_report_validator.py index bff9467..1c82ab3 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -88,7 +88,8 @@ _ALREADY_LANDED_RE = re.compile( re.IGNORECASE, ) _ELIGIBLE_REVIEW_RE = re.compile( - r"eligible for (?:review|merge)|ready for (?:review|merge)|next eligible pr", + r"eligible for (?:review|merge)|ready for (?:review|merge)|" + r"(?:next|oldest)\s+eligible\s+pr", re.IGNORECASE, ) _REVIEWED_LANDED_RE = re.compile( diff --git a/review_proofs.py b/review_proofs.py index 43dfbd3..b667665 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -1505,6 +1505,17 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "reasons": [], "violations": [], } + if report_text: + already_landed_classification = assess_already_landed_classification_report( + report_text + ) + else: + already_landed_classification = { + "proven": True, + "block": False, + "reasons": [], + "violations": [], + } contamination_status = contamination.get("status", "unknown") checkout_proven = bool(checkout_proof.get("proven")) @@ -1659,6 +1670,11 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "reviewer baseline validation proof missing or failed (#325)" ) downgrade_reasons.extend(baseline_validation.get("reasons", [])) + if not already_landed_classification.get("proven"): + downgrade_reasons.append( + "already-landed classification wording missing or invalid (#295)" + ) + downgrade_reasons.extend(already_landed_classification.get("reasons", [])) merge_allowed = ( identity_eligible @@ -1751,6 +1767,12 @@ def build_final_report(checkout_proof, inventory, validation, contamination, "baseline_validation_violations": list( baseline_validation.get("violations") or [] ), + "already_landed_classification_proven": bool( + already_landed_classification.get("proven") + ), + "already_landed_classification_violations": list( + already_landed_classification.get("reasons") or [] + ), } @@ -4460,6 +4482,15 @@ def assess_merge_simulation_report(report_text, **kwargs): return _assess(report_text, **kwargs) +def assess_already_landed_classification_report(report_text, **kwargs): + """#295: already-landed PRs are reconciliation-only, not review eligible.""" + from reviewer_already_landed_classification import ( + assess_already_landed_classification_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/reviewer_already_landed_classification.py b/reviewer_already_landed_classification.py new file mode 100644 index 0000000..b5c5f10 --- /dev/null +++ b/reviewer_already_landed_classification.py @@ -0,0 +1,143 @@ +"""Already-landed PR classification verifier for reviewer reports (#295).""" + +from __future__ import annotations + +import re +from typing import Any + +ALREADY_LANDED_ELIGIBILITY_CLASS = "ALREADY_LANDED_RECONCILE_REQUIRED" + +_LANDED_CONTEXT_RE = re.compile( + r"already[- ]landed|ancestor proof\s*:\s*passed|" + r"eligibility class\s*:\s*already_landed", + re.IGNORECASE, +) +_ELIGIBILITY_CLASS_LINE_RE = re.compile( + r"eligibility class\s*:\s*(.+)$", + re.IGNORECASE | re.MULTILINE, +) +_INELIGIBLE_SELECTION_RE = re.compile( + r"\b(?:next|oldest)\s+eligible\s+pr\b", + re.IGNORECASE, +) +_REVIEW_ELIGIBLE_RE = re.compile( + r"\beligible for (?:review|merge)\b|\bready for (?:review|merge)\b", + re.IGNORECASE, +) +_PINNED_REVIEWED_HEAD_RE = re.compile( + r"\bpinned reviewed head\s*:", + re.IGNORECASE, +) +_CANDIDATE_HEAD_RE = re.compile( + r"\bcandidate head sha\s*:", + re.IGNORECASE, +) +_REVIEWED_HEAD_NONE_RE = re.compile( + r"\breviewed head sha\s*:\s*none\b", + re.IGNORECASE, +) +_VALIDATION_PROOF_RE = re.compile( + r"(?:validation passed|pytest.*passed|diff review passed|" + r"validated on pinned head)", + re.IGNORECASE, +) +_QUEUE_BLOCKED_RE = re.compile( + r"queue blocked by already[- ]landed|" + r"reconciliation(?:-only)?\s+(?:next step|required)", + re.IGNORECASE, +) + + +def _landed_context(report_text: str, eligibility_class: str | None) -> bool: + if (eligibility_class or "").strip().upper() == ALREADY_LANDED_ELIGIBILITY_CLASS: + return True + return bool(_LANDED_CONTEXT_RE.search(report_text or "")) + + +def assess_already_landed_classification_report( + report_text: str, + *, + eligibility_class: str | None = None, + selected_pr_already_landed: bool | None = None, +) -> dict[str, Any]: + """#295: already-landed PRs are reconciliation-only, not review eligible.""" + text = report_text or "" + reasons: list[str] = [] + + landed = _landed_context(text, eligibility_class) + if selected_pr_already_landed is True: + landed = True + if not landed: + return { + "proven": True, + "block": False, + "landed_context": False, + "reasons": [], + "safe_next_action": "proceed", + } + + if _INELIGIBLE_SELECTION_RE.search(text): + reasons.append( + "already-landed PR must not be described as oldest/next eligible PR; " + "use 'Oldest open PR requiring action' and " + f"'Eligibility class: {ALREADY_LANDED_ELIGIBILITY_CLASS}'" + ) + + if _REVIEW_ELIGIBLE_RE.search(text): + reasons.append( + "already-landed PR must not be described as eligible for review or merge" + ) + + class_match = _ELIGIBILITY_CLASS_LINE_RE.search(text) + if class_match: + declared = class_match.group(1).strip().upper() + if declared != ALREADY_LANDED_ELIGIBILITY_CLASS: + reasons.append( + "already-landed PR eligibility class must be " + f"{ALREADY_LANDED_ELIGIBILITY_CLASS}" + ) + elif selected_pr_already_landed is True: + reasons.append( + f"already-landed selected PR must declare Eligibility class: " + f"{ALREADY_LANDED_ELIGIBILITY_CLASS}" + ) + + if _PINNED_REVIEWED_HEAD_RE.search(text): + if not _VALIDATION_PROOF_RE.search(text): + reasons.append( + "already-landed pre-review report must use Candidate head SHA, " + "not Pinned reviewed head, unless validation and diff review passed" + ) + + if selected_pr_already_landed is True and not _CANDIDATE_HEAD_RE.search(text): + if _PINNED_REVIEWED_HEAD_RE.search(text) or "head sha" in text.lower(): + reasons.append( + "already-landed ancestry check must report Candidate head SHA" + ) + + if selected_pr_already_landed is True and not _REVIEWED_HEAD_NONE_RE.search(text): + if _PINNED_REVIEWED_HEAD_RE.search(text) and not _VALIDATION_PROOF_RE.search(text): + reasons.append( + "already-landed report must state 'Reviewed head SHA: none' " + "when validation did not run" + ) + + if selected_pr_already_landed is True and not _QUEUE_BLOCKED_RE.search(text): + reasons.append( + "already-landed queue blocker must state reconciliation requirement " + "or queue blocked wording" + ) + + proven = not reasons + return { + "proven": proven, + "block": not proven, + "landed_context": True, + "reasons": reasons, + "safe_next_action": ( + "classify as ALREADY_LANDED_RECONCILE_REQUIRED, use Candidate head SHA, " + "and stop normal review" + if not proven + else "proceed" + ), + } \ No newline at end of file diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index f8e8848..0a0124d 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -190,6 +190,20 @@ class TestReviewPrRules(unittest.TestCase): ) ) + def test_already_landed_oldest_eligible_pr_blocks(self): + report = ( + _review_handoff() + + "\nAlready landed.\nOldest eligible PR: PR #278." + ) + result = assess_final_report_validator(report, "review_pr") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reviewer.already_landed_eligible_wording" + for f in result["findings"] + ) + ) + def test_git_fetch_must_not_be_readonly_only(self): report = ( _review_handoff() diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 0c2b89a..f27c5b5 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -158,3 +158,9 @@ def test_worktree_ownership_verifier_exported(): from review_proofs import assess_worktree_ownership_report assert callable(assess_worktree_ownership_report) + + +def test_already_landed_classification_verifier_exported(): + from review_proofs import assess_already_landed_classification_report + + assert callable(assess_already_landed_classification_report) diff --git a/tests/test_reviewer_already_landed_classification.py b/tests/test_reviewer_already_landed_classification.py new file mode 100644 index 0000000..7506eeb --- /dev/null +++ b/tests/test_reviewer_already_landed_classification.py @@ -0,0 +1,81 @@ +import unittest + +from reviewer_already_landed_classification import ( # noqa: E402 + ALREADY_LANDED_ELIGIBILITY_CLASS, + assess_already_landed_classification_report, +) + + +class TestAlreadyLandedClassification(unittest.TestCase): + def test_non_landed_report_passes(self): + result = assess_already_landed_classification_report( + "Selected the next eligible PR: PR #286." + ) + self.assertTrue(result["proven"]) + self.assertFalse(result["landed_context"]) + + def test_oldest_eligible_pr_on_landed_blocks(self): + result = assess_already_landed_classification_report( + "PR is already landed.\n" + "Oldest eligible PR: PR #278." + ) + self.assertFalse(result["proven"]) + self.assertTrue(result["block"]) + + def test_next_eligible_pr_on_landed_blocks(self): + result = assess_already_landed_classification_report( + "Ancestor proof: passed\n" + "Selected the next eligible PR: PR #278." + ) + self.assertFalse(result["proven"]) + + def test_eligible_for_review_on_landed_blocks(self): + result = assess_already_landed_classification_report( + "Already landed on master and eligible for review next." + ) + self.assertFalse(result["proven"]) + + def test_proper_landed_wording_passes(self): + result = assess_already_landed_classification_report( + "Oldest open PR requiring action: PR #278\n" + f"Eligibility class: {ALREADY_LANDED_ELIGIBILITY_CLASS}\n" + "Candidate head SHA: abc123\n" + "Reviewed head SHA: none\n" + "Queue blocked by already-landed PR requiring reconciliation", + selected_pr_already_landed=True, + ) + self.assertTrue(result["proven"]) + + def test_pinned_reviewed_head_without_validation_blocks(self): + result = assess_already_landed_classification_report( + "Already landed.\n" + "Pinned reviewed head: abc123def456", + selected_pr_already_landed=True, + ) + self.assertFalse(result["proven"]) + + def test_wrong_eligibility_class_blocks(self): + result = assess_already_landed_classification_report( + "Already landed.\n" + "Eligibility class: REVIEW_ELIGIBLE", + selected_pr_already_landed=True, + ) + self.assertFalse(result["proven"]) + + def test_missing_queue_blocker_blocks(self): + result = assess_already_landed_classification_report( + f"Eligibility class: {ALREADY_LANDED_ELIGIBILITY_CLASS}\n" + "Candidate head SHA: abc123\n" + "Reviewed head SHA: none", + selected_pr_already_landed=True, + ) + self.assertFalse(result["proven"]) + + def test_exported_from_review_proofs(self): + from review_proofs import assess_already_landed_classification_report as exported + + self.assertTrue(callable(exported)) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From 749e480baf26f9e80ca57ed3abcf35041e80eab2 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:25:55 -0400 Subject: [PATCH 3/5] feat: add reconciliation workflow MCP tools for already-landed PRs (Closes #301) Expose read-only assess/scan tools and capability planning so already-landed open PRs can be reconciled without review or merge. Register the gitea-reconcile-landed-pr skill and workflow-source verification. --- gitea_mcp_server.py | 195 +++++++++++++++++ reconciliation_workflow.py | 292 +++++++++++++++++++++++++ review_proofs.py | 5 + role_session_router.py | 4 + task_capability_map.py | 8 + tests/test_llm_workflow_split.py | 10 + tests/test_reconciliation_mcp_tools.py | 106 +++++++++ tests/test_reconciliation_workflow.py | 149 +++++++++++++ 8 files changed, 769 insertions(+) create mode 100644 reconciliation_workflow.py create mode 100644 tests/test_reconciliation_mcp_tools.py create mode 100644 tests/test_reconciliation_workflow.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index b5415eb..abd5ab7 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -477,6 +477,7 @@ import review_proofs # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import merged_cleanup_reconcile # noqa: E402 +import reconciliation_workflow # noqa: E402 # Fail-closed exact-issue-lock file (#204): written by gitea_lock_issue, @@ -3425,6 +3426,178 @@ def gitea_reconcile_merged_cleanups( return {"success": True, "performed": True, **report} +@mcp.tool() +def gitea_assess_already_landed_reconciliation( + pr_number: int, + target_branch: str = "master", + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Read-only: assess already-landed reconciliation for one open PR (#301). + + Fetches the PR live, refreshes the target branch, checks ancestor proof, + and returns a capability plan for the active profile. Does not review, + approve, request changes, merge, or mutate Gitea state. + + Args: + pr_number: Open PR to assess. + target_branch: Branch used for ancestry proof (default master). + remote: Known instance — 'dadeschools' or 'prgs'. + host: Override the Gitea host. + org: Override the owner/organization. + repo: Override the repository name. + + Returns: + dict with eligibility proof, capability plan, and safe next action. + """ + 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"), + } + + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + pr = api_request("GET", f"{repo_api_url(h, o, r)}/pulls/{pr_number}", auth) + + assessment = reconciliation_workflow.assess_open_pr_reconciliation( + pr=pr, + project_root=PROJECT_ROOT, + remote=remote, + target_branch=target_branch, + ) + + profile = get_profile() + capabilities = reconciliation_workflow.profile_reconciliation_capabilities( + profile.get("allowed_operations"), + profile.get("forbidden_operations"), + ) + plan = reconciliation_workflow.resolve_reconciliation_plan( + assessment=assessment, + capabilities=capabilities, + ) + + return { + "success": True, + "performed": False, + "pr_number": pr_number, + "assessment": assessment, + "capabilities": capabilities, + "plan": plan, + "workflow_source": "skills/llm-project-workflow/workflows/reconcile-landed-pr.md", + "task_mode": "reconcile-landed-pr", + "review_merge_allowed": False, + } + + +@mcp.tool() +def gitea_scan_already_landed_open_prs( + target_branch: str = "master", + limit: int = 50, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Read-only: list open PRs already landed on the target branch (#301). + + Traverses open PR inventory with pagination proof, assesses each candidate, + and returns PRs classified as ``ALREADY_LANDED_RECONCILE_REQUIRED``. Does + not review, approve, merge, or mutate Gitea state. + + Args: + target_branch: Branch used for ancestry proof (default master). + limit: Max open PRs to inspect across pages. + remote: Known instance — 'dadeschools' or 'prgs'. + host: Override the Gitea host. + org: Override the owner/organization. + repo: Override the repository name. + + Returns: + dict with candidates, pagination metadata, and target branch SHA proof. + """ + 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"), + } + + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + url = f"{repo_api_url(h, o, r)}/pulls?state=open" + + open_prs: list[dict] = [] + current_page = 1 + pages_fetched = 0 + last_meta: dict = {} + per_page = min(50, max(1, int(limit))) + while pages_fetched < 100 and len(open_prs) < limit: + raw_page, last_meta = api_fetch_page( + url, auth, page=current_page, limit=per_page + ) + pages_fetched += 1 + remaining = limit - len(open_prs) + open_prs.extend(raw_page[:remaining]) + if last_meta["is_final_page"] or len(open_prs) >= limit: + break + current_page += 1 + + pagination = { + "page": 1, + "per_page": per_page, + "returned_count": len(open_prs), + "has_more": False, + "next_page": None, + "is_final_page": bool(last_meta.get("is_final_page")), + "pages_fetched": pages_fetched, + "inventory_complete": bool(last_meta.get("is_final_page")), + "total_count": len(open_prs) if last_meta.get("is_final_page") else None, + } + + target_fetch = reconciliation_workflow.fetch_target_branch( + PROJECT_ROOT, remote, target_branch + ) + + candidates: list[dict] = [] + for pr in open_prs: + assessment = reconciliation_workflow.assess_open_pr_reconciliation( + pr=pr, + project_root=PROJECT_ROOT, + remote=remote, + target_branch=target_branch, + target_fetch=target_fetch, + ) + if assessment.get("eligibility_class") == ( + reconciliation_workflow.ELIGIBILITY_ALREADY_LANDED + ): + candidates.append(assessment) + + return { + "success": True, + "performed": False, + "target_branch": target_branch, + "target_branch_sha": target_fetch.get("target_branch_sha"), + "git_ref_mutations": ( + [target_fetch["git_fetch_command"]] + if target_fetch.get("git_fetch_command") + else [] + ), + "candidates": candidates, + "candidate_count": len(candidates), + "pagination": pagination, + "task_mode": "reconcile-landed-pr", + "review_merge_allowed": False, + } + + @mcp.tool() def gitea_close_issue( issue_number: int, @@ -4182,6 +4355,28 @@ _PROJECT_SKILLS = { "Do not merge unless the operator explicitly authorizes it.", ], }, + "gitea-reconcile-landed-pr": { + "description": "Reconcile open PRs whose heads are already on the " + "target branch without review or merge.", + "when_to_use": "An open PR blocks the review queue but its head SHA " + "is already an ancestor of master; use the dedicated " + "reconciliation path instead of review/merge.", + "required_operations": ["gitea.read"], + "status": "available", + "notes": "Load skills/llm-project-workflow/workflows/reconcile-landed-pr.md " + "first. PR close requires exact gitea.pr.close; review/merge " + "capabilities must not be used on this path.", + "steps": [ + "Resolve task: gitea_resolve_task_capability(task='reconcile_landed_pr').", + "Load canonical workflow via mcp_get_skill_guide or " + "skills/llm-project-workflow/workflows/reconcile-landed-pr.md.", + "Scan queue: gitea_scan_already_landed_open_prs (pagination proof).", + "Assess one PR: gitea_assess_already_landed_reconciliation.", + "Follow the plan outcome: full close only with gitea.pr.close, " + "comment-then-stop when close is missing, recovery handoff otherwise.", + "Never approve, request changes, or merge on this path.", + ], + }, "gitea-pr-merge": { "description": "Gated merge of an approved pull request.", "when_to_use": "Reviewer/merger profile with explicit operator " diff --git a/reconciliation_workflow.py b/reconciliation_workflow.py new file mode 100644 index 0000000..48867b7 --- /dev/null +++ b/reconciliation_workflow.py @@ -0,0 +1,292 @@ +"""Already-landed PR reconciliation workflow helpers (#301). + +Read-only assessment and capability planning for reconciling open PRs whose +heads are already ancestors of the target branch. Does not invoke review or +merge paths. +""" + +from __future__ import annotations + +import subprocess +from typing import Any + +import gitea_config +from merged_cleanup_reconcile import extract_linked_issue, is_head_ancestor_of_ref + +ELIGIBILITY_ALREADY_LANDED = "ALREADY_LANDED_RECONCILE_REQUIRED" +ELIGIBILITY_NOT_LANDED = "NOT_ALREADY_LANDED" +ELIGIBILITY_STALE_TARGET = "TARGET_BRANCH_UNVERIFIED" +ELIGIBILITY_PR_NOT_OPEN = "PR_NOT_OPEN" + +OUTCOME_FULL_RECONCILE = "FULL_RECONCILE_CLOSE_ALLOWED" +OUTCOME_PARTIAL_COMMENT = "PARTIAL_RECONCILE_COMMENT_THEN_STOP" +OUTCOME_RECOVERY_HANDOFF = "RECOVERY_HANDOFF_ONLY" +OUTCOME_NOT_LANDED = "NOT_LANDED_NO_ACTION" +OUTCOME_GATE_NOT_PROVEN = "GATE_NOT_PROVEN" + +RECONCILE_WORKFLOW_MARKERS = ( + "workflows/reconcile-landed-pr.md", + "reconcile-landed-pr.md", +) + +RECONCILE_TASK_MARKERS = ( + "reconcile-landed-pr", + "reconcile already-landed", + "reconcile_already_landed", +) + + +def fetch_target_branch(project_root: str, remote: str, branch: str) -> dict[str, Any]: + """Fetch *branch* from *remote* and return the resolved SHA.""" + ref = f"{remote}/{branch}" + fetch = subprocess.run( + ["git", "-C", project_root, "fetch", remote, branch], + capture_output=True, + text=True, + check=False, + ) + if fetch.returncode != 0: + return { + "success": False, + "target_branch": branch, + "target_ref": ref, + "target_branch_sha": None, + "reasons": [ + f"git fetch {remote} {branch} failed: " + f"{(fetch.stderr or fetch.stdout or '').strip()}" + ], + } + + rev = subprocess.run( + ["git", "-C", project_root, "rev-parse", ref], + capture_output=True, + text=True, + check=False, + ) + if rev.returncode != 0: + return { + "success": False, + "target_branch": branch, + "target_ref": ref, + "target_branch_sha": None, + "reasons": [ + f"git rev-parse {ref} failed: " + f"{(rev.stderr or rev.stdout or '').strip()}" + ], + } + + return { + "success": True, + "target_branch": branch, + "target_ref": ref, + "target_branch_sha": (rev.stdout or "").strip(), + "reasons": [], + "git_fetch_command": f"git fetch {remote} {branch}", + } + + +def profile_reconciliation_capabilities( + allowed: list[str] | None, + forbidden: list[str] | None, +) -> dict[str, bool]: + """Map active profile operations to reconciliation capabilities.""" + allowed = allowed or [] + forbidden = forbidden or [] + + def can(op: str) -> bool: + return gitea_config.check_operation(op, allowed, forbidden)[0] + + return { + "read": can("gitea.read"), + "comment_pr": can("gitea.pr.comment"), + "comment_issue": can("gitea.issue.comment"), + "close_pr": can("gitea.pr.close"), + "close_issue": can("gitea.issue.close"), + "review_pr": can("gitea.pr.review") or can("gitea.pr.approve"), + "merge_pr": can("gitea.pr.merge"), + } + + +def assess_open_pr_reconciliation( + *, + pr: dict[str, Any], + project_root: str, + remote: str, + target_branch: str, + target_fetch: dict[str, Any] | None = None, +) -> dict[str, Any]: + """Assess whether an open PR is eligible for already-landed reconciliation.""" + pr_number = int(pr.get("number") or 0) + pr_state = (pr.get("state") or "").strip().lower() + head = pr.get("head") or {} + head_sha = head.get("sha") if isinstance(head, dict) else None + head_ref = head.get("ref") if isinstance(head, dict) else None + base = pr.get("base") or {} + base_ref = base.get("ref") if isinstance(base, dict) else None + title = pr.get("title") or "" + body = pr.get("body") or "" + + fetch_result = target_fetch or fetch_target_branch( + project_root, remote, target_branch + ) + linked_issue = extract_linked_issue(title, body) + + result: dict[str, Any] = { + "pr_number": pr_number, + "pr_state": pr_state, + "candidate_head_sha": head_sha, + "head_ref": head_ref, + "base_ref": base_ref or target_branch, + "target_branch": target_branch, + "target_branch_sha": fetch_result.get("target_branch_sha"), + "linked_issue": linked_issue, + "git_ref_mutations": [], + "reasons": [], + "review_merge_allowed": False, + } + if fetch_result.get("git_fetch_command"): + result["git_ref_mutations"].append(fetch_result["git_fetch_command"]) + + if pr_state != "open": + result["eligibility_class"] = ELIGIBILITY_PR_NOT_OPEN + result["ancestor_proof"] = None + result["reconciliation_allowed"] = False + result["reasons"].append(f"PR #{pr_number} state is {pr_state!r}, not open") + return result + + if not fetch_result.get("success"): + result["eligibility_class"] = ELIGIBILITY_STALE_TARGET + result["ancestor_proof"] = None + result["reconciliation_allowed"] = False + result["reasons"].extend(fetch_result.get("reasons") or []) + return result + + target_ref = fetch_result.get("target_ref") or f"{remote}/{target_branch}" + ancestor = is_head_ancestor_of_ref(project_root, head_sha, target_ref) + result["ancestor_proof"] = ancestor + + if ancestor is None: + result["eligibility_class"] = ELIGIBILITY_STALE_TARGET + result["reconciliation_allowed"] = False + result["reasons"].append( + f"ancestor check failed for head {head_sha!r} against {target_ref}" + ) + return result + + if ancestor: + result["eligibility_class"] = ELIGIBILITY_ALREADY_LANDED + result["reconciliation_allowed"] = True + return result + + result["eligibility_class"] = ELIGIBILITY_NOT_LANDED + result["reconciliation_allowed"] = False + result["reasons"].append( + f"PR head {head_sha} is not an ancestor of {target_ref}" + ) + return result + + +def resolve_reconciliation_plan( + *, + assessment: dict[str, Any], + capabilities: dict[str, bool] | None, +) -> dict[str, Any]: + """Map eligibility + profile capabilities to a reconciliation outcome.""" + caps = capabilities or {} + reasons: list[str] = [] + + if not assessment.get("reconciliation_allowed"): + outcome = OUTCOME_NOT_LANDED + if assessment.get("eligibility_class") in { + ELIGIBILITY_STALE_TARGET, + ELIGIBILITY_PR_NOT_OPEN, + }: + outcome = OUTCOME_GATE_NOT_PROVEN + reasons.extend(assessment.get("reasons") or []) + return { + "outcome": outcome, + "close_pr_allowed": False, + "comment_pr_allowed": False, + "close_issue_allowed": False, + "comment_issue_allowed": False, + "review_merge_allowed": False, + "missing_capabilities": _missing_reconciliation_capabilities(caps), + "safe_next_action": ( + "Do not reconcile via review/merge; repair missing proof first." + ), + "reasons": reasons, + } + + close_pr = bool(caps.get("close_pr")) + comment_pr = bool(caps.get("comment_pr")) + close_issue = bool(caps.get("close_issue")) + comment_issue = bool(caps.get("comment_issue")) + + if caps.get("review_pr") or caps.get("merge_pr"): + reasons.append( + "reconciliation path must not use review/merge capabilities" + ) + + if close_pr: + outcome = OUTCOME_FULL_RECONCILE + safe_next_action = ( + "Run reconciliation close via exact gitea.pr.close after proof; " + "do not approve or merge." + ) + elif comment_pr: + outcome = OUTCOME_PARTIAL_COMMENT + safe_next_action = ( + "Post one reconciliation comment with ancestor proof, then stop " + "for an authorized close profile." + ) + else: + outcome = OUTCOME_RECOVERY_HANDOFF + safe_next_action = ( + "Produce a recovery handoff naming missing gitea.pr.close and/or " + "gitea.pr.comment; do not loop through review/merge." + ) + reasons.append("gitea.pr.close is not available in the active profile") + + return { + "outcome": outcome, + "close_pr_allowed": close_pr, + "comment_pr_allowed": comment_pr, + "close_issue_allowed": close_issue, + "comment_issue_allowed": comment_issue, + "review_merge_allowed": False, + "missing_capabilities": _missing_reconciliation_capabilities(caps), + "safe_next_action": safe_next_action, + "reasons": reasons, + } + + +def _missing_reconciliation_capabilities(caps: dict[str, bool]) -> list[str]: + missing = [] + if not caps.get("read"): + missing.append("gitea.read") + if not caps.get("close_pr"): + missing.append("gitea.pr.close") + if not caps.get("comment_pr"): + missing.append("gitea.pr.comment") + return missing + + +def assess_reconcile_workflow_source(report_text: str) -> dict[str, Any]: + """Reconciliation reports must cite the canonical workflow file.""" + lower = (report_text or "").lower() + reasons = [] + if not any(marker in lower for marker in RECONCILE_WORKFLOW_MARKERS): + reasons.append( + "reconciliation report missing workflow source " + "(workflows/reconcile-landed-pr.md)" + ) + if not any(marker in lower for marker in RECONCILE_TASK_MARKERS): + reasons.append( + "reconciliation report missing task mode declaration " + "(reconcile-landed-pr)" + ) + return { + "complete": not reasons, + "downgraded": bool(reasons), + "reasons": reasons, + } \ No newline at end of file diff --git a/review_proofs.py b/review_proofs.py index 89e3701..e6a4518 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -17,6 +17,11 @@ here weakens or replaces them. import re import issue_duplicate_gate +from reconciliation_workflow import ( + RECONCILE_TASK_MARKERS, + RECONCILE_WORKFLOW_MARKERS, + assess_reconcile_workflow_source, +) from reviewer_worktree import assess_reviewer_worktree_proof _FULL_SHA = re.compile(r"^[0-9a-f]{40}$") diff --git a/role_session_router.py b/role_session_router.py index a7db807..d14750e 100644 --- a/role_session_router.py +++ b/role_session_router.py @@ -72,6 +72,8 @@ AUTHOR_TASKS = frozenset({ "comment_pr", "address_pr_change_requests", "delete_branch", + "reconcile_landed_pr", + "reconcile-landed-pr", }) TASK_REQUIRED_ROLE = { @@ -90,6 +92,8 @@ TASK_REQUIRED_ROLE = { "blind_pr_queue_review": "reviewer", "request_changes_pr": "reviewer", "approve_pr": "reviewer", + "reconcile_landed_pr": "author", + "reconcile-landed-pr": "author", } WRONG_ROLE_REVIEWER_MSG = ( diff --git a/task_capability_map.py b/task_capability_map.py index 9d162c1..16c1221 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -96,6 +96,14 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.read", "role": "author", }, + "reconcile_landed_pr": { + "permission": "gitea.read", + "role": "author", + }, + "reconcile-landed-pr": { + "permission": "gitea.read", + "role": "author", + }, } # Issue-mutating MCP tools and their resolver task keys. diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 550c007..48d2f5e 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -118,6 +118,16 @@ def test_start_issue_template_references_work_issue_workflow(): assert "workflows/work-issue.md" in text +def test_reconcile_skill_registered_in_mcp_server(): + from mcp_server import _PROJECT_SKILLS + + assert "gitea-reconcile-landed-pr" in _PROJECT_SKILLS + steps = _PROJECT_SKILLS["gitea-reconcile-landed-pr"]["steps"] + joined = " ".join(steps).lower() + assert "gitea_scan_already_landed_open_prs" in joined + assert "gitea_assess_already_landed_reconciliation" in joined + + def test_reviewer_fallback_verifier_exported(): from review_proofs import assess_reviewer_fallback_report diff --git a/tests/test_reconciliation_mcp_tools.py b/tests/test_reconciliation_mcp_tools.py new file mode 100644 index 0000000..974df92 --- /dev/null +++ b/tests/test_reconciliation_mcp_tools.py @@ -0,0 +1,106 @@ +"""Tests for reconciliation MCP assessment tools (#301).""" +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import mcp_server +from mcp_server import ( + gitea_assess_already_landed_reconciliation, + gitea_scan_already_landed_open_prs, +) + +AUTHOR_PROFILE = { + "profile_name": "prgs-author", + "allowed_operations": [ + "gitea.read", + "gitea.pr.comment", + "gitea.issue.comment", + "gitea.issue.close", + ], + "forbidden_operations": ["gitea.pr.close"], + "audit_label": "prgs-author", +} + +OPEN_PR = { + "number": 278, + "title": "Landed (Closes #263)", + "body": "", + "state": "open", + "head": {"ref": "feat/x", "sha": "a" * 40}, + "base": {"ref": "master"}, +} + + +class TestReconciliationMcpTools(unittest.TestCase): + def setUp(self): + self.mock_api = patch("mcp_server.api_request").start() + self.mock_auth = patch( + "mcp_server.get_auth_header", return_value="token test" + ).start() + patch("mcp_server.get_profile", return_value=AUTHOR_PROFILE).start() + mcp_server._IDENTITY_CACHE.clear() + + def tearDown(self): + patch.stopall() + mcp_server._IDENTITY_CACHE.clear() + + def test_assess_returns_plan_without_mutations(self): + self.mock_api.return_value = OPEN_PR + with patch( + "mcp_server.reconciliation_workflow.assess_open_pr_reconciliation", + return_value={ + "reconciliation_allowed": True, + "eligibility_class": "ALREADY_LANDED_RECONCILE_REQUIRED", + "candidate_head_sha": OPEN_PR["head"]["sha"], + "target_branch_sha": "deadbeef", + "linked_issue": 263, + "review_merge_allowed": False, + }, + ): + res = gitea_assess_already_landed_reconciliation( + pr_number=278, remote="prgs" + ) + self.assertTrue(res["success"]) + self.assertFalse(res["performed"]) + self.assertEqual(res["plan"]["outcome"], "PARTIAL_RECONCILE_COMMENT_THEN_STOP") + self.assertFalse(res["review_merge_allowed"]) + patch_calls = [ + c for c in self.mock_api.call_args_list if c.args[0] == "PATCH" + ] + self.assertEqual(patch_calls, []) + + def test_scan_returns_candidates_with_pagination(self): + with patch( + "mcp_server.api_fetch_page", + return_value=([OPEN_PR], { + "page": 1, + "per_page": 50, + "is_final_page": True, + "has_more": False, + "next_page": None, + }), + ), patch( + "mcp_server.reconciliation_workflow.fetch_target_branch", + return_value={ + "success": True, + "target_branch_sha": "deadbeef", + "git_fetch_command": "git fetch prgs master", + }, + ), patch( + "mcp_server.reconciliation_workflow.assess_open_pr_reconciliation", + return_value={ + "pr_number": 278, + "eligibility_class": "ALREADY_LANDED_RECONCILE_REQUIRED", + "reconciliation_allowed": True, + }, + ): + res = gitea_scan_already_landed_open_prs(remote="prgs", limit=50) + self.assertTrue(res["success"]) + self.assertEqual(res["candidate_count"], 1) + self.assertTrue(res["pagination"]["inventory_complete"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file diff --git a/tests/test_reconciliation_workflow.py b/tests/test_reconciliation_workflow.py new file mode 100644 index 0000000..e3235c4 --- /dev/null +++ b/tests/test_reconciliation_workflow.py @@ -0,0 +1,149 @@ +"""Tests for already-landed reconciliation workflow helpers (#301).""" +import sys +import unittest +from unittest.mock import patch + +sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent)) + +import reconciliation_workflow +from review_proofs import assess_reconcile_workflow_source + + +class TestReconciliationAssessment(unittest.TestCase): + def test_already_landed_open_pr(self): + with patch( + "reconciliation_workflow.is_head_ancestor_of_ref", + return_value=True, + ): + assessment = reconciliation_workflow.assess_open_pr_reconciliation( + pr={ + "number": 278, + "state": "open", + "title": "Fix (Closes #263)", + "body": "", + "head": {"ref": "feat/x", "sha": "abc" * 13 + "a"}, + "base": {"ref": "master"}, + }, + project_root="/tmp/repo", + remote="prgs", + target_branch="master", + target_fetch={ + "success": True, + "target_ref": "prgs/master", + "target_branch_sha": "deadbeef", + "git_fetch_command": "git fetch prgs master", + }, + ) + self.assertTrue(assessment["reconciliation_allowed"]) + self.assertEqual( + assessment["eligibility_class"], + reconciliation_workflow.ELIGIBILITY_ALREADY_LANDED, + ) + + def test_not_landed_pr(self): + with patch( + "reconciliation_workflow.is_head_ancestor_of_ref", + return_value=False, + ): + assessment = reconciliation_workflow.assess_open_pr_reconciliation( + pr={ + "number": 1, + "state": "open", + "title": "WIP", + "body": "", + "head": {"ref": "feat/y", "sha": "fff" * 13 + "f"}, + "base": {"ref": "master"}, + }, + project_root="/tmp/repo", + remote="prgs", + target_branch="master", + target_fetch={ + "success": True, + "target_ref": "prgs/master", + "target_branch_sha": "deadbeef", + }, + ) + self.assertFalse(assessment["reconciliation_allowed"]) + + +class TestReconciliationPlan(unittest.TestCase): + def test_full_close_when_close_pr_allowed(self): + plan = reconciliation_workflow.resolve_reconciliation_plan( + assessment={ + "reconciliation_allowed": True, + "eligibility_class": reconciliation_workflow.ELIGIBILITY_ALREADY_LANDED, + }, + capabilities={ + "close_pr": True, + "comment_pr": True, + "close_issue": True, + "comment_issue": True, + }, + ) + self.assertEqual( + plan["outcome"], + reconciliation_workflow.OUTCOME_FULL_RECONCILE, + ) + self.assertTrue(plan["close_pr_allowed"]) + self.assertFalse(plan["review_merge_allowed"]) + + def test_comment_then_stop_without_close(self): + plan = reconciliation_workflow.resolve_reconciliation_plan( + assessment={ + "reconciliation_allowed": True, + "eligibility_class": reconciliation_workflow.ELIGIBILITY_ALREADY_LANDED, + }, + capabilities={ + "close_pr": False, + "comment_pr": True, + }, + ) + self.assertEqual( + plan["outcome"], + reconciliation_workflow.OUTCOME_PARTIAL_COMMENT, + ) + + def test_recovery_handoff_without_comment_or_close(self): + plan = reconciliation_workflow.resolve_reconciliation_plan( + assessment={ + "reconciliation_allowed": True, + "eligibility_class": reconciliation_workflow.ELIGIBILITY_ALREADY_LANDED, + }, + capabilities={"close_pr": False, "comment_pr": False}, + ) + self.assertEqual( + plan["outcome"], + reconciliation_workflow.OUTCOME_RECOVERY_HANDOFF, + ) + + def test_not_landed_no_action(self): + plan = reconciliation_workflow.resolve_reconciliation_plan( + assessment={ + "reconciliation_allowed": False, + "eligibility_class": reconciliation_workflow.ELIGIBILITY_NOT_LANDED, + "reasons": ["not ancestor"], + }, + capabilities={"close_pr": True}, + ) + self.assertEqual( + plan["outcome"], + reconciliation_workflow.OUTCOME_NOT_LANDED, + ) + + +class TestReconcileWorkflowSource(unittest.TestCase): + def test_valid_report_passes(self): + report = ( + "Task mode: reconcile-landed-pr\n" + "Workflow source: workflows/reconcile-landed-pr.md\n" + ) + result = assess_reconcile_workflow_source(report) + self.assertTrue(result["complete"]) + + def test_missing_workflow_fails(self): + result = assess_reconcile_workflow_source("Task mode: reconcile-landed-pr") + self.assertFalse(result["complete"]) + + +if __name__ == "__main__": + unittest.main() \ No newline at end of file From dc24fe3afec11c556ff25ad50f0a99f8f2942a7a Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 09:31:45 -0400 Subject: [PATCH 4/5] feat: enforce canonical reconciliation handoff schema (Closes #307) Reconciliation final reports must emit only the canonical reconciliation handoff schema: - Expand _RECONCILE_REQUIRED_FIELDS from 5 to the full 27-field canonical schema (Task through No review/merge confirmation), so incomplete reconciliation handoffs downgrade with per-field reasons. - Add 'issue lock proof', 'claim/comment status', and 'workspace mutations' to _RECONCILE_STALE_FIELDS so stale author/reviewer fields block. - Make the 'PR number opened' rejection conditional on a new session_pr_opened proof kwarg: the field is allowed only when a PR was actually opened in the session. - Close the git-fetch reporting gap: a fetch observed only in the action log (not report text) now requires a Git ref mutations entry as well. - Update the reconciliation handoff fixture to the canonical schema and add TestCanonicalReconcileSchema covering blocked, successful-close, and comment-only reconciliation plus missing-close-capability, stale-field, and conditional PR-number-opened paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- final_report_validator.py | 46 ++++++- tests/test_final_report_validator.py | 186 ++++++++++++++++++++++++++- 2 files changed, 222 insertions(+), 10 deletions(-) diff --git a/final_report_validator.py b/final_report_validator.py index bff9467..9d3f6d7 100644 --- a/final_report_validator.py +++ b/final_report_validator.py @@ -101,11 +101,37 @@ _RECONCILE_STALE_FIELDS = ( "scratch worktree used", "review decision", "merge result", + "issue lock proof", + "claim/comment status", + "workspace mutations", ) +# Issue #307: reconciliation handoffs must carry the full canonical schema. _RECONCILE_REQUIRED_FIELDS = ( + "Task", + "Repo", + "Role/profile", + "Identity", "Selected PR", - "Eligibility class", + "PR live state", + "Candidate head SHA", + "Target branch", + "Target branch SHA", + "Ancestor proof", + "Linked issue", "Linked issue live status", + "Eligibility class", + "Capabilities proven", + "Missing capabilities", + "PR comments posted", + "Issue comments posted", + "PRs closed", + "Issues closed", + "Git ref mutations", + "Worktree mutations", + "MCP/Gitea mutations", + "External-state mutations", + "Read-only diagnostics", + "Blocker", "Safe next action", "No review/merge confirmation", ) @@ -346,13 +372,15 @@ def _rule_reviewer_git_fetch_readonly( "classify git fetch under Git ref mutations, not read-only diagnostics", ) ] - if not git_ref_reported and _GIT_FETCH_RE.search(text): + if not git_ref_reported: + # Issue #307: an observed git fetch (report text or action log) + # must be listed under Git ref mutations. return [ validator_finding( "reviewer.git_fetch_readonly", "downgrade", "Git ref mutations", - "git fetch mentioned without Git ref mutation classification", + "git fetch occurred without Git ref mutation classification", "record git fetch under Git ref mutations", ) ] @@ -541,10 +569,18 @@ def _rule_reconcile_controller_handoff(report_text: str) -> list[dict[str, str]] return [] -def _rule_reconcile_stale_author_fields(report_text: str) -> list[dict[str, str]]: +def _rule_reconcile_stale_author_fields( + report_text: str, + *, + session_pr_opened: bool = False, +) -> list[dict[str, str]]: text = (report_text or "").lower() findings = [] for field in _RECONCILE_STALE_FIELDS: + if field == "pr number opened" and session_pr_opened: + # Issue #307: allowed only when a PR was actually opened + # in this session. + continue if f"{field}:" in text: findings.append( validator_finding( @@ -803,6 +839,7 @@ def assess_final_report_validator( mutations_observed: bool = False, local_edits: bool = False, issue_filing_lock: dict | None = None, + session_pr_opened: bool = False, ) -> dict[str, Any]: """Validate final-report text against task-specific proof rules (#327). @@ -857,6 +894,7 @@ def assess_final_report_validator( "action_log": action_log, "mutations_observed": mutations_observed, "local_edits": local_edits, + "session_pr_opened": session_pr_opened, } for rule in _RULES_BY_TASK.get(normalized_kind, ()): diff --git a/tests/test_final_report_validator.py b/tests/test_final_report_validator.py index f8e8848..4c1644c 100644 --- a/tests/test_final_report_validator.py +++ b/tests/test_final_report_validator.py @@ -53,26 +53,47 @@ def _review_handoff(**overrides): return text -def _reconcile_handoff(**overrides): +def _reconcile_handoff(drop=(), **overrides): lines = [ "## Controller Handoff", "", "- Task: reconcile already-landed PR #99", "- Repo: Scaled-Tech-Consulting/Gitea-Tools", - "- Role: reconciler", - "- Identity: sysadmin / prgs-author", + "- Role/profile: reconciler / prgs-reconciler", + "- Identity: sysadmin", "- Selected PR: #99", - "- Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED", + "- PR live state: open", + "- Candidate head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Target branch: master", + "- Target branch SHA: 5e023dc71b0e2b813a0b1eee6b0f42630c47a95c", + "- Ancestor proof: candidate head is ancestor of target branch SHA", "- Linked issue: #98", "- Linked issue live status: not verified in this session", + "- Eligibility class: ALREADY_LANDED_RECONCILE_REQUIRED", + "- Capabilities proven: gitea.read, gitea.pr.comment", + "- Missing capabilities: gitea.pr.close", + "- PR comments posted: 1 reconciliation comment on PR #99", + "- Issue comments posted: none", + "- PRs closed: none", + "- Issues closed: none", "- File edits by reconciler: none", "- Git ref mutations: git fetch prgs master", + "- Worktree mutations: none", + "- MCP/Gitea mutations: PR comment posted", + "- External-state mutations: none", + "- Read-only diagnostics: gitea_view_pr, gitea_view_issue", "- Reconciliation mutations: PR comment posted", "- Current status: reconciliation complete", - "- Safe next action: none", + "- Blocker: missing gitea.pr.close capability", + "- Safe next action: hand off to a profile with gitea.pr.close", "- No review/merge confirmation: confirmed", ] - text = "\n".join(lines) + kept = [ + line + for line in lines + if not any(line.startswith(f"- {name}:") for name in drop) + ] + text = "\n".join(kept) for key, value in overrides.items(): if f"- {key}:" in text: text = text.replace(f"- {key}:", f"- {key}: {value}") @@ -251,6 +272,159 @@ class TestReconciliationRules(unittest.TestCase): ) +class TestCanonicalReconcileSchema(unittest.TestCase): + """Issue #307: reconciliation workflows emit only the canonical schema.""" + + def test_comment_only_reconciliation_passes(self): + result = assess_final_report_validator( + _reconcile_handoff(), + "reconcile_already_landed", + ) + self.assertEqual(result["grade"], "A") + self.assertEqual(result["findings"], []) + + def test_successful_close_reconciliation_passes(self): + report = _reconcile_handoff().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", + ) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertEqual(result["grade"], "A") + + def test_blocked_reconciliation_passes(self): + report = _reconcile_handoff().replace( + "- Capabilities proven: gitea.read, gitea.pr.comment", + "- Capabilities proven: gitea.read", + ).replace( + "- Missing capabilities: gitea.pr.close", + "- Missing capabilities: gitea.pr.close, gitea.pr.comment", + ).replace( + "- PR comments posted: 1 reconciliation comment on PR #99", + "- PR comments posted: none", + ).replace( + "- MCP/Gitea mutations: PR comment posted", + "- MCP/Gitea mutations: none", + ).replace( + "- Reconciliation mutations: PR comment posted", + "- Reconciliation mutations: none", + ) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertEqual(result["grade"], "A") + + def test_missing_capabilities_field_required(self): + report = _reconcile_handoff(drop=("Missing capabilities",)) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertEqual(result["grade"], "downgraded") + self.assertTrue( + any( + f["rule_id"] == "reconcile.controller_handoff" + and "Missing capabilities" in f["reason"] + for f in result["findings"] + ) + ) + + def test_missing_ancestor_proof_and_candidate_sha_downgrade(self): + report = _reconcile_handoff(drop=("Ancestor proof", "Candidate head SHA")) + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertEqual(result["grade"], "downgraded") + reasons = " ".join(f["reason"] for f in result["findings"]) + self.assertIn("Ancestor proof", reasons) + self.assertIn("Candidate head SHA", reasons) + + def test_stale_issue_lock_proof_blocks(self): + report = _reconcile_handoff() + "\n- Issue lock proof: locked before diff" + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reconcile.stale_author_reviewer_fields" + and f["field"] == "issue lock proof" + for f in result["findings"] + ) + ) + + def test_stale_claim_comment_status_blocks(self): + report = _reconcile_handoff() + "\n- Claim/comment status: claimed" + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reconcile.stale_author_reviewer_fields" + and f["field"] == "claim/comment status" + for f in result["findings"] + ) + ) + + def test_stale_workspace_mutations_blocks_without_observed_mutations(self): + report = _reconcile_handoff() + "\n- Workspace mutations: None" + result = assess_final_report_validator(report, "reconcile_already_landed") + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reconcile.stale_author_reviewer_fields" + and f["field"] == "workspace mutations" + for f in result["findings"] + ) + ) + + def test_pr_number_opened_allowed_when_session_opened_pr(self): + report = _reconcile_handoff() + "\n- PR number opened: #12" + result = assess_final_report_validator( + report, + "reconcile_already_landed", + session_pr_opened=True, + ) + self.assertFalse( + any( + f["rule_id"] == "reconcile.stale_author_reviewer_fields" + and f["field"] == "pr number opened" + for f in result["findings"] + ) + ) + + def test_pr_number_opened_still_blocked_without_session_proof(self): + report = _reconcile_handoff() + "\n- PR number opened: #12" + result = assess_final_report_validator( + report, + "reconcile_already_landed", + session_pr_opened=False, + ) + self.assertTrue(result["blocked"]) + self.assertTrue( + any( + f["rule_id"] == "reconcile.stale_author_reviewer_fields" + and f["field"] == "pr number opened" + for f in result["findings"] + ) + ) + + def test_action_log_fetch_requires_git_ref_mutation_entry(self): + report = _reconcile_handoff().replace( + "- Git ref mutations: git fetch prgs master", + "- Git ref mutations: none", + ) + result = assess_final_report_validator( + report, + "reconcile_already_landed", + action_log=[{"command": "git fetch prgs master", "performed": True}], + ) + self.assertTrue( + any( + f["rule_id"] == "reviewer.git_fetch_readonly" + 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 a164700ab9e4bd27c3eecb945b0fb1b90ea43e89 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Tue, 7 Jul 2026 10:04:06 -0400 Subject: [PATCH 5/5] feat: add reviewer final-report schema MCP validator (Closes #391) Expose gitea_validate_review_final_report composing the composable final-report validator with review-specific schema gates for legacy fields, merge/issue claims, blocked handoff replay, and narrative drift. --- gitea_mcp_server.py | 29 +++ review_final_report_schema.py | 319 +++++++++++++++++++++++ review_proofs.py | 7 + tests/test_llm_workflow_split.py | 6 + tests/test_review_final_report_schema.py | 158 +++++++++++ 5 files changed, 519 insertions(+) create mode 100644 review_final_report_schema.py create mode 100644 tests/test_review_final_report_schema.py diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 9913535..4cb024a 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -4859,6 +4859,35 @@ def gitea_get_shell_health() -> dict: return native_mcp_preference.shell_health_status() +@mcp.tool() +def gitea_validate_review_final_report( + report_text: str, + review_decision_lock: dict | None = None, + linked_issue_lock: dict | None = None, + validation_report: dict | None = None, + action_log: list[dict] | None = None, + mutations_observed: bool = False, + local_edits: bool = False, +) -> dict: + """Read-only: machine-validate reviewer final report schema before output (#391). + + Composes the composable final-report validator with review-specific schema + gates (legacy fields, mutation categories, merge/issue claims, blocked + handoff replay, and narrative/handoff consistency). + """ + from review_final_report_schema import assess_review_final_report_schema + + return assess_review_final_report_schema( + report_text, + review_decision_lock=review_decision_lock, + linked_issue_lock=linked_issue_lock, + validation_report=validation_report, + action_log=action_log, + mutations_observed=mutations_observed, + local_edits=local_edits, + ) + + @mcp.tool() def gitea_get_runtime_context( remote: str = "dadeschools", diff --git a/review_final_report_schema.py b/review_final_report_schema.py new file mode 100644 index 0000000..01e2e34 --- /dev/null +++ b/review_final_report_schema.py @@ -0,0 +1,319 @@ +"""Reviewer final-report schema verification before session output (#391). + +Composes the composable ``assess_final_report_validator`` (#327) with +review-specific schema gates required before a reviewer session completes. +""" + +from __future__ import annotations + +import re +from typing import Any + +from final_report_validator import ( + assess_final_report_validator, + validator_finding, + _handoff_fields, +) + +_LEGACY_STALE_FIELDS = ( + "pinned reviewed head", + "scratch worktree used", + "workspace mutations", +) + +_REVIEWED_HEAD_RE = re.compile( + r"(?:pinned reviewed head|reviewed head sha)\s*:\s*([0-9a-f]{7,40})", + re.IGNORECASE, +) +_VALIDATION_PASS_RE = re.compile( + r"validation\s*:\s*(?:pass|passed|strong|ok|green)", + re.IGNORECASE, +) +_MERGED_CLAIM_RE = re.compile( + r"\b(?:merged|merge result\s*:\s*merged|pr\s+#\d+\s+merged)\b", + re.IGNORECASE, +) +_MERGE_RESULT_RE = re.compile(r"merge result\s*:", re.IGNORECASE) +_ANCESTRY_PROOF_RE = re.compile( + r"(?:ancestor proof|target branch sha|merge commit)", + re.IGNORECASE, +) +_ISSUE_CLOSED_RE = re.compile( + r"(?:linked issue(?:\s+live)?\s+status\s*:\s*closed|issue\s+#\d+\s+closed)", + re.IGNORECASE, +) +_LIVE_ISSUE_PROOF_RE = re.compile( + r"gitea_view_issue|(?:issue|linked issue).{0,40}fetched live|live fetch proof", + re.IGNORECASE, +) +_FULL_SUITE_PASS_RE = re.compile( + r"(?:full suite passed|full test suite passed|\d+\s+passed,\s*0\s+failed)", + re.IGNORECASE, +) +_TESTS_IGNORED_RE = re.compile( + r"(?:ignored tests|tests?\s+ignored|skipped tests|not run|tests?\s+skipped)", + re.IGNORECASE, +) +_BLOCKED_HANDOFF_RE = re.compile( + r"(?:blocker\s*:|recovery handoff|infra_stop|capability stop|mutation blocked)", + re.IGNORECASE, +) +_REPLAY_COMMAND_RE = re.compile( + r"(?:gitea_submit_pr_review|gitea_merge_pr|gitea_review_pr|" + r"submitted\s+['\"]approve['\"]|replay approve|replay merge)", + re.IGNORECASE, +) +_PENDING_RE = re.compile(r"\bPENDING\b", re.IGNORECASE) +_APPROVED_RE = re.compile( + r"(?:review decision\s*:\s*approve|submitted\s+approve|official review submitted)", + re.IGNORECASE, +) +_DRY_RUN_RE = re.compile(r"dry[- ]run", re.IGNORECASE) +_FINDING_READY_RE = re.compile( + r"(?:finding ready|ready to submit|not yet submitted)", + re.IGNORECASE, +) +_NARRATIVE_APPROVE_RE = re.compile( + r"(?:^|\n)\s*(?:##\s+)?(?:summary|verdict|recommendation)\b[^\n]*\bapprove\b", + re.IGNORECASE | re.MULTILINE, +) +_HANDOFF_DECISION_RE = re.compile( + r"review decision\s*:\s*(\w+)", + re.IGNORECASE, +) + + +def _rule_legacy_stale_fields(report_text: str) -> list[dict[str, str]]: + fields = _handoff_fields(report_text) + findings: list[dict[str, str]] = [] + for stale in _LEGACY_STALE_FIELDS: + if stale in fields and fields[stale].lower() not in {"", "none", "n/a"}: + findings.append( + validator_finding( + "reviewer.legacy_stale_field", + "block", + stale.title(), + f"legacy or stale handoff field '{stale}' must not appear in " + "canonical reviewer final reports", + "remove stale fields; use Candidate head SHA and precise " + "mutation categories instead", + ) + ) + return findings + + +def _rule_reviewed_head_without_validation(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + if not _REVIEWED_HEAD_RE.search(text): + return [] + if _VALIDATION_PASS_RE.search(text): + return [] + return [ + validator_finding( + "reviewer.reviewed_head_without_validation", + "block", + "Pinned reviewed head", + "reviewed/pinned head SHA reported without validation pass proof", + "document validation command, cwd, and pass result before pinning head SHA", + ) + ] + + +def _rule_merged_without_proof(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + if not _MERGED_CLAIM_RE.search(text): + return [] + if _MERGE_RESULT_RE.search(text) and _ANCESTRY_PROOF_RE.search(text): + return [] + return [ + validator_finding( + "reviewer.merged_without_proof", + "block", + "Merge result", + "merged outcome claimed without merge result and target ancestry proof", + "include Merge result, target branch SHA, and ancestor proof before claiming merged", + ) + ] + + +def _rule_issue_closed_without_live_proof(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + if not _ISSUE_CLOSED_RE.search(text): + return [] + fields = _handoff_fields(text) + status_value = fields.get("linked issue live status", "") + readonly_value = fields.get("read-only diagnostics", "") + proof_blob = f"{status_value} {readonly_value}".lower() + if _LIVE_ISSUE_PROOF_RE.search(proof_blob): + return [] + return [ + validator_finding( + "reviewer.issue_closed_without_live_proof", + "block", + "Linked issue", + "issue closed claimed without live linked-issue verification proof", + "fetch linked issue live (gitea_view_issue) and record Linked issue live status", + ) + ] + + +def _rule_full_suite_pass_ignored_tests(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + if not (_FULL_SUITE_PASS_RE.search(text) and _TESTS_IGNORED_RE.search(text)): + return [] + if re.search(r"explicitly\s+(?:ignored|skipped)", text, re.IGNORECASE): + return [] + return [ + validator_finding( + "reviewer.full_suite_pass_ignored_tests", + "block", + "Validation", + "full suite pass claimed while tests were ignored or skipped without " + "explicit disclosure", + "state which tests were ignored/skipped or downgrade validation verdict", + ) + ] + + +def _rule_blocked_handoff_replay(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + if not _BLOCKED_HANDOFF_RE.search(text): + return [] + if not _REPLAY_COMMAND_RE.search(text): + return [] + return [ + validator_finding( + "reviewer.blocked_handoff_replay", + "block", + "Safe next action", + "blocked recovery handoff includes direct approve or merge replay commands", + "restart the full workflow after the blocker clears; do not replay mutations", + ) + ] + + +def _rule_narrative_handoff_drift(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + narrative_approve = bool(_NARRATIVE_APPROVE_RE.search(text)) + decision_match = _HANDOFF_DECISION_RE.search(text) + if not narrative_approve or not decision_match: + return [] + handoff_decision = decision_match.group(1).strip().lower() + if handoff_decision in {"approve", "approved"}: + return [] + return [ + validator_finding( + "reviewer.narrative_handoff_drift", + "block", + "Review decision", + f"narrative approves PR but controller handoff says '{handoff_decision}'", + "align narrative summary with controller handoff Review decision field", + ) + ] + + +def _rule_review_state_ambiguous(report_text: str) -> list[dict[str, str]]: + text = report_text or "" + findings: list[dict[str, str]] = [] + if _PENDING_RE.search(text) and _APPROVED_RE.search(text): + findings.append( + validator_finding( + "reviewer.pending_vs_approved", + "block", + "Review decision", + "report conflates PENDING review state with APPROVED outcome", + "distinguish official submitted review from pending or ready-to-submit state", + ) + ) + if _DRY_RUN_RE.search(text) and _APPROVED_RE.search(text): + if "dry-run only" not in text.lower(): + findings.append( + validator_finding( + "reviewer.dry_run_vs_submitted", + "block", + "Review mutations", + "dry-run language mixed with official approve/submitted claims", + "mark dry-run explicitly or document the live review mutation proof", + ) + ) + if _FINDING_READY_RE.search(text) and _APPROVED_RE.search(text): + findings.append( + validator_finding( + "reviewer.finding_ready_vs_submitted", + "downgrade", + "Review decision", + "finding-ready wording combined with submitted-approve claims", + "state whether review was submitted live or only prepared for submission", + ) + ) + return findings + + +_SCHEMA_RULES = ( + _rule_legacy_stale_fields, + _rule_reviewed_head_without_validation, + _rule_merged_without_proof, + _rule_issue_closed_without_live_proof, + _rule_full_suite_pass_ignored_tests, + _rule_blocked_handoff_replay, + _rule_narrative_handoff_drift, + _rule_review_state_ambiguous, +) + + +def _merge_validator_results( + base: dict[str, Any], + extra_findings: list[dict[str, str]], +) -> dict[str, Any]: + findings = list(base.get("findings") or []) + extra_findings + blocked = base.get("blocked") or any(f["severity"] == "block" for f in extra_findings) + downgraded = base.get("downgraded") or any( + f["severity"] == "downgrade" for f in extra_findings + ) + grade = base.get("grade", "A") + if blocked: + grade = "blocked" + elif downgraded and grade == "A": + grade = "downgraded" + reasons = [f"{f['rule_id']}: {f['reason']}" for f in findings] + safe_next = base.get("safe_next_action") or "none" + if extra_findings: + safe_next = extra_findings[0].get("safe_next_action", safe_next) + return { + **base, + "grade": grade, + "blocked": blocked, + "downgraded": downgraded, + "findings": findings, + "reasons": reasons, + "safe_next_action": safe_next, + "complete": grade == "A", + "schema_rules_applied": len(_SCHEMA_RULES), + } + + +def assess_review_final_report_schema( + report_text: str, + *, + review_decision_lock: dict | None = None, + linked_issue_lock: dict | None = None, + validation_report: dict | None = None, + action_log: list[dict] | None = None, + mutations_observed: bool = False, + local_edits: bool = False, +) -> dict[str, Any]: + """Validate reviewer final report text before session completion (#391).""" + base = assess_final_report_validator( + report_text, + "review_pr", + review_decision_lock=review_decision_lock, + linked_issue_lock=linked_issue_lock, + validation_report=validation_report, + action_log=action_log, + mutations_observed=mutations_observed, + local_edits=local_edits, + ) + extra: list[dict[str, str]] = [] + for rule in _SCHEMA_RULES: + extra.extend(rule(report_text)) + return _merge_validator_results(base, extra) \ No newline at end of file diff --git a/review_proofs.py b/review_proofs.py index e56185d..4bd04b5 100644 --- a/review_proofs.py +++ b/review_proofs.py @@ -4803,6 +4803,13 @@ def assess_final_report_validator(report_text, task_kind, **kwargs): return _validate(report_text, task_kind, **kwargs) +def assess_review_final_report_schema(report_text, **kwargs): + """#391: reviewer final-report schema verification before session output.""" + from review_final_report_schema import assess_review_final_report_schema as _assess + + return _assess(report_text, **kwargs) + + _QUEUE_STATUS_REPORT_HINT = re.compile( r"queue[- ]status|selected pr:\s*none|no pr selected|" r"queue[- ]status[- ]only", diff --git a/tests/test_llm_workflow_split.py b/tests/test_llm_workflow_split.py index 39c92d0..9058f6b 100644 --- a/tests/test_llm_workflow_split.py +++ b/tests/test_llm_workflow_split.py @@ -136,6 +136,12 @@ def test_final_report_validator_exported(): assert callable(assess_final_report_validator) +def test_review_final_report_schema_verifier_exported(): + from review_proofs import assess_review_final_report_schema + + assert callable(assess_review_final_report_schema) + + def test_create_issue_final_report_verifier_exported(): from review_proofs import assess_create_issue_final_report diff --git a/tests/test_review_final_report_schema.py b/tests/test_review_final_report_schema.py new file mode 100644 index 0000000..fbcb5a2 --- /dev/null +++ b/tests/test_review_final_report_schema.py @@ -0,0 +1,158 @@ +"""Tests for reviewer final-report schema verification (#391).""" +import sys +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from review_final_report_schema import assess_review_final_report_schema # noqa: E402 + + +def _minimal_review_report(**overrides): + lines = [ + "## Controller Handoff", + "", + "- Task: review PR #203", + "- Repo: Scaled-Tech-Consulting/Gitea-Tools", + "- Role: reviewer", + "- Identity: sysadmin / prgs-reviewer", + "- Issue/PR: #182 / PR #203", + "- Branch/SHA: feat/x @ 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Files changed: review_proofs.py", + "- Validation: pytest tests/test_review_proofs.py -q in branches/review-203", + "- Mutations: review only", + "- File edits by reviewer: none", + "- Worktree/index mutations: none", + "- Git ref mutations: git fetch prgs master", + "- MCP/Gitea mutations: review submitted", + "- Review mutations: request_changes submitted", + "- Merge mutations: none", + "- Cleanup mutations: none", + "- External-state mutations: none", + "- Read-only diagnostics: gitea_view_pr, gitea_view_issue", + "- Current status: PR open", + "- Blockers: none", + "- Next: await author", + "- Safety: no self-review; no self-merge", + "- Selected PR: #203", + "- Reviewer eligibility: eligible", + "- Candidate head SHA: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9", + "- Worktree path: branches/review-203", + "- Worktree dirty: clean", + "- Unrelated local mutations: none", + "- Review decision: request_changes", + "- Merge result: not attempted", + "- Linked issue: #182", + "- Linked issue live status: open (gitea_view_issue)", + "- Cleanup status: none", + ] + text = "\n".join(lines) + for key, value in overrides.items(): + prefix = f"- {key}:" + replaced = False + new_lines = [] + for line in text.splitlines(): + if line.strip().startswith(prefix): + new_lines.append(f"{prefix} {value}") + replaced = True + else: + new_lines.append(line) + text = "\n".join(new_lines) + if not replaced: + text += f"\n{prefix} {value}" + return text + + +class TestReviewFinalReportSchema(unittest.TestCase): + def test_clean_report_passes(self): + result = assess_review_final_report_schema( + _minimal_review_report(), + linked_issue_lock={"issue_number": 182, "pr_number": 203}, + ) + self.assertFalse(result["blocked"]) + self.assertIn(result["grade"], ("A", "downgraded")) + + def test_legacy_pinned_reviewed_head_blocks(self): + report = _minimal_review_report(**{"Pinned reviewed head": "abc123"}) + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.legacy_stale_field", rule_ids) + + def test_reviewed_head_without_validation_blocks(self): + report = _minimal_review_report().replace( + "- Validation: pytest tests/test_review_proofs.py -q in branches/review-203", + "- Validation: not run", + ) + report += "\n- Pinned reviewed head: 0fdc8f582026b72a229d59a172c0a63ac4aaeaf9" + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.reviewed_head_without_validation", rule_ids) + + def test_merged_without_proof_blocks(self): + report = _minimal_review_report() + "\n\nPR #203 merged successfully." + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.merged_without_proof", rule_ids) + + def test_issue_closed_without_live_proof_blocks(self): + report = _minimal_review_report( + **{ + "Linked issue live status": "closed", + "Read-only diagnostics": "gitea_view_pr only", + } + ) + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.issue_closed_without_live_proof", rule_ids) + + def test_full_suite_pass_with_ignored_tests_blocks(self): + report = ( + _minimal_review_report() + + "\nFull test suite passed with 0 failed; some tests ignored." + ) + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.full_suite_pass_ignored_tests", rule_ids) + + def test_blocked_handoff_replay_blocks(self): + report = ( + _minimal_review_report(**{"Blockers": "capability stop"}) + + "\nSafe next action: run gitea_merge_pr to finish." + ) + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.blocked_handoff_replay", rule_ids) + + def test_narrative_handoff_drift_blocks(self): + report = ( + _minimal_review_report() + + "\n## Summary\nVerdict: APPROVE this PR." + ) + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.narrative_handoff_drift", rule_ids) + + def test_pending_vs_approved_blocks(self): + report = _minimal_review_report( + **{"Review decision": "PENDING official review submitted approve"} + ) + result = assess_review_final_report_schema(report) + rule_ids = [f["rule_id"] for f in result["findings"]] + self.assertIn("reviewer.pending_vs_approved", rule_ids) + + def test_exported_from_review_proofs(self): + from review_proofs import assess_review_final_report_schema as exported + + self.assertTrue(callable(exported)) + + +class TestMcpValidateReviewFinalReport(unittest.TestCase): + def test_mcp_tool_callable(self): + import gitea_mcp_server + + result = gitea_mcp_server.gitea_validate_review_final_report( + _minimal_review_report() + ) + self.assertIn("grade", result) + self.assertIn("findings", result) + self.assertFalse(result["blocked"]) \ No newline at end of file