feat(review): add confidence annotation to review verdicts - #396
feat(review): add confidence annotation to review verdicts#396Benkapner wants to merge 3 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 QodoAdd confidence annotation to review verdict output
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
I added "Closes fullsend-ai/fullsend#5514" to the PR body. |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| #### 6g. Determine confidence level | ||
|
|
||
| After the verdict is set, derive a confidence level based on the signals | ||
| available from the sub-agent and challenger passes. Confidence does not | ||
| change the verdict or any routing behavior. It is an informational | ||
| annotation for humans reviewing the agent's output. | ||
|
|
||
| **Confidence levels:** | ||
|
|
||
| - **high:** all sub-agents agreed on severity for every finding, the | ||
| challenger removed zero or one findings, and the verdict was not close | ||
| to a threshold boundary (e.g., no medium findings when the verdict is | ||
| approve) | ||
| - **medium:** the challenger removed more than one finding (initial | ||
| disagreement that was resolved), OR sub-agents flagged the same code | ||
| with different severities, OR the verdict is one finding away from | ||
| flipping (e.g., one medium finding on an approve verdict) | ||
| - **low:** significant sub-agent disagreement (same code flagged at | ||
| severities two or more levels apart), OR the challenger failed and | ||
| the pre-challenger set was used, OR the change scope is ambiguous | ||
| (e.g., large refactoring PR where correctness is hard to verify) | ||
|
|
||
| Include the confidence level in the review result JSON as a top-level | ||
| `confidence` field. This field is optional in the schema; omitting it | ||
| is acceptable if you cannot determine it. | ||
|
|
There was a problem hiding this comment.
1. Protected skills/pr-review modified 📜 Skill insight § Compliance
This PR modifies skills/pr-review/SKILL.md, which is a protected governance/infrastructure path and must not be auto-approved. A human review is required to avoid governance/control-plane changes being merged solely via automation.
There was a problem hiding this comment.
acknowledged, this needs human review by a codeowner.
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 8:15 PM UTC · Completed 8:31 PM UTC |
ReviewFindingsMedium
Low
Labels: PR modifies review agent schema and skill definition |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| }, | ||
| "label_actions": { | ||
| "$ref": "#/$defs/label_actions" | ||
| }, |
There was a problem hiding this comment.
[low] schema-inconsistency
The new confidence field uses a string enum (high/medium/low), while prioritize-result.schema.json and scribe-result.schema.json define confidence as a numeric field (0-1 range). The semantics differ — review confidence reflects sub-agent agreement and verdict stability, not a continuous probability — so the different representation is defensible, but documenting the rationale would prevent future confusion.
Suggested fix: Document why review confidence semantics differ from prioritize/scribe confidence and warrant a different type.
fullsend-ai-review
Bot
added
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
One more finding on step 6g's confidence logic — this one references code outside this PR's diff (skills/pr-review/SKILL.md:655, in step 6b), so it can't be attached as an inline comment there:
[MEDIUM] skills/pr-review/SKILL.md:655 — 6g's "different severities" medium-confidence trigger can never fire, because 6b already discards that exact signal
Step 6g's "medium" trigger #2 is "sub-agents flagged the same code with different severities." But step 6b, which runs earlier in the same synthesis pipeline, merges any two findings sharing category and location and instructs (line 655): "Keep the higher severity" — with no instruction to record that the inputs disagreed. By the time 6g runs, the one case where this signal is structurally detectable (two sub-agents, same category, same location, different severities) has already been silently collapsed into a single finding at the higher severity; the fact of disagreement is gone. As written, this specific 6g trigger has no data left to act on.
Separately, 6c deliberately keeps different-category findings on the same code as distinct entries (e.g., a correctness finding and a security finding on the same line) — normal, expected output, not disagreement — but 6g's phrasing doesn't distinguish this from genuine same-category severity disagreement, risking the trigger being misapplied to routine multi-dimension findings.
Suggestion: add an instruction to 6b to carry forward a disagreement marker (e.g., the discarded severity, or a count) when merging same-category/same-location findings, for 6g to consume. Clarify in 6g that the "different severities" trigger applies only to such same-category merges, not to 6c's intentionally distinct cross-category findings.
| Use `reject` only when no amount of code-level iteration will make | ||
| the PR mergeable. | ||
|
|
||
| #### 6g. Determine confidence level |
There was a problem hiding this comment.
[MEDIUM] Hardcoded numeric confidence thresholds go beyond what was asked for and aren't validated
Step 6g hardcodes absolute-count cutoffs: challenger removed "zero or one" findings = high vs. "more than one" = medium; severities "two or more levels apart" = low vs. any lesser gap = medium. These use counts rather than rates — a 1-finding PR where the challenger removes it (100% disagreement) buckets the same as a 20-finding PR where 1 of 20 is removed (5% disagreement), while a 20-finding PR where 2 of 20 are removed (10%) buckets into "medium."
The originating issue, fullsend-ai/fullsend#5514, proposes only qualitative bands ("high: ... no challenger overrides"; "medium: sub-agents disagreed ... challenger removed some but not all"; "low: significant sub-agent disagreement") — no 0-vs-1 count or two-level severity-gap numbers appear anywhere in it. Those specific cutoffs were introduced in this PR.
The linked problem doc (docs/problems/graduated-approval-policy.md) states directly, under "What we do not yet know": "Evidence for thresholds ... The thresholds need to be derived from observed outcomes, not guessed," and its "Path forward" section prescribes writing eval cases and demonstrating improvement before proposing thresholds. eval/review/cases/ in this repo currently contains only a .gitkeep — no eval cases exist to justify these numbers, despite eval/run-functional.sh already providing a harness to exercise them.
There's also direct precedent in this same problem area: fullsend-ai/fullsend#2255 documents a human reviewer closing fullsend-ai/fullsend#2012 specifically because it "was fundamentally a solution proposal (numeric scoring system, 5-tier routing table, three implementation approaches) filed as a problem doc."
Suggestion: walk the thresholds back to the qualitative language #5514 actually proposed (flagging any numeric boundary as a provisional heuristic pending calibration), or add eval cases under eval/review/cases/ exercising the 0/1/2+ boundaries and cite them as justification, per the problem doc's own prescribed path.
| the pre-challenger set was used, OR the change scope is ambiguous | ||
| (e.g., large refactoring PR where correctness is hard to verify) | ||
|
|
||
| Include the confidence level in the review result JSON as a top-level |
There was a problem hiding this comment.
[MEDIUM] Confidence has no consumer: not in the review body template, not in post-review.sh, not in interactive mode
Step 6g calls confidence "an informational annotation for humans reviewing the agent's output," but nothing renders it. Section 7's body template is restricted to the hidden SHA comment plus findings sections ("No summary section," "No footer ... do not repeat the outcome or include boilerplate") with no slot for confidence.
scripts/post-review.sh reads .action, .label_actions, .label_actions.reason, and .label_actions.actions[], but has no reference to .confidence anywhere. In interactive mode (no $FULLSEND_OUTPUT_DIR), no JSON artifact is written at all, so the computed value has no destination there either.
This repo already has a wired-up precedent for the same pattern: scripts/post-prioritize.sh reads .confidence via jq and writes it into the posted comment's RICE table and a tracked field. This PR doesn't follow that precedent.
Separately, the originating issue (fullsend-ai/fullsend#5514) lists three deliverables, the third being "Post-script PR comment: display confidence alongside the verdict." This PR implements only the first two, and the description doesn't flag the third as deferred.
(This is distinct from the existing LOW "consumer-completeness" finding already posted on this PR, which asks only for a cross-reference to confidence inside step 7's own documentation — it doesn't note that post-review.sh never reads the field, or that interactive mode has no destination for it at all.)
Suggestion: wire .confidence into scripts/post-review.sh and/or the body template so it actually reaches a human, following the post-prioritize.sh precedent, or state explicitly in the PR description that display is deferred to a tracked follow-up — and handle interactive mode so the computed value isn't silently discarded there too.
|
|
||
| **Confidence levels:** | ||
|
|
||
| - **high:** all sub-agents agreed on severity for every finding, the |
There was a problem hiding this comment.
[MEDIUM] "All sub-agents agreed" is vacuously true whenever findings don't overlap — the normal case
The "high" bullet requires "all sub-agents agreed on severity for every finding." Per the synthesis pipeline, each dispatched sub-agent runs in full context isolation (no sub-agent sees findings from other dimensions), and 6a/6b only compare severities when two findings land in the same file within 5 lines of each other and share a category. Per the dispatch table, the normal case is non-overlapping fan-out — e.g., a "bug fix in auth middleware" change dispatches correctness, security, style-conventions, and intent-coherence sub-agents, each typically flagging distinct concerns.
When sub-agents report entirely non-overlapping findings (the common case), there's nothing to compare, so "all agreed" holds vacuously no matter how many independent issues were found. A PR where 4 sub-agents each surface one distinct, non-overlapping low-severity finding qualifies for the same "high" label as a PR where sub-agents actually examined the same code and converged on it — conflating "no detected conflict" with "actively corroborated."
Suggestion: require some minimum location overlap before "high" can be claimed on agreement grounds, or explicitly document that this criterion only asserts the absence of detected conflicts rather than independent corroboration.
|
|
||
| - **high:** all sub-agents agreed on severity for every finding, the | ||
| challenger removed zero or one findings, and the verdict was not close | ||
| to a threshold boundary (e.g., no medium findings when the verdict is |
There was a problem hiding this comment.
[MEDIUM] The "high" bullet's threshold-boundary example is tautological — the same bug already fixed in the adjacent "medium" bullet wasn't fixed here
The parenthetical "(e.g., no medium findings when the verdict is approve)" is vacuously true for every approve verdict: per 6f, approve is only reached via "no findings" or "low/info findings only (no medium+)," so an approve verdict can never have a medium finding regardless of how borderline anything was. The example can never disqualify a case from "high," so it provides no actual discriminating signal.
The exact same defect existed in the adjacent "medium" bullet ("one medium finding on an approve verdict" — an impossible combination under 6f). qodo-code-review flagged that specific phrase earlier in this PR's review, and it was fixed in commit c70786a ("changed 'approve verdict' to 'comment-only verdict' to match step 6f") — the line below now correctly reads "one medium finding on a comment-only verdict." That fix only addressed the medium bullet's text; the high bullet's separate, analogous tautology here was left unchanged.
Suggestion: replace the parenthetical with a reachable boundary case (e.g., zero findings vs. several low/info findings a sub-agent nearly classified as medium), or drop the example since 6f already guarantees zero medium+ findings on any approve verdict.
c70786a to
b835553
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated review-only pass on step 6g's confidence rubric and the schema change. 4 findings below (1 high, 3 medium); none overlap with the review comments already posted on this PR.
| **Confidence levels:** | ||
|
|
||
| - **high:** all sub-agents agreed on severity for every finding, the | ||
| challenger removed zero or one findings, and the verdict was not close |
There was a problem hiding this comment.
[HIGH] No precedence rule when confidence-band criteria overlap (challenger-failure vs. full-agreement, and medium vs. low severity-gap)
Step 6g's three confidence bands are independent OR/AND lists with no stated evaluation order, and two concrete collisions exist in the current text (verified on head b835553):
- High requires "the challenger removed zero or one findings." Per step 6d (lines 915-925, unchanged by this PR), when the challenger sub-agent fails, the orchestrator falls back to the pre-challenger set and records an info-level
sub-agent-failurefinding — but the effective "removed count" signal is also zero, making a failed challenger pass indistinguishable from one that ran and agreed with everything. Low separately lists "the challenger failed and the pre-challenger set was used" (line 1149) as its own trigger, but nothing in 6g instructs checking for that condition before evaluating high's removed-count criterion, so a failed challenger pass risks being read as maximum confidence instead of the low confidence intended. - Medium's "sub-agents flagged the same code with different severities" (lines 1145-1146) is a strict superset of low's "severities two or more levels apart" (lines 1148-1149) — every case satisfying low's condition also satisfies medium's, with no tie-break stated for which wins.
Suggestion: state an explicit evaluation order, e.g., "check low's triggers first (including the step 6d sub-agent-failure marker); if none apply, check medium; otherwise high," and narrow medium's severity-difference clause to exclude gaps of two or more levels so it doesn't overlap with low's condition.
| approve) | ||
| - **medium:** the challenger removed more than one finding (initial | ||
| disagreement that was resolved), OR sub-agents flagged the same code | ||
| with different severities, OR the verdict is one finding away from |
There was a problem hiding this comment.
[MEDIUM] comment-only verdicts are structurally incapable of ever reaching high confidence
Verified on head b835553: step 6f (line 1119) defines comment-only as exactly "one medium finding (but no critical/high)" — every comment-only verdict has precisely one medium finding by construction. Step 6g's medium band cites "one medium finding on a comment-only verdict" (line 1147) as its illustrative example of "the verdict is one finding away from flipping." Since this is true of every single comment-only verdict without exception, it is unconditionally satisfied whenever the outcome is comment-only.
High's third requirement is that "the verdict was not close to a threshold boundary" (lines 1141-1142) — but the only description of what counts as "close to a boundary" is the same condition medium's example describes, which every comment-only verdict trivially meets. The practical consequence: no comment-only verdict can ever be labeled "high" confidence, no matter how unanimous the sub-agents were and how little the challenger changed, because the class of comment-only verdicts is definitionally always "one finding from flipping."
This is a distinct, deeper defect from the already-posted note that the high bullet's own approve-verdict example is vacuous — this is about an entire verdict category being capped, not about one example being uninformative.
Suggestion: either explicitly document that comment-only caps out at medium confidence (if intentional), or add an exception allowing high when the single medium finding was unanimous across sub-agents and survived the challenger unchanged.
| Use `reject` only when no amount of code-level iteration will make | ||
| the PR mergeable. | ||
|
|
||
| #### 6g. Determine confidence level |
There was a problem hiding this comment.
[MEDIUM] Confidence rubric gives no guidance for the reject verdict, the most subjective and highest-stakes case
Verified on head b835553: step 6f's reject clause (lines 1126-1129) fires on a subjective architectural judgment ("the approach is fundamentally wrong... no amount of code-level iteration will make the PR mergeable"), not on a tally of finding severities. All three bands in step 6g (lines 1140-1151), however, are defined purely in terms of per-finding signals: sub-agent severity agreement, challenger removal counts, and proximity to the approve/comment-only/request-changes thresholds. None of these naturally describe confidence in a reject call. Mechanically applying the stated rules, a reject verdict whose underlying findings all happened to have agreed severities and zero challenger removals would score "high" — even though what's actually contestable about a reject call is the architectural judgment itself, not the findings' severities.
Suggestion: add an explicit rule for reject, e.g., default it to "medium" unless the rejection rationale itself (not just finding severities) was independently corroborated by multiple sub-agents, or state that confidence for reject should reflect agreement on the architectural judgment rather than on finding tallies.
| "label_actions": { | ||
| "$ref": "#/$defs/label_actions" | ||
| }, | ||
| "confidence": { |
There was a problem hiding this comment.
[MEDIUM] Schema does not forbid confidence alongside action: "failure", allowing a self-contradictory result
Verified on head b835553: the confidence property (line 30) has no allOf conditional restricting it. The schema's allOf block already has a targeted conditional for action == "failure" (line 54) requiring only ["reason"], and a separate targeted conditional for approve+protected-path exclusion (lines 57-75) — establishing the pattern for this kind of restriction — but no equivalent guard excludes confidence from a failure result. Step 6g frames confidence as derived "after the verdict is set" (line 1133), presupposing a completed review, while action: "failure" represents the opposite (SKILL.md's failure-output guidance states the PR was not reviewed at all). As written, a result like {"action":"failure","reason":"tool-failure","confidence":"high"} currently validates against the schema, which is incoherent — "high confidence" in a verdict that was never reached.
Suggestion: add an allOf entry mirroring the existing pattern: {"if": {"properties": {"action": {"const": "failure"}}, "required": ["action"]}, "then": {"not": {"required": ["confidence"]}}}.
|
On precedence collisions i agree as the bands need explicit evaluation order. ill add "check low triggers first if none apply, check medium otherwise high" in the next push, what do you think? On comment-only capped - this is actually intentional. A comment-only verdict is by definition close to a threshold boundary (one more medium finding would flip to request-changes). ill document this explicitly rather than leaving it implicit. on reject having no confidence guidance: not sure, i mean, its fair.. reject is a qualitative judgment call.. what do you say about default reject to "medium" confidence, because rejection is inherently a judgment call and only bump to "high" if multiple sub-agents independently said something like "this approach is fundamentally flawed" (not just "i found a bug"). That way the human reviewer sees "reject (medium confidence)" and knows to look carefully at whether reject is warranted vs. just requesting changes. @ben-alkov @waynesun09 @rh-hemartin On schema allowing confidence with action: "failure": will add the allOf conditional to exclude confidence from failure results, matching the existing pattern. |
b835553 to
261f903
Compare
|
@waynesun09 thanks for the detailed review. I rebased the branch onto current Here is how each finding maps to the change (the rubric now lives in
Also in this revision, so the field actually reaches the PR: |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head 261f903 on the revised step 6g rubric, the 6b "synthesis notes" addition, and the new post-script annotation. 3 medium findings below; none overlaps an existing thread on this PR.
| # Append confidence annotation to body (skips failure, which has no body) | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| CONFIDENCE=$(jq -r '.confidence // empty' "${RESULT_FILE}") |
There was a problem hiding this comment.
[MEDIUM] Confidence annotation is posted against a verdict the post-script already overrode
Verified at head 261f903. The new block reads .confidence and appends **Confidence:** <value> to the body, but by that point post-review.src.sh may already have replaced the agent's verdict, and the confidence value is never revisited.
Two confirmed override paths run BEFORE the confidence block:
- Severity filter: when filtering removes every finding,
jq 'del(.findings) | .action = "comment"'rewritesrequest-changes/rejecttocomment. This runs beforeACTIONis read, so ACTION is already the rewritten value. - Protected-path check:
jq '.action = "comment" | .body = (.body + $notice)'rewritesapprovetocommentand appends a "human reviewer must approve" notice, settingDOWNGRADED=true. The script explicitly documents that theACTIONshell variable retains the original value here ("ACTION retains the original value for the entire script — not re-read after protected-path downgrade").
In both paths the confidence the agent computed for the original verdict is rendered verbatim under the new one. A protected-path downgrade will routinely post comment + **Confidence:** high — a combination the new rubric says is essentially unreachable (SKILL.md §6g caps comment-only at medium unless a narrow corroboration test passes). Per §6g confidence is a property of the action ("how strongly the evidence and sub-agent agreement support this action"), so after a downgrade the posted value describes an action that no longer exists.
This is not cosmetic: the PR's stated purpose is to emit this datum for downstream graduated-approval work, and the value is wrong precisely on the protected-path and all-findings-filtered paths — the paths where a human (and any future automation) most needs an accurate signal. Note that skills/pr-review/ is itself a protected path, so this scenario fires on this repo's own reviews of PRs like this one. Neither of the two added tests in post-review-test.sh covers a downgrade combined with confidence.
Suggestion: Re-read the action after the downgrade paths, or gate the annotation on a downgrade flag, and either drop confidence from the body when the post-script changed the verdict or scope it to the agent's original verdict, e.g. **Confidence:** high (agent verdict: approve — downgraded by protected-path check). Note that a flag-based fix must cover BOTH paths: DOWNGRADED exists only for the protected-path branch, while the severity-filter branch keeps original_action local to its own block. Add post-review-test.sh cases for approve+confidence+protected path and for request-changes+confidence fully filtered, asserting the resulting annotation.
| Omit `confidence` entirely for the `failure` action. | ||
|
|
||
| **Evaluation order.** The bands below can overlap, so evaluate them in a | ||
| fixed order and assign the **first** band whose condition holds: low |
There was a problem hiding this comment.
[MEDIUM] 6g's "first matching band wins, never promote" rule contradicts its own promotion clauses for reject and comment-only
Verified against the diff at head 261f903. §6g states an absolute precedence rule: "evaluate them in a fixed order and assign the first band whose condition holds: low first, then medium, then high. The most cautious matching band wins; never promote to a higher band once a lower one has matched."
But two of the Medium band's triggers are unconditional properties of the action, not evidence conditions:
- "The action is
comment-only. Medium is the ceiling forcomment-onlyunless the single driving medium finding was raised by more than one sub-agent AND survived the challenger unchanged; only then maycomment-onlyreach high." - "The action is
reject. ... default to medium. Promote to high only when the architectural objection is corroborated ..."
The clearest defect is a direct wording collision that survives any reading: the reject bullet literally says "Promote to high," while the ordering rule literally says "never promote to a higher band once a lower one has matched." Two literal-minded agents will band the same review differently.
Under the strict first-match reading, the Medium band matches by definition for every comment-only and every reject verdict, so the escape clauses in those same bullets are dead text and no reject or comment-only verdict can ever be assigned high. There is a more charitable parse — reading the condition as "comment-only AND NOT(corroborated)" — but the rubric never says which parse is intended, and that ambiguity is itself the problem in a document whose whole purpose is to make banding deterministic.
This matters because the author's revision comment lists these as separate fixes (item 1 = add evaluation order, item 6 = comment-only ceiling, item 7 = reject default), each added verbatim in response to a prior reviewer request. Item 1 silently undercuts the exceptions added by 6 and 7, so the fixes do not compose.
Suggestion: Separate band selection from band caps. Step 1: pick a band from the evidence conditions only (challenger failure, severity gaps, reconciliation, corroboration, missing context) using the low-then-medium-then-high order. Step 2: apply action-based ceilings (comment-only and reject cap at medium unless the stated corroboration test passes). Ceilings can then only lower a band, never collide with the first-match/never-promote rule, and the word "promote" disappears from the bullets.
| - Preserve `actionable: true` if either finding had it | ||
| - If the merged findings disagreed on severity, record that disagreement | ||
| and the size of the gap (in severity levels) in your synthesis notes. | ||
| Merging to the higher severity discards the disagreement from the |
There was a problem hiding this comment.
[MEDIUM] 6g depends on "synthesis notes", an artifact that exists nowhere else in the skill or the output contract
Verified at head 261f903. The 6b addition instructs: "record that disagreement and the size of the gap (in severity levels) in your synthesis notes ... step 6g reads these notes when setting confidence."
The term is introduced by this PR and never defined. A repo-wide code search for "synthesis notes" returns zero occurrences, and a notes search scoped to skills/pr-review matches only unrelated GitLab API comment endpoints (/issues/<iid>/notes) — there is no note-taking artifact concept anywhere in the skill.
Nothing says where these notes live: a file under $FULLSEND_OUTPUT_DIR, in-context scratch text, or a field on the merged finding. They are not part of review-result.schema.json (the schema diff adds only confidence), so they are unobservable and untestable. Nothing states that they must be written before, or survive, the challenger pass in 6d/6e, or any context compaction between 6b and 6g.
The severity-gap machinery in §6g depends entirely on them — both the low band's "two or more levels" trigger and the medium band's "exactly one level" trigger read from this undefined store. This is the author's stated fix (revision item 2) for the earlier finding that the "different severities" trigger could never fire because 6b collapses to the higher severity. An agent holding 6b and 6g in one context may well carry the data forward in working memory, so the trigger is not strictly unimplementable — but the fix relocates the requirement into an undefined carrier rather than giving the data a defined home, which leaves the mechanism unverifiable and fragile.
Suggestion: Make the disagreement observable at the point 6g needs it: either keep the pre-merge severities on the merged finding (e.g. an internal merged_from: [{severity: low}, {severity: high}] note carried through 6c-6f), or define "synthesis notes" concretely in 6b — what file or section, what fields, and that it must be written before the challenger pass. Otherwise drop the severity-gap triggers from 6g until there is a defined carrier.
Add an optional `confidence` field (high/medium/low) to the review result, letting the review agent signal how strongly the evidence and sub-agent agreement support its verdict. This is a minimal step toward graduated approval: the value is advisory and does not change the action. Addresses the review feedback on the prior confidence proposal: - Precedence: confidence bands are evaluated low, then medium, then high, with the first (most cautious) matching band winning, so overlapping conditions resolve deterministically. - Bands are qualitative, not numeric; the residual severity-gap splits are marked provisional pending eval-case calibration. - 6b merges now record severity disagreement in synthesis notes so the medium "sub-agents disagreed" trigger can actually fire (merging to the higher severity previously discarded it). - High requires absence of detected conflict plus corroboration of the driving findings, not the vacuous "all agreed" when scopes are disjoint. - comment-only caps at medium unless the single driving finding was corroborated and survived the challenger; reject defaults to medium. - Dropped the tautological high boundary example. - Schema forbids `confidence` when action is `failure`. The post-review script appends a "Confidence:" annotation to the body for non-failure actions; agents/review.md documents the field. Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
Post-review can rewrite the action (severity filter, protected-path) before the confidence line is appended. Name the agent's original verdict when that happens. Split 6g into evidence-band then action ceilings, and carry 6b severity disagreement as merged_from instead of undefined synthesis notes. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
261f903 to
392d2a1
Compare
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass at head 392d2a1 on the new step 6g rubric, the 6b/6d merged_from plumbing, the agents/review.md field table, and the post-script confidence block. 9 findings (1 high, 8 medium); each was checked against the threads already open on this PR and none restates one.
8 are inline below. The remaining one lands outside the diff hunks, so it is recorded here:
[MEDIUM] agents/review.md:283 — agents/review.md's jq templates are labelled "the exact JSON shape for each action" but none of them set confidence
This PR adds the confidence row to the top-level field table (agents/review.md:259). But agents/review.md:283-284 still reads "Schema validation failures trigger a harness retry iteration. The jq examples below show the exact JSON shape for each action." and the four jq -n templates that follow — approve/comment, request-changes/reject, failure, and the label_actions variant — contain no confidence argument.
Since agents/review.md is the harness agent prompt (harness/review.yaml) and carries the standing directive to emit only the listed fields because the schema is strict, an orchestrator that copies the templates literally will never emit confidence — which the PR description names as the deliverable. The table row alone does not fix this while the templates are labelled as the exact shape.
This is not covered by the file+line dedupe rule: the existing thread on this topic (qodo 3637924755/3637974494) is anchored on skills/pr-review/SKILL.md:981, and the author's reply there deferred agents/review.md as "a different file." The table row has since been added; the jq templates were not.
Suggestion: Add --arg confidence "<high|medium|low>" and confidence: $confidence to the approve/comment and request-changes/reject templates — or, to keep omission valid, show the optional form + (if $confidence != "" then {confidence: $confidence} else {} end). Also soften the "exact JSON shape" wording so the templates are not read as an exhaustive field list.
| block the PR. When the summary language and the verdict action | ||
| contradict each other, escalate the verdict to match the language. | ||
|
|
||
| #### 6g. Determine confidence level |
There was a problem hiding this comment.
[HIGH] skills/pr-review/SKILL.md now exceeds its skillsaw context-budget baseline ceiling — make lint / CI will fail, and the failure is currently invisible on the PR
Empirically verified by running the pinned linter (uvx skillsaw@0.18.0 --strict, the version in .skillsaw.yaml and used by make lint/.github/workflows/lint.yml) on both revisions of a fresh clone:
- base
a359832(merge-base with main):Errors: 0,Baseline: 10 suppressed - PR head
392d2a1:Errors: 1—ERROR (context-budget) [skills/pr-review/SKILL.md]: Estimated 16,753 tokens exceeds skill error limit of 6,000,Baseline: 9 suppressed
The pre-existing violation is suppressed by .skillsaw-baseline.json in baseline_mode: "ceiling" with "value": 15709. This PR grows SKILL.md from 1,353 to 1,435 lines (8,190 → 8,799 words), pushing the estimate to 16,753 — 1,044 tokens above the ceiling — so the suppression no longer applies and the error surfaces.
CONTRIBUTING.md:39 is explicit: "Do not regenerate the baseline for violations introduced or increased by your PR; fix those instead."
This is not visible on the PR page: for head 392d2a1 the CI and Script tests workflow runs are both completed/action_required (fork PR awaiting maintainer approval) and never executed, so nothing on GitHub has evaluated the branch. Merging as-is lands a red make lint on main.
Suggestion: Cut SKILL.md back under the 15,709-token ceiling rather than re-baselining. Cheapest option that preserves the rubric: move the 6g body (evidence bands, action ceilings, provisional-boundaries note) into a sibling reference file — e.g. skills/pr-review/confidence.md — that step 6g instructs the orchestrator to read, mirroring the pattern the skill already uses for meta-prompt.md. Then re-run uvx skillsaw@0.18.0 --strict and confirm Errors: 0. Separately, a maintainer should approve the workflow runs so this surfaces on the PR before merge.
| **Low** (checked first). Assign if any of: | ||
|
|
||
| - The challenger pass failed and you fell back to the pre-challenger | ||
| finding set (a `sub-agent-failure` info finding is present, see 6d). |
There was a problem hiding this comment.
[MEDIUM] 6g's Low trigger for challenger failure names an info finding, but 6d emits low — the trigger matches the wrong event and misses the one it describes
SKILL.md:1239-1240 (added by this PR) reads: "The challenger pass failed and you fell back to the pre-challenger finding set (a sub-agent-failure info finding is present, see 6d)."
But 6d item 4 (SKILL.md:1008-1018, unchanged by this PR) records the challenger-failure marker as "severity": "low", with the surrounding prose reading "Record a low-level finding (info is below the posting threshold)" — i.e. 6d explicitly chose low over info.
The only info-severity sub-agent-failure findings the skill produces come from step 5 (SKILL.md:839-845): Sonnet-tier dimension sub-agent failures (intent-coherence, style-conventions, docs-currency, cross-repo-contracts). So an orchestrator matching the parenthetical literally will assign Low when e.g. docs-currency timed out — which is not what the bullet is about — and will not recognise the challenger's actual low marker as satisfying the trigger it was written for.
Note for the author: the already-posted thread 3683140857 itself described 6d's marker as "an info-level sub-agent-failure finding" — that comment is almost certainly where the wording was copied from, so the fix is a one-word correction, not a redesign.
Suggestion: Change the parenthetical to "(a low-severity sub-agent-failure finding from 6d item 4 is present)". Separately decide whether a dimension sub-agent failure (step 5's info/high markers) should influence confidence and, if so, list it as its own explicit condition rather than letting it fire this one by accident.
|
|
||
| - The challenger pass failed and you fell back to the pre-challenger | ||
| finding set (a `sub-agent-failure` info finding is present, see 6d). | ||
| - A 6b merge combined findings that disagreed on severity by two or more |
There was a problem hiding this comment.
[MEDIUM] Step 1's confidence rubric is not total — a two-level merged_from gap on a non-verdict-driving finding matches no band
Step 1 says "Assign the first band whose condition holds" (SKILL.md:1233-1234) but supplies no residual clause, and High is written as an exclusion list rather than an else-branch. A reachable combination falls through all three:
A 6b merge produced a merged_from severity gap of two or more levels on a finding that does not drive the verdict (e.g. an info↔medium merge on a side finding, alongside a separately corroborated high that actually sets the verdict).
- Low (1241-1244) requires the two-level gap "and that finding drives the verdict" → does not hold.
- Medium (1256-1257) requires a gap of "exactly one level" → does not hold.
- High (1260-1262) requires "no
merged_fromseverity disagreement in any 6b merge" → barred.
No band applies and the orchestrator is left to guess, which defeats the deterministic-ordering fix added in response to thread 3862380162. The asymmetry is the root cause: the two-level Low condition carries a "drives the verdict" qualifier while the one-level Medium condition is unconditional, and nothing explains why. Three reviewers independently landed on this same gap.
Suggestion: Add an explicit residual rule at the end of Step 1 — e.g. "If no low and no medium condition holds but a high exclusion applies, assign medium." And resolve the asymmetry directly: either drop "and that finding drives the verdict" from the Low two-level condition, or apply the same qualifier to the one-level Medium condition so the two are parallel.
| not corroborate each other, so high additionally requires that each | ||
| finding driving the verdict was either raised by more than one | ||
| sub-agent or confirmed by the challenger. | ||
| - For an `approve` with no findings, high is appropriate when all |
There was a problem hiding this comment.
[MEDIUM] Step 1 says "Do not consider the action" and then its own High bullet conditions on approve; Step 2's ceiling assumes a single medium finding that 6f does not guarantee
Two internal inconsistencies in the new 6g, both verified at head 392d2a1:
-
SKILL.md:1233-1235 states "Do not consider the action (
comment-only,reject,approve) in this step," yet the second High bullet at 1267-1268 reads "For anapprovewith no findings, high is appropriate when..." — an action-conditioned rule inside the step that forbids action-awareness. The whole point of the two-step split (added in response to thread 3862380162) was to keep selection and ceilings from colliding; this bullet reintroduces the collision, and two literal-minded agents will resolve it differently. -
Step 2's ceiling at SKILL.md:1273-1275 says "
comment-only: cap at medium unless the single driving medium finding was raised by more than one sub-agent AND survived the challenger unchanged." But 6f:1195-1198 defines comment-only as "One or more medium findings that are all stylistic/advisory/process-related" — so the singular is stale and the ceiling has no defined behaviour when a comment-only verdict rests on two or more mediums.
Suggestion: (1) Move the approve/no-findings clause into Step 2 as an explicit approve rule, or restate it action-neutrally in Step 1 ("no findings, and every dispatched dimension sub-agent and the challenger returned without error"). (2) Reword the comment-only ceiling as "unless every driving medium finding was raised by more than one sub-agent AND survived the challenger unchanged."
| finding driving the verdict was either raised by more than one | ||
| sub-agent or confirmed by the challenger. | ||
| - For an `approve` with no findings, high is appropriate when all | ||
| dimension sub-agents ran and returned without error. |
There was a problem hiding this comment.
[MEDIUM] High's "all dimension sub-agents ran" condition contradicts step 3c's selective dispatch, making it unreachable for the small clean PRs it targets
SKILL.md:1267-1268: "For an approve with no findings, high is appropriate when all dimension sub-agents ran and returned without error."
But dispatch is explicitly selective. SKILL.md:291 reads "Dispatch sub-agents based on the classification — typically 3-6." and SKILL.md:293 says to match "the specific change rather than dispatching all agents by default." Step 3c adds a re-review narrowing rule on top (SKILL.md:309-359) that further reduces the dispatched set. A typo-fix PR may dispatch only correctness + style-conventions, deliberately skipping the other dimensions.
Read literally as the full roster, the condition can essentially never be satisfied — and it fails hardest on exactly the small, clean, approve-with-no-findings PRs that the linked graduated-approval-policy.md most wants marked high. Read as "the ones actually dispatched," it is satisfiable, but the rubric never says which reading is intended.
Suggestion: Reword to "every dispatched dimension sub-agent returned without error" (and, if a deliberately narrow dispatch should itself limit confidence, say so explicitly rather than leaving it to the ambiguity).
| `adjudicated_findings` (then re-append anything withheld). Copy | ||
| each finding's internal `merged_from` (if present) from the | ||
| pre-challenger finding that shares category and location — the | ||
| challenger is not shown that field, and 6g still needs it. |
There was a problem hiding this comment.
[MEDIUM] 6d's merged_from copy-back is justified by a claim the skill's own text contradicts, and is undefined for challenger_action: "merged"
The new copy-back instruction at SKILL.md:999-1002 says to copy merged_from "from the pre-challenger finding that shares category and location — the challenger is not shown that field, and 6g still needs it."
The stated justification is false as written. 6d Part 3 (SKILL.md:944-946) passes "<JSON array of all findings from steps 6a–6c>" verbatim into the challenger prompt, and 6b (SKILL.md:902-903) instructs "Carry this field through 6c–6f." Nothing strips merged_from before dispatch, so the challenger is shown it. The real (and unstated) reason a copy-back is needed is that skills/pr-review/sub-agents/challenger.md "Output format" (lines ~48-75) enumerates the fields of adjudicated_findings and does not include merged_from, so a model following that schema will drop it.
The substantive gap: the copy-back key — "the pre-challenger finding that shares category and location" — has no answer when the challenger returns challenger_action: "merged" (challenger.md), collapsing two pre-challenger findings at different locations into one. There is then no single source finding to copy from, and 6g's severity-gap triggers silently lose their input on exactly the findings where adjudication was most active.
Suggestion: Either strip merged_from explicitly in 6d Part 3 before dispatch (making the stated reason true, and preventing an internal synthesis field from influencing the challenger), or drop the claim and say instead that the challenger's output format omits it. Then define the copy-back for challenger_action: "merged" — e.g. union the merged_from arrays of every pre-challenger finding the challenger merged, or treat a challenger merge as a disagreement event in its own right.
| - Combine descriptions if they add complementary detail | ||
| - Keep the more specific remediation | ||
| - Preserve `actionable: true` if either finding had it | ||
| - If the merged findings disagreed on severity, attach an internal |
There was a problem hiding this comment.
[MEDIUM] High's corroboration test ("raised by more than one sub-agent") has no carrier, because 6b records merged_from only when severities disagreed
6b at SKILL.md:900-903 attaches the marker conditionally: "If the merged findings disagreed on severity, attach an internal merged_from array..." When two sub-agents raise the same category at the same location with the same severity — the agreeing case, and the only case that constitutes positive corroboration — 6b merges them silently and leaves no marker at all.
But the new High band (SKILL.md:1263-1266) requires "each finding driving the verdict was either raised by more than one sub-agent or confirmed by the challenger," and both Step 2 ceilings (1273-1278) key on "raised by more than one sub-agent." The provenance those rules read is exactly what an agreeing merge discards. Additionally, merged_from records only severities, never the source dimension, so even when present it cannot distinguish two sub-agents agreeing from one sub-agent reporting the same location twice.
This is the mirror image of the defect the author already accepted and fixed on the disagreement side (thread 3862381546, which replaced undefined "synthesis notes" with merged_from) — the same missing-carrier problem, now on the corroboration side. It is not covered by thread 3674506022, whose resolution introduced this corroboration requirement rather than giving it a data source.
Suggestion: Attach merged_from on every 6b merge, not only on disagreement, and record the source dimension per entry — e.g. merged_from: [{dimension: correctness, severity: high}, {dimension: security, severity: high}]. High and the Step 2 ceilings can then test for two or more distinct dimensions, and the severity-gap triggers remain derivable from the same field. (Keep the existing strip-before-agent-result.json instruction at SKILL.md:904.)
| # CONFIDENCE_AGENT_ACTION / CONFIDENCE_DOWNGRADE_REASON record a verdict the | ||
| # post-script overrode so the confidence annotation can name the agent's | ||
| # original action (severity-filter sets them above; protected-path below). | ||
| CONFIDENCE_AGENT_ACTION="${CONFIDENCE_AGENT_ACTION:-}" |
There was a problem hiding this comment.
[MEDIUM] CONFIDENCE_AGENT_ACTION / CONFIDENCE_DOWNGRADE_REASON are initialized with :-, so a pre-set environment variable can make the script post a downgrade that never happened
scripts/post-review.src.sh:188-189 (and the generated scripts/post-review.sh) read:
CONFIDENCE_AGENT_ACTION="${CONFIDENCE_AGENT_ACTION:-}"
CONFIDENCE_DOWNGRADE_REASON="${CONFIDENCE_DOWNGRADE_REASON:-}"
Bash's :- preserves any value already present in the environment, so an exported variable of either name — leaked from CI, a parent process, or a prior sourcing in the same shell — survives into the confidence block. At line 400 a non-empty CONFIDENCE_DOWNGRADE_REASON is the sole gate for the downgrade wording, so the script would post **Confidence:** high (agent verdict: <stale> — downgraded by <stale>) on a review where neither override path ran, falsely telling a human that the post-script changed the verdict.
The file's own convention is unconditional initialization: DOWNGRADED=false at line 197 is a plain assignment. The two in-script writers are the severity filter (lines 171-172) and the protected-path check (lines 283-284), and only those should ever set these values. Note the fix must be placed above line 171, not at 188 — the severity-filter block runs first, so clearing at 188 would erase a legitimate in-script setting (and set -euo pipefail at line 27 means the variables must be initialized before any read).
Suggestion: Replace the :- defaults with unconditional CONFIDENCE_AGENT_ACTION="" / CONFIDENCE_DOWNGRADE_REASON="" placed before the severity-filter block (above line 171), matching the DOWNGRADED=false pattern, and delete the assignments at 188-189. Regenerate scripts/post-review.sh with make script-build so make check-bundle stays green.
Move the 6g body under the skillsaw ceiling. Record merged_from on every 6b merge, strip it for the challenger, copy it back, and ignore stale CONFIDENCE_* env. Signed-off-by: Benjamin Kapner <bkapner@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
Summary
Adds a confidence level (
high/medium/low) to the review agent's structured output. This is the minimal first step toward graduated approval, emitting the data without changing any routing or auto-merge behavior.Ported from fullsend-ai/fullsend#5515 per rh-hemartin's feedback that agents have moved to this repo.
What changes
confidencefield (high/medium/low)Confidence derivation
What does NOT change
Related
Checklist