From 3751d44a5b3dd1b47a0c1ebe4161aeb9e5833053 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Wed, 5 Aug 2026 13:50:06 +0000 Subject: [PATCH 1/3] docs: show the report inline instead of linking four missing screenshots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job summary and PR comment sections pointed at four PNGs under commit-check/.github/screenshot/. That directory does not exist and never has — the repository is five files, and its full history contains no screenshot at any point — so all four have been broken images. They could not have rendered even once. The URLs are /blob/ links, which serve an HTML page rather than an image; Markdown needs raw. githubusercontent.com, as this action's own logo reference already uses. Reproduce the report inline instead. It is Markdown, so GitHub renders it here exactly as it renders it in a job summary — a table and a collapsed tree, not a picture of one — which keeps it accurate for free, selectable, searchable and readable by a screen reader. Two examples rather than four: render_job_summary and render_pr_comment both return render_report, so the surfaces cannot differ, and the comment section now says so and links up rather than repeating itself. Both examples were generated by calling the renderer, not written by hand. Two deliberate departures, both noted in the text: the title is bold rather than a heading, so it stays out of this page's table of contents, and the footer says rather than pinning one that would drift. The comment section also describes the lifecycle, which no screenshot could: the comment is edited in place and survives a passing run, older marked duplicates are deleted, and a pre-marker comment is adopted only when a bot posted it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn --- README.md | 89 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6eb9557..f0e9703 100644 --- a/README.md +++ b/README.md @@ -231,25 +231,96 @@ or gate on individual rules. ## GitHub Action Job Summary -By default, commit-check-action results are shown on the job summary page of the workflow. +By default, commit-check-action results are shown on the job summary page of the +workflow. The report below is reproduced as the action renders it, except that +its title is a heading in the real thing — it is bold here so it stays out of +this page's table of contents — and the footer names the version that actually +ran. ### Success Job Summary -![Success job summary](https://github.com/commit-check/.github/blob/main/screenshot/success-job-summary.png) +Passing runs stay to one line, with the detail folded away: -### Failure Job Summary +> **Commit Check** +> +> ✅ **All 3 checks passed** +> +>
+> Show all 3 checks +> +> ```text +> Commit message +> ✔ PR title (feat: add login page) +> ✔ Commit 1/2 (feat: add login page) +> Branch +> ✔ Branch (feature/add-login) +> ``` +> +>
+> +> _commit-check <version> · [Rules reference](https://commit-check.com/rules/)_ -![Failure job summary](https://github.com/commit-check/.github/blob/main/screenshot/failure-job-summary.png) +### Failure Job Summary -## GitHub Pull Request Comments +Failures open with a count, then a table of only the scopes that failed — every +rule ID links to its documentation — with the full tree still one click away: -### Success Pull Request Comment +> **Commit Check** +> +> ❌ **2 of 4 checks failed** +> +> | Scope | Checked value | Failed checks | +> |---|---|---| +> | Commit 2/2 | `bad msg` | [CC001 message](https://commit-check.com/rules/#cc001) | +> | Branch | `my-changes` | [CC201 branch](https://commit-check.com/rules/#cc201) | +> +>
+> Show all 4 checks +> +> ```text +> Commit message +> ✔ PR title (feat: add login page) +> ✔ Commit 1/2 (feat: add login page) +> ✖ Commit 2/2 (1 failure) +> CC001 message +> value: bad msg +> The commit message should follow Conventional Commits. +> Suggest: Use (): +> Branch +> ✖ Branch (1 failure) +> CC201 branch +> value: my-changes +> The branch should follow Conventional Branch. +> Suggest: Use / with allowed types +> ``` +> +>
+> +> _commit-check <version> · [Rules reference](https://commit-check.com/rules/)_ -![Success pull request comment](https://github.com/commit-check/.github/blob/main/screenshot/success-pr-comments.png) +A scope is one thing that was checked — a commit message, the branch, the author +— not one rule evaluation, so the total matches the ✔/✖ lines you can count and +does not grow with the number of rules in your config. -### Failure Pull Request Comment +## GitHub Pull Request Comments -![Failure pull request comment](https://github.com/commit-check/.github/blob/main/screenshot/failure-pr-comments.png) +With `pr-comments: true` the same report is posted as a pull request comment. +It is the same Markdown: the job summary and the comment are both rendered by +`render_report`, so the two surfaces cannot disagree. See +[Success Job Summary](#success-job-summary) and +[Failure Job Summary](#failure-job-summary) above for what it looks like. + +What differs is the lifecycle rather than the content: + +- The comment is **edited in place** on later runs rather than added to, so a + pull request carries one Commit Check comment however many times CI runs. It + stays after the checks pass, showing the ✅ report rather than disappearing. +- Comments are identified by a hidden `` marker, so + reformatting the visible text does not orphan the previous one. If several + marked comments somehow exist, the newest is kept and the rest deleted. +- A comment from a version predating the marker is adopted rather than + duplicated — but only when a bot posted it, since the older signal was just a + title prefix that a person could type by hand. ## Fork PR Comments From 2123307957b08d0e626a63151a0af301c0974f01 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Thu, 6 Aug 2026 06:19:33 +0000 Subject: [PATCH 2/3] fix: warn when a PR comment fails to post, and name the right permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn --- README.md | 5 +- docs/fork-pr-comments.md | 4 +- examples/commit-check-workflow-a.yml | 2 +- main.py | 9 ++- main_test.py | 82 ++++++++++++++++++++++++++++ 5 files changed, 93 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index f0e9703..3f51ec9 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,6 @@ Create a new GitHub Actions workflow in your project, e.g. at [.github/workflows name: Commit Check on: - push: pull_request: branches: 'main' @@ -50,7 +49,7 @@ jobs: contents: read pull-requests: write steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 with: fetch-depth: 0 # Required for merge-base checks - uses: commit-check/commit-check-action@v2 @@ -60,7 +59,7 @@ jobs: author-name: false author-email: false job-summary: true - pr-comments: ${{ github.event_name == 'pull_request' }} + pr-comments: true ``` > [!NOTE] diff --git a/docs/fork-pr-comments.md b/docs/fork-pr-comments.md index 82ff63a..b267011 100644 --- a/docs/fork-pr-comments.md +++ b/docs/fork-pr-comments.md @@ -56,7 +56,7 @@ jobs: check: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 with: fetch-depth: 0 - uses: commit-check/commit-check-action@v2 @@ -148,7 +148,7 @@ jobs: pull-requests: write steps: # SAFE: checkout the merge commit, NOT the PR head - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 with: fetch-depth: 0 - uses: commit-check/commit-check-action@v2 diff --git a/examples/commit-check-workflow-a.yml b/examples/commit-check-workflow-a.yml index 9d1bece..8c6c2ba 100644 --- a/examples/commit-check-workflow-a.yml +++ b/examples/commit-check-workflow-a.yml @@ -16,7 +16,7 @@ jobs: check: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@v7 with: fetch-depth: 0 - uses: commit-check/commit-check-action@v2 diff --git a/main.py b/main.py index 8a76005..c4fe148 100755 --- a/main.py +++ b/main.py @@ -884,15 +884,18 @@ def add_pr_comments(results: list[ScopeResult]) -> int: if e.status == 403: print( "::warning::Unable to post PR comment (403 Forbidden). " - "Ensure your workflow grants 'issues: write' permission. " + "Ensure your workflow grants 'pull-requests: write' permission. " f"Error: {e.data.get('message', str(e))}", file=sys.stderr, ) return 0 - print(f"Error posting PR comment: {e}", file=sys.stderr) + # Annotated, not just printed: posting the comment is best-effort and + # never fails the step, so without an annotation the run is green, the + # comment is absent, and nothing says why. + print(f"::warning::Unable to post PR comment: {e}", file=sys.stderr) return 0 except Exception as e: - print(f"Error posting PR comment: {e}", file=sys.stderr) + print(f"::warning::Unable to post PR comment: {e}", file=sys.stderr) return 0 diff --git a/main_test.py b/main_test.py index d1e6bcd..70faef9 100644 --- a/main_test.py +++ b/main_test.py @@ -1085,6 +1085,88 @@ def test_skips_when_comment_is_up_to_date(self): mock_pull_request.create_comment.assert_not_called() +class _StubGithubException(Exception): + """Stands in for github.GithubException, which is mocked away in these tests. + + The real class has to be a genuine exception type or the ``except`` clause + in add_pr_comments raises TypeError before the handler is reached. + """ + + def __init__(self, status, data=None): + super().__init__(f"status {status}") + self.status = status + self.data = data or {} + + +class TestAddPrCommentsFailures(unittest.TestCase): + """Posting the comment is best-effort, but it must never fail silently. + + Every branch here returns 0 so the step stays green — which is the point: + without an annotation the run is green, the comment is absent, and nothing + on the page says why. + """ + + def _run(self, side_effect): + mock_pull_request = MagicMock() + mock_pull_request.get_comments.return_value = [] + mock_pull_request.create_comment.side_effect = side_effect + mock_repo = MagicMock() + mock_repo.get_issue.return_value = mock_pull_request + + github_module = MagicMock() + github_module.GithubException = _StubGithubException + github_module.Github.return_value.get_repo.return_value = mock_repo + + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch("main.is_fork_pr_with_readonly_token", return_value=False), + patch.dict( + os.environ, + { + "GITHUB_TOKEN": "token", + "GITHUB_REPOSITORY": "owner/repo", + "GITHUB_REF": "refs/pull/12/merge", + }, + ), + patch.dict(sys.modules, {"github": github_module}), + patch("builtins.print") as mock_print, + ): + rc = main.add_pr_comments([fail_scope()]) + printed = [ + call[0][0] + for call in mock_print.call_args_list + if call[0] and isinstance(call[0][0], str) + ] + return rc, printed + + def test_forbidden_names_the_permission_that_actually_grants_this(self): + rc, printed = self._run( + _StubGithubException(403, {"message": "Resource not accessible"}) + ) + self.assertEqual(rc, 0) + warning = next(w for w in printed if "::warning::" in w) + # pull-requests, not issues: a PR comment is written with the + # pull-requests scope, and this hint is the only guidance a user gets. + self.assertIn("pull-requests: write", warning) + self.assertNotIn("issues: write", warning) + + def test_other_api_errors_are_annotated(self): + rc, printed = self._run(_StubGithubException(500, {"message": "boom"})) + self.assertEqual(rc, 0) + self.assertTrue( + any("::warning::" in w for w in printed), + f"a failed post must be annotated, got: {printed}", + ) + + def test_unexpected_errors_are_annotated(self): + rc, printed = self._run(RuntimeError("network went away")) + self.assertEqual(rc, 0) + self.assertTrue( + any("::warning::" in w and "network went away" in w for w in printed), + f"a failed post must be annotated, got: {printed}", + ) + + class TestIsForkPrWithReadonlyToken(unittest.TestCase): def test_fork_pr_with_pull_request_event(self): with ( From afedd6f6582312e3ac3fde1b0fe26f8fac72f685 Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Thu, 6 Aug 2026 06:30:03 +0000 Subject: [PATCH 3/3] fix: handle a 403 whose body is not a JSON object MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn --- main.py | 7 ++++++- main_test.py | 19 ++++++++++++++++++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/main.py b/main.py index c4fe148..477b4cd 100755 --- a/main.py +++ b/main.py @@ -882,10 +882,15 @@ def add_pr_comments(results: list[ScopeResult]) -> int: return 0 if all(scope.status == "pass" for scope in results) else 1 except GithubException as e: if e.status == 403: + # GithubException.data is whatever the response decoded to, which + # is None for an empty body and a str for a non-JSON one. Reaching + # for .get unguarded would raise inside this handler and escape the + # function, turning the best-effort path into a step failure. + detail = e.data.get("message") if isinstance(e.data, dict) else None print( "::warning::Unable to post PR comment (403 Forbidden). " "Ensure your workflow grants 'pull-requests: write' permission. " - f"Error: {e.data.get('message', str(e))}", + f"Error: {detail or e}", file=sys.stderr, ) return 0 diff --git a/main_test.py b/main_test.py index 70faef9..7aa330d 100644 --- a/main_test.py +++ b/main_test.py @@ -1095,7 +1095,9 @@ class _StubGithubException(Exception): def __init__(self, status, data=None): super().__init__(f"status {status}") self.status = status - self.data = data or {} + # Stored as given, exactly as PyGithub does. Coercing a falsy payload + # to {} here would hide the very shapes the handler has to survive. + self.data = data class TestAddPrCommentsFailures(unittest.TestCase): @@ -1149,6 +1151,21 @@ def test_forbidden_names_the_permission_that_actually_grants_this(self): # pull-requests scope, and this hint is the only guidance a user gets. self.assertIn("pull-requests: write", warning) self.assertNotIn("issues: write", warning) + # The API's own wording is what tells the user which resource was + # refused, so the hint has to carry it through. + self.assertIn("Resource not accessible", warning) + + def test_forbidden_with_a_non_mapping_payload_still_warns(self): + # data is whatever the body decoded to: None when empty, a str when it + # is not JSON. Reaching for .get on either raises inside the handler + # and escapes the function, which would fail the step. + for payload in (None, "forbidden"): + with self.subTest(payload=payload): + rc, printed = self._run(_StubGithubException(403, payload)) + self.assertEqual(rc, 0) + warning = next(w for w in printed if "::warning::" in w) + self.assertIn("pull-requests: write", warning) + self.assertIn("status 403", warning) def test_other_api_errors_are_annotated(self): rc, printed = self._run(_StubGithubException(500, {"message": "boom"}))