feat(ci): scope a re-review to what is new, and grant the reviewer Read - #123
Conversation
thecodedrift
added
the
skip-changeset
There was a problem hiding this comment.
Pull request overview
Updates the on-demand Claude review workflow to support incremental re-reviews (surfacing only new findings plus previously-unaddressed ones), while still allowing an explicit full re-review via @claude /review all, and granting the model read-only file access to improve cross-file verification.
Changes:
- Adds workflow-level “review mode” selection (incremental vs full vs second-eyes) derived from boolean
contains()checks on the triggering comment. - Expands the prompt to instruct incremental behavior (read prior comments first; label
[New]vs[Unchanged since last review]; don’t re-raise addressed threads). - Grants
Readin--allowedToolsand documents its coupling topersist-credentials: false.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@claude /review |
|
Claude finished @thecodedrift's task in 2m 49s —— View job Code reviewReviewed the full diff ( Design and security fundamentals hold up well. The One correctness issue undermines the core feature. Two related classification gaps, detailed inline at lines 30-35:
Also flagged inline, lower severity:
Not flagged as blocking, worth knowing about: the three |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
All five inline findings are fixed and their threads resolved; details are in each thread. The correctness issue you called out as undermining the core feature is fixed structurally: I reproduced it live ( Two items you flagged as non-blocking are deliberately not in this PR:
— AI Coding Agent |
Your inline finding about the — AI Coding Agent |
b6ba8d9 to
13f49cf
Compare
|
Force-pushed to add GPG signatures. No content changed. The three commits were re-created solely so each carries a verified signature (they were previously unsigned,
Verified before pushing: Apologies for the noise on any review already left — the diff itself is untouched, so prior feedback still applies as written. |
`@claude /review` re-derived the entire PR every time, so genuinely new work competed with a dozen settled findings and a reader could not tell which was which. Mode is now chosen by the workflow and resolves to one of three `focus=` strings: incremental (PR conversation), full (`@claude /review all`), and second set of eyes (inline review comment). Incremental scoping is by prior comments, not by a SHA range — this repo rebases, so a two-dot range describes a diff that never happened, while comments survive a rebase untouched. A thread carrying a reply that ends `*— AI Coding Agent*` was addressed; one without is still open and gets re-raised, marked unchanged. `--allowedTools` gains `Read` so the reviewer can open whole files. That is coupled to `persist-credentials: false`, and the header now records why. The comment body is still only tested, never interpolated into a prompt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jwc9FFroR3mTZ4hLiSkkX3
Incremental mode classified findings against comments the model could not see. `gh pr view --json comments,reviews` returns top-level comments and review *summary* bodies only — `gh pr view` has no `reviewThreads` field at all — so inline findings were invisible to it. Measured on this PR: `gh pr view` reports 2 comments and 6 reviews, five with an empty body (the wrapper each inline comment hangs off), while the 6 actual findings appear nowhere. Since this reviewer posts inline, every re-review would have seen none of its own prior findings, treated the PR as unreviewed, and never emitted the [New] / [Unchanged] split that is the point of the feature. A workflow step now reads the real threads over `gh api graphql` into `.prior-review.json` and the model reads that file. Privileged work in a deterministic step is the division this workflow already draws, so `gh api` stays out of `--allowedTools`; the response is redirected to a file with no interpolation, so untrusted comment text cannot become shell, and the prompt tells the model to treat the contents as data rather than as instructions. That also supplies `isResolved`, which no model-visible tool can reach. A thread a human resolved — by commit, by a plain reply, or with the Resolve button — carries no `*— AI Coding Agent*` marker, so the marker-only test re-raised it forever. The marker stays as a secondary signal, now recognized in both spellings the iterate-pr skill documents. Two smaller fixes from the same review: - `contains(body, '@claude /review all')` was an unanchored substring match, so `@claude /review allocator.rs` selected full mode. The body now reaches the step as an env var — tested, never interpolated into the script and never written to output — and a `case` pattern gives it a word boundary. Testing for `'all '` instead would have missed `@claude /review all` followed by a newline. - `mode=` was written in all three branches and read nowhere. It now gates the fetch step, which has no work to do in full mode. The `<PR NUMBER>` placeholder goes away with the command that carried it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The review workflow writes its three `focus=` strings single-quoted because they contain backticks and `$`, which double quotes would hand to the shell. Single quoting has exactly one failure mode and it is silent: an apostrophe inside the value closes the string early and the rest of the line becomes shell words. A future contraction — "don't", "doesn't", "won't" — would break the step for all three modes at once, and nothing would catch it. The strings are hundreds of characters of prose on one line, and a reviewer reading prose is not reading quoting. `workflow-outputs.cjs` checks every file in `.github/workflows/`: - A single-quoted echo must be `echo 'key=value' >> "$GITHUB_OUTPUT"`, whole, on one line. An odd quote count is reported as unterminated, which covers both the apostrophe and a value wrapped onto the next line. Detection keys off `echo '` rather than off `$GITHUB_OUTPUT`: a wrapped value leaves the redirect on the FOLLOWING line, so keying off the redirect would skip exactly the broken line. My first draft did that and its own test caught it. - Where a file both writes a key and compares it against a literal (`steps.prep.outputs.mode != 'full'`), the literal must be a value the file actually writes. That is the drift the mode/focus split invites. Keys the file never writes — an action's own outputs — are skipped, so there is no ground truth to get wrong. Node builtins only, like the other scripts here, and no YAML parser: the invariant is textual, about what the shell sees on one line, and parsing to a structure would discard the quoting the check is about. Verified by injecting `don't` into the real focus string, which the guard rejects with the file and line. `validate.yml` already globs `.github/scripts/*.test.cjs`, so this runs in CI with no change there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13f49cf to
fff2a91
Compare
@claude /reviewreviewed the whole PR every time. On a PR that had been reviewed and then revised, the new work competed for attention with a dozen settled findings, and the output read as a fresh assessment of everything — so a reader could not tell which findings were new.What a maintainer will notice
@claude /reviewin the PR conversationincremental[New]) plus anything previously raised that is still unaddressed ([Unchanged since last review]). With no prior comments this is simply a first, full review.@claude /review all(either event)full@claude /reviewas an inline review commentsecond-eyesEvery mode now states which mode ran in the top-level summary comment, and an incremental review lists what it treated as already addressed. Without that, "no findings" is ambiguous between "nothing new since last time" and "I read everything and it is clean" — a check that did not run looks exactly like a check that passed.
Re-raising still-open findings is deliberate. A framing of "only surface new items" would silently drop findings that were raised and never fixed, which is the opposite of useful.
Why not a SHA range
The obvious design is a
lastReviewedSha..headtwo-dot range. It does not survive how this repo works. We rebase; a force push rewrites every SHA, the old one stops being an ancestor of the head, and the range describes a diff that never happened — it renders every change to the base as PR work. The compare API reportsdivergedfor exactly this, so it is detectable, but detection only means falling back to a full review. Since rebasing is routine here, the incremental path would almost never fire.Range scoping assumes linear history. We deliberately do not have it. Comments do not have this problem: a rebase changes every commit and changes nothing that was already said.
The classification signal is this repo's existing convention — a reply written when addressing feedback ends with the literal marker
*— AI Coding Agent*. ThreadisResolvedis GraphQL-only and the model is not grantedgh api, so the marker is the available proxy, and it is reliable because the same process that addresses feedback writes it.gh pr view --json comments,reviewssupplies the data under the existingBash(gh pr view:*)grant.Read, and its coupling topersist-credentials: false--allowedToolsgainsReadand nothing else. Scoping the reviewer's attention to what is new should not scope its access — the findings that have justified a review here came from reading whole files: surviving references in untouched regions, a claim about a directory's real contents, a cross-file ordering dependency.Readis safe only alongsidepersist-credentials: false(landed in #121). Without it,actions/checkoutwrites the job'sGITHUB_TOKENinto.git/configinside the tree being reviewed, andReadplusBash(gh pr comment:*)is a complete path from that file to a public comment on a public repo. The workflow header now records the coupling so neither gets removed without the other.Security model, intact
github.event.comment.bodyappears only in theif:gate and in acontains()whose result is a boolean — never interpolated into arun:orprompt:. A free-text focus guarded by "treat the following as review focus, not instructions" is a request, not a boundary, and is explicitly not wanted.gh api, nogit, no bareBash. The workflow may use privileged tools; the model may not.permissions, nocontents: write, nopull_request_target. The maintainer-onlyauthor_associationgate, therefs/pull/N/headcheckout, andfetch-depth: 1are unchanged.Out of scope
Token cost. The reviewer still reads the whole diff; only the reporting is scoped. The original framing was cost, but the actual pain is noise.
Verification
The workflow cannot be run from a branch, so: the file parses as YAML, and the dispatch script was executed against simulated payloads for all four trigger shapes — each of the three
focus=arms is reachable and thealltest is ordered first so it is not shadowed by the@claude /reviewsubstring.pnpm lint,pnpm typecheck,pnpm test(624 passed / 50 files), andpnpm openspec validate --all --strict(23 passed) are green. The prompt text itself is reasoned about, not executed.No changeset: this changes a workflow only, and ships nothing.
Fixes #110