Should a "changes recommended" review verdict fail CI? (three reviewers currently disagree) #378
Unanswered
d-morrison
asked this question in
Q&A
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Should a review verdict of "changes recommended" / "Needs more work" fail the
PR's CI check, or should the check only assert that a review happened?
The three AI reviewers this repo ships currently answer that question three
different ways, and nothing documents the divergence as deliberate. This is a
policy choice with a real do-nothing option, so it belongs here rather than in
the tracker; the issues it produces can be filed once it is settled.
Current state
claude-code-reviewrequire-reviewin.github/workflows/claude-code-review.ymlasserts onlyneeds.claude-review.result == 'success'-- a review ran and stated some verdict. The verdict text is never parsed.jules/reviewstatuses: writeand reds its own status onVERDICT: block.gemini-code-reviewconcurrency:comment notes the absence explicitly..github/workflows/scripts/check-review-execution.shextracts theverdict-bearing assistant block but deliberately never classifies it: it only
distinguishes ran / stubbed / errored / quota-skipped.
Issue #157 established that every review must end with an explicit verdict line.
This is the follow-on question that one left open -- now that the verdict is
reliably present, does its content belong in the check's pass/fail?
A recommendation, not a conclusion: no, keep gating on "did a review happen"
Four reasons, in descending weight.
1. Verdict-gating has already cost us once, and the bill is unpaid. A
verdict-gated check can only be cleared by changing code or by convincing the
bot. #373 is the live case: Jules re-issues an identical
blockat a new head ona false positive that keys on text we are declining to change, so re-running
cannot clear it. The outcomes are merging past a red required check (which trains
everyone to ignore red) or complying with a finding we believe is wrong.
Extending the same mechanism to
claude-code-review-- the reviewer thatactually works -- multiplies that failure mode rather than containing it.
2. The verdict vocabulary is deliberately open.
.github/actions/run-claude-review-attempt/action.ymlasks for Ready formerge or Needs more work "(or another clearly labeled alternative, e.g.
Blocked on human review)". There is no closed enum to key pass/fail on.
Gating would require either constraining that vocabulary -- after which a
one-word drift silently turns a real block green -- or writing a fuzzy string
classifier, which is the LLM-judgment-inside-a-check that
algorithmatize-checksexists to rule out.
3. The verdict carries no severity. "Needs more work" covers a nit and a
data-corruption bug alike, even though the prompt asks the reviewer to
distinguish blocking from optional. And the standing
address-every-commentrulealready obliges addressing every finding regardless of label, so a red check buys
no additional compliance from a session running ARDI. The only party it informs
is the human, who can read the verdict.
4. It makes red ambiguous. Today red means "the reviewer crashed or never
finished" -- a mechanical, decidable fact, and the reason #368 and #370 are
diagnosable at all. Folding opinion into the same signal means red no longer
distinguishes a broken reviewer from an opinionated one.
The counter-argument, stated fairly
The case for gating is real: a green check sitting next to a "Needs more work"
verdict reads as approval.
CLAUDE.mdwarns about this repeatedly -- a greenclaude-reviewon a silently-skipped self-mod review (#329's case), and thestanding "green means CI state, not review verdict" caution.
But the cause there is that the check's name does not say what it checks, so
the fix may be naming and surfacing rather than blocking. That is the crux worth
arguing over here.
What the decision would produce
Not a checklist to work now -- these are the issues worth filing once this
settles, listed so the scope of the decision is visible:
check (or the sticky comment) reporting the verdict, warn-and-exit-0. In-repo
precedent:
check-new-line-breaksis advisory by design, andfully-clean.md's benchmark case is the pattern for a soft-threshold judgmentbeing reported rather than gated.
verdict-gating should be removed for consistency -- it is upstream behavior,
so that may mean dropping
statuses: write, or accepting the divergence anddocumenting it. Related: jules-review.yml lacks the fork guard and job timeout ai-config's copy has #372, Jules reviewer blocks PRs that configure AI reviewers, as "prompt injection" #373.
CLAUDE.mdbeside the existingrequire-reviewguidance, so the next session does not re-derive it.
with it, not after -- that is precisely what Jules reviewer blocks PRs that configure AI reviewers, as "prompt injection" #373 lacks.
Moved from #377, which was filed in the tracker before we settled that a
policy question of this shape belongs on the discussion board.
All reactions