From 3f3f8801470d2c12dfcae57e2961b9465eb46978 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Thu, 9 Jul 2026 15:18:48 -0400 Subject: [PATCH] feat: canonical cleanup for stale #332 review decision locks (#594) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Durable review-decision locks (#559) can outlive the PR they protect. When the last terminal mutation references a PR that is already merged/closed, expose gitea_cleanup_stale_review_decision_lock so a reviewer can clear the lock with identity/profile gates and a durable audit trail — without weakening #332 for open PRs or deleting session-state files by hand. Also auto-clear same-profile locks after a successful merge of the approved PR, and document allowed vs forbidden cleanup. Closes #594 --- docs/llm-workflow-runbooks.md | 66 +++ gitea_mcp_server.py | 230 ++++++++++- .../workflows/review-merge-pr.md | 2 + stale_review_decision_lock.py | 252 ++++++++++++ task_capability_map.py | 11 + ...test_stale_review_decision_lock_cleanup.py | 378 ++++++++++++++++++ 6 files changed, 938 insertions(+), 1 deletion(-) create mode 100644 stale_review_decision_lock.py create mode 100644 tests/test_stale_review_decision_lock_cleanup.py diff --git a/docs/llm-workflow-runbooks.md b/docs/llm-workflow-runbooks.md index effb9f7..5fc591b 100644 --- a/docs/llm-workflow-runbooks.md +++ b/docs/llm-workflow-runbooks.md @@ -1049,6 +1049,72 @@ Special states: Final reports must not claim a comment was posted when `canonical_comment_validation.allowed` is false (#496 AC14). +## Stale #332 review-decision lock cleanup (#594) + +#332 hard-stops a reviewer session after a terminal live review mutation +(`approve` / `request_changes`). After #559 those locks are **durable** under +`~/.cache/gitea-tools/session-state/review_decision_lock-.json`, so they +can outlive the PR they protected. + +### When cleanup is **allowed** + +Use `gitea_cleanup_stale_review_decision_lock` from a **reviewer** profile when: + +1. A durable review-decision lock has a terminal live mutation, **and** +2. Live Gitea state shows that terminal PR is already **merged** or **closed** + (so no same-PR merge sequence remains), **and** +3. The active profile identity matches the lock, **and** +4. Authenticated identity can be verified. + +Workflow: + +```text +# 1) Assess only (default) +gitea_cleanup_stale_review_decision_lock(apply=false, remote=prgs, ...) + +# 2) Apply after is_moot=true and cleanup_allowed=true +gitea_cleanup_stale_review_decision_lock( + apply=true, + expected_terminal_pr=, + remote=prgs, + ... +) +``` + +Successful apply: + +* clears in-memory + durable decision lock for that profile +* returns a structured `audit` payload +* posts an audit comment on the mooted PR when `gitea.pr.comment` is allowed + (`post_audit_comment=true` default) + +Same-profile auto-expire: if `gitea_merge_pr` succeeds on a PR that this same +profile just approved, the decision lock is cleared after merge. Cross-profile +locks (reviewer vs merger) still require the cleanup tool. + +### When cleanup is **forbidden** + +Do **not** clear when: + +* the last terminal PR is still **open** (active #332 hard-stop — continue merge + for that approve, or stop after request_changes) +* live PR state cannot be fetched (fail closed) +* profile identity does not match the lock +* identity cannot be verified +* `expected_terminal_pr` does not match the last terminal PR + +**Never** use manual deletion of session-state files as the normal workflow. +**Never** use cleanup to skip review of an open PR or to bypass eligibility / +mark-ready / submit gates on active work. + +### Related tools + +| Tool | Purpose | +|------|---------| +| `gitea_cleanup_stale_review_decision_lock` | Moot decision-lock cleanup (#594) | +| `gitea_authorize_review_correction` | Operator-approved correction after a **mistaken** live review on still-active work (#211) | +| `gitea_cleanup_post_merge_moot_lease` | Moot **lease** cleanup after merge (#515) — different object | + ## Fail-closed behavior Before any mutating action the workflow verifies identity, active profile, diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index dd50942..18b9d03 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -807,6 +807,7 @@ import review_proofs # noqa: E402 import review_workflow_boundary # noqa: E402 import review_workflow_load # noqa: E402 import mcp_session_state # noqa: E402 +import stale_review_decision_lock # noqa: E402 import agent_temp_artifacts import issue_lock_worktree # noqa: E402 import issue_lock_provenance # noqa: E402 @@ -3096,10 +3097,15 @@ def terminal_review_hard_stop_reasons(pr_number: int, operation: str) -> list[st ) else: guidance = "the session must stop and produce a final report" + recovery = ( + "if that PR is already merged/closed, use " + "gitea_cleanup_stale_review_decision_lock (apply=true) after live-state " + "proof (#594); never delete session-state files by hand" + ) return [ "terminal review mutation already consumed in this run " f"({last.get('action')} on PR #{last.get('pr_number')}); {guidance} " - "(fail closed, #332)" + f"(fail closed, #332); {recovery}" ] @@ -3872,6 +3878,205 @@ def gitea_authorize_review_correction( return {"authorized": True, "correction_reason": reason, "reasons": []} +@mcp.tool() +def gitea_cleanup_stale_review_decision_lock( + apply: bool = False, + expected_terminal_pr: int | None = None, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, + post_audit_comment: bool = True, +) -> dict: + """Clear a durable #332 review-decision lock only when it is moot (#594). + + Read-first by default (``apply=False``). A lock is moot when its last + terminal live mutation references a PR that is already **merged** or + **closed** on live Gitea — so no same-PR merge sequence remains. + + Fail-closed when: + - no lock / no terminal mutation + - live PR is still open (active #332 hard-stop preserved) + - live PR state cannot be fetched + - active profile identity does not match the lock + - apply requested without reviewer capability (``gitea.pr.review``) + + Never weakens #332 for open PRs. Never instructs manual session-state + file deletion. On successful apply, clears memory + durable store and + returns a durable audit record (optionally posted as a PR comment). + """ + read_block = _profile_operation_gate("gitea.read") + if read_block: + return { + "success": False, + "cleanup_performed": False, + "is_moot": False, + "cleanup_allowed": False, + "mode": "apply" if apply else "read_only", + "reasons": read_block, + "permission_report": _permission_block_report("gitea.read"), + } + + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + binding = _decision_lock_binding() + active_identity = binding.get("profile_identity") + lock = _load_review_decision_lock() + last = stale_review_decision_lock.last_terminal_mutation(lock) + pr_live = None + pr_lookup_error = None + last_pr = last.get("pr_number") if last else None + + if last_pr is not None: + try: + pr_live = api_request( + "GET", f"{repo_api_url(h, o, r)}/pulls/{last_pr}", auth + ) or {} + if not pr_live: + pr_lookup_error = "empty PR payload" + except Exception as exc: # noqa: BLE001 — surface redacted + pr_lookup_error = _redact(str(exc)) + + assessment = stale_review_decision_lock.assess_stale_review_decision_lock( + lock, + pr_live=pr_live, + pr_lookup_error=pr_lookup_error, + active_profile_identity=active_identity, + ) + + # Optional pin: refuse apply against a different terminal PR than expected. + if ( + expected_terminal_pr is not None + and assessment.get("last_terminal_pr") is not None + and int(expected_terminal_pr) != int(assessment["last_terminal_pr"]) + ): + assessment = dict(assessment) + assessment["cleanup_allowed"] = False + assessment["reasons"] = list(assessment.get("reasons") or []) + [ + f"expected_terminal_pr #{expected_terminal_pr} does not match " + f"last terminal PR #{assessment.get('last_terminal_pr')} " + "(fail closed, #594)" + ] + + try: + actor = _authenticated_username(h) + except Exception: + actor = None + profile = get_profile() + profile_name = (profile.get("profile_name") or "").strip() or None + + audit = stale_review_decision_lock.build_cleanup_audit_record( + assessment=assessment, + actor_username=actor, + profile_name=profile_name, + applied=False, + ) + + report = { + "success": True, + "cleanup_performed": False, + "mode": "apply" if apply else "read_only", + "remote": remote, + "org": o, + "repo": r, + "active_profile_identity": active_identity, + "authenticated_username": actor, + "has_lock": assessment.get("has_lock"), + "is_moot": assessment.get("is_moot"), + "cleanup_allowed": assessment.get("cleanup_allowed"), + "last_terminal_pr": assessment.get("last_terminal_pr"), + "last_terminal_action": assessment.get("last_terminal_action"), + "pr_state": assessment.get("pr_state"), + "pr_merged": assessment.get("pr_merged"), + "pr_merged_or_closed": assessment.get("pr_merged_or_closed"), + "merge_commit_sha": assessment.get("merge_commit_sha"), + "lock_summary": assessment.get("lock_summary"), + "audit": audit, + "audit_comment_id": None, + "reasons": list(assessment.get("reasons") or []), + "manual_file_delete_forbidden": True, + } + + if not apply: + return report + + if not assessment.get("cleanup_allowed"): + report["success"] = False + report["cleanup_skipped_reason"] = list(assessment.get("reasons") or []) + return report + + if not actor: + report["success"] = False + report["reasons"] = [ + "authenticated identity could not be verified; refuse lock " + "cleanup (fail closed, #594)" + ] + return report + + review_block = _profile_operation_gate("gitea.pr.review") + if review_block: + report["success"] = False + report["reasons"] = review_block + report["permission_report"] = _permission_block_report("gitea.pr.review") + return report + + session_reasons = _review_decision_session_reasons(lock) + if session_reasons: + report["success"] = False + report["reasons"] = session_reasons + return report + + # Clear durable + in-memory lock only after all gates pass. + prior_summary = assessment.get("lock_summary") + _save_review_decision_lock(None) + audit = stale_review_decision_lock.build_cleanup_audit_record( + assessment=assessment, + actor_username=actor, + profile_name=profile_name, + applied=True, + ) + audit["prior_lock_summary"] = prior_summary + report["cleanup_performed"] = True + report["audit"] = audit + report["reasons"] = list(assessment.get("reasons") or []) + [ + f"cleared durable review decision lock for moot terminal mutation " + f"on PR #{assessment.get('last_terminal_pr')} (#594)" + ] + + if post_audit_comment and assessment.get("last_terminal_pr") is not None: + comment_block = _profile_operation_gate("gitea.pr.comment") + if comment_block: + report["audit_comment_skipped"] = comment_block + else: + try: + body = stale_review_decision_lock.format_cleanup_audit_comment( + audit + ) + comment_url = ( + f"{repo_api_url(h, o, r)}/issues/" + f"{assessment['last_terminal_pr']}/comments" + ) + with _audited( + "comment_pr", + host=h, + remote=remote, + org=o, + repo=r, + pr_number=assessment["last_terminal_pr"], + request_metadata={ + "source": "cleanup_stale_review_decision_lock" + }, + ): + posted = api_request( + "POST", comment_url, auth, {"body": body} + ) + report["audit_comment_id"] = (posted or {}).get("id") + except Exception as exc: # noqa: BLE001 + report["audit_comment_error"] = _redact(str(exc)) + + return report + + @mcp.tool() def gitea_dry_run_pr_review( pr_number: int, @@ -4609,6 +4814,29 @@ def gitea_merge_pr( reviewer_pr_lease.get_session_lease() ) ) + # Same-profile auto-expire (#594): if *this* profile's decision lock ends + # with an approve of the PR just merged, clear it so durable state cannot + # block later unrelated reviews. Cross-profile locks (e.g. reviewer vs + # merger) still require gitea_cleanup_stale_review_decision_lock. + try: + lock_after = _load_review_decision_lock() + last_term = stale_review_decision_lock.last_terminal_mutation(lock_after) + if ( + last_term + and last_term.get("action") == "approve" + and last_term.get("pr_number") == pr_number + ): + _save_review_decision_lock(None) + result["review_decision_lock_cleared_after_merge"] = True + reasons.append( + f"cleared same-profile review decision lock after merge of " + f"approved PR #{pr_number} (#594)" + ) + else: + result["review_decision_lock_cleared_after_merge"] = False + except Exception as clear_exc: # noqa: BLE001 — never fail the merge + result["review_decision_lock_cleared_after_merge"] = False + result["review_decision_lock_clear_error"] = _redact(str(clear_exc)) reasons.append(f"all gates passed; merged PR #{pr_number} via '{do}'") return result diff --git a/skills/llm-project-workflow/workflows/review-merge-pr.md b/skills/llm-project-workflow/workflows/review-merge-pr.md index 6c35ce9..d27c961 100644 --- a/skills/llm-project-workflow/workflows/review-merge-pr.md +++ b/skills/llm-project-workflow/workflows/review-merge-pr.md @@ -1043,6 +1043,8 @@ Blocked handoffs must say to rerun the full workflow after the blocker clears. If the blocker is a terminal review mutation already consumed in the current run, the handoff must say that the next run must start from the beginning and must not reuse stale ready/approved state. +If the referenced terminal PR is already **merged or closed** on live Gitea, the durable #332 decision lock is **moot**. Do **not** delete session-state files by hand. Use `gitea_cleanup_stale_review_decision_lock` (assess with `apply=false`, then `apply=true` with `expected_terminal_pr` set) from the reviewer profile after identity/profile checks (#594). Cleanup is **forbidden** while that PR is still open. + ## 30. Final report must be precise Include: diff --git a/stale_review_decision_lock.py b/stale_review_decision_lock.py new file mode 100644 index 0000000..28b6fc9 --- /dev/null +++ b/stale_review_decision_lock.py @@ -0,0 +1,252 @@ +"""Stale / moot #332 review-decision lock detection and cleanup policy (#594). + +#332 correctly hard-stops a reviewer session after a terminal live review +mutation. After #559 those locks are durable on disk, so they can outlive the +work they protect: when the referenced PR is already merged or closed, no +same-PR merge sequence remains, yet the durable ledger still blocks unrelated +new terminal reviews. + +This module is pure policy (no Gitea I/O). The MCP tool +``gitea_cleanup_stale_review_decision_lock`` fetches live PR state, calls these +helpers, and only then clears durable state when cleanup is allowed. + +Manual deletion of ``~/.cache/gitea-tools/session-state/*`` is never the +canonical path. +""" + +from __future__ import annotations + +from datetime import datetime, timezone +from typing import Any + +# Keep in sync with gitea_mcp_server._TERMINAL_REVIEW_ACTIONS. +TERMINAL_REVIEW_ACTIONS = frozenset({"approve", "request_changes"}) + + +def last_terminal_mutation(lock: dict | None) -> dict | None: + """Return the last terminal live mutation on *lock*, or None.""" + if not lock: + return None + terminals = [ + m + for m in (lock.get("live_mutations") or []) + if isinstance(m, dict) and m.get("action") in TERMINAL_REVIEW_ACTIONS + ] + return terminals[-1] if terminals else None + + +def classify_pr_live_state(pr_live: dict | None) -> dict[str, Any]: + """Normalize a Gitea PR payload into merge/closed flags.""" + pr = pr_live or {} + merged = bool(pr.get("merged") or pr.get("merged_at")) + state = (pr.get("state") or "").strip().lower() or None + closed = state == "closed" + return { + "pr_state": state, + "pr_merged": merged, + "pr_merged_or_closed": merged or closed, + "merge_commit_sha": pr.get("merge_commit_sha") + or pr.get("merged_commit_sha"), + } + + +def lock_summary(lock: dict | None) -> dict[str, Any] | None: + """LLM-safe summary of a decision lock (no secrets).""" + if lock is None: + return None + last = last_terminal_mutation(lock) + mutations = list(lock.get("live_mutations") or []) + return { + "task": lock.get("task"), + "remote": lock.get("remote"), + "org": lock.get("org") or lock.get("ready_org"), + "repo": lock.get("repo") or lock.get("ready_repo"), + "profile_identity": lock.get("profile_identity") + or lock.get("session_profile_lock") + or lock.get("session_profile"), + "session_profile": lock.get("session_profile"), + "ready_pr_number": lock.get("ready_pr_number"), + "ready_action": lock.get("ready_action"), + "live_mutations_count": len(mutations), + "last_terminal": ( + { + "pr_number": last.get("pr_number"), + "action": last.get("action"), + "review_id": last.get("review_id"), + } + if last + else None + ), + "correction_authorized": bool(lock.get("correction_authorized")), + "updated_at": lock.get("updated_at") or lock.get("recorded_at"), + } + + +def assess_stale_review_decision_lock( + lock: dict | None, + *, + pr_live: dict | None = None, + pr_lookup_error: str | None = None, + active_profile_identity: str | None = None, +) -> dict[str, Any]: + """Assess whether a durable review-decision lock is stale/moot (#594). + + *pr_live* must be the live Gitea payload for the **last terminal + mutation's PR**. When no lock / no terminal mutation exists, cleanup is + not applicable. When the PR is still open (or lookup fails), cleanup is + forbidden and #332 hard-stop remains in force. + """ + summary = lock_summary(lock) + result: dict[str, Any] = { + "has_lock": lock is not None, + "lock_summary": summary, + "last_terminal_pr": None, + "last_terminal_action": None, + "is_moot": False, + "cleanup_allowed": False, + "profile_match": True, + "pr_state": None, + "pr_merged": False, + "pr_merged_or_closed": False, + "merge_commit_sha": None, + "reasons": [], + "recovery_tool": "gitea_cleanup_stale_review_decision_lock", + } + + if lock is None: + result["reasons"].append("no review decision lock present") + return result + + # Profile identity gate (caller may re-check with env; pure assess still + # reports mismatch when active identity is supplied). + stored = ( + (lock.get("profile_identity") or lock.get("session_profile_lock") or "") + .strip() + ) + active = (active_profile_identity or "").strip() + if active and stored and active != stored: + result["profile_match"] = False + result["reasons"].append( + "review decision lock profile identity does not match active " + f"profile '{active}' (fail closed, #594)" + ) + return result + + last = last_terminal_mutation(lock) + if last is None: + result["reasons"].append( + "review decision lock has no terminal live mutations; nothing to clear" + ) + return result + + pr_number = last.get("pr_number") + action = last.get("action") + result["last_terminal_pr"] = pr_number + result["last_terminal_action"] = action + + if pr_number is None: + result["reasons"].append( + "last terminal mutation missing pr_number; refuse cleanup (fail closed)" + ) + return result + + if pr_lookup_error: + result["reasons"].append( + f"could not load live state for PR #{pr_number}: {pr_lookup_error} " + "(fail closed, #594)" + ) + return result + + if pr_live is None: + result["reasons"].append( + f"live PR state required for terminal PR #{pr_number} before cleanup " + "(fail closed, #594)" + ) + return result + + live = classify_pr_live_state(pr_live) + result.update( + { + "pr_state": live["pr_state"], + "pr_merged": live["pr_merged"], + "pr_merged_or_closed": live["pr_merged_or_closed"], + "merge_commit_sha": live["merge_commit_sha"], + } + ) + + if not live["pr_merged_or_closed"]: + result["reasons"].append( + f"terminal review mutation on open PR #{pr_number} is still active " + f"({action}); #332 hard-stop remains — cleanup forbidden (#594)" + ) + return result + + # Merged or closed: lock is moot. Same-PR merge continuation is impossible. + result["is_moot"] = True + result["cleanup_allowed"] = True + result["reasons"].append( + f"terminal mutation ({action} on PR #{pr_number}) is moot: PR is " + f"{'merged' if live['pr_merged'] else 'closed'}; canonical cleanup allowed (#594)" + ) + return result + + +def build_cleanup_audit_record( + *, + assessment: dict[str, Any], + actor_username: str | None, + profile_name: str | None, + applied: bool, +) -> dict[str, Any]: + """Structured durable audit payload for a cleanup attempt.""" + last = (assessment.get("lock_summary") or {}).get("last_terminal") or {} + return { + "event": "stale_review_decision_lock_cleanup", + "issue_ref": "#594", + "applied": bool(applied), + "timestamp": datetime.now(timezone.utc).isoformat(), + "actor_username": actor_username, + "profile_name": profile_name, + "last_terminal_pr": assessment.get("last_terminal_pr") or last.get("pr_number"), + "last_terminal_action": assessment.get("last_terminal_action") + or last.get("action"), + "pr_state": assessment.get("pr_state"), + "pr_merged": assessment.get("pr_merged"), + "pr_merged_or_closed": assessment.get("pr_merged_or_closed"), + "merge_commit_sha": assessment.get("merge_commit_sha"), + "prior_live_mutations_count": ( + (assessment.get("lock_summary") or {}).get("live_mutations_count") + ), + "prior_profile_identity": ( + (assessment.get("lock_summary") or {}).get("profile_identity") + ), + "cleanup_allowed": assessment.get("cleanup_allowed"), + "is_moot": assessment.get("is_moot"), + "reasons": list(assessment.get("reasons") or []), + } + + +def format_cleanup_audit_comment(audit: dict[str, Any]) -> str: + """Markdown body for a durable Gitea audit comment after cleanup.""" + status = "APPLIED" if audit.get("applied") else "ASSESSED (not applied)" + lines = [ + "## Stale #332 review-decision lock cleanup (#594)", + "", + f"Status: **{status}**", + "", + f"- actor: `{audit.get('actor_username')}`", + f"- profile: `{audit.get('profile_name')}`", + f"- timestamp: `{audit.get('timestamp')}`", + f"- last terminal: `{audit.get('last_terminal_action')}` on " + f"PR #{audit.get('last_terminal_pr')}", + f"- PR state: `{audit.get('pr_state')}` " + f"(merged={audit.get('pr_merged')})", + f"- merge_commit_sha: `{audit.get('merge_commit_sha')}`", + f"- prior live_mutations_count: " + f"`{audit.get('prior_live_mutations_count')}`", + f"- prior profile_identity: `{audit.get('prior_profile_identity')}`", + "", + "Manual deletion of session-state files is **not** the workflow.", + "This path only clears a lock when the referenced PR is merged/closed.", + ] + return "\n".join(lines) diff --git a/task_capability_map.py b/task_capability_map.py index 14f3c70..3842c2b 100644 --- a/task_capability_map.py +++ b/task_capability_map.py @@ -96,6 +96,17 @@ TASK_CAPABILITY_MAP: dict[str, dict[str, str]] = { "permission": "gitea.pr.approve", "role": "reviewer", }, + # #594: clear durable #332 decision lock only when last terminal PR is + # already merged/closed (moot). Apply path requires reviewer review + # permission; assessment itself uses gitea.read inside the tool. + "cleanup_stale_review_decision_lock": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, + "gitea_cleanup_stale_review_decision_lock": { + "permission": "gitea.pr.review", + "role": "reviewer", + }, "delete_branch": { "permission": "gitea.branch.delete", "role": "author", diff --git a/tests/test_stale_review_decision_lock_cleanup.py b/tests/test_stale_review_decision_lock_cleanup.py new file mode 100644 index 0000000..1425644 --- /dev/null +++ b/tests/test_stale_review_decision_lock_cleanup.py @@ -0,0 +1,378 @@ +"""Stale #332 review-decision lock cleanup (#594). + +Proves: + a. active terminal locks still block unrelated terminal reviews + b. merged/closed PR locks can be cleared canonically + c. cleanup cannot bypass review gates on open PRs + d. after moot cleanup, mark_ready for a different PR can proceed + (PR #592-style after PR #586-style stale approve) +""" + +from __future__ import annotations + +import os +import unittest +from unittest.mock import patch + +import mcp_server +import stale_review_decision_lock as srdl +from mcp_server import ( + gitea_cleanup_stale_review_decision_lock, + gitea_mark_final_review_decision, +) + +HEAD = "a" * 40 +FAKE_AUTH = "Basic dGVzdDp0ZXN0" + +REVIEWER_ENV = { + "GITEA_PROFILE_NAME": "prgs-reviewer", + "GITEA_ALLOWED_OPERATIONS": ( + "gitea.read,gitea.pr.review,gitea.pr.approve," + "gitea.pr.request_changes,gitea.pr.comment" + ), + "GITEA_SESSION_PROFILE_LOCK": "prgs-reviewer", +} + +REVIEWER_PROFILE = { + "profile_name": "prgs-reviewer", + "allowed_operations": [ + "gitea.read", + "gitea.pr.review", + "gitea.pr.approve", + "gitea.pr.request_changes", + "gitea.pr.comment", + ], + "forbidden_operations": [ + "gitea.pr.merge", + "gitea.pr.create", + "gitea.branch.push", + ], +} + + +def _reviewer_profile_patch(): + return patch.object(mcp_server, "get_profile", return_value=REVIEWER_PROFILE) + + +def _lock(mutations=None, correction=False): + return { + "task": "review_pr", + "remote": "prgs", + "org": "Scaled-Tech-Consulting", + "repo": "Gitea-Tools", + "session_pid": os.getpid(), + "session_profile": "prgs-reviewer", + "session_profile_lock": "prgs-reviewer", + "profile_identity": "prgs-reviewer", + "final_review_decision_ready": False, + "ready_pr_number": None, + "ready_action": None, + "ready_expected_head_sha": None, + "ready_remote": None, + "ready_org": None, + "ready_repo": None, + "live_mutations": list(mutations or []), + "correction_authorized": correction, + "correction_reason": None, + } + + +APPROVED_586 = { + "pr_number": 586, + "action": "approve", + "review_id": 395, + "review_state": "approve", +} +APPROVED_OPEN = { + "pr_number": 700, + "action": "approve", + "review_id": 1, + "review_state": "approve", +} +RC_OPEN = { + "pr_number": 701, + "action": "request_changes", + "review_id": 2, + "review_state": "request_changes", +} + + +def _seed(mutations=None, correction=False): + import review_workflow_load + + review_workflow_load.record_review_workflow_load(mcp_server.PROJECT_ROOT) + mcp_server._save_review_decision_lock(_lock(mutations, correction)) + mcp_server.gitea_load_review_workflow() + + +def _merged_pr(pr_number=586, sha="6913ac9" + "0" * 33): + return { + "number": pr_number, + "state": "closed", + "merged": True, + "merged_at": "2026-07-09T18:20:03Z", + "merge_commit_sha": sha, + } + + +def _open_pr(pr_number=700): + return { + "number": pr_number, + "state": "open", + "merged": False, + "merged_at": None, + "merge_commit_sha": None, + } + + +# --------------------------------------------------------------------------- # +# Pure assessment +# --------------------------------------------------------------------------- # +class TestAssessStaleLock(unittest.TestCase): + def test_no_lock(self): + a = srdl.assess_stale_review_decision_lock(None) + self.assertFalse(a["cleanup_allowed"]) + self.assertFalse(a["is_moot"]) + + def test_no_terminal_mutations(self): + a = srdl.assess_stale_review_decision_lock(_lock([])) + self.assertFalse(a["cleanup_allowed"]) + + def test_open_pr_not_cleanable(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_OPEN]), pr_live=_open_pr(700) + ) + self.assertFalse(a["is_moot"]) + self.assertFalse(a["cleanup_allowed"]) + self.assertTrue(any("open PR" in r for r in a["reasons"])) + + def test_merged_pr_is_moot_and_cleanable(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_586]), pr_live=_merged_pr(586) + ) + self.assertTrue(a["is_moot"]) + self.assertTrue(a["cleanup_allowed"]) + self.assertEqual(a["last_terminal_pr"], 586) + + def test_closed_unmerged_is_moot(self): + a = srdl.assess_stale_review_decision_lock( + _lock([RC_OPEN]), + pr_live={"number": 701, "state": "closed", "merged": False}, + ) + self.assertTrue(a["is_moot"]) + self.assertTrue(a["cleanup_allowed"]) + + def test_profile_mismatch_blocks(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_586]), + pr_live=_merged_pr(586), + active_profile_identity="other-profile", + ) + self.assertFalse(a["cleanup_allowed"]) + self.assertFalse(a["profile_match"]) + + def test_lookup_error_fail_closed(self): + a = srdl.assess_stale_review_decision_lock( + _lock([APPROVED_586]), + pr_lookup_error="timeout", + ) + self.assertFalse(a["cleanup_allowed"]) + self.assertTrue(any("timeout" in r for r in a["reasons"])) + + +# --------------------------------------------------------------------------- # +# Hard-stop still blocks active locks +# --------------------------------------------------------------------------- # +class TestActiveHardStopPreserved(unittest.TestCase): + def tearDown(self): + mcp_server._save_review_decision_lock(None) + mcp_server.review_workflow_load.clear_review_workflow_load() + + def test_open_approve_still_blocks_other_pr_mark_ready(self): + _seed([APPROVED_OPEN]) + reasons = mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready") + self.assertTrue(reasons) + self.assertIn("#332", reasons[0]) + self.assertIn("gitea_cleanup_stale_review_decision_lock", reasons[0]) + + def test_request_changes_still_blocks_everything(self): + _seed([RC_OPEN]) + for op in ("merge", "mark_ready", "review"): + self.assertTrue( + mcp_server.terminal_review_hard_stop_reasons(592, op), + msg=op, + ) + + +# --------------------------------------------------------------------------- # +# MCP cleanup tool +# --------------------------------------------------------------------------- # +class TestCleanupTool(unittest.TestCase): + def setUp(self): + self._env = patch.dict(os.environ, REVIEWER_ENV, clear=False) + self._env.start() + + def tearDown(self): + self._env.stop() + mcp_server._save_review_decision_lock(None) + mcp_server.review_workflow_load.clear_review_workflow_load() + + def test_read_only_reports_moot_without_clearing(self): + _seed([APPROVED_586]) + with patch.object(mcp_server, "api_request", return_value=_merged_pr()), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]): + r = gitea_cleanup_stale_review_decision_lock( + apply=False, remote="prgs", + org="Scaled-Tech-Consulting", repo="Gitea-Tools", + ) + self.assertTrue(r["success"]) + self.assertTrue(r["is_moot"]) + self.assertTrue(r["cleanup_allowed"]) + self.assertFalse(r["cleanup_performed"]) + self.assertIsNotNone(mcp_server._load_review_decision_lock()) + + def test_apply_clears_moot_lock(self): + _seed([APPROVED_586]) + posts = [] + + def _api(method, url, auth, payload=None): + if method == "GET" and "/pulls/586" in url: + return _merged_pr() + if method == "POST" and "/comments" in url: + posts.append(payload) + return {"id": 9999} + return {} + + with patch.object(mcp_server, "api_request", side_effect=_api), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]), \ + patch.object( + mcp_server, "_audited", + side_effect=lambda *a, **k: __import__( + "contextlib" + ).nullcontext(), + ): + r = gitea_cleanup_stale_review_decision_lock( + apply=True, + expected_terminal_pr=586, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(r["cleanup_performed"], r) + self.assertTrue(r["audit"]["applied"]) + self.assertIsNone(mcp_server._load_review_decision_lock()) + self.assertEqual(r.get("audit_comment_id"), 9999) + self.assertTrue(posts) + + def test_apply_refuses_open_pr(self): + _seed([APPROVED_OPEN]) + with patch.object(mcp_server, "api_request", return_value=_open_pr(700)), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]): + r = gitea_cleanup_stale_review_decision_lock( + apply=True, remote="prgs", + org="Scaled-Tech-Consulting", repo="Gitea-Tools", + ) + self.assertFalse(r["cleanup_performed"]) + self.assertFalse(r["cleanup_allowed"]) + self.assertIsNotNone(mcp_server._load_review_decision_lock()) + + def test_expected_terminal_pr_pin(self): + _seed([APPROVED_586]) + with patch.object(mcp_server, "api_request", return_value=_merged_pr()), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]): + r = gitea_cleanup_stale_review_decision_lock( + apply=True, + expected_terminal_pr=999, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertFalse(r["cleanup_performed"]) + self.assertTrue(any("expected_terminal_pr" in x for x in r["reasons"])) + + def test_after_moot_cleanup_other_pr_mark_ready_unblocked(self): + """PR #592-style: after clearing merged #586 lock, new mark_ready works.""" + _seed([APPROVED_586]) + with patch.object(mcp_server, "api_request", return_value=_merged_pr()), \ + patch.object(mcp_server, "_auth", return_value=FAKE_AUTH), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ), \ + _reviewer_profile_patch(), \ + patch.object(mcp_server, "_profile_operation_gate", return_value=[]), \ + patch.object( + mcp_server, "_audited", + side_effect=lambda *a, **k: __import__( + "contextlib" + ).nullcontext(), + ): + cleaned = gitea_cleanup_stale_review_decision_lock( + apply=True, + expected_terminal_pr=586, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + post_audit_comment=False, + ) + self.assertTrue(cleaned["cleanup_performed"], cleaned) + + # Hard-stop gone; re-init clean lock like a fresh review_pr resolve. + mcp_server.init_review_decision_lock(remote="prgs", task="review_pr") + mcp_server.gitea_load_review_workflow() + self.assertEqual( + mcp_server.terminal_review_hard_stop_reasons(592, "mark_ready"), + [], + ) + + no_lease = {"block": False, "reasons": [], "mutation_allowed": True} + with patch.object(mcp_server, "_list_pr_lease_comments", return_value=[]), \ + patch.object( + mcp_server, "_pr_work_lease_reviewer_block", return_value=no_lease + ), \ + patch.object( + mcp_server, + "gitea_check_pr_eligibility", + return_value={ + "eligible": True, + "reasons": [], + "authenticated_user": "sysadmin", + "pr_author": "jcwalker3", + "self_author": False, + }, + ), \ + patch.object( + mcp_server, "_authenticated_username", return_value="sysadmin" + ): + marked = gitea_mark_final_review_decision( + pr_number=592, + action="approve", + expected_head_sha=HEAD, + remote="prgs", + org="Scaled-Tech-Consulting", + repo="Gitea-Tools", + ) + self.assertTrue(marked.get("marked_ready"), marked) + + +if __name__ == "__main__": + unittest.main()