fix(ci): stop bonk-check from failing on status writes#356
fix(ci): stop bonk-check from failing on status writes#356geoquant merged 1 commit intocloudflare:mainfrom
Conversation
|
/bonk review |
commit: |
There was a problem hiding this comment.
This is a solid fix that addresses the 403 error by removing the problematic commit status API call. The approach of letting the workflow job itself be the pass/fail signal is cleaner and more robust.
Highlights:
- Removing
statuses: writepermission and thegh api .../statuses/...call correctly fixes the 403 error from fork PRs - Adding
pull_request_reviewbody checking fixes the false-negative where/bonkin review comments wasn't recognized - Using
@base64encoding with jq for parsing is much more robust than the previoussedapproach - The
set -euo pipefailaddition improves shell script safety
Minor suggestion: Consider consolidating the duplicate output writes (lines 53-55 and 70-71 both write found=true and bonk_user) into a single location at the end of the script for better maintainability. Not blocking - the current approach works correctly.
|
I've completed the review for PR #356. Here's a summary: Review submitted: View review Assessment: This is a solid fix that properly addresses the failing Bonk Check workflow. The key improvements are:
Minor non-blocking suggestion: The script writes The PR correctly identifies and fixes the root cause - the commit status API requires elevated permissions that aren't available when workflows run from fork PRs. Using the workflow's own pass/fail state as the required check signal is the right approach. |
Docs PreviewCommit: |
|
Closing in favor of a single self-contained follow-up PR that replaces the manual /bonk gate with automatic Bonk reviews on PR open/update. |
There was a problem hiding this comment.
This is a well-structured fix that correctly addresses the root cause of the failing Bonk Check workflow.
Key improvements:
-
Fixes the 403 error - Removing
statuses: writepermission and the commit status API call is the right approach. Fork PRs don't have access to write commit statuses via the API. -
Fixes the false-negative on reviews - Adding
pull_request_reviewbody checking ensures/bonkin review comments is properly recognized, not just issue comments. -
More robust parsing - Using
@base64encoding with jq instead ofsed 's/ ||| .*//'is much safer and handles special characters correctly. -
Better shell practices -
set -euo pipefailand quoted variable expansions improve script reliability. -
Cleaner architecture - Using the workflow job's own pass/fail state as the required check signal is simpler and more idiomatic than manually posting commit statuses.
Minor non-blocking suggestion: Consider consolidating the duplicate output writes (lines 53-55 and 70-71 both write found=true and bonk_user). You could set these once at the end based on the FOUND variable's final state.
Overall, this is a solid improvement that removes the failing code path and fixes the review body detection issue.
| if [ "$STATUS" = "204" ]; then | ||
| if [ "$EVENT_NAME" = "pull_request_review" ] && printf '%s' "$REVIEW_BODY" | grep -q '/bonk'; then | ||
| if is_collaborator "$REVIEW_USER"; then | ||
| FOUND=true |
There was a problem hiding this comment.
Non-blocking suggestion: Consider consolidating these output writes with the identical block at line 70. You could set found and bonk_user once at the end of the script based on the final FOUND value to reduce duplication.
|
I've completed my review of PR #356 and approved it. Here's my assessment: SummaryThis is a well-structured fix that correctly addresses the root cause of the failing Bonk Check workflow. Key Improvements
Minor Non-blocking SuggestionI noted that the output writes ( Overall: Solid improvement that removes the failing code path and fixes the review body detection issue. ✅ |
Fixes failing Bonk Check job: https://github.com/cloudflare/kumo/actions/runs/23924593430/job/69779025414
The Bonk Check workflow was failing in
Post commit statuswithResource not accessible by integration (HTTP 403)when it tried to create a commit status from PR/review contexts.This change removes the manual commit-status API call and lets the workflow job itself be the required pass/fail signal instead. It also fixes a false-negative on
pull_request_review: the old script only scanned issue comments, so/bonkin a review body did not satisfy the check.Specifically, this PR:
statuses: writefrom the workflow permissionsgh api .../statuses/...step that was causing the 403/bonkhas not been called by a collaborator/bonkin review bodies as well as PR commentssedsplitting