Merge branch 'fix/issue-546-reviewer-deadlock' into master (bootstrap deadlock bypass)

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)

tests/test_mcp_server.py

@@ -3965,3 +3965,167 @@ 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"])