Skip to content

Commit d1a7d3a

Browse files
committed
fix: annotate failed PR comment posts and name the right permission
The 403 hint told users to grant 'issues: write', but a PR comment is written with the pull-requests scope — following the hint does not fix the failure. Every other failure path only printed to stderr, so a run whose comment never posted stayed green with nothing on the page saying why; those now emit ::warning:: too. Also refresh the workflow examples: drop the push trigger from the README example (pr-comments only works on pull_request, which made the event_name conditional dead weight) and move actions/checkout from v5 to v7.
1 parent 3751d44 commit d1a7d3a

5 files changed

Lines changed: 93 additions & 9 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ Create a new GitHub Actions workflow in your project, e.g. at [.github/workflows
3939
name: Commit Check
4040

4141
on:
42-
push:
4342
pull_request:
4443
branches: 'main'
4544

@@ -50,7 +49,7 @@ jobs:
5049
contents: read
5150
pull-requests: write
5251
steps:
53-
- uses: actions/checkout@v5
52+
- uses: actions/checkout@v7
5453
with:
5554
fetch-depth: 0 # Required for merge-base checks
5655
- uses: commit-check/commit-check-action@v2
@@ -60,7 +59,7 @@ jobs:
6059
author-name: false
6160
author-email: false
6261
job-summary: true
63-
pr-comments: ${{ github.event_name == 'pull_request' }}
62+
pr-comments: true
6463
```
6564
6665
> [!NOTE]

‎docs/fork-pr-comments.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ jobs:
5656
check:
5757
runs-on: ubuntu-latest
5858
steps:
59-
- uses: actions/checkout@v5
59+
- uses: actions/checkout@v7
6060
with:
6161
fetch-depth: 0
6262
- uses: commit-check/commit-check-action@v2
@@ -148,7 +148,7 @@ jobs:
148148
pull-requests: write
149149
steps:
150150
# SAFE: checkout the merge commit, NOT the PR head
151-
- uses: actions/checkout@v5
151+
- uses: actions/checkout@v7
152152
with:
153153
fetch-depth: 0
154154
- uses: commit-check/commit-check-action@v2

‎examples/commit-check-workflow-a.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ jobs:
1616
check:
1717
runs-on: ubuntu-latest
1818
steps:
19-
- uses: actions/checkout@v5
19+
- uses: actions/checkout@v7
2020
with:
2121
fetch-depth: 0
2222
- uses: commit-check/commit-check-action@v2

‎main.py‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -884,15 +884,18 @@ def add_pr_comments(results: list[ScopeResult]) -> int:
884884
if e.status == 403:
885885
print(
886886
"::warning::Unable to post PR comment (403 Forbidden). "
887-
"Ensure your workflow grants 'issues: write' permission. "
887+
"Ensure your workflow grants 'pull-requests: write' permission. "
888888
f"Error: {e.data.get('message', str(e))}",
889889
file=sys.stderr,
890890
)
891891
return 0
892-
print(f"Error posting PR comment: {e}", file=sys.stderr)
892+
# Annotated, not just printed: posting the comment is best-effort and
893+
# never fails the step, so without an annotation the run is green, the
894+
# comment is absent, and nothing says why.
895+
print(f"::warning::Unable to post PR comment: {e}", file=sys.stderr)
893896
return 0
894897
except Exception as e:
895-
print(f"Error posting PR comment: {e}", file=sys.stderr)
898+
print(f"::warning::Unable to post PR comment: {e}", file=sys.stderr)
896899
return 0
897900

898901

‎main_test.py‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,6 +1085,88 @@ def test_skips_when_comment_is_up_to_date(self):
10851085
mock_pull_request.create_comment.assert_not_called()
10861086

10871087

1088+
class _StubGithubException(Exception):
1089+
"""Stands in for github.GithubException, which is mocked away in these tests.
1090+
1091+
The real class has to be a genuine exception type or the ``except`` clause
1092+
in add_pr_comments raises TypeError before the handler is reached.
1093+
"""
1094+
1095+
def __init__(self, status, data=None):
1096+
super().__init__(f"status {status}")
1097+
self.status = status
1098+
self.data = data or {}
1099+
1100+
1101+
class TestAddPrCommentsFailures(unittest.TestCase):
1102+
"""Posting the comment is best-effort, but it must never fail silently.
1103+
1104+
Every branch here returns 0 so the step stays green — which is the point:
1105+
without an annotation the run is green, the comment is absent, and nothing
1106+
on the page says why.
1107+
"""
1108+
1109+
def _run(self, side_effect):
1110+
mock_pull_request = MagicMock()
1111+
mock_pull_request.get_comments.return_value = []
1112+
mock_pull_request.create_comment.side_effect = side_effect
1113+
mock_repo = MagicMock()
1114+
mock_repo.get_issue.return_value = mock_pull_request
1115+
1116+
github_module = MagicMock()
1117+
github_module.GithubException = _StubGithubException
1118+
github_module.Github.return_value.get_repo.return_value = mock_repo
1119+
1120+
with (
1121+
patch("main.PR_COMMENTS_ENABLED", True),
1122+
patch("main.is_fork_pr_with_readonly_token", return_value=False),
1123+
patch.dict(
1124+
os.environ,
1125+
{
1126+
"GITHUB_TOKEN": "token",
1127+
"GITHUB_REPOSITORY": "owner/repo",
1128+
"GITHUB_REF": "refs/pull/12/merge",
1129+
},
1130+
),
1131+
patch.dict(sys.modules, {"github": github_module}),
1132+
patch("builtins.print") as mock_print,
1133+
):
1134+
rc = main.add_pr_comments([fail_scope()])
1135+
printed = [
1136+
call[0][0]
1137+
for call in mock_print.call_args_list
1138+
if call[0] and isinstance(call[0][0], str)
1139+
]
1140+
return rc, printed
1141+
1142+
def test_forbidden_names_the_permission_that_actually_grants_this(self):
1143+
rc, printed = self._run(
1144+
_StubGithubException(403, {"message": "Resource not accessible"})
1145+
)
1146+
self.assertEqual(rc, 0)
1147+
warning = next(w for w in printed if "::warning::" in w)
1148+
# pull-requests, not issues: a PR comment is written with the
1149+
# pull-requests scope, and this hint is the only guidance a user gets.
1150+
self.assertIn("pull-requests: write", warning)
1151+
self.assertNotIn("issues: write", warning)
1152+
1153+
def test_other_api_errors_are_annotated(self):
1154+
rc, printed = self._run(_StubGithubException(500, {"message": "boom"}))
1155+
self.assertEqual(rc, 0)
1156+
self.assertTrue(
1157+
any("::warning::" in w for w in printed),
1158+
f"a failed post must be annotated, got: {printed}",
1159+
)
1160+
1161+
def test_unexpected_errors_are_annotated(self):
1162+
rc, printed = self._run(RuntimeError("network went away"))
1163+
self.assertEqual(rc, 0)
1164+
self.assertTrue(
1165+
any("::warning::" in w and "network went away" in w for w in printed),
1166+
f"a failed post must be annotated, got: {printed}",
1167+
)
1168+
1169+
10881170
class TestIsForkPrWithReadonlyToken(unittest.TestCase):
10891171
def test_fork_pr_with_pull_request_event(self):
10901172
with (

0 commit comments

Comments
 (0)