fix: resolve reviewer lease identity via host not remote alias
_authenticated_username expects a host for auth/API calls. Lease acquire/adopt/gate/release paths passed the remote alias (prgs), so identity resolved empty and same-identity release fail-closed across sessions — blocking author conflict-fix pushes after REQUEST_CHANGES. Pass resolved host h from _resolve into _authenticated_username.
This commit is contained in:
+10
-4
@@ -6153,7 +6153,9 @@ def _reviewer_pr_lease_gate(
|
|||||||
"""Return block reasons when the session lacks an owned PR reviewer lease."""
|
"""Return block reasons when the session lacks an owned PR reviewer lease."""
|
||||||
session = reviewer_pr_lease.get_session_lease()
|
session = reviewer_pr_lease.get_session_lease()
|
||||||
session_id = (session or {}).get("session_id")
|
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:
|
try:
|
||||||
comments = _fetch_pr_comments(
|
comments = _fetch_pr_comments(
|
||||||
pr_number, remote=remote, host=host, org=org, repo=repo)
|
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)
|
h, o, r = _resolve(remote, host, org, repo)
|
||||||
auth = _auth(h)
|
auth = _auth(h)
|
||||||
profile = get_profile()
|
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()
|
sid = (session_id or "").strip() or reviewer_pr_lease.new_session_id()
|
||||||
repo_label = f"{o}/{r}"
|
repo_label = f"{o}/{r}"
|
||||||
|
|
||||||
@@ -6350,7 +6353,8 @@ def gitea_adopt_merger_pr_lease(
|
|||||||
auth = _auth(h)
|
auth = _auth(h)
|
||||||
profile = get_profile()
|
profile = get_profile()
|
||||||
profile_name = profile.get("profile_name") or "unknown"
|
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()
|
sid = (session_id or "").strip() or reviewer_pr_lease.new_session_id()
|
||||||
repo_label = f"{o}/{r}"
|
repo_label = f"{o}/{r}"
|
||||||
|
|
||||||
@@ -6738,7 +6742,9 @@ def gitea_release_reviewer_pr_lease(
|
|||||||
"reasons": ["no active reviewer lease found on PR"],
|
"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 = reviewer_pr_lease.get_session_lease() or {}
|
||||||
session_id = session.get("session_id")
|
session_id = session.get("session_id")
|
||||||
|
|
||||||
|
|||||||
@@ -4288,3 +4288,81 @@ class TestIssue546Deadlock(unittest.TestCase):
|
|||||||
self.assertIsNone(reviewer_pr_lease.get_session_lease())
|
self.assertIsNone(reviewer_pr_lease.get_session_lease())
|
||||||
# verify comment phase is released
|
# verify comment phase is released
|
||||||
self.assertIn("phase: released", posted_comments[0]["body"])
|
self.assertIn("phase: released", posted_comments[0]["body"])
|
||||||
|
|
||||||
|
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)
|
||||||
|
|||||||
Reference in New Issue
Block a user