Run the review as a status check rather than a comment - #943
Closed
kwsantiago wants to merge 1 commit into
Closed
Conversation
WalkthroughChangesClaude review automation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant GitHubActions
participant ClaudeAction
participant StatusCheck
PullRequest->>GitHubActions: Trigger review for pull request targeting main
GitHubActions->>ClaudeAction: Send repository history and security review prompt
ClaudeAction-->>GitHubActions: Return structured pass or block result
GitHubActions->>StatusCheck: Validate result and emit annotations
StatusCheck-->>PullRequest: Pass or fail the review check
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/claude-review.yml:
- Around line 90-105: Update the verdict validation in the workflow block after
the `.blockers` output so only a complete, consistent result is accepted: allow
pass only with zero blockers and block only with one or more blockers, while
rejecting unknown or missing verdicts and contradictory blocker counts. Preserve
the existing blocker logging and fail the job with the current error behavior
for invalid results or blockers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b5beee40-fcbc-4474-9f37-440600e7d214
📒 Files selected for processing (1)
.github/workflows/claude-review.yml
Comment on lines
+90
to
+105
| if [ -z "${RESULT}" ] || ! printf '%s' "${RESULT}" | jq -e . >/dev/null 2>&1; then | ||
| echo "::error::The review produced no parseable verdict. Failing closed." | ||
| exit 1 | ||
| fi | ||
|
|
||
| printf '%s' "${RESULT}" | jq -r '.summary' | ||
|
|
||
| printf '%s' "${RESULT}" | jq -r ' | ||
| .blockers[]? | | ||
| "::error file=\(.file)\(if .line then ",line=\(.line)" else "" end)::\(.finding) — \(.failure)" | ||
| ' | ||
|
|
||
| if [ "$(printf '%s' "${RESULT}" | jq -r '.verdict')" = "block" ]; then | ||
| echo "::error::Review found blockers. Fix them, or state why each is wrong." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject contradictory verdicts before passing the check.
Line 102 only fails when .verdict equals "block". The declared schema permits "verdict":"pass" with non-empty blockers. Lines 97-100 emit those blockers, but the job exits successfully. A review can therefore report a concrete blocker while the required status check passes.
Validate the verdict and blocker count together. Reject unknown or incomplete verdict objects.
Proposed fix
if [ -z "${RESULT}" ] || ! printf '%s' "${RESULT}" | jq -e . >/dev/null 2>&1; then
echo "::error::The review produced no parseable verdict. Failing closed."
exit 1
fi
+ if ! printf '%s' "${RESULT}" | jq -e '
+ (.verdict == "pass" or .verdict == "block") and
+ (.blockers | type == "array") and
+ (.verdict == "block" or (.blockers | length == 0))
+ ' >/dev/null; then
+ echo "::error::The review produced an invalid verdict. Failing closed."
+ exit 1
+ fi
+
printf '%s' "${RESULT}" | jq -r '.summary'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -z "${RESULT}" ] || ! printf '%s' "${RESULT}" | jq -e . >/dev/null 2>&1; then | |
| echo "::error::The review produced no parseable verdict. Failing closed." | |
| exit 1 | |
| fi | |
| printf '%s' "${RESULT}" | jq -r '.summary' | |
| printf '%s' "${RESULT}" | jq -r ' | |
| .blockers[]? | | |
| "::error file=\(.file)\(if .line then ",line=\(.line)" else "" end)::\(.finding) — \(.failure)" | |
| ' | |
| if [ "$(printf '%s' "${RESULT}" | jq -r '.verdict')" = "block" ]; then | |
| echo "::error::Review found blockers. Fix them, or state why each is wrong." | |
| exit 1 | |
| fi | |
| if [ -z "${RESULT}" ] || ! printf '%s' "${RESULT}" | jq -e . >/dev/null 2>&1; then | |
| echo "::error::The review produced no parseable verdict. Failing closed." | |
| exit 1 | |
| fi | |
| if ! printf '%s' "${RESULT}" | jq -e ' | |
| (.verdict == "pass" or .verdict == "block") and | |
| (.blockers | type == "array") and | |
| (.verdict == "block" or (.blockers | length == 0)) | |
| ' >/dev/null; then | |
| echo "::error::The review produced an invalid verdict. Failing closed." | |
| exit 1 | |
| fi | |
| printf '%s' "${RESULT}" | jq -r '.summary' | |
| printf '%s' "${RESULT}" | jq -r ' | |
| .blockers[]? | | |
| "::error file=\(.file)\(if .line then ",line=\(.line)" else "" end)::\(.finding) — \(.failure)" | |
| ' | |
| if [ "$(printf '%s' "${RESULT}" | jq -r '.verdict')" = "block" ]; then | |
| echo "::error::Review found blockers. Fix them, or state why each is wrong." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/claude-review.yml around lines 90 - 105, Update the
verdict validation in the workflow block after the `.blockers` output so only a
complete, consistent result is accepted: allow pass only with zero blockers and
block only with one or more blockers, while rejecting unknown or missing
verdicts and contradictory blocker counts. Preserve the existing blocker logging
and fail the job with the current error behavior for invalid results or
blockers.
Contributor
Author
|
Closing: not adding an API key to this repository. The review stays advisory rather than gating. |
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.
Summary
The review step is the one that has been catching things, and nothing requires it. Merging needs seven status checks to pass and no review at all, so the loop is: write, CI passes, merge. This adds the review as a check so that step cannot be skipped.
The argument for gating on it rather than leaving it advisory is that CI has repeatedly gone green on defective changes in this repository. A clamp that made audit-registry corruption permanent and invisible, a latch that froze registry maintenance for the lifetime of the process, a lookup fix that resurrected deleted values, and a storage migration that erased a share after a write that reported success while storing nothing. Every one of those passed the full suite. Every one was caught by reading the diff. That asymmetry is what the check encodes.
The review returns a structured verdict rather than prose, and a following step fails the job on it. It fails closed: an empty or unparseable result is treated as a failure, because a gate that opens when it cannot evaluate is not a gate.
Severity is deliberately narrow. It blocks on four things: data that can be destroyed or made unreachable, a failure that resolves to the more permissive outcome, a test that asserts broken behavior is correct or that passes whether or not the code under test runs, and key material reaching any egress. Everything else is a note. A reviewer that blocks on everything gets turned off, and then nothing is reviewed at all.
Two conventions carried over from the existing guard workflow. The action is pinned by commit rather than the
v1tag, since a mutable tag on a job that gates merges is its own supply-chain hole. And there is nopaths:filter, because a path-filtered job can never be a required check: a pull request touching none of the filtered globs reports nothing and blocks forever.Test plan
The workflow parses. It cannot pass until an
ANTHROPIC_API_KEYsecret exists on the repository, so the run on this pull request is expected to fail on the missing secret, which is the fail-closed path working. Sequence to finish enabling it: add the secret, confirm a green run here, then add the check to branch protection. Adding it to protection before the secret exists would block every pull request.Worth stating plainly since it is a recurring cost: this runs a model over the diff on every pull request, so it adds per-PR spend. Fork pull requests will not run it at all, because secrets are unavailable to them.
Summary by CodeRabbit