Skip to content

feat(claude-review): converge reviews in fewer rounds - #99

Merged
crowecawcaw merged 3 commits into
aws-deadline:mainlinefrom
crowecawcaw:claude-review-converge
Oct 6, 2026
Merged

crowecawcaw merged 3 commits into
aws-deadline:mainlinefrom
crowecawcaw:claude-review-converge

Conversation

@crowecawcaw

@crowecawcaw crowecawcaw commented Oct 5, 2026 •

Copy link
Copy Markdown

What was the problem/requirement? (What/Why)

The Claude reviewer takes many rounds to settle. Last month: 1,506 bot comments on 174 PRs.

  • Each round brings ~2.5 new comments, however late; up to 18 rounds on one PR.
  • ~40% of later-round comments hit code that existed at round 1, because the prompt asked for one partial pass. The rest hit code written to fix earlier comments.
  • Comments average ~1,600 chars and have no severity.
  • ~15% of runs fail; the ones checked were Dependabot runs.

What was the solution? (How)

  • First review covers the whole PR. Later revisions review only what changed since the last reviewed commit.
  • Later reviews re-check open threads and resolve the ones that are fixed, or where the author's reply checks out.
  • Severities: blocking / should-fix / nit. Nits only on the first review. Comments are capped at ~120 words.
  • Done signal: a sticky summary comment, plus a Claude review commit status.
  • Two jobs:
    • review runs the model with a read-only token and returns structured output.
    • post runs no model; it validates the findings and posts them.
  • 30s debounce; skip superseded heads and bot-triggered runs.
  • Opus 5.5 by default, including for release notes (which now use tool_choice: auto, since 5.5 rejects a forced tool choice).
  • Overlaps with feat: surface Claude PR review progress as a check run #95 (a check run vs. this commit status).

What is the impact of this change?

  • Before merging: the AWS_CLAUDE_PR_REVIEW_ROLE role must allow us.anthropic.claude-opus-5-5.
  • Optional per-repo grants, used only by post:
    • statuses: write for the commit status.
    • contents: write (instead of contents: read) to resolve threads. Without it, threads get an "Addressed" reply and count as closed.
  • A later review is typically ~1 min and $0.10–$0.30.

How was this change tested?

Was this change documented?

Yes, in workflow comments and the script docstring.

Is this a breaking change?

No. Callers need no changes.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

A month of review data (1,506 comments on 174 PRs) showed the reviewer
never settles: every revision gets ~2.5 new comments regardless of round,
~40% of later-round comments hit code that existed at round one, and
comments average ~1,600 chars. The prompt asked for one partial pass per
run, and every push re-reviewed the whole diff.

- The first review is a full, thorough pass. Later revisions are reviewed
  incrementally against the last fully reviewed commit, recorded in a
  sticky summary comment: open threads are re-checked and marked addressed
  when fixed (or when a reply's reason holds up), and only newly changed
  code is reviewed.
- Findings carry a severity (blocking / should-fix / nit). Nits are posted
  only on the full review; incremental should-fix findings must land on
  changed lines. The prompt excludes pre-existing issues and speculative
  scenarios and caps comments at ~120 words.
- A summary comment and a "Claude review" commit status report what is
  still open, so authors and their agents know when to stop. The status
  needs `statuses: write` from the caller; the job now inherits the
  caller's permissions so that grant is opt-in rather than a startup
  failure.
- The agent no longer holds a GitHub token or any write tool. It returns
  findings as structured output (--json-schema); a deterministic script
  validates anchors, dedupes by fingerprint, and posts one batched review.
- Debounce 30s and skip heads a newer push superseded; skip bot-triggered
  runs (Dependabot), which claude-code-action rejects anyway.
- Default to Claude Opus 5.5, also for release notes. Opus 5.5 rejects a
  forced tool_choice, so release notes use auto and keep the existing
  tool-call check.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw
crowecawcaw force-pushed the claude-review-converge branch from 24e984c to bc5d9ff Compare October 5, 2026 23:45
Resolving a review thread needs contents: write, which the job that runs
the model must never hold. Split the workflow: the review job runs with a
read-only token and hands its results over as an artifact; the post job
inherits the caller's grants, posts, and resolves addressed threads when
the caller grants contents: write (otherwise the marked reply alone closes
the thread, as before).

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
… token could resolve them

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
@crowecawcaw
crowecawcaw marked this pull request as ready for review October 6, 2026 16:25
@crowecawcaw
crowecawcaw requested a review from a team as a code owner October 6, 2026 16:25
@crowecawcaw
crowecawcaw merged commit 55f1886 into aws-deadline:mainline Oct 6, 2026
7 checks passed
crowecawcaw added a commit that referenced this pull request Oct 7, 2026
…us 5.5 (#100)

* fix(claude-review): harden posting against forged markers, partial posts, and re-runs

Follow-ups to #99 from review feedback:

- Only trust a review thread's fp marker when the bot wrote its first
  comment. Anyone can open a thread, and the fp format is predictable, so a
  forged marker could suppress the real finding, skew the open counts, or get
  a human's thread replied to and resolved.
- Neutralize HTML comments in model text before posting, so a marker the PR
  induces the model to write cannot outrank the one we append.
- A partial post no longer counts as a finished review: the baseline stays
  put, the status is an error, and the summary lists the findings that did
  not land.
- Findings can anchor to removed code (side LEFT), so a finding on a deleted
  file is no longer dropped for want of a new-side line.
- Name the review artifact per run rather than per attempt, so re-running
  only a failed post job finds it, and re-read the PR's threads before
  posting so that re-run does not duplicate the comments that landed.
- Release notes: auto tool choice can return plain text; retry up to three
  times with a reminder instead of failing on the first text answer. An
  empty entries list is a valid result.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>

* fix(claude-review): bump claude-code-action to v1.0.244 for Opus 5.5

v1.0.136 bundles Claude Code CLI 2.1.163 (claude-agent-sdk 0.3.163), which
Bedrock rejects for us.anthropic.claude-opus-5-5 with a 400: version 2.1.280
or newer is required. v1.0.244 bundles CLI 2.1.292. The action's inputs are
unchanged between the two versions.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>

* fix(claude-review): allow non-write workflow_run actors

claude-code-action v1.0.244 checks write access for workflow_run events, for both github.actor and workflow_run.actor. On fork PRs that's the fork author, who never has write access, so every external review failed before Claude ran. The review job's token is read-only, Claude only gets read tools, and posting happens in the separate post job, so bypassing the check is safe here.

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>

---------

Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants