fix: resolve reviewer preflight capability/lease deadlock and add gitea_release_reviewer_pr_lease tool (closes #546)
This commit is contained in:
+122
-1
@@ -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)
|
||||
|
||||
@@ -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"])
|
||||
|
||||
|
||||
Reference in New Issue
Block a user