ci(claude): let the review agent submit approving PR reviews - #1105
Merged
Conversation
Mirrors the review rubric now on seqeralabs/sched master. The Claude PR assistant could comment but never file a formal review, so every clean PR still waited on a human to click approve. Three changes make that possible. The gh CLI gains the four commands a review needs (view, diff, review, comment) -- the action ships no review-submitting MCP tool and tag mode allows no Bash beyond git, so the allowlist is explicit rather than a blanket gh api. The pull-requests permission goes from read to write, without which the review call is rejected. And an appended system prompt lifts Claude Code's built-in refusal to approve, replacing it with a rubric: approve when the change is surgical or mechanical, whatever its size or module count; request changes only for security, data-loss, runtime bugs and unmigrated breaking changes; withhold for core design changes, backward-incompatible contracts, destructive migrations, and diffs that turn on context the PR does not carry. The job also gains the actor guard sched carries. Without it the workflow feeds itself: it triggers on pull_request_review, and Claude's own review body echoes the @claude mention that asked for it. Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
The previous commit on this branch does not achieve what it set out to do. The same configuration in sched reached an approve verdict on a PR and then posted it as a comment: no formal review was ever filed. In tag mode the action generates the task prompt itself, and that prompt hard-codes "What You CANNOT Do: Submit formal GitHub PR reviews / Approve pull requests (for security reasons)". The rubric arrived through --append-system-prompt, so the model held an authorization in its system prompt and a prohibition in its task prompt, and followed the task prompt. No wording of a system-prompt override can lift it. Supplying a `prompt` input switches the action to agent mode, where that prompt is used verbatim and the generated block never exists. The rubric is unchanged in substance and now names the review event to submit for each verdict. Verified end to end in sched, where the same workflow approved seqeralabs/sched#1041 on the first run after the switch. The trigger comment is interpolated into the prompt as maintainer guidance, so `@claude review -- focus on the blob caching path` steers where the review looks. The action checks write permission before dispatching a mode, so that text can only come from someone who can already merge; the prompt still frames it as direction that cannot lower the bar or authorize an approval without reading the diff. Two consequences of agent mode are handled here rather than discovered later. It performs no branch setup, so the checkout takes an explicit PR head ref -- a comment event otherwise points at master and every file read would return master's copy while the diff showed the change. And it injects no tools of its own, so the allowlist is now the complete tool set, Read and Grep included. Review bodies go through --body-file: inline --body full of backticks is how a shell-quoting denial lands on the one command that matters. The issues trigger is dropped. It required the mention in the issue body, so all 31 firings were skipped, and had one matched it would have failed on its first call: tag mode opens a tracking comment through issues.createComment, which needs issues:write against the issues:read granted here. actions:read is added so the CI-status MCP server installs instead of warning and skipping on every run, and the timeout moves to the step's `timeout-minutes` -- the action has no `timeout_minutes` input, so the old value was silently ignored. The sticky progress comment goes away with tag mode; the review itself is the output. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Paolo Di Tommaso <paolo.ditommaso@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The Claude PR assistant in this repo can comment on a pull request but cannot file a formal review, so even a clean PR still waits on a human to click Approve. This gives it that ability, using the same rubric that now runs on
seqeralabs/schedmaster.Nothing about when the assistant runs changes: it still only wakes on an explicit
@claudemention.Three pieces are needed for an approval to go through
ghallowlist. The action ships no review-submitting MCP tool, and tag mode permits no Bash beyondgit, so a review can only be filed through theghCLI. Four commands are allowed —gh pr view,gh pr diff,gh pr review,gh pr comment. Deliberately notBash(gh api:*), which would hand the job write access to every GitHub endpoint.pull-requests: write(wasread). Without it the review submission is rejected by the API.--append-system-promptlifts that refusal (as opposed to--system-prompt, which would throw away the built-in prompt entirely) and installs the rubric below.The rubric
Approve when no correctness, security or data-loss problem was found, and the change is either surgical — one root cause or one behaviour, regardless of how many modules, classes or files it touches — or mechanical at any size (renames, dependency bumps, message wording, docs, tests, CI, config defaults), or extends a contract additively (new optional field or endpoint, new column or index, new enum value older readers ignore). Style nits, performance suggestions and missing tests go in the approval body instead of blocking it.
Request changes only for security vulnerabilities, data corruption or loss risks, clear runtime bugs, and breaking changes with no migration path.
Comment when the verdict is unclear, or when a code comment, the PR description or an existing review already explains that the concern is intentional.
Withhold approval, however clean the diff, for four cases a bot verdict cannot cover: a design change to a core component or a new/reshaped shared abstraction; a backward-incompatible contract change; a destructive or irreversible migration; a diff that turns on context the PR does not carry. When it withholds on scope it must say so explicitly — that it is the scope and not a defect — so a human owns the call.
Also: an actor guard on the job
The job now skips itself when the actor is
claude[bot]. Without this the workflow feeds itself, because it triggers onpull_request_reviewand Claude's own review body echoes the@claudemention that asked for the review. sched already carries this guard; this repo did not need it while the assistant could not file reviews, and does now.Verification
The workflow parses and the
--append-system-promptblock scalar survives intact (the rubric comes back underclaude_args, not reparsed as sibling YAML keys), and no rubric line begins with#— the action strips comment lines fromclaude_args, so such a rule would silently vanish.claude_argsis a supported input at the action SHA this repo pins, verified against the action's ownaction.ymlat that commit, so no version bump is involved.🤖 Generated with Claude Code