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/4] 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/4] 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/4] 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/4] 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")