From dfbee45309abf42c107a686e041996f9aee34950 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 15:29:18 -0400 Subject: [PATCH 1/2] fix: resolve reviewer preflight capability/lease deadlock and add gitea_release_reviewer_pr_lease tool (closes #546) --- gitea_mcp_server.py | 123 ++++++++++++++++++++++++++++- tests/test_mcp_server.py | 165 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 287 insertions(+), 1 deletion(-) diff --git a/gitea_mcp_server.py b/gitea_mcp_server.py index 3aa0229..5c7446d 100644 --- a/gitea_mcp_server.py +++ b/gitea_mcp_server.py @@ -2409,10 +2409,18 @@ def _review_decision_session_reasons(lock: dict | None) -> list[str]: return reasons -def init_review_decision_lock(remote: str | None, task: str | None): +def init_review_decision_lock(remote: str | None, task: str | None, force: bool = True): """Seed read-only-until-ready state for reviewer PR review tasks.""" if task != "review_pr": return + if not force: + lock = _load_review_decision_lock() + if lock is not None: + if lock.get("remote") == remote and lock.get("session_pid") == os.getpid(): + env_lock = (os.environ.get(SESSION_PROFILE_LOCK_ENV) or "").strip() + stored_lock = (lock.get("session_profile_lock") or "").strip() + if not env_lock or not stored_lock or env_lock == stored_lock: + return review_workflow_load.clear_review_workflow_load() profile = get_profile() profile_name = (profile.get("profile_name") or "").strip() @@ -5939,6 +5947,118 @@ def gitea_cleanup_post_merge_moot_lease( return report +@mcp.tool() +def gitea_release_reviewer_pr_lease( + pr_number: int, + worktree: str | None = None, + remote: str = "dadeschools", + host: str | None = None, + org: str | None = None, + repo: str | None = None, +) -> dict: + """Safely release an active reviewer lease owned by the current session on an open PR. + + This provides a canonical way to clean up reviewer leases when a review fails + before submission. + + Args: + pr_number: The PR number whose reviewer lease to release. + worktree: Optional worktree path (resolves automatically if not supplied). + remote: Known instance — 'dadeschools' or 'prgs'. + host: Override the Gitea host. + org: Override the owner/organization. + repo: Override the repository name. + + Returns: + dict reporting release status. + """ + _verify_role_mutation_workspace( + remote, worktree=worktree, task="review_pr" + ) + h, o, r = _resolve(remote, host, org, repo) + auth = _auth(h) + + comments = _fetch_pr_comments( + pr_number, remote=remote, host=host, org=org, repo=repo + ) + active = reviewer_pr_lease.find_active_reviewer_lease( + comments, pr_number=pr_number + ) + + if not active: + return { + "success": True, + "released": False, + "reasons": ["no active reviewer lease found on PR"], + } + + identity = _authenticated_username(remote) or "" + session = reviewer_pr_lease.get_session_lease() or {} + session_id = session.get("session_id") + + owner_identity = active.get("reviewer_identity") + owner_session_id = active.get("session_id") + + # Authorize release: identity matches, session_id matches, or lease is expired/reclaimable + authorized = False + reasons = [] + if owner_identity and identity and owner_identity == identity: + authorized = True + elif owner_session_id and session_id and owner_session_id == session_id: + authorized = True + else: + freshness = reviewer_pr_lease.classify_lease_freshness(active) + if freshness in {"expired", "reclaimable"}: + authorized = True + + if not authorized: + return { + "success": False, + "released": False, + "reasons": [ + f"unauthorized to release active lease: owned by {owner_identity} " + f"(session {owner_session_id})" + ], + } + + # Format and post release comment + body = reviewer_pr_lease.format_lease_body( + repo=active.get("repo") or f"{o}/{r}", + pr_number=pr_number, + issue_number=active.get("issue_number"), + reviewer_identity=owner_identity or identity, + profile=active.get("profile") or "reviewer", + session_id=owner_session_id or session_id or "unknown", + worktree=active.get("worktree") or "", + phase="released", + candidate_head=active.get("candidate_head"), + target_branch=active.get("target_branch") or "master", + target_branch_sha=active.get("target_branch_sha"), + blocker="manual-release", + ) + + comment_url = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments" + with _audited( + "comment_pr", + host=h, + remote=remote, + org=o, + repo=r, + pr_number=pr_number, + request_metadata={"source": "release_reviewer_pr_lease"}, + ): + posted = api_request("POST", comment_url, auth, {"body": body}) + + reviewer_pr_lease.clear_session_lease() + + return { + "success": True, + "released": True, + "comment_id": posted.get("id"), + "reasons": [], + } + + @mcp.tool() def gitea_list_issue_comments( issue_number: int, @@ -8491,6 +8611,7 @@ def gitea_resolve_task_capability( init_review_decision_lock( remote if remote in REMOTES else None, task, + force=False, ) record_mutation_authority(profile["profile_name"], username, remote if remote in REMOTES else None, task) diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 77e028a..fff2ade 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -3965,3 +3965,168 @@ class TestPreflightVerification(unittest.TestCase): self.assertIn("worktree_path argument", msg) self.assertIn("task_file.py", msg) self.assertIn("author namespace", msg) + + +class TestIssue546Deadlock(unittest.TestCase): + def setUp(self): + import reviewer_pr_lease + import review_workflow_load + import mcp_server + reviewer_pr_lease.clear_session_lease() + review_workflow_load.clear_review_workflow_load() + mcp_server._preflight_capability_called = False + mcp_server._preflight_resolved_role = None + mcp_server._REVIEW_DECISION_LOCK = None + + self.auth_user_patch = patch("mcp_server._authenticated_username", return_value="reviewer-bot") + self.auth_user_patch.start() + self.addCleanup(self.auth_user_patch.stop) + + self.verify_purity_patch = patch("mcp_server.verify_preflight_purity", return_value=None) + self.verify_purity_patch.start() + self.addCleanup(self.verify_purity_patch.stop) + + self.verify_workspace_patch = patch("mcp_server._verify_role_mutation_workspace", return_value="/workspace") + self.verify_workspace_patch.start() + self.addCleanup(self.verify_workspace_patch.stop) + + self.get_profile_patch = patch("mcp_server.get_profile", return_value={ + "profile_name": "prgs-reviewer", + "allowed_operations": ["gitea.read", "gitea.pr.comment", "gitea.pr.review", "gitea.pr.approve"], + "forbidden_operations": [], + }) + self.get_profile_patch.start() + self.addCleanup(self.get_profile_patch.stop) + + self.pr_work_lease_patch = patch( + "mcp_server._pr_work_lease_reviewer_block", + return_value=dict(_NO_PR_WORK_LEASE_BLOCK), + ) + self.pr_work_lease_patch.start() + self.addCleanup(self.pr_work_lease_patch.stop) + + self.auth_header_patch = patch("mcp_server.get_auth_header", return_value="token test") + self.auth_header_patch.start() + self.addCleanup(self.auth_header_patch.stop) + + def test_workflow_no_deadlock(self): + import mcp_server + import reviewer_pr_lease + + # 1. Resolve capability + res = mcp_server.gitea_resolve_task_capability(task="review_pr", remote="prgs") + self.assertTrue(res["allowed_in_current_session"]) + + # 2. Load review workflow + res = mcp_server.gitea_load_review_workflow() + self.assertTrue(res["success"]) + + # Mock API requests for lease comments and PR retrieval + comments = [] + def mock_api_side(method, url, auth, data=None): + if "/api/v1/user" in url: + return {"login": "reviewer-bot"} + if "/pulls/" in url: + if "/reviews" in url: + if method == "POST": + return {"id": 2001, "state": "APPROVED"} + return [{"id": 2001, "user": {"login": "reviewer-bot"}, "state": "APPROVED", "commit_id": "abc123"}] + return {"user": {"login": "author-user"}, "state": "open", "head": {"sha": "abc123"}, "mergeable": True} + if "/comments" in url: + if method == "POST": + comments.append({"id": 1001, "body": data["body"], "user": {"login": "reviewer-bot"}}) + return {"id": 1001} + return comments + return {} + + with patch("mcp_server.api_request", side_effect=mock_api_side): + # 3. Acquire reviewer lease + res = mcp_server.gitea_acquire_reviewer_pr_lease( + pr_number=550, + worktree="/workspace", + candidate_head="abc123", + remote="prgs", + ) + self.assertTrue(res["success"]) + self.assertEqual(res["comment_id"], 1001) + + # verify lease is recorded in session + session_lease = reviewer_pr_lease.get_session_lease() + self.assertIsNotNone(session_lease) + self.assertEqual(session_lease["pr_number"], 550) + + # 4. Resolve capability again (simulating subsequent tool call preflight check/token refresh) + # This should NOT clear the session lease or decision lock! + mcp_server._preflight_capability_called = False # simulate clearance from prior mutation + res = mcp_server.gitea_resolve_task_capability(task="review_pr", remote="prgs") + self.assertTrue(res["allowed_in_current_session"]) + self.assertIsNotNone(reviewer_pr_lease.get_session_lease()) # Preserved! + + # 5. Mark final review decision APPROVED + res = mcp_server.gitea_mark_final_review_decision( + pr_number=550, + action="approve", + expected_head_sha="abc123", + remote="prgs", + ) + self.assertTrue(res["marked_ready"]) + + # 6. Submit review (should succeed without deadlock) + res = mcp_server.gitea_submit_pr_review( + pr_number=550, + action="approve", + body="APPROVED", + expected_head_sha="abc123", + final_review_decision_ready=True, + remote="prgs", + worktree_path="/workspace", + ) + self.assertTrue(res["performed"]) + + def test_release_reviewer_pr_lease(self): + import mcp_server + import reviewer_pr_lease + + # Resolve capability and load workflow + mcp_server.gitea_resolve_task_capability(task="review_pr", remote="prgs") + mcp_server.gitea_load_review_workflow() + + comments = [_reviewer_lease_comment(550, session_id=_DEFAULT_LEASE_SESSION, head_sha="abc123")] + posted_comments = [] + + def mock_api_side(method, url, auth, data=None): + if "/api/v1/user" in url: + return {"login": "reviewer-bot"} + if "/pulls/" in url: + return {"user": {"login": "author-user"}, "state": "open", "head": {"sha": "abc123"}, "mergeable": True} + if "/comments" in url: + if method == "POST": + new_c = {"id": 1002, "body": data["body"], "user": {"login": "reviewer-bot"}} + comments.append(new_c) + posted_comments.append(new_c) + return {"id": 1002} + return comments + return {} + + # Set session lease in memory + import merger_lease_adoption as mla + reviewer_pr_lease.record_session_lease({ + "pr_number": 550, + "session_id": _DEFAULT_LEASE_SESSION, + "candidate_head": "abc123", + "target_branch": "master", + "comment_id": 9001, + }, lease_provenance=mla.build_lease_provenance(source=mla.SOURCE_ACQUIRE, comment_id=9001)) + + with patch("mcp_server.api_request", side_effect=mock_api_side): + # Release lease + res = mcp_server.gitea_release_reviewer_pr_lease(pr_number=550, remote="prgs") + self.assertTrue(res["success"]) + self.assertTrue(res["released"]) + self.assertEqual(res["comment_id"], 1002) + + # verify lease is cleared in session + self.assertIsNone(reviewer_pr_lease.get_session_lease()) + # verify comment phase is released + self.assertIn("phase: released", posted_comments[0]["body"]) + From 535da1bc5afb273296275bc6cf470ce5f4380232 Mon Sep 17 00:00:00 2001 From: Jason Walker <913443@dadeschools.net> Date: Wed, 8 Jul 2026 15:40:12 -0400 Subject: [PATCH 2/2] chore: remove trailing extra blank line at EOF in test_mcp_server.py --- tests/test_mcp_server.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index fff2ade..503e879 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -4129,4 +4129,3 @@ class TestIssue546Deadlock(unittest.TestCase): self.assertIsNone(reviewer_pr_lease.get_session_lease()) # verify comment phase is released self.assertIn("phase: released", posted_comments[0]["body"]) -