fix: isolate #519 PR lookup from shared lease comment listing
Keep _fetch_pr_lease_comments_safe for gitea_assess_conflict_fix_push only. Restore _list_pr_lease_comments to a single comments GET so merge approval feedback and #485 non-list handling keep stable API call order. Closes #519
This commit is contained in:
+19
-10
@@ -2940,16 +2940,25 @@ def _list_pr_lease_comments(
|
||||
repo: str | None,
|
||||
limit: int = 100,
|
||||
) -> list[dict]:
|
||||
"""Fetch PR/issue thread comments used for reviewer/conflict-fix leases."""
|
||||
fetched = _fetch_pr_lease_comments_safe(
|
||||
pr_number,
|
||||
remote=remote,
|
||||
host=host,
|
||||
org=org,
|
||||
repo=repo,
|
||||
limit=limit,
|
||||
)
|
||||
return fetched["comments"]
|
||||
"""Fetch PR/issue thread comments used for reviewer/conflict-fix leases.
|
||||
|
||||
Intentionally does **not** pre-fetch the PR via GET /pulls/{n}. Shared
|
||||
callers (merge approval feedback, reviewer lease gates) depend on a single
|
||||
comments GET so mock sequences and fail-open #485 non-list handling stay
|
||||
stable. Structured PR-lookup failures belong only to
|
||||
:func:`_fetch_pr_lease_comments_safe` used by conflict-fix push assessment
|
||||
(#519).
|
||||
"""
|
||||
h, o, r = _resolve(remote, host, org, repo)
|
||||
auth = _auth(h)
|
||||
api = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments"
|
||||
comments = api_request("GET", api, auth)
|
||||
# Fail safe to no lease comments when the API returns a non-list payload
|
||||
# (e.g. an error object such as an HTTP 401 body): lease state can only be
|
||||
# proven from real comment entries, never inferred from an error shape (#485).
|
||||
if not isinstance(comments, list):
|
||||
return []
|
||||
return list(comments[:limit])
|
||||
|
||||
|
||||
def _pr_work_lease_reviewer_block(
|
||||
|
||||
@@ -16,31 +16,26 @@ AUTHOR_ENV = {
|
||||
"GITEA_PROFILE_NAME": "gitea-author",
|
||||
"GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.issue.comment",
|
||||
}
|
||||
OPEN_PR = {"number": 12, "state": "open", "head": {"sha": "abc123"}}
|
||||
|
||||
|
||||
def _pr_then_comments(mock_api, comments_payload):
|
||||
def _api(method, url, auth, *args, **kwargs):
|
||||
if "/pulls/" in url:
|
||||
return OPEN_PR
|
||||
return comments_payload
|
||||
|
||||
mock_api.side_effect = _api
|
||||
|
||||
|
||||
class TestPrLeaseCommentsNonListGuard(unittest.TestCase):
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_list_pr_lease_comments_non_list_payload_returns_empty(self, _auth, mock_api):
|
||||
_pr_then_comments(mock_api, {"message": "Unauthorized"})
|
||||
mock_api.return_value = {"message": "Unauthorized"}
|
||||
result = _list_pr_lease_comments(
|
||||
12, remote="prgs", host=None, org=None, repo=None)
|
||||
self.assertEqual(result, [])
|
||||
# Single comments GET only — no pre-flight PR lookup (#519 isolation).
|
||||
mock_api.assert_called_once()
|
||||
_method, url, _auth_arg = mock_api.call_args[0][:3]
|
||||
self.assertIn("/issues/12/comments", url)
|
||||
self.assertNotIn("/pulls/", url)
|
||||
|
||||
@patch("mcp_server.api_request")
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_list_pr_lease_comments_none_returns_empty(self, _auth, mock_api):
|
||||
_pr_then_comments(mock_api, None)
|
||||
mock_api.return_value = None
|
||||
result = _list_pr_lease_comments(
|
||||
12, remote="prgs", host=None, org=None, repo=None)
|
||||
self.assertEqual(result, [])
|
||||
@@ -49,7 +44,7 @@ class TestPrLeaseCommentsNonListGuard(unittest.TestCase):
|
||||
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||
def test_list_pr_lease_comments_list_payload_unchanged(self, _auth, mock_api):
|
||||
comment = {"id": 7, "body": "<!-- mcp-review-lease:v1 -->"}
|
||||
_pr_then_comments(mock_api, [comment])
|
||||
mock_api.return_value = [comment]
|
||||
result = _list_pr_lease_comments(
|
||||
12, remote="prgs", host=None, org=None, repo=None, limit=5)
|
||||
self.assertEqual(result, [comment])
|
||||
@@ -83,4 +78,4 @@ class TestPrLeaseCommentsNonListGuard(unittest.TestCase):
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
unittest.main()
|
||||
|
||||
Reference in New Issue
Block a user