feat(ci): fix what the review finds, in the same run - #199
Conversation
Two people work this repo with separate agent setups, and the review→fix handoff was a person reading a comment and telling their agent to go. That is a notification, not automation — someone still has to be at a desk. So do the fixing in CI. claude-code-review.yml gains a second job: the review job publishes how many inline comments landed on the current head commit, and the fix job runs only when that is above zero, checks out the PR branch, addresses the comments and pushes. Nobody starts either one. It cannot loop, structurally: the fix job pushes with GITHUB_TOKEN, and GitHub does not start workflows from GITHUB_TOKEN pushes, so the fix commit triggers no second review and therefore no second fix. One review, one fix pass, then it waits for a person. A round guard on [agent-fix] commits is the backstop if anyone later swaps in a PAT to get re-review chaining, and trips agent:needs-human rather than going again. The fixer is told to address only what the review raised and to skip comments it judges wrong rather than comply — automated review is wrong sometimes, and an unattended agent widening its own scope is how a small PR becomes an unreviewable one. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The action defaults to claude-sonnet-5 and neither job set a model, so both were running on the default. Pin both to claude-opus-5. Review quality is the entire point of the review job — a defect it misses costs more than the tokens it saved. The fix job matters more still: it writes code and pushes it with nobody watching, so its judgement about which review comments are worth acting on, and which are wrong, is the thing standing between an unattended agent and a bad commit. Note for quota: Opus draws down the Max subscription faster than Sonnet, and each PR can now run both jobs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
First live run: claude-review posted 5 inline comments, claude-fix triggered correctly, and then did nothing — its injected PR context arrived with no review comments at all, so it reported 'no inline review comments were present' and declined to guess. That was the right call by the fixer and a real gap in the pipeline: the trigger said there were findings, the payload did not carry them. Collect the comments for the head commit in a workflow step and write them to .review-findings.json, which the prompt names as the authoritative list. Two reasons to prefer that over letting the fixer query GitHub: it is deterministic, and it keeps unscoped gh out of the allowlist of the one job that writes code unattended. The file is added to .git/info/exclude so it cannot be committed. The prompt now also says to stop if the file is empty, rather than going looking for things to fix. Turned on show_full_output for this job too — the 9 permission denials behind the first failure were invisible without it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A fix job woken with findings that produces no commit exits 0 and shows the same green tick as one that fixed everything — which is exactly what happened on the first live run. Compare the head before and after; if it did not move, warn, label agent:needs-human, and fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Claude finished @MaryWylde's task in 2m 44s —— View job Review — PR #199
No application code changed here — the diff is CI YAML plus docs — so the React/TypeScript/SCSS checks in my brief don't apply. I reviewed this as workflow security and control flow instead. OverallThe design is sound and the writing around it is unusually honest: the reviewer's Blocking1. The findings list isn't filtered by author — .github/workflows/claude-code-review.yml:227
2. The loudness check reads the local HEAD, not the remote — .github/workflows/claude-code-review.yml:296
Worth calling out in the PR body as a known limit: the fixer can never repair 3. AGENTS.md asserts a guarantee the PR body says isn't true — AGENTS.md:509 Docs: "GitHub does not start workflows from Worth checking before merge4. Smaller notes, not blocking
What I did not checkI couldn't fetch |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | ||
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\") | ||
| | {path, line: (.line // .original_line), body}]" \ | ||
| > .review-findings.json |
There was a problem hiding this comment.
Security: the findings list is not filtered by author.
repos/$REPO/pulls/$PR/comments returns every inline review comment on the PR, from any account. On a public repo, any GitHub user can leave one. The only filter here is commit_id, so a third party's comment lands in .review-findings.json as an authoritative "finding" — and the prompt at line 250-252 tells the fixer that file is the authoritative list.
The chain is reachable without the attacker triggering anything: comment on an open PR, wait for the author's next push. The review runs, the count at line 151 (same missing filter) includes the planted comment so claude-fix fires, and the fixer applies it with contents: write and Edit/Write in its allowlist. The reviewer's own allowlist is carefully scoped against exactly this class of problem (lines 117-121); the job that actually writes code should get the same treatment.
Filtering on the bot author closes it in both places:
| gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | |
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\") | |
| | {path, line: (.line // .original_line), body}]" \ | |
| > .review-findings.json | |
| gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | |
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\" | |
| and .user.login == \"github-actions[bot]\") | |
| | {path, line: (.line // .original_line), body}]" \ | |
| > .review-findings.json |
If human findings should be actionable, that's a reasonable product call — but then it wants to be a deliberate allowlist (collaborators with write access), not "anyone who can type in a comment box".
| after=$(git rev-parse HEAD) | ||
| echo "head before: $BEFORE" | ||
| echo "head after: $after" | ||
| if [ "$after" = "$BEFORE" ]; then | ||
| echo "::warning::claude-fix was woken with findings but pushed nothing." | ||
| gh pr edit "$PR" --repo "$REPO" --add-label "agent:needs-human" 2>/dev/null || true | ||
| exit 1 | ||
| fi | ||
| echo "pushed $(git rev-list --count "$BEFORE..$after") commit(s)" |
There was a problem hiding this comment.
This checks the local HEAD, so it can't see a failed push.
git rev-parse HEAD moves as soon as the fixer commits. Whether the push landed is never consulted, so the one failure this step exists to catch — the fixer produced a commit that never reached the branch — reports pushed 1 commit(s) and a green tick.
That isn't hypothetical on this very PR: GITHUB_TOKEN cannot push changes under .github/workflows/ (no workflow scope), so a fix job triggered by a review of this file commits locally, gets rejected at push, and this step congratulates it. Comparing the remote ref catches both that and any other push rejection:
| after=$(git rev-parse HEAD) | |
| echo "head before: $BEFORE" | |
| echo "head after: $after" | |
| if [ "$after" = "$BEFORE" ]; then | |
| echo "::warning::claude-fix was woken with findings but pushed nothing." | |
| gh pr edit "$PR" --repo "$REPO" --add-label "agent:needs-human" 2>/dev/null || true | |
| exit 1 | |
| fi | |
| echo "pushed $(git rev-list --count "$BEFORE..$after") commit(s)" | |
| branch="${{ github.event.pull_request.head.ref }}" | |
| git fetch origin "$branch" --quiet | |
| after=$(git rev-parse "origin/$branch") | |
| echo "head before: $BEFORE" | |
| echo "head after: $after" | |
| if [ "$after" = "$BEFORE" ]; then | |
| echo "::warning::claude-fix was woken with findings but pushed nothing." | |
| gh pr edit "$PR" --repo "$REPO" --add-label "agent:needs-human" 2>/dev/null || true | |
| exit 1 | |
| fi | |
| echo "pushed $(git rev-list --count "$BEFORE..$after") commit(s)" |
Worth noting separately in the PR description too: this workflow is self-modifying-proof — the fixer can never repair .github/workflows/*, so findings on workflow files always need a human.
| findings=$(gh api "repos/$REPO/pulls/$PR/comments" --paginate \ | ||
| --jq "[.[] | select(.commit_id == \"$HEAD_SHA\")] | length") | ||
| echo "inline findings on $HEAD_SHA: $findings" | ||
| echo "findings=${findings:-0}" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
Two things about this count, beyond the author filter noted on the collection step:
-
commit_idis not stable per review round. GitHub re-maps a still-valid review comment onto the new head after a push, so comments from an earlier round can keep matching$HEAD_SHA. If that holds, a PR whose second review is clean can still wakeclaude-fixwith findings the previous round already fixed, and the fixer either re-applies no-ops or gets told to fix already-fixed code. Cheap to confirm on a probe PR: comment on round one, push, and printcommit_idhere on round two. If it does re-map, filtering oncreated_at > <run start>or on the review ID this run produced is the sturdier signal. -
The count is a proxy for "the review found something". A comment the reviewer left as praise or a question counts as a finding and starts a code-writing job. Not a blocker — the fixer is told to skip what it judges wrong — but it means the trigger is noisier than the prose above it claims.
| **Why this cannot loop.** The fix job pushes with `GITHUB_TOKEN`, and GitHub | ||
| does not start workflows from `GITHUB_TOKEN` pushes. So the fix commit does not | ||
| trigger a second review, which means it cannot trigger a second fix. One review, | ||
| one fix pass, then it stops. A re-review happens when a person next pushes. |
There was a problem hiding this comment.
This states the strong guarantee, but the PR description states the observed one — and they disagree:
That is a weaker guarantee than "workflows are never triggered": approving those runs would start a review on the agent's commits, which could hand off to another fix pass.
What you actually saw on #198 is GitHub parking the runs at action_required pending maintainer approval, not declining to create them. The distinction matters precisely for the person this section is written for: someone who reads "does not trigger a second review", then sees a queued run awaiting approval and clicks it, believing the docs rule out a chain. The round guard still holds, but they'd be approving something the docs told them couldn't exist.
The workflow comment at lines 165-169 carries the same claim and wants the same correction. Suggest matching the PR body's framing:
| **Why this cannot loop.** The fix job pushes with `GITHUB_TOKEN`, and GitHub | |
| does not start workflows from `GITHUB_TOKEN` pushes. So the fix commit does not | |
| trigger a second review, which means it cannot trigger a second fix. One review, | |
| one fix pass, then it stops. A re-review happens when a person next pushes. | |
| **Why this cannot run away.** The fix job pushes with `GITHUB_TOKEN`. GitHub | |
| parks the resulting workflow runs at `action_required`, pending maintainer | |
| approval, because the pushing actor is a bot — so no fix commit starts a review | |
| on its own. Approving one *would* start a review on the agent's commits, which | |
| can hand off to another fix pass; each pass's push is gated the same way, so it | |
| cannot run unattended. A re-review otherwise happens when a person next pushes. |
github-actions
Bot
added
the
agent:review-ready
|
Claude finished @MaryWylde's task in 2m 57s —— View job Fix pass — 1 pushed, 2 blocked, 1 skippedRead
The blocker, which the review itself predictedFinding #2 said this out loud:
That is not a hypothesis any more: So the fixer can never repair its own workflow file. Findings on There's a second-order irony worth catching before merge: this job's own "Did it actually change anything" step will show green, because it reads local 1. Author filter — correct, fixed, unpushableReal and reachable: I could not verify the bot login empirically ( I did not act on the finding's alternative — allowlisting write-access collaborators — since that's a product call, not a defect fix. 2. Push verification — correct, fixed, unpushable
One deviation from the suggested patch: I used an explicit refspec rather than a bare --- a/.github/workflows/claude-code-review.yml
+++ b/.github/workflows/claude-code-review.yml
@@ -148,8 +148,13 @@ jobs:
HEAD_SHA: ${{ github.event.pull_request.head.sha }}
REPO: ${{ github.repository }}
run: |
+ # Filter on the bot author as well as the commit. This endpoint returns
+ # every inline comment on the PR from any account, and on a public repo
+ # anyone can leave one — without that filter a stranger's comment counts
+ # as a finding and wakes the job that writes code unattended.
findings=$(gh api "repos/$REPO/pulls/$PR/comments" --paginate \
- --jq "[.[] | select(.commit_id == \"$HEAD_SHA\")] | length")
+ --jq "[.[] | select(.commit_id == \"$HEAD_SHA\"
+ and .user.login == \"github-actions[bot]\")] | length")
echo "inline findings on $HEAD_SHA: $findings"
@@ -224,8 +229,12 @@ jobs:
REPO: ${{ github.repository }}
run: |
+ # Same author filter as the count step, and for the same reason: this is
+ # the list the fixer is told to treat as authoritative, so anything that
+ # is not the reviewer's own comment must not reach it.
gh api "repos/$REPO/pulls/$PR/comments" --paginate \
- --jq "[.[] | select(.commit_id == \"$HEAD_SHA\")
+ --jq "[.[] | select(.commit_id == \"$HEAD_SHA\"
+ and .user.login == \"github-actions[bot]\")
| {path, line: (.line // .original_line), body}]" \
> .review-findings.json
@@ -293,7 +302,12 @@ jobs:
BEFORE: ${{ github.event.pull_request.head.sha }}
run: |
- after=$(git rev-parse HEAD)
+ # Compare the remote ref, not local HEAD: HEAD moves as soon as the
+ # fixer commits, so a commit that was rejected at push would report
+ # success here — the one failure this step exists to catch.
+ branch="${{ github.event.pull_request.head.ref }}"
+ git fetch --quiet origin "+refs/heads/$branch:refs/remotes/origin/$branch"
+ after=$(git rev-parse "refs/remotes/origin/$branch")
echo "head before: $BEFORE"
echo "head after: $after"
if [ "$after" = "$BEFORE" ]; then3.
|
…rantee The review section claimed GitHub does not start workflows from GITHUB_TOKEN pushes. What was actually observed on the probe PR is GitHub parking those runs at action_required pending maintainer approval — it creates them, it just will not start them unattended. The difference matters for the reader this section is written for: someone who takes 'cannot loop' literally, then sees a queued run awaiting approval and clicks it, believing the docs rule out a chain.
1 participant
Today the review→fix handoff is a person reading a comment and telling their agent to go. That's a notification, not automation — someone has to be at a desk. This does the fixing in CI instead.
claude-code-review.ymlgains a second job:claude-reviewreviews the PR, leaves inline comments, and publishes how many landed on the current head commit.claude-fixruns only when that count is above zero — checks out the PR branch, addresses the comments, commits with[agent-fix]in the subject, and pushes.Nobody starts either one. Open a PR and both run.
Proven, not assumed
Exercised end to end on a throwaway probe PR (#198, closed, not merged) carrying a component with deliberate defects. Second run, on Opus:
It caught all five planted defects, stayed inside the probe files, and applied the AGENTS.md barrel convention (
index.tsx→index.ts, default export) rather than just patching the symptom.The first run failed, and that's why two of these commits exist. The fixer triggered correctly and then did nothing: the action's injected PR context arrived with no review comments, so it reported "no inline review comments were present" and declined to guess. Right call by the fixer, broken plumbing around it. Findings are now collected in a workflow step and written to
.review-findings.json, which the prompt names as the authoritative list — deterministic, and it keeps unscopedghout of the allowlist of the one job that writes code unattended.Why it can't run away
The fix job pushes with
GITHUB_TOKEN. In practice those pushes do not run: GitHub parks them ataction_requiredpending maintainer approval, because the pushing actor is a bot. Observed, not assumed — see the two gated runs on #198.That is a weaker guarantee than "workflows are never triggered": approving those runs would start a review on the agent's commits, which could hand off to another fix pass. It still cannot loop unattended, since each pass's push is gated the same way. Behind that sits a round guard — two
[agent-fix]commits on a PR and the job labelsagent:needs-humanand stops.Failing loudly
A fix job that produces nothing exits 0 and shows the same green tick as one that fixed everything — exactly what happened on the first run. The last step compares head before and after; if it didn't move, it warns, labels
agent:needs-human, and fails.Judgement, deliberately constrained
The fixer is told to touch only what the review raised, and to skip a comment it judges wrong rather than comply — automated review is mistaken sometimes, and an unattended agent that "improves" adjacent code is how a small PR becomes unreviewable. Read its summary before merging; a fix pushed unattended still needs a human to agree with it.
Both jobs are pinned to
claude-opus-5(the action defaults to Sonnet). Opus draws down the Max subscription faster, and each PR can now run two Claude jobs.Test plan
claude-reviewcomments andclaude-fixpushes an[agent-fix]commitclaude-fixdoes not run at allaction_requiredruns after a fix push stay parked🤖 Generated with Claude Code