fix: warn when a PR comment fails to post, and name the right permission - #254
Conversation
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 <version> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
|
Warning Review limit reached
Next review available in: 52 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR updates pull-request comment failure warnings and tests. It upgrades checkout actions in workflow examples. The README now documents rendered job summaries, scope-based counts, and pull-request comment update and deduplication behavior. ChangesComment handling and documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
d1a7d3a to
2123307
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
main_test.py (1)
1142-1151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the 403 error detail.
This test checks the permission hint, but it does not check
Error: Resource not accessible. The implementation could remove the API error detail and this test would still pass. Add:self.assertIn("pull-requests: write", warning) self.assertNotIn("issues: write", warning) + self.assertIn("Resource not accessible", warning)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main_test.py` around lines 1142 - 1151, Update test_forbidden_names_the_permission_that_actually_grants_this to assert that the warning output includes the API error detail “Error: Resource not accessible”, while preserving the existing pull-requests permission assertion and issues permission exclusion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.py`:
- Around line 884-891: Update the 403 handling in add_pr_comments to normalize
GithubException.data before extracting an error message: only call get when the
payload is mapping-like, and otherwise fall back to str(e) or an equivalent safe
detail. Preserve the warning output and return 0 fallback, and add a regression
test covering a non-mapping 403 payload.
---
Nitpick comments:
In `@main_test.py`:
- Around line 1142-1151: Update
test_forbidden_names_the_permission_that_actually_grants_this to assert that the
warning output includes the API error detail “Error: Resource not accessible”,
while preserving the existing pull-requests permission assertion and issues
permission exclusion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5396b4f7-30b6-4a33-ae80-b3cc371a1d53
📒 Files selected for processing (5)
README.mddocs/fork-pr-comments.mdexamples/commit-check-workflow-a.ymlmain.pymain_test.py
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
bedcc27 to
afedd6f
Compare
One line in `docs/guides/integrations.md`: `actions/checkout@v5` → `@v7`. v5 is two majors behind. This pin is copied verbatim by anyone following the guide, so it seeds every new workflow with a stale action. It was the only `checkout@` pin left in `docs/`. The matching pins in `commit-check-action` (README, `docs/fork-pr-comments.md`, `examples/`) move in commit-check/commit-check-action#254. --- _Generated by [Claude Code](https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn)_ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Updated the GitHub Actions checkout configuration to use the latest supported version. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude <noreply@anthropic.com>
The 403 hint named the wrong permission
add_pr_commentscaught a 403 and told the user to grantissues: write. A PRcomment is written with the pull-requests scope —
issues: writedoes notgrant it, so following the hint does not fix the failure. The hint is the only
guidance a user gets at that point, which is what made it worth fixing.
Every other failure was silent
Posting the comment is best-effort: all three failure paths
return 0so thestep stays green. That is deliberate and unchanged. But the non-403 paths only
printed to stderr, so the outcome was a green run, no comment, and nothing on
the page saying why. They now emit
::warning::like the 403 path already did.main_test.pygainsTestAddPrCommentsFailurescovering all three. Each testwas checked against the un-fixed code first: reverting the permission name
fails the first, reverting either
::warning::prefix fails the other two.98 passed after the fix.
Workflow examples
push:from the README example.pr-commentsonly does anything onpull_request, which madepr-comments: ${{ github.event_name == 'pull_request' }}a conditional that could only ever be true — it is now just
true.actions/checkoutv5 → v7 in the README, bothdocs/fork-pr-comments.mdexamples, and
examples/commit-check-workflow-a.yml. The repo's ownworkflow was already on v7.0.1.
Correction to a commit already on this branch
3751d44("show the report inline instead of linking four missingscreenshots") is on this branch, and its commit message is wrong on the
facts. It claims
commit-check/.github/screenshot/"does not exist and neverhas". It does exist and contains all four PNGs — I had been inspecting a
different repository that happens to share the name
.github.The change itself still stands, for two reasons that do hold:
/blob/URLs, which servetext/html, so Markdown renderedthem as broken images regardless of the files being there.
code block, no result table, no rule IDs. They no longer describe what this
action prints.
Refreshing the four screenshots in
commit-check/.githuband restoring themwith
raw.githubusercontent.comURLs remains an option; it is not in this PR.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
pull-requests: write.Documentation