ci: Claude automated PR review on every PR - #93
Conversation
Runs anthropics/claude-code-action@v1 on every pull_request (opened, synchronize, reopened) to review the diff and post findings as inline comments + a summary. Review prompt is tuned to SUS's risk areas: the app->platform trust boundary (identity/trusted-proxy/proxy handling, client-supplied values, secrets), shell injection in build-pod scripts, and Helm template/values correctness. - Skips draft PRs; one review per PR (cancel-in-progress on new pushes). - Least-privilege permissions (contents: read; PR/issue write for comments). - Does not run on fork PRs (GitHub withholds secrets from forks). Requires a repository secret ANTHROPIC_API_KEY (only the repo owner can add it). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The action's default flow exchanges a Claude GitHub App token and 401s if the
app isn't installed on the repo. Providing github_token: ${{ secrets.GITHUB_TOKEN }}
makes it use the workflow's built-in token instead (inherits the permissions
block for posting PR/inline comments), so only the ANTHROPIC_API_KEY secret is
required.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @dev-dull's task in 2m 46s —— View job Review: Claude automated PR review workflowSolid, and the security posture on the part that usually goes wrong is right: this uses Three findings, posted inline:
Minor, take or leave:
One thing I checked and it's not a problem: Note on process: the prompt asked for the summary via |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| # Skip drafts — review once it's marked ready. | ||
| if: github.event.pull_request.draft == false |
There was a problem hiding this comment.
Fork PRs will fail loudly, not skip. The PR description says fork PRs "won't be reviewed", but on: pull_request still fires for them — the job runs with an empty secrets.ANTHROPIC_API_KEY and a read-only GITHUB_TOKEN (GitHub downgrades pull-requests: write for fork events). The result is a red ❌ check on every outside contributor's PR, which reads as "your PR is broken."
Gate on the head repo so fork PRs are skipped cleanly instead:
| # Skip drafts — review once it's marked ready. | |
| if: github.event.pull_request.draft == false | |
| # Skip drafts — review once it's marked ready. | |
| # Skip fork PRs: secrets aren't available there, so the run would fail | |
| # rather than no-op. | |
| if: >- | |
| github.event.pull_request.draft == false && | |
| github.event.pull_request.head.repo.full_name == github.repository |
| Post specific issues as inline comments on the relevant lines using | ||
| mcp__github_inline_comment__create_inline_comment (with confirmed: true), | ||
| and leave one short summary comment via `gh pr comment`. |
There was a problem hiding this comment.
gh pr comment conflicts with track_progress: true. In tracked mode the action creates and owns a single comment (the "Claude Code is working…" one above) and instructs the model that all communication happens by editing that comment — "Never create new comments." So this last line either gets ignored or produces a second, redundant summary comment alongside the tracked one.
I hit exactly this on the current run: I'm reviewing under the prompt below, and the tracked-comment instruction takes precedence over gh pr comment. Dropping the sentence removes the ambiguity; the tracked comment already serves as the summary.
| Post specific issues as inline comments on the relevant lines using | |
| mcp__github_inline_comment__create_inline_comment (with confirmed: true), | |
| and leave one short summary comment via `gh pr comment`. | |
| Post specific issues as inline comments on the relevant lines using | |
| mcp__github_inline_comment__create_inline_comment (with confirmed: true), | |
| and put a short overall summary in your tracked comment. |
| mcp__github_inline_comment__create_inline_comment (with confirmed: true), | ||
| and leave one short summary comment via `gh pr comment`. | ||
| claude_args: | | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" |
There was a problem hiding this comment.
The allowlist can't do what the prompt asks for Helm. The prompt makes charts/sus/ "template rendering, values wiring, and backward-compat when auth is off" a priority, but the only Bash entries here are gh subcommands — so helm template/helm lint are denied and the reviewer can only eyeball the templates, never render them. Verified on this run: helm is on the ubuntu-latest image (/usr/local/bin/helm), and non-allowlisted Bash is genuinely blocked (my gh auth status probe came back "This command requires approval", which in CI means denied).
Rendering the chart with auth on and off is the single highest-value check for this repo's stated risk area, so it's worth allowing:
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" | |
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(helm template:*),Bash(helm lint:*)" |
(Bash(gh pr comment:*) dropped to match the tracked-comment note above — drop only if you take that suggestion too.)
The workflow's first successful run reviewed its own PR and flagged three real issues (plus minors); applying them: - Skip fork PRs via a head-repo guard (they have no secret + read-only token, so they were failing red instead of no-op'ing). - Drop the "summary via gh pr comment" instruction and Bash(gh pr comment) — track_progress owns a single comment and forbids new ones; summary now goes in the tracked comment. - Allow Bash(helm template:*) / Bash(helm lint:*) (helm is preinstalled on the runner) so the reviewer can actually render the chart — the highest-value check for this repo — instead of only eyeballing templates. - Add timeout-minutes: 20; set cancel-in-progress: false (a cancelled run froze its tracking comment); drop unused id-token: write; rename .yml -> .yaml to match the repo's other workflows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @dev-dull's task in 3m 18s —— View job Re-review: all three prior findings fixed and verified
All three findings from the last round are applied, and I verified the two I could only assert before by actually running them in this job:
The minors were taken too: I also chased one thing that turned out not to be a problem, since it's easy to assume otherwise: the prompt asks to confirm "auth-off output is unchanged", which needs persisting two renders and diffing them. Redirects to Findings1. A draft PR marked "Ready for review" never gets reviewed (line 12, inline) — 2. Minor: Nothing else. One thing worth knowing going forward: because this passes I didn't implement finding #1 — I'm unable to modify files in |
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
A draft PR marked "Ready for review" never gets reviewed. The draft guard on line 25 skips the run, and the comment says "review once it's marked ready" in spirit — but ready_for_review isn't in this types list, so that transition fires no workflow. The sequence is:
- PR opened as draft →
openedfires → skipped by the draft guard. ✅ intended - Author clicks "Ready for review" →
ready_for_reviewfires → no run at all, because it's not intypes. - Review only happens on the next push (
synchronize).
So the common "draft while I work, ready when done, no further commits" flow gets zero review. Adding the type fixes it; the draft guard is still needed for opened/synchronize on drafts.
| types: [opened, synchronize, reopened] | |
| types: [opened, synchronize, reopened, ready_for_review] |
| mcp__github_inline_comment__create_inline_comment (with confirmed: true), | ||
| and put a short overall summary in your tracked comment. | ||
| claude_args: | | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(helm template:*),Bash(helm lint:*)" |
There was a problem hiding this comment.
Minor, take or leave: this relies on the helm that happens to ship in the ubuntu-latest image, while publish-chart.yaml pins one via azure/setup-helm@v4. So the version that reviews the chart can drift from the version that publishes it, and a runner-image bump could change rendering behavior (or drop helm) with no signal here.
Adding the same step the sibling workflow uses keeps the two in sync and makes the dependency explicit rather than incidental:
- uses: azure/setup-helm@v4
- uses: anthropics/claude-code-action@v1Not urgent — I confirmed on this run that helm lint charts/sus and helm template sus charts/sus --set auth.enabled=true both work as-is.
Two follow-up findings from the workflow's own re-review of this PR: - Add ready_for_review to pull_request types, so a draft marked ready (with no further push) is reviewed instead of silently waiting for the next commit. - Add azure/setup-helm@v4 (same action publish-chart.yaml uses) so the helm that reviews the chart matches the one that ships it, rather than relying on the runner image's bundled version. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @dev-dull's task in 2m 27s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
| Draft → "Ready for review" never reviewed | Fixed — ready_for_review added to types (line 14), draft guard correctly kept for opened/synchronize |
| Review helm vs. publish helm can drift | Addressed, but see below |
One finding
azure/setup-helm@v4 doesn't pin anything (line 43, inline). Its version input defaults to latest, and neither workflow sets it — so the comment's "pin helm … matches the one that ships it" isn't what the config does. I checked the tool cache in this run: it installed helm 4.2.3, a major-version jump from the runner image's bundled helm that the previous commit was using. The two workflows still resolve latest independently at their own run times.
The bump changed real behavior in a way that matters for this workflow specifically: helm 3's --post-renderer accepted an arbitrary executable path, which would have been a clean escape from the Bash(helm template:*) allowlist. Helm 4 requires an installed plugin, and I confirmed the escape is rejected on this run. Worth pinning on purpose rather than landing on the safe side by luck — and worth knowing that pinning back to 3.x would reopen it.
Verified still working under 4.2.3: helm lint charts/sus → 0 failed; helm template → 183 lines auth-off vs 500 auth-on. The allowlist does what the prompt needs.
Everything else holds
Still pull_request rather than pull_request_target, still no PR-controlled string reaching a run: block or the prompt, contents: read with write scoped to comments, forks skipped cleanly. With forks excluded, all reviewed code comes from people who already have push access, so prompt-injection blast radius stays at comment text. timeout-minutes: 20, cancel-in-progress: false, and the .yaml rename are all in. Nothing else to flag — this is close to done.
I didn't implement the fix: I can't modify files under .github/workflows (the GitHub App's permissions exclude workflow changes), so this is feedback only. Details in the FAQ.
· branch ci/claude-pr-review
| # Pin helm via the same action publish-chart.yaml uses, so the version | ||
| # that reviews the chart matches the one that ships it (avoids drift from | ||
| # relying on whatever helm the runner image happens to bundle). | ||
| - uses: azure/setup-helm@v4 |
There was a problem hiding this comment.
This doesn't actually pin helm — and it silently moved the review to helm 4. azure/setup-helm's version input defaults to latest, and neither this workflow nor publish-chart.yaml sets it. I checked the tool cache in this run: the action installed helm 4.2.3, a major-version jump from the helm the runner image bundles (what the previous commit's review used).
So the comment's guarantee doesn't hold two ways: nothing is pinned, and the two workflows resolve latest independently at their own run times — a tag push weeks from now can ship with a different helm than the one that reviewed the chart. It's still an improvement over "whatever the image happens to have" (explicit dependency, same action as the sibling), just not a pin.
That major bump is not cosmetic here. --post-renderer took an arbitrary executable path in helm 3, which would have been a straight escape from the Bash(helm template:*) allowlist; helm 4 requires an installed plugin instead. I confirmed it on this run:
$ helm template sus charts/sus --post-renderer /usr/bin/tee
Error: invalid argument "/usr/bin/tee" for "--post-renderer" flag:
plugin: {Name:/usr/bin/tee Type:postrenderer/v1} not found
The allowlist holds today only because latest happens to be 4.x. Worth pinning deliberately — and note that pinning back to a 3.x line would reopen that path:
| - uses: azure/setup-helm@v4 | |
| # Install helm explicitly (same action publish-chart.yaml uses) rather than | |
| # relying on whatever the runner image bundles. Pinned so chart rendering | |
| # here can't shift under a helm release; keep in sync with publish-chart.yaml. | |
| - uses: azure/setup-helm@v4 | |
| with: | |
| version: v4.2.3 |
Also worth adding the same version: to publish-chart.yaml, or the "matches the one that ships it" claim stays aspirational. (Separately: that workflow is now on an unpinned helm 4 for package/push too — pre-existing, not this PR, but you inherit it the moment you rely on the versions matching.)
Re-review caught that azure/setup-helm@v4 with no version defaults to latest, so nothing was actually pinned (it installed helm 4.2.3) and my 'pin' comment was wrong. Pin v4.2.3 explicitly in both workflows so they genuinely match. The pin matters for security, not just reproducibility: helm 3's --post-renderer accepted an arbitrary executable path, a clean escape from the review workflow's Bash(helm template:*) allowlist; helm 4.x requires an installed plugin and closes it. Letting the version float on 'latest' means the allowlist is only safe by luck — and pinning back to a 3.x line would reopen the hole. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @dev-dull's task in 3m 18s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
azure/setup-helm@v4 didn't pin anything (defaulted to latest) |
Fixed and verified — version: v4.2.3 in both workflows; tool cache shows /opt/hostedtoolcache/helm/4.2.3/x64.complete, so the pin resolves and is what this run used |
The comment now describes what the config actually does, including the --post-renderer rationale, and publish-chart.yaml got the matching version: — so "the helm that reviews the chart is the same one that packages it" is now a real guarantee rather than two independent latest resolutions.
Chart still healthy under the pin: helm lint charts/sus → 0 failed; renders are 183 lines auth-off / 500 auth-on, byte-identical counts to the pre-pin run, so pinning changed no output.
One finding
The allowlist can't perform the regression half of its own chart instruction (line 80, inline). The prompt makes "auth-off output is unchanged" a priority, but that's a base-vs-head comparison, and there's no way to get the base charts/sus/ onto disk — I tried git worktree add, git checkout <ref> -- <path>, and git archive | tar, and all three came back "This command requires approval"; /tmp writes are sandboxed off. So the reviewer can only render head with auth on and off.
Worth knowing for the fix: the workspace is writable with no allowlist entry at all (mkdir, > redirect, and rm all ran un-prompted), and diff works, and git show origin/main:… works despite fetch-depth: 1. So allowing a git command that writes tracked content into the workspace adds the missing piece without widening write access. Details and a suggestion are in the inline comment.
I'll flag one correction to my own earlier round: last time I said this comparison was "genuinely achievable with this allowlist." That was wrong — I'd checked that redirects and diff work, but not that the base chart can't be materialized in the first place, which is the actual blocker.
Minor, take or leave
publish-chart.yaml is now pinned to helm 4, but I can't exercise its path from here — helm package, helm push, and helm registry login aren't allowlisted, and that workflow only fires on a v* tag. It was already resolving to latest (= 4.2.3), so this freezes today's behavior rather than changing it; the risk is just that a Helm 4 packaging regression would first show up at release time. A throwaway tag on a fork, or a one-off local helm package charts/sus with 4.2.3, would retire that unknown cheaply.
Everything else still holds
Still pull_request rather than pull_request_target; no PR-controlled string (title, body, branch name) reaches a run: block or the prompt; contents: read with write scoped to comments; forks skipped cleanly by the head-repo guard; ready_for_review in types with the draft guard correctly retained for opened/synchronize. timeout-minutes: 20, cancel-in-progress: false, and the .yaml naming are all in. With forks excluded, every reviewed diff comes from someone who already has push access, so prompt-injection blast radius stays at comment text.
This is in good shape — the remaining item is a prompt/allowlist mismatch, not a correctness or security problem.
I didn't implement the fix: I can't modify files under .github/workflows (the GitHub App's permissions exclude workflow changes), so this is feedback only. Details in the FAQ.
· branch ci/claude-pr-review
| mcp__github_inline_comment__create_inline_comment (with confirmed: true), | ||
| and put a short overall summary in your tracked comment. | ||
| claude_args: | | ||
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(helm template:*),Bash(helm lint:*)" |
There was a problem hiding this comment.
The allowlist can render the chart but can't do the regression half of the check on line 69. "auth-off output is unchanged" is a before/after comparison — it needs the base chart rendered too. helm template only reads from disk, so that requires materializing origin/main's charts/sus/ somewhere. Every route to that is denied under this allowlist. I tried all of them on this run:
| Attempt | Result |
|---|---|
git worktree add /tmp/base origin/main |
This command requires approval |
git checkout origin/main -- charts/sus |
This command requires approval |
git archive origin/main charts/sus | tar -x … |
This command requires approval |
mkdir -p /tmp/base |
blocked — writes confined to the workspace |
So the reviewer can only ever render head with auth on and off. That's still the valuable part, but it's a different check: it catches "auth-on is broken," not "this PR changed auth-off output."
Two things that do work, which is why this is one small gap rather than a redesign:
- The base objects are present despite
fetch-depth: 1—git show origin/main:charts/sus/Chart.yamlreturned content, so the action's base fetch is sufficient. - The workspace is writable without any allowlist entry (
mkdir,>redirect, andrmall succeeded un-prompted; only/tmpis off-limits). So persisting two renders and running plaindiffworks — I confirmeddiffruns.
That second point matters for the fix: adding a git command that writes tracked content into the workspace grants no new write capability over what the reviewer already has. It only supplies the one missing piece.
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(helm template:*),Bash(helm lint:*)" | |
| --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr diff:*),Bash(gh pr view:*),Bash(helm template:*),Bash(helm lint:*),Bash(git worktree add:*)" |
Caveat on that suggestion: I couldn't execute git worktree add to prove it works here, precisely because it's denied — I only verified its prerequisite (base objects present). If it turns out shallow-clone worktrees misbehave, the alternative is to reword line 69 to ask for what's reachable, e.g. "render with auth on and off and check values wiring; if the auth-off path changed, say so from the diff" — that keeps the prompt honest rather than asking for a check the tooling can't perform.
Re-review noted the prompt asked to confirm auth-off output is 'unchanged', which is a base-vs-head comparison the review sandbox can't do (git worktree/ checkout/archive to get the base charts/ onto disk are all blocked). Reword to the achievable check: render head with auth on/off and confirm the auth-off render contains no Authelia resources (feature stays opt-in), reasoning from the PR diff rather than a base render. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Claude finished @dev-dull's task in 3m 4s —— View job Re-review of
|
| Prior finding | Status |
|---|---|
| Prompt asked for a base-vs-head comparison the sandbox can't do | Fixed — reworded to an achievable invariant, and I executed it on this run to confirm |
I'd suggested adding Bash(git worktree add:*); rewording instead is the better call — it keeps the sandbox narrow rather than widening it to make a nice-to-have check possible. And the replacement check isn't a downgrade to something vacuous. I ran it here:
helm lint charts/sus→ 1 linted, 0 failed- auth-off render: 183 lines, 0
autheliamatches — the invariant the prompt asks for holds - auth-on render: 500 lines, 33
autheliamatches — so the check has real signal and would actually fail if auth leaked into the default render
The parenthetical is accurate, too. I re-probed the base-materialization routes: git ls-tree -r origin/main charts/sus came back "This command requires approval" this run, so you can't even enumerate the base chart's files. (git show origin/main:<known-path> still works, so a one-file-at-a-time partial comparison is technically reachable — but that's not a base render, and it's not worth widening the allowlist for.)
One finding — not a blocker
Nothing enforces the chart invariant deterministically (inline, line 69). This prompt is now the only place in the repo that renders the chart at all — I searched, and no script, Makefile, test, or other workflow runs helm lint/helm template. More broadly, this workflow is the only thing that fires on a pull request: build-images.yaml is push-to-main/tags, publish-chart.yaml is tags-only. So a template that fails to render lands on main and first breaks at install time (helm package doesn't render templates, so the tag-push path won't catch it either), and "auth stays opt-in" rests on an LLM's best-effort judgment with no failing check behind it. A ~15-line pull_request job needs no secrets — so it'd cover fork PRs, which this review job correctly skips — and reuses the pin this PR just added. Sketch in the inline comment.
That's separate work; it doesn't hold up this PR.
Everything else still holds
pull_request rather than pull_request_target; no PR-controlled string (title, body, branch name) reaches a run: block or the prompt; contents: read with write scoped to comments (issues: write is genuinely needed — the tracked comment goes through the issues API); forks skipped by the head-repo guard; ready_for_review in types with the draft guard retained for opened/synchronize; timeout-minutes: 20; cancel-in-progress: false. version: v4.2.3 is in both workflows, so the "same helm reviews and publishes" claim is a real guarantee. With forks excluded, every reviewed diff comes from someone who already has push access, so prompt-injection blast radius stays at comment text.
This looks good to merge. The remaining item is a follow-up, not a defect in what's here.
One carry-over unknown, unchanged from last round: publish-chart.yaml is pinned to helm 4 but I can't exercise helm package/push from here (not allowlisted, and that workflow only fires on v*). The pin froze what it was already resolving to, so this isn't a regression — just an untested path until the next release. A one-off local helm package charts/sus under 4.2.3 retires it cheaply.
· branch ci/claude-pr-review
| params), secret handling, and shell injection in build-pod/*.sh. | ||
| - Helm chart correctness (charts/sus/): when the chart changed, render | ||
| it with `helm template` (both auth on and off) and check values | ||
| wiring. For auth-off, confirm the render contains no Authelia/auth |
There was a problem hiding this comment.
The reworded check is right, and it also surfaces that nothing enforces it deterministically. The new wording is achievable and non-vacuous — I ran it on this job: helm lint charts/sus → 0 failed; auth-off render is 183 lines with 0 authelia matches; auth-on is 500 lines with 33. So the invariant holds today and the check has real signal.
The gap is that this prompt is now the only place in the repo where the chart is rendered at all. I searched: no script, Makefile, test, or workflow runs helm lint/helm template anywhere else. And more broadly, this workflow is the only thing in .github/workflows/ that fires on a pull request — build-images.yaml is push-to-main/tags, publish-chart.yaml is tags-only. So:
- A template that fails to render lands on
mainand first breaks at install time.helm package(the tag-push path) doesn't render templates, so it won't catch it either — a broken chart gets published. - "auth-off contains no Authelia resources" is enforced by an LLM's judgment on a best-effort, non-deterministic pass, with no failing check behind it.
- The review job is correctly fork-skipped and secret-gated, so fork PRs get zero chart validation.
A deterministic job fixes all three, needs no secrets (so it covers forks), and reuses the helm pin this PR just added:
chart:
runs-on: ubuntu-latest
timeout-minutes: 5
permissions:
contents: read
steps:
- uses: actions/checkout@v4
- uses: azure/setup-helm@v4
with:
version: v4.2.3
- run: helm lint charts/sus
- name: Render with auth on and off
run: |
helm template sus charts/sus --set auth.enabled=true > /dev/null
helm template sus charts/sus > auth-off.yaml
- name: auth-off must not contain Authelia resources
run: |
if grep -qi authelia auth-off.yaml; then
echo "::error::auth.enabled=false rendered Authelia resources; auth must stay opt-in"
exit 1
fiNot a blocker for this PR — the LLM review is strictly better than the nothing that's there now, and this can land separately. But it's worth knowing that the chart bullet above describes a best-effort check, not a gate.
1 participant
Adds
.github/workflows/claude-review.yml— Claude reviews every PR automatically.What it does
pull_request(opened, synchronize, reopened); skips drafts.anthropics/claude-code-action@v1, which reviews the diff and posts inline comments on specific lines plus one summary comment.build-pod/*.sh, and Helm chart template/values correctness (incl. auth-off backward-compatibility).cancel-in-progresson new pushes; least-privilege permissions (contents: read, PR/issue write for comments).The repo owner must add a repository secret
ANTHROPIC_API_KEY(Settings → Secrets and variables → Actions → New repository secret). Without it, the action fails.Alternatives if you prefer not to use a raw API key: a
CLAUDE_CODE_OAUTH_TOKENsecret (swap the input toclaude_code_oauth_token), or Bedrock/Vertex via the action'suse_bedrock/use_vertexflags.Notes
--model <id>toclaude_argsif desired.🤖 Generated with Claude Code