From 6a9fd852958ecf644351e84f8715852f6108f620 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 5 Jun 2026 09:37:42 -0700 Subject: [PATCH] fix: guard reviewer GraphQL against null repository (#1426) GitHub returns repository: null when the token can't read the repo (SAML, expired token, private/deleted). dict.get(k, {}) doesn't coalesce explicit null, so fetch_pr_review_threads crashed with AttributeError and publish_review could never post a review. Guard with isinstance checks and return collected threads on null repository; sweep the same pattern in resolve_review_thread. Co-authored-by: open-swe[bot] <215916821+open-swe[bot]@users.noreply.github.com> --- agent/reviewer_publish.py | 27 +++++++++++++++++++-------- tests/test_reviewer_publish.py | 18 ++++++++++++++++++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index abc8ed0a..56d4b47e 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -503,12 +503,21 @@ async def fetch_pr_review_threads( ) return out data = response.json() - threads = ( - data.get("data", {}) - .get("repository", {}) - .get("pullRequest", {}) - .get("reviewThreads", {}) - ) + data_root = data.get("data") if isinstance(data, dict) else None + repository = data_root.get("repository") if isinstance(data_root, dict) else None + if not isinstance(repository, dict): + logger.warning( + "Null repository in review-threads response for %s/%s#%s " + "(token likely lacks access: SAML, expired token, or private/deleted repo)", + owner, + repo, + pr_number, + ) + return out + pull_request = repository.get("pullRequest") + threads = pull_request.get("reviewThreads") if isinstance(pull_request, dict) else None + if not isinstance(threads, dict): + return out for thread in threads.get("nodes", []) or []: if not isinstance(thread, dict): continue @@ -668,8 +677,10 @@ async def resolve_review_thread(*, thread_node_id: str, token: str) -> bool: if data.get("errors"): logger.warning("resolveReviewThread errors: %s", data["errors"]) return False - thread = data.get("data", {}).get("resolveReviewThread", {}).get("thread", {}) - return bool(thread.get("isResolved")) + data_root = data.get("data") if isinstance(data, dict) else None + resolved = data_root.get("resolveReviewThread") if isinstance(data_root, dict) else None + thread = resolved.get("thread") if isinstance(resolved, dict) else None + return bool(thread.get("isResolved")) if isinstance(thread, dict) else False async def reply_to_review_comment( diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 95490433..de26da33 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -356,6 +356,24 @@ async def test_resolve_review_thread_returns_true_on_success() -> None: assert ok is True +@pytest.mark.asyncio +async def test_fetch_pr_review_threads_handles_null_repository() -> None: + """GitHub returns ``repository: null`` when the token can't read the repo + (SAML, expired token, private/deleted). ``dict.get(k, {})`` does not coalesce + explicit null, so the fetch must guard against it and return collected threads.""" + response = MagicMock() + response.json.return_value = {"data": {"repository": None}} + response.raise_for_status.return_value = None + + client_cm = AsyncMock() + client_cm.__aenter__.return_value = client_cm + client_cm.post = AsyncMock(return_value=response) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + threads = await fetch_pr_review_threads(owner="o", repo="r", pr_number=1, token="t") + assert threads == [] + + @pytest.mark.asyncio async def test_post_pull_request_review_non_dict_body_surfaces_status_and_excerpt() -> None: """A non-dict GitHub response body must surface status code + body excerpt