fix: guard PR/issue comment listing against non-list API payloads (Closes #485)
_list_pr_lease_comments and gitea_list_issue_comments now fail safe to an empty list when the comments API returns a non-list payload (e.g. an error object), preventing KeyError: slice on the lease and listing paths. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
+12
-2
@@ -2515,7 +2515,12 @@ def _list_pr_lease_comments(
|
|||||||
h, o, r = _resolve(remote, host, org, repo)
|
h, o, r = _resolve(remote, host, org, repo)
|
||||||
auth = _auth(h)
|
auth = _auth(h)
|
||||||
api = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments"
|
api = f"{repo_api_url(h, o, r)}/issues/{pr_number}/comments"
|
||||||
comments = api_request("GET", api, auth) or []
|
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])
|
return list(comments[:limit])
|
||||||
|
|
||||||
|
|
||||||
@@ -5119,7 +5124,12 @@ def gitea_list_issue_comments(
|
|||||||
h, o, r = _resolve(remote, host, org, repo)
|
h, o, r = _resolve(remote, host, org, repo)
|
||||||
auth = _auth(h)
|
auth = _auth(h)
|
||||||
api = f"{repo_api_url(h, o, r)}/issues/{issue_number}/comments"
|
api = f"{repo_api_url(h, o, r)}/issues/{issue_number}/comments"
|
||||||
comments = api_request("GET", api, auth) or []
|
comments = api_request("GET", api, auth)
|
||||||
|
# Fail safe to no comments when the API returns a non-list payload (e.g. an
|
||||||
|
# error object such as an HTTP 401 body) so listing never crashes on a
|
||||||
|
# malformed response (#485).
|
||||||
|
if not isinstance(comments, list):
|
||||||
|
comments = []
|
||||||
reveal = _reveal_endpoints()
|
reveal = _reveal_endpoints()
|
||||||
out = []
|
out = []
|
||||||
for c in comments[:limit]:
|
for c in comments[:limit]:
|
||||||
|
|||||||
@@ -0,0 +1,76 @@
|
|||||||
|
"""Regression tests for non-list API payloads on PR/issue comment listing (#485)."""
|
||||||
|
import os
|
||||||
|
import sys
|
||||||
|
import unittest
|
||||||
|
from unittest.mock import patch
|
||||||
|
|
||||||
|
sys.path.insert(0, str(__import__("pathlib").Path(__file__).resolve().parent.parent))
|
||||||
|
|
||||||
|
from mcp_server import ( # noqa: E402
|
||||||
|
_list_pr_lease_comments,
|
||||||
|
gitea_list_issue_comments,
|
||||||
|
)
|
||||||
|
|
||||||
|
FAKE_AUTH = "Basic dGVzdDp0ZXN0"
|
||||||
|
AUTHOR_ENV = {
|
||||||
|
"GITEA_PROFILE_NAME": "gitea-author",
|
||||||
|
"GITEA_ALLOWED_OPERATIONS": "gitea.read,gitea.issue.comment",
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
|
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):
|
||||||
|
mock_api.return_value = {"message": "Unauthorized"}
|
||||||
|
result = _list_pr_lease_comments(
|
||||||
|
12, remote="prgs", host=None, org=None, repo=None)
|
||||||
|
self.assertEqual(result, [])
|
||||||
|
|
||||||
|
@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):
|
||||||
|
mock_api.return_value = None
|
||||||
|
result = _list_pr_lease_comments(
|
||||||
|
12, remote="prgs", host=None, org=None, repo=None)
|
||||||
|
self.assertEqual(result, [])
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request")
|
||||||
|
@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 -->"}
|
||||||
|
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])
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request")
|
||||||
|
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||||
|
def test_list_issue_comments_non_list_payload_returns_empty(self, _auth, mock_api):
|
||||||
|
mock_api.return_value = {"message": "Unauthorized"}
|
||||||
|
with patch.dict(os.environ, AUTHOR_ENV, clear=True):
|
||||||
|
result = gitea_list_issue_comments(issue_number=9, remote="prgs")
|
||||||
|
self.assertTrue(result["success"])
|
||||||
|
self.assertEqual(result["comments"], [])
|
||||||
|
|
||||||
|
@patch("mcp_server.api_request")
|
||||||
|
@patch("mcp_server.get_auth_header", return_value=FAKE_AUTH)
|
||||||
|
def test_list_issue_comments_list_payload_unchanged(self, _auth, mock_api):
|
||||||
|
mock_api.return_value = [
|
||||||
|
{
|
||||||
|
"id": 101,
|
||||||
|
"user": {"login": "alice"},
|
||||||
|
"body": "hello",
|
||||||
|
"created_at": "2026-07-03T00:00:00Z",
|
||||||
|
"updated_at": "2026-07-03T01:00:00Z",
|
||||||
|
}
|
||||||
|
]
|
||||||
|
with patch.dict(os.environ, AUTHOR_ENV, clear=True):
|
||||||
|
result = gitea_list_issue_comments(issue_number=9, remote="prgs")
|
||||||
|
self.assertTrue(result["success"])
|
||||||
|
self.assertEqual(len(result["comments"]), 1)
|
||||||
|
self.assertEqual(result["comments"][0]["author"], "alice")
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
Reference in New Issue
Block a user