fix(ci): auto-run bonk for pull requests#357
fix(ci): auto-run bonk for pull requests#357geoquant wants to merge 7 commits intocloudflare:mainfrom
Conversation
commit: |
Docs PreviewCommit: |
|
/bonk review |
|
@geoquant Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bonk review again |
There was a problem hiding this comment.
This PR implements a well-designed auto-Bonk system that removes the manual /bonk requirement while maintaining backward compatibility. Overall this is a solid approach.
What works well:
- Clean separation between
bonk-check.yml(orchestrator) andbonk.yml(executor) - Using
pull_request_targetproperly avoids exposing secrets to untrusted code - The comment-deduplication logic prevents noise on PR updates
token_permissions: NO_PUSHis the right call for automatic runs- Preserving manual
/bonkcommands for human-triggered reviews
Minor observations (non-blocking):
-
Concurrency group edge case: When a PR is opened and quickly updated, both
openedandsynchronizeevents could fire and dispatch two separate Bonk runs since concurrency is managed withinbonk.yml, not across the dispatch boundary. This is probably fine given Bonk idempotency, but worth noting. -
Hardcoded workflow filename: The dispatch in
bonk-check.ymlreferencesbonk.ymldirectly. If this file is ever renamed, the dispatch will fail silently. Consider whether this needs to be more resilient. -
Missing checkout comment: The PR description mentions "checks out the repo for automatic Bonk runs" but
bonk-check.ymldoesn't actually checkout code - it only dispatches. The checkout happens inbonk.ymlwhich is correct sincepull_request_targetcheckout behavior has security implications. The description is slightly misleading but the implementation is correct.
One clarification question:
The bonk-check job no longer posts a commit status. If the repo ruleset currently requires the bonk-check context, won't this break branch protection? Or is the plan to update the ruleset when this merges? The PR description says it "keeps that context alive" but I don't see a status being posted anymore.
| pull_request_review: | ||
| types: [submitted] | ||
| pull_request_target: | ||
| types: [opened, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
pull_request_target is the right choice here since this workflow only dispatches another workflow and doesn't execute untrusted code. This maintains access to GITHUB_TOKEN for the dispatch API call.
| fi | ||
| fi | ||
| done <<< "$COMMENTS" | ||
| existing_comment_id=$(gh api "repos/${{ github.repository }}/issues/$PR_NUMBER/comments" --paginate \ |
There was a problem hiding this comment.
Nice approach for comment deduplication - using the HTML comment as a marker allows updating the same comment on PR updates instead of creating noise with new comments each time.
| fetch-depth: 1 | ||
|
|
||
| - name: Setup pnpm | ||
| if: github.event_name != 'workflow_dispatch' |
There was a problem hiding this comment.
Good conditional skipping here. The workflow_dispatch path doesn't need pnpm/node setup since it relies on the ask-bonk action to handle its own environment.
| model: "cloudflare-ai-gateway/workers-ai/@cf/moonshotai/kimi-k2.5" | ||
| agent: kumo | ||
| permissions: any | ||
| token_permissions: NO_PUSH |
There was a problem hiding this comment.
Using NO_PUSH for automatic reviews is the correct security posture. This ensures the auto-triggered review can only submit PR reviews, not push commits or make code changes.
| agent: kumo | ||
| permissions: any | ||
| token_permissions: NO_PUSH | ||
| prompt: | |
There was a problem hiding this comment.
The prompt explicitly instructs submitting a real PR review rather than just outputting to stdout - this addresses a key gap in automated AI review workflows where the review often just prints and doesn't actually submit back to GitHub.
|
I've submitted a review on PR #357. Here's a summary of my findings: Review SummaryOverall Assessment: This is a well-designed implementation of automatic Bonk reviews. The architecture is clean with proper separation of concerns between the orchestrator ( ✅ What Works Well
|
|
/bonk review again |
Fixes the manual
/bonkworkflow gap for pull requests.This PR is the real auto-Bonk fix. It replaces the current human-triggered Bonk gate with an automatic Bonk review flow on PR open/update while keeping the existing
bonk-checkcontext compatible with the current repository ruleset.Specifically, this PR:
bonk-checkfrom a manual/bonkgate into apull_request_targetwrapper that auto-runs for PR opens/updates/bonk reviewcomment on the PR for transparencybonk.ymlautomatically so Bonk reviews the PR without a human typing/bonkworkflow_dispatchsupport tobonk.ymlso the wrapper can invoke Bonk directly/bonkcomments working for issue comments and PR review commentspull_request_reviewtriggers frombonk.ymlandbigbonk.ymltoken_permissions: NO_PUSHfor the automatic Bonk path so the auto-run stays review-onlydispatch_refoverride inbonk-check.ymlfor pre-merge smoke tests; normal post-merge behavior dispatches against the PR base refNotes:
bonk-checkbehavior.bonk-checkcontext. This PR keeps that context alive by havingbonk-checkact as the automatic wrapper instead of enforcing a human collaborator comment.bonk-checkas a required check entirely after this merges, that still requires a GitHub ruleset/settings change outside the repo./bonk reviewcomment plus an automatic Bonk review on a PR targetingmain, while remaining blocked by the current legacymainprotections until this PR lands.