Skip to content

Commit bedcc27

Browse files
shenxianpengclaude
andcommitted
fix: survive a 403 whose body is not a JSON object
GithubException.data is whatever the response decoded to: None for an empty body, a str for a non-JSON one. Calling .get on it raises inside the 403 handler, and an exception raised there is not caught by the sibling except clauses — it escapes add_pr_comments and fails the step. That is the opposite of what this handler exists to do. Verified both shapes raise before the guard and warn after it. The test stub now stores data exactly as PyGithub does; coercing a falsy payload to {} was hiding the shapes the handler has to survive. The 403 test also asserts the API's own message reaches the warning, so dropping the detail cannot pass unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
1 parent 2123307 commit bedcc27

2 files changed

Lines changed: 24 additions & 2 deletions

File tree

main.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -882,10 +882,15 @@ def add_pr_comments(results: list[ScopeResult]) -> int:
882882
return 0 if all(scope.status == "pass" for scope in results) else 1
883883
except GithubException as e:
884884
if e.status == 403:
885+
# GithubException.data is whatever the response decoded to, which
886+
# is None for an empty body and a str for a non-JSON one. Reaching
887+
# for .get unguarded would raise inside this handler and escape the
888+
# function, turning the best-effort path into a step failure.
889+
detail = e.data.get("message") if isinstance(e.data, dict) else None
885890
print(
886891
"::warning::Unable to post PR comment (403 Forbidden). "
887892
"Ensure your workflow grants 'pull-requests: write' permission. "
888-
f"Error: {e.data.get('message', str(e))}",
893+
f"Error: {detail or e}",
889894
file=sys.stderr,
890895
)
891896
return 0

main_test.py

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1095,7 +1095,9 @@ class _StubGithubException(Exception):
10951095
def __init__(self, status, data=None):
10961096
super().__init__(f"status {status}")
10971097
self.status = status
1098-
self.data = data or {}
1098+
# Stored as given, exactly as PyGithub does. Coercing a falsy payload
1099+
# to {} here would hide the very shapes the handler has to survive.
1100+
self.data = data
10991101

11001102

11011103
class TestAddPrCommentsFailures(unittest.TestCase):
@@ -1149,6 +1151,21 @@ def test_forbidden_names_the_permission_that_actually_grants_this(self):
11491151
# pull-requests scope, and this hint is the only guidance a user gets.
11501152
self.assertIn("pull-requests: write", warning)
11511153
self.assertNotIn("issues: write", warning)
1154+
# The API's own wording is what tells the user which resource was
1155+
# refused, so the hint has to carry it through.
1156+
self.assertIn("Resource not accessible", warning)
1157+
1158+
def test_forbidden_with_a_non_mapping_payload_still_warns(self):
1159+
# data is whatever the body decoded to: None when empty, a str when it
1160+
# is not JSON. Reaching for .get on either raises inside the handler
1161+
# and escapes the function, which would fail the step.
1162+
for payload in (None, "forbidden"):
1163+
with self.subTest(payload=payload):
1164+
rc, printed = self._run(_StubGithubException(403, payload))
1165+
self.assertEqual(rc, 0)
1166+
warning = next(w for w in printed if "::warning::" in w)
1167+
self.assertIn("pull-requests: write", warning)
1168+
self.assertIn("status 403", warning)
11521169

11531170
def test_other_api_errors_are_annotated(self):
11541171
rc, printed = self._run(_StubGithubException(500, {"message": "boom"}))

0 commit comments

Comments
 (0)