fix: resolve reviewer lease identity via host not remote alias (Closes #578) #581

Merged
sysadmin merged 2 commits from fix/issue-578-reviewer-lease-identity into master 2026-07-09 11:21:30 -05:00
2 changed files with 243 additions and 4 deletions
+10 -4
View File
@@ -6153,7 +6153,9 @@ def _reviewer_pr_lease_gate(
"""Return block reasons when the session lacks an owned PR reviewer lease."""
session = reviewer_pr_lease.get_session_lease()
session_id = (session or {}).get("session_id")
identity = _authenticated_username(remote) or ""
# Resolve host first: _authenticated_username expects a host, not remote name.
h, _, _ = _resolve(remote, host, org, repo)
identity = _authenticated_username(h) or ""
try:
comments = _fetch_pr_comments(
pr_number, remote=remote, host=host, org=org, repo=repo)
@@ -6207,7 +6209,8 @@ def gitea_acquire_reviewer_pr_lease(
h, o, r = _resolve(remote, host, org, repo)
auth = _auth(h)
profile = get_profile()
identity = _authenticated_username(remote) or profile.get("username") or ""
# Host (not remote name) — remote aliases like "prgs" do not resolve identity.
identity = _authenticated_username(h) or profile.get("username") or ""
sid = (session_id or "").strip() or reviewer_pr_lease.new_session_id()
repo_label = f"{o}/{r}"
@@ -6350,7 +6353,8 @@ def gitea_adopt_merger_pr_lease(
auth = _auth(h)
profile = get_profile()
profile_name = profile.get("profile_name") or "unknown"
identity = _authenticated_username(remote) or profile.get("username") or ""
# Host (not remote name) — remote aliases like "prgs" do not resolve identity.
identity = _authenticated_username(h) or profile.get("username") or ""
sid = (session_id or "").strip() or reviewer_pr_lease.new_session_id()
repo_label = f"{o}/{r}"
@@ -6738,7 +6742,9 @@ def gitea_release_reviewer_pr_lease(
"reasons": ["no active reviewer lease found on PR"],
}
identity = _authenticated_username(remote) or ""
# Host (not remote name). Passing "prgs"/"dadeschools" yields empty identity
# and incorrectly blocks same-identity release across sessions.
identity = _authenticated_username(h) or ""
session = reviewer_pr_lease.get_session_lease() or {}
session_id = session.get("session_id")
+233
View File
@@ -4288,3 +4288,236 @@ 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"])
def test_reviewer_pr_lease_gate_resolves_identity_with_host(self):
"""Lease mutation gate must resolve identity using host, not remote alias."""
import mcp_server
resolved_hosts = []
def tracking_username(host):
resolved_hosts.append(host)
return None if host in {"prgs", "dadeschools"} else "reviewer-bot"
with _install_owned_reviewer_lease(
550,
session_id=_DEFAULT_LEASE_SESSION,
head_sha="abc123",
), patch(
"mcp_server._resolve",
return_value=("gitea.prgs.cc", "Scaled-Tech-Consulting", "Gitea-Tools"),
), patch(
"mcp_server._authenticated_username",
side_effect=tracking_username,
):
reasons = mcp_server._reviewer_pr_lease_gate(
pr_number=550,
remote="prgs",
host=None,
org=None,
repo=None,
mutation="approve",
live_head_sha="abc123",
pinned_head_sha="abc123",
)
self.assertEqual(reasons, [])
self.assertIn("gitea.prgs.cc", resolved_hosts)
self.assertNotIn("prgs", resolved_hosts)
def test_acquire_reviewer_pr_lease_resolves_identity_with_host(self):
"""Acquire path must not pass a remote alias into identity lookup."""
import mcp_server
resolved_hosts = []
posted = []
def tracking_username(host):
resolved_hosts.append(host)
return None if host in {"prgs", "dadeschools"} else "reviewer-bot"
def mock_api(method, url, auth, data=None):
if "/pulls/" in url:
return {
"user": {"login": "author-user"},
"state": "open",
"head": {"sha": "abc123"},
"mergeable": True,
}
if "/comments" in url:
if method == "POST":
posted.append(data["body"])
return {"id": 1003}
return []
return {}
with patch("mcp_server.api_request", side_effect=mock_api), patch(
"mcp_server._authenticated_username",
side_effect=tracking_username,
), patch(
"mcp_server._resolve",
return_value=("gitea.prgs.cc", "Scaled-Tech-Consulting", "Gitea-Tools"),
):
res = mcp_server.gitea_acquire_reviewer_pr_lease(
pr_number=550,
worktree="/workspace",
candidate_head="abc123",
remote="prgs",
)
self.assertTrue(res.get("success"), res)
self.assertTrue(res.get("acquired"), res)
self.assertEqual(res.get("comment_id"), 1003)
self.assertTrue(posted)
self.assertIn("gitea.prgs.cc", resolved_hosts)
self.assertNotIn("prgs", resolved_hosts)
def test_adopt_merger_pr_lease_resolves_identity_with_host(self):
"""Adopt path must not pass a remote alias into identity lookup."""
import mcp_server
import reviewer_pr_lease
reviewer_pr_lease.clear_session_lease()
head = "a" * 40
resolved_hosts = []
posted = []
reviewer_comment = _reviewer_lease_comment(
550,
session_id="reviewer-session",
head_sha=head,
reviewer="reviewer-bot",
)
def tracking_username(host):
resolved_hosts.append(host)
return None if host in {"prgs", "dadeschools"} else "merger-bot"
def mock_api(method, url, auth, data=None):
if "/pulls/" in url and "/comments" not in url:
return {
"number": 550,
"user": {"login": "author-user"},
"state": "open",
"head": {"sha": head},
"mergeable": True,
"merged": False,
}
if "/comments" in url:
if method == "POST":
posted.append(data["body"])
return {"id": 1004}
return [reviewer_comment]
return {}
merger_profile = {
"profile_name": "prgs-merger",
"allowed_operations": ["gitea.read", "gitea.pr.comment", "gitea.pr.merge"],
"forbidden_operations": [],
}
with patch("mcp_server.get_profile", return_value=merger_profile), patch(
"mcp_server.api_request",
side_effect=mock_api,
), patch(
"mcp_server._authenticated_username",
side_effect=tracking_username,
), patch(
"mcp_server._resolve",
return_value=("gitea.prgs.cc", "Scaled-Tech-Consulting", "Gitea-Tools"),
), patch(
"mcp_server.gitea_get_pr_review_feedback",
return_value={
"approval_at_current_head": True,
"latest_approved_head_sha": head,
},
):
res = mcp_server.gitea_adopt_merger_pr_lease(
pr_number=550,
worktree="/workspace",
expected_head_sha=head,
remote="prgs",
)
self.assertTrue(res.get("success"), res)
self.assertTrue(res.get("adopted"), res)
self.assertEqual(res.get("adoption_comment_id"), 1004)
self.assertTrue(posted)
self.assertIn("gitea.prgs.cc", resolved_hosts)
self.assertNotIn("prgs", resolved_hosts)
def test_release_reviewer_pr_lease_same_identity_cross_session(self):
"""Same reviewer identity may release a lease claimed by another session.
Regression: release used to call ``_authenticated_username(remote)``
with the remote alias (e.g. ``prgs``) instead of the resolved host,
so identity resolved empty and same-identity release fail-closed even
when the authenticated user owned the lease.
"""
import mcp_server
import reviewer_pr_lease
mcp_server.gitea_resolve_task_capability(task="review_pr", remote="prgs")
mcp_server.gitea_load_review_workflow()
reviewer_pr_lease.clear_session_lease()
foreign_session = "46820-dd78cdc4aacc"
comments = [
_reviewer_lease_comment(
457,
session_id=foreign_session,
head_sha="ef287eaf5841d9e542a4e888ce0ae7d679dbd685",
reviewer="sysadmin",
)
]
posted = []
resolved_hosts = []
def mock_api(method, url, auth, data=None):
if "/api/v1/user" in url:
return {"login": "sysadmin"}
if "/comments" in url:
if method == "POST":
new_c = {
"id": 8071,
"body": data["body"],
"user": {"login": "sysadmin"},
}
comments.append(new_c)
posted.append(new_c)
return {"id": 8071}
return comments
if "/pulls/" in url:
return {
"user": {"login": "jcwalker3"},
"state": "open",
"head": {"sha": "ef287eaf5841d9e542a4e888ce0ae7d679dbd685"},
"mergeable": False,
}
return {}
def tracking_username(host):
resolved_hosts.append(host)
# Empty identity when passed a remote alias (the pre-fix bug path).
if host in {"prgs", "dadeschools"}:
return None
return "sysadmin"
with patch("mcp_server.api_request", side_effect=mock_api), patch(
"mcp_server._authenticated_username", side_effect=tracking_username
), patch(
"mcp_server._resolve",
return_value=("gitea.prgs.cc", "Scaled-Tech-Consulting", "Gitea-Tools"),
), patch("mcp_server._auth", return_value={"Authorization": "token x"}), patch(
"mcp_server._verify_role_mutation_workspace", return_value=None
):
res = mcp_server.gitea_release_reviewer_pr_lease(
pr_number=457, remote="prgs"
)
self.assertTrue(res.get("success"), res)
self.assertTrue(res.get("released"), res)
self.assertEqual(res.get("comment_id"), 8071)
self.assertTrue(posted)
self.assertIn("phase: released", posted[0]["body"])
# Must resolve identity with host, never the remote alias alone.
self.assertIn("gitea.prgs.cc", resolved_hosts)
self.assertNotIn("prgs", resolved_hosts)