docs(review): skip the challenger when there are no findings - #1009
docs(review): skip the challenger when there are no findings#1009guyoron1 wants to merge 7 commits into
Conversation
Functional tests did not runFunctional tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the |
PR Summary by QodoSkip challenger dispatch when review findings are empty
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit c302b85 |
|
/review |
PR Reviewer Guide 🔍Warning
Here are some key observations to aid the review process:
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 7cf0f6a |
|
Hello! Could you attach transcripts so we can see how bad it is the challenger? So find multiple no findings reviews and scan their transcripts with the "analyze-transcript" skill on |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (2 inline comments, both MEDIUM).
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit dfcd074 |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit c76ba43 |
c76ba43 to
28c2610
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 28c2610 |
The challenger is an opus dispatch whose only inputs are the merged findings from steps 6a-6c; on a clean PR it adjudicates an empty list at the cost of a serial opus call and latency segment. Skip the dispatch when the set is empty and proceed straight to the verdict - the empty-findings approval path is unchanged. The skip is noted in the orchestrator's own reasoning for auditability; there is no field for it in the strict agent-result.json schema and it does not belong in the posted review body. Signed-off-by: guy oron <goron@redhat.com>
… sets Address review findings on the challenger-skip change: - The empty-findings branch told the orchestrator to proceed straight to the verdict, bypassing the orchestrator-only checks (6e) that run after the challenger and can add findings on their own (protected paths, scope authorization, PR metadata). Reword so only the challenger dispatch is skipped and the workflow continues through 6e, 6e-1, and 6f; the no-findings approval applies only when the set is still empty after those checks. - The re-review dispatch rule and examples still listed the challenger unconditionally. Clarify that step 6d's condition is the current review's 6a-6c findings — prior findings alone do not qualify it — and mark the example rows accordingly. Signed-off-by: guy oron <goron@redhat.com>
…allenger mention Address round-3 review findings: - The skip rationale cited a sub-agent-failure "info finding below", but the dimension-failure finding is recorded in step 5 above (high for Opus-tier, info for Sonnet-tier) and keeps the 6a-6c set non-empty, so the "regardless of why the set is empty" case was unreachable and the citation pointed at the challenger-failure fallback instead. State it correctly: a failed dimension cannot produce an empty set, and an empty set means every dispatched dimension came back clean. - Remaining unconditional challenger language contradicted the findings-gated rule: qualify the 3c parallelism note and the 6d opener, and define the adjudicated set (challenger output, or the unchanged 6a-6c set on skip or failure fallback) once in 6d so 6e, 6e-1, and 6f no longer presume the challenger ran. Signed-off-by: guy oron <goron@redhat.com>
…inding The adjudicated-set definition said a challenger failure leaves the 6a-6c set unchanged, but 6d step 4's fallback also records an info-level sub-agent-failure finding; as written the definition would drop it before 6e-6f. On failure the adjudicated set is the 6a-6c findings plus that recorded finding. Signed-off-by: guy oron <goron@redhat.com>
28c2610 to
c122fdf
Compare
|
@rh-hemartin Can't produce that from here — I don't have a runner with org access to execute real reviews against fullsend-ai/fullsend, and there's no skill literally named If you can point me at a handful of recent real "no findings" review runs (or grant this a runner), I'll pull and analyze the actual transcripts rather than guess at the challenger's false-positive rate. |
Conflict in skills/pr-review/SKILL.md (3c paragraph): main reworded risk-assessment as composed in 3c-2 among the parallel sub-agents; this branch gates the challenger on step 6d. Kept both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: guy oron <goron@redhat.com>
Head branch was pushed to by a user without write access
|
Oh, he meant this skill: https://github.com/fullsend-ai/fullsend/tree/main/skills/analyze-transcript (from the fullsend-ai/fullsend repo). You should be able to scrape the gh workflow runs history in this repo and the other one to find cases that support this change - even without org membership. All those logs are public. |
The skip rule's rationale claimed a cost saving that transcripts do not bear out: the orchestrator already skips the challenger on every clean run and dispatches whenever a finding exists. State the rule as codifying that behaviour, name the info-only case as open, and drop the cost claim. Raise the skill's context-budget ceiling for the added sentences. Signed-off-by: guy oron <goron@redhat.com>
Head branch was pushed to by a user without write access
|
@rh-hemartin @ralphbean — Ralph, thanks, that unblocked it. I was wrong on 09-03, twice. Method. Every Result: the case this PR's description targets does not occur.
All 31 dispatches had something to adjudicate, between 1 and 13 findings, The 26 skips are the actual finding. Every one succeeded and approved
So 6d says dispatch unconditionally after 6a–6c, and in practice the One design question this raises for the PR: as written the skip fires on an What that means for this PR. The cost argument is dead and I've cut it from Runs where no challenger was dispatched (PR · run id): 6601 · 32883020213 | 6619 · 32923569525 | 6623 · 32926932646 | 6639 · 33014604876 | 6641 · 32975833205 | 6692 · 33290305617 | 6709 · 33119185132 | 6722 · 33167094492 | 6723 · 33171507849 | 6724 · 33176354922 | 6727 · 33354041541 | 6751 · 33354051186 | 6774 · 33434745778 | 6786 · 33336906484 | 6819 · 33435583475 | 6823 · 33438242023 | 6881 · 33561248443 | 6896 · 33582655967 | 6924 · 33696464676 | 6935 · 33688286645 | 6938 · 33694247039 | 6969 · 33833291809 | 6995 · 33833269991 | 7002 · 33854676062 | 7028 · 33928582960 | 7062 · 34183658348 Largest and smallest challenger inputs: #6801 n=13, #6909 n=8, #6804 n=7, #6839 n=6, #6378 n=1, #6606 n=1, #6622 n=1, #6657 n=1. |
4 participants
Heyaa : )
Revised 2026-09-08 after measuring it. The original cost argument was wrong;
the analysis is in the thread.
Step 6d dispatches the
challengerunconditionally once steps 6a–6c haveproduced a merged finding set, and the re-review rule says "always dispatch".
Neither says what to do when that set is empty. In production the orchestrator
already decides for itself: across 57
fullsend-ai/fullsendreviews that postedzero findings, it skipped the challenger 26 times — every time when nothing
above
infohad come back, 24 of those times saying so in its reasoning — anddispatched it 31 times, every time with at least one finding to adjudicate.
Not once was the challenger handed an empty list.
This PR turns that into a rule instead of a per-run judgment call. One file,
skills/pr-review/SKILL.md:dispatch; 6e–6f still run and can add findings. A dimension failure cannot
produce an empty set because step 5 records a
sub-agent-failurefinding forit, so a failed dimension still gets a challenger.
current review's findings; prior findings alone do not qualify it.
challenger's output, or the untouched 6a–6c set when it was skipped.
Verdict logic is unchanged. Dispatch mechanics, output parsing and the failure
fallback are untouched. The skip is noted in the orchestrator's own reasoning,
not the review body: the result schema is
additionalProperties: falseandstep 7 forbids footers.
What this does not claim. No cost saving. I originally argued the challenger
burns opus adjudicating empty lists; the transcripts say that never happens.
This buys determinism only.
One trade-off, disclosed: this forfeits the challenger's secondary, not-owned
allowance to flag something it notices while reading the diff. In the 26
observed skips the orchestrator had already forfeited it.
Prompt-only change (the baseline bump is bookkeeping); the challenger exists only on the claude review path.
Refs #1144