Post changeset review issues as inline file comments#13026
Post changeset review issues as inline file comments#13026petebacondarwin wants to merge 3 commits intomainfrom
Conversation
When the AI determines a changeset could be improved, post the feedback as a PR review comment attached to the specific changeset file. This creates a resolvable thread that must be addressed before merging. When the changeset is fine, post a normal non-resolvable PR comment. On each re-run, clean up any prior review and marker comments so stale threads don't accumulate.
DO NOT MERGE - remove this commit before merging. - test-good-changeset.md: well-formed patch changeset - test-bad-changeset.md: has h1 header, vague description, questionable version type
🦋 Changeset detectedLatest commit: f774928 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| fi | ||
| echo "Deleting marker comment ${COMMENT_ID}" | ||
| gh api "repos/${REPO}/issues/comments/${COMMENT_ID}" -X DELETE | ||
| done |
There was a problem hiding this comment.
Bug: Submitted reviews cannot be deleted via the REST API.
The GitHub API DELETE /repos/{owner}/{repo}/pulls/{pull_number}/reviews/{review_id} only works on pending (unsubmitted) reviews. The workflow creates reviews with event: COMMENT (line 142), which submits them immediately. This means the DELETE call here will always fail with a 422, and the || true silently swallows the error. Stale review threads will accumulate across pushes — the exact problem the cleanup step is meant to prevent.
To actually remove old reviews, you have two options:
- Dismiss the review via
PUT .../reviews/{id}/dismissals— this doesn't delete the comments but marks the review as dismissed. - Delete each review comment individually — list comments with
GET .../reviews/{id}/comments, thenDELETE .../pulls/comments/{comment_id}for each one. This fully removes the inline threads.
Option 2 is probably what you want since the goal is to remove stale resolvable threads.
| Write your review to changeset-review.md." | ||
| if [ "$STATUS" = "issues" ]; then | ||
| # Build the comments array: one entry per file with issues, anchored to line 1 | ||
| COMMENTS=$(jq '[.files[] | select(.status == "issues") | {path: .path, position: 1, body: .comment}]' changeset-review.json) |
There was a problem hiding this comment.
position: 1 may not be the right field here.
The position field in the reviews API is the 1-indexed offset within the diff hunk (lines after the @@ header), not a file line number. For newly added changeset files this will land on the first content line of the file, which happens to work — but the intent here is really a file-level comment rather than a line-specific one.
Consider using subject_type: "file" instead of position, which creates a file-level review comment that isn't anchored to a specific line:
| COMMENTS=$(jq '[.files[] | select(.status == "issues") | {path: .path, position: 1, body: .comment}]' changeset-review.json) | |
| COMMENTS=$(jq '[.files[] | select(.status == "issues") | {path: .path, body: .comment, subject_type: "file"}]' changeset-review.json) |
This is semantically more accurate and won't break if the diff structure changes.
|
I'm Bonk, and I've done a quick review of your PR. This PR changes the changeset review workflow to post AI feedback as inline PR review comments on specific changeset files (with resolvable threads) instead of using sticky comments, and adds a cleanup step to remove stale reviews across pushes. Two issues found, ranked by severity:
Both comments have been posted as inline review comments on the PR. |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| "wrangler": minor | ||
| --- | ||
|
|
||
| # fix bug |
There was a problem hiding this comment.
🔴 Changeset uses h1 header, violating mandatory changeset formatting rule
.changeset/test-bad-changeset.md:5 uses # fix bug — an h1 (#) header — in the changeset description. AGENTS.md explicitly states under Changeset Rules: "No h1/h2/h3 headers in changeset descriptions (changelog uses h3)". This is also documented in .changeset/README.md:84: "Changeset descriptions must NOT use h1 (#), h2 (##), or h3 (###) headers." If merged, this h1 would break the generated changelog formatting since the changelog already uses h3 for section headers.
| # fix bug | |
| fix bug |
Was this helpful? React with 👍 or 👎 to provide feedback.
- Use --input - with a jq-built JSON body to correctly send the comments array to the GitHub API (--field was serialising it as a string, causing HTTP 422) - Use subject_type: "file" instead of position: 1 for semantically correct file-level review comments - Delete individual review comments rather than the review itself, since the GitHub API only allows deleting pending (unsubmitted) reviews - Add || true to all gh api DELETE calls in cleanup to avoid failing the step on a 404 (already-deleted comment/race condition)
| --- | ||
| "wrangler": minor | ||
| --- | ||
|
|
||
| # fix bug | ||
|
|
||
| update stuff |
There was a problem hiding this comment.
🔴 Test changeset files would trigger unintended wrangler releases if merged
Both .changeset/test-bad-changeset.md ("wrangler": minor) and .changeset/test-good-changeset.md ("wrangler": patch) are test fixtures for workflow validation (commit message says "TEMPORARY") but are included in the PR diff. If merged, they would cause real wrangler version bumps with fake changelog entries. Per AGENTS.md, "CI/workflow changes that don't affect package behavior" do not require changesets, and per .changeset/README.md, changeset descriptions must correspond to actual code changes. test-good-changeset.md describes a fictional bug fix that doesn't exist in this PR, and test-bad-changeset.md uses a vague "fix bug" / "update stuff" description that .changeset/README.md:75 explicitly calls out as a bad example.
Prompt for agents
Delete both .changeset/test-bad-changeset.md and .changeset/test-good-changeset.md before merging. These are temporary test fixtures that would cause unintended wrangler version bumps (a minor and a patch release) with fake changelog entries if they land in main.
Was this helpful? React with 👍 or 👎 to provide feedback.
| gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews/${REVIEW_ID}/comments" \ | ||
| | jq -r '.[].id' | while read -r RC_ID; do | ||
| gh api "repos/${REPO}/pulls/comments/${RC_ID}" -X DELETE || true | ||
| done |
There was a problem hiding this comment.
🔴 Missing error handling on review-comments listing API call can permanently block the workflow
At .github/workflows/changeset-review.yml:70, the gh api call to list a prior review's comments has no || true error handling. GitHub Actions runs bash with set -eo pipefail, so if the referenced review no longer exists (e.g. manually deleted through the UI while the marker comment remains), this call returns a 404 and the entire cleanup step fails. Since cleanup runs before review posting, this blocks the new review from being created. Worse, the orphaned marker comment persists, so every subsequent workflow run will hit the same failure until the marker is manually removed. The individual DELETE calls on line 72 correctly use || true, but the LIST call that feeds them does not.
Problematic code
gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews/${REVIEW_ID}/comments" \
| jq -r '.[].id' | while read -r RC_ID; do
gh api "repos/${REPO}/pulls/comments/${RC_ID}" -X DELETE || true
done| gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews/${REVIEW_ID}/comments" \ | |
| | jq -r '.[].id' | while read -r RC_ID; do | |
| gh api "repos/${REPO}/pulls/comments/${RC_ID}" -X DELETE || true | |
| done | |
| gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews/${REVIEW_ID}/comments" 2>/dev/null | |
| | jq -r '.[].id' | while read -r RC_ID; do | |
| gh api "repos/${REPO}/pulls/comments/${RC_ID}" -X DELETE || true | |
| done || true |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Fixes #[N/A - internal workflow improvement].
When the changeset review AI finds issues, post feedback as PR review comments attached to the specific changeset file(s). This creates resolvable conversation threads that must be addressed before merging. When changesets are fine, post a normal non-resolvable PR comment instead.
Changes
changeset-review.jsonwith per-file status/comments instead of free-form markdowngh apito create a PR review withevent: COMMENTand one inline comment per problematic changeset fileTesting
The second commit adds a good and a bad test changeset to exercise both paths. Remove that commit before merging.
test-good-changeset.md: well-formed patch changeset (should get ✅)test-bad-changeset.md: has an h1 header, vague description, questionable version type (should get inline review comment)