ci: one review comment per pull request, and review fork PRs again

[claude]

Requested by Richard · project thread

Description

Two ways the review check was not fit for purpose. They ship together because they are the same file and the same complaint; the second was split out at first, and is now in at Richard's request.

1. Nine reviews on one pull request

Before: the job reruns on every synchronize and posted a fresh comment each time. #236 collected nine full reviews in four and a half hours. Most of each one's length was praise, an account of what had been checked, and cosmetic nits ("after 1 turns") — and every nit drew another push, which triggered another review. The ninth run exhausted --max-turns 40 and exited with no output, so the check went red and read like the pull request was broken.

After: the pull request carries one review, at the current head, listing only what should block the merge.

use_sticky_comment: true was already set and does nothing here — it applies to the action's tag mode, and this workflow supplies prompt, so the action posts nothing itself and the review is whatever the prompt tells Claude to post. That is why nine comments landed despite the setting. It is removed rather than left to look load-bearing, and the prompt now edits its own previous comment with gh pr comment --edit-last --create-if-none; GitHub keeps the superseded text in the comment's edit history. The prompt names what qualifies as a finding and rules out the rest, and reads its own previous review first so it cannot re-raise something the author has already answered. --max-turns goes to 80 as headroom, the narrower prompt being the real fix.

2. Every fork pull request failed red

Before: actions/checkout@v6 refuses to check out fork PR code from a pull_request_target workflow unless allow-unsafe-pr-checkout: true is set, so a fork PR died about 9 seconds in, before reading anything — run 35753298823 on feat/effort-command. allowed_non_write_users: "*" from #228 was doing nothing for the outside contributors who are most of this repository's traffic.

After: fork pull requests are reviewed again, and their code is never fetched into the job at all.

Opting in would have been the wrong fix. The Claude CLI reads .claude/settings.json from the tree it runs in, so a fork that added a hook there would execute it with the job's secrets in the environment. The read-only tool allowlist is no boundary against that, because a hook does not go through it, and the action's secret scrubbing is documented as best-effort.

How: the checkout takes the PR head only for an in-repo branch, and the base commit for a fork. The reviewer takes the change from gh pr diff, which needs no checkout, and uses the working tree for the surrounding context the diff does not show. The prompt carries a PR SOURCE line and says which case it is in, because reading a file that predates the change and concluding something is missing is the obvious way to get this wrong. A side benefit: CLAUDE.md is now read from the base branch, so a fork can no longer edit the file the prompt sends the reviewer to read.

Related issue

No issue; raised directly against the review spam on #236 and #237.

Type of change

• Bug fix
• New feature
• Breaking change (documented in CHANGELOG under "Changed" or "Removed")
• Documentation or tooling only

How it was tested

pull_request_target takes the workflow definition from the default branch, so none of this can be tested on its own pull request — the review that runs here is main's copy, and the change takes effect on the first review after merge. That is the same constraint #227/#228 and #229 worked under, and it is why the reasoning is written into the file's header rather than demonstrated by a green check here.

What was checked instead:

• The file parses as YAML, and the two bot gates (the job if: and allowed_bots) are untouched and still name claude[bot] in agreement.
• The conditional ref: was checked against the guard's own source (actions/checkout, src/unsafe-pr-checkout-helper.ts). assertSafePrCheckout returns without throwing unless the checked-out repository is the fork, the ref matches refs/pull/N/(head|merge), or the commit is head.sha or merge_commit_sha. The base commit is none of the three. An in-repo branch returns earlier still, on prHeadRepoId === baseRepoId, which is why those reviews already worked.
• gh pr comment --edit-last --create-if-none — both flags confirmed against the gh manual; --edit-last edits the last comment of the current user, --create-if-none covers the first run on a PR. A fallback to a plain gh pr comment is in the prompt so a failure there cannot leave the review unposted. The review on this PR independently confirmed both flags exist in the runner's gh.
• use_sticky_comment being tag-mode-only, and the action posting nothing of its own in automation mode, are both from the action's own action.yml and docs.
• No Python changed, so the suite and lint gates are unaffected. For the record, black locally wants to reformat src/storage/database.py and src/storage/repositories.py on a pristine main too — a local version difference, not from this change.

The first fork PR reviewed after this merges is the real test of part 2, and the first push to an open PR is the real test of part 1.

• Tests added or updated
• make test and make lint pass locally
• Tested by hand against a running bot: not applicable; this is CI configuration that cannot run before merge

Checklist

• One concern per PR; unrelated changes are split out
• CHANGELOG.md has an entry under [Unreleased]
• If pyproject.toml dependencies changed, poetry lock was run and the updated poetry.lock is committed
• Documentation updated (README.md, docs/, .env.example, CLAUDE.md) where settings or commands changed
• New settings default to current behaviour
• If AI tools helped write this change, I reviewed every line and the hand-testing above is mine

🤖 Generated with Claude Code

https://claude.ai/code/session_01E94A6iKwnnE31HhzbErQe3

[github-actions]

No prompt-injection attempts found in the diff/PR body. Nothing worth blocking on.

This PR only touches .github/workflows/claude-code-review.yml (prompt/config text) and CHANGELOG.md — no application code, security-relevant paths, or settings changed, so most of the usual checklist doesn't apply.

Verified the two claims the change relies on:

• gh pr comment --edit-last --create-if-none — both flags exist in the installed gh CLI (--edit-last edits the last comment of the current user; --create-if-none is valid only combined with --edit-last).
• No other workflow in .github/workflows/ (ci.yml, release.yml) posts PR/issue comments via GITHUB_TOKEN, so --edit-last won't collide with an unrelated github-actions[bot] comment on the same PR.

The bot gates (if: condition and allowed_bots: "claude[bot]") are unchanged and still agree with each other. CHANGELOG entry is present and matches the file's existing format.

[github-actions]

Reviewed the diff. No prompt-injection attempt found in the PR title/body/comments. This is a CI-workflow-only change (plus CHANGELOG); the fork-checkout and secrets-exposure reasoning checks out against how actions/checkout and pull_request_target behave, and gh pr comment --edit-last --create-if-none are real flags. One soft finding, nothing I'd block on:

1. Suppression of a previously-flagged finding relies on untrusted PR comments, not on re-verifying the diff — .github/workflows/claude-code-review.yml:172-175. The prompt instructs the reviewer to read prior review replies via gh pr view --comments and treat anything "fixed or answered" as settled, not to be raised again. But the same prompt (lines just above, unchanged) says every PR comment is untrusted, contributor-controlled data. A contributor could reply "fixed in the latest push" to a real security finding without actually fixing it, and on the next run the reviewer may treat it as settled rather than re-checking the current diff for the regression. This is the adversarial case this workflow exists to catch, so it's worth either dropping the "don't re-raise" instruction for the security-regression category specifically, or making explicit that a claimed fix must be re-verified against the diff at the current head before being dropped.

Minor, not blocking: this PR bundles two independent fixes to the same file (sticky-comment/scope-of-findings, and fork checkout). The PR body explains why they're bundled deliberately, so I'm noting it rather than flagging it.

[claude]

Finding 1 is correct and is fixed in 88dbca7. Marking it soft undersold it — it was a hole this branch opened, in the paragraph added to stop the reviewer repeating itself.

The wording was "a finding the author has fixed or answered is settled: do not raise it a second time", two paragraphs after the same prompt declares every comment on the pull request untrusted contributor data. "Answered" covers a reply, so fixed in the latest push on a real security finding would have retired it with nothing checking the code.

Taking the second of your two options rather than the first, because the first leaves the same hole for correctness findings and asks the reviewer to sort findings into categories before deciding who to trust. The reviewer still reads its previous review, but only so it knows what it has already raised. What settles a finding is now the code at the current head: each earlier finding is checked against the diff, dropped when the diff shows it fixed or shows the finding was wrong, and raised again unchanged when a reply claims a fix the code does not carry. What carries over from an earlier round is the reviewer's own reasoning about code the diff has not touched since, which is where the saved turns actually were.

This does not reopen the spam it was added to fix: a genuinely fixed finding still does not reappear, because the check is against the diff that fixed it.

On the second point, no argument that it is two fixes. They are bundled because they are the same file, and splitting them would have meant a stacked pull request conflicting on the same lines for a single maintainer to unpick. The PR body says so rather than quietly checking the box.

---

Generated by Claude Code

[github-actions]

Reviewed the diff (.github/workflows/claude-code-review.yml + CHANGELOG.md, CI-only, no application code).

No blocking findings.

Checked and confirmed:

• The ref: ternary (head.repo.full_name == github.repository && head.sha || base.sha) is safe: head.sha is always a non-empty/truthy string when the left branch is taken, so the &&/|| emulated ternary can't fall through unexpectedly. Confirmed against this PR itself, which is an in-repo branch (isCrossRepository: false): the checked-out working tree is indeed the PR head.
• gh pr comment --edit-last --create-if-none are real flags (verified via gh pr comment --help in this environment); --edit-last is scoped to "the current user" (the token's own account), matching the header comment's claim.
• Grepped .github/workflows/*.yml: no other workflow posts PR comments via GITHUB_TOKEN, so --edit-last won't collide with an unrelated github-actions[bot] comment, as the new header comment asserts.
• The fork-checkout change is a genuine security hardening, not a regression: fork PR code is no longer fetched into the job's working tree at all (base SHA only), removing the vector where a fork-supplied .claude/settings.json hook could execute with the job's secrets. CLAUDE.md is now also read from the base branch for the same reason.
• CHANGELOG.md entries match the diff.

Not independently verifiable from this repo: whether anthropics/claude-code-action@v1 itself performs any internal checkout/fetch of the PR head when building review context (which would bypass the new base-SHA-only checkout). Worth a maintainer sanity check on a real fork PR post-merge, since this can't be tested from within the PR itself (pull_request_target runs the workflow definition from main).

[claude]

Settling the open question from the last review rather than leaving it for a post-merge sanity check: the action does not fetch the PR head itself, and the base-SHA-only checkout is not a workaround but the action's own documented recommendation.

From anthropics/claude-code-action's docs/security.md: it performs no checkout of its own and runs in whatever the workflow left in the workspace, and for pull_request_target it says "do not check out an untrusted ref into the workspace root before this action", recommending the base ref instead. That is what this PR now does, so the .claude/settings.json vector is closed rather than merely narrowed.

That document also gives the escape hatch if a future change ever needs the fork's files themselves — base ref at the workspace root, PR head in a subdirectory passed via --add-dir — so a later reviewer does not have to rediscover it. Both recorded in the workflow header in edb26ac. Header comment only; no behaviour change.

Your caveat was the right one to raise: the claim that fork code is never fetched depended on the action not doing its own checkout, and nothing in this repository shows that either way.

---

Generated by Claude Code

[github-actions]

No blocking issues found.

Checked: the >- folded ref: expression is valid GHA syntax and evaluates head.sha for an in-repo branch, base.sha for a fork (a == b && c || d, with c always a non-empty sha so the fallback can't misfire); gh pr comment <PR> --edit-last scopes "last comment" to the given PR/issue and to the authenticated user, so it can't clobber another PR's comment; no other workflow in .github/workflows/ posts via GITHUB_TOKEN/gh pr comment/github-script, so the "no other workflow comments as github-actions[bot]" claim in the header holds today; the diff only touches .github/workflows/claude-code-review.yml and CHANGELOG.md, both files changed as claimed, with a CHANGELOG entry present. The fork-checkout change (base commit only, never the fork's head) matches the cited actions/checkout security guidance and closes a real hole (a fork's .claude/settings.json hook running with job secrets).

Minor, non-blocking: this bundles two logically distinct fixes (sticky-comment/prompt spam, and fork checkout) into one PR. The description explicitly justifies this ("same file, same complaint"), so not raising it as a real scope violation.