-
Notifications
You must be signed in to change notification settings - Fork 16
feat(review): add confidence annotation to review verdicts #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -897,6 +897,14 @@ When merging | |
| - 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 | ||
| `merged_from` array on the merged finding listing each input | ||
| severity, e.g. `merged_from: [{severity: low}, {severity: high}]`. | ||
| Carry this field through 6c–6f so step 6g can measure the gap. | ||
| Strip `merged_from` before writing `agent-result.json` — it is not | ||
| part of the output schema. This applies only to the same-category | ||
| merges here in 6b, not to the distinct-category findings preserved | ||
| in 6c. | ||
|
|
||
| #### 6c. Preserve distinct-category findings | ||
|
|
||
|
|
@@ -988,7 +996,10 @@ budget section), skip the challenger: keep the merged finding set from | |
| that removes all findings is unlikely — an empty result more likely | ||
| indicates a parsing error or context truncation. | ||
| - Otherwise, replace the challenged subset with the challenger's | ||
| `adjudicated_findings` (then re-append anything withheld). | ||
| `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. | ||
| - Log any `removed_findings` for transparency but do not include | ||
| them in the final review. | ||
|
|
||
|
|
@@ -1205,6 +1216,73 @@ require action, because `comment` (COMMENTED review state) does not | |
| 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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
|
|
||
| After the outcome is fixed (6f), set an optional `confidence` value | ||
| (`high`, `medium`, or `low`) describing how strongly the evidence and | ||
| sub-agent agreement support the verdict. Confidence is advisory: it does | ||
| not change the action, it only annotates the verdict for the human | ||
| reviewer and for downstream graduated-approval work (see | ||
| [`graduated-approval-policy.md`](https://github.com/fullsend-ai/fullsend/blob/main/docs/problems/graduated-approval-policy.md)). | ||
| Omit `confidence` entirely for the `failure` action. | ||
|
|
||
| Confidence is two steps that must not be mixed: pick a band from | ||
| evidence, then apply action ceilings that can only lower it. | ||
|
|
||
| **Step 1 — evidence band.** Evaluate only the evidence conditions below, | ||
| in order: low first, then medium, then high. Assign the first band whose | ||
| condition holds. Do not consider the action (`comment-only`, `reject`, | ||
| `approve`) in this step. | ||
|
|
||
| **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). | ||
| - A 6b merge combined findings that disagreed on severity by two or more | ||
| levels (read `merged_from` on the merged finding; for example | ||
| `{severity: low}` and `{severity: high}`), and that finding drives the | ||
| verdict. | ||
| - The verdict rests on a finding the challenger downgraded, or on a | ||
| reconciliation (6e-1) that resolved a direct contradiction between | ||
| sub-agents. | ||
| - Required PR context was missing or partial. | ||
|
|
||
| **Medium** (checked next). Assign if no low condition holds and any of: | ||
|
|
||
| - A 6b merge combined findings that disagreed on severity by exactly one | ||
| level (read `merged_from`). | ||
| - The verdict rests on a single finding with no corroboration from a | ||
| second sub-agent or from the challenger. | ||
|
|
||
| **High** (checked last). Assign only if no low or medium condition holds | ||
| and: | ||
|
|
||
| - No detected conflict survived synthesis: no `sub-agent-failure` | ||
| finding, no `merged_from` severity disagreement in any 6b merge, and no | ||
| reconciliation contradiction. This is *absence of detected conflict*, | ||
| not positive corroboration. Sub-agents that examined disjoint areas do | ||
| 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 | ||
| dimension sub-agents ran and returned without error. | ||
|
|
||
| **Step 2 — action ceilings.** After step 1, apply these caps. A ceiling | ||
| may only lower the band; it never raises it. | ||
|
|
||
| - `comment-only`: cap at medium unless the single driving medium finding | ||
| was raised by more than one sub-agent AND survived the challenger | ||
| unchanged. Only then may the step-1 band of high stand. | ||
| - `reject`: cap at medium unless the architectural objection was raised | ||
| independently by more than one sub-agent or explicitly confirmed by | ||
| the challenger. Only then may the step-1 band of high stand. | ||
|
|
||
| **Provisional boundaries.** The one-level and two-level severity-gap | ||
| splits above are provisional heuristics, not calibrated thresholds. Per | ||
| `graduated-approval-policy.md`, confidence bands should ultimately be | ||
| derived from observed review outcomes; treat this rubric as a starting | ||
| point pending eval-case calibration. | ||
|
|
||
|
Comment on lines
+1219
to
+1285
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. acknowledged, this needs human review by a codeowner. |
||
| ### 7. Produce the review result | ||
|
|
||
| Compose the review comment using this structure: | ||
|
|
@@ -1281,6 +1359,10 @@ The table below lists the **additional** required fields per action: | |
| | failure | `failure` | `reason` (body optional) | | ||
| | reject | `reject` | `body`, `head_sha`, `findings[]` | | ||
|
|
||
| `confidence` (`high`/`medium`/`low`, from step 6g) is an optional field on | ||
| every action except `failure`. Include it when you have determined a band; | ||
| the schema rejects it on `failure`. | ||
|
|
||
| #### Pipeline mode (`$FULLSEND_OUTPUT_DIR` is set) | ||
|
|
||
| Write the result to `$FULLSEND_OUTPUT_DIR/agent-result.json` following | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MEDIUM] Confidence annotation is posted against a verdict the post-script already overrode
Verified at head 261f903. The new block reads
.confidenceand 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:
jq 'del(.findings) | .action = "comment"'rewritesrequest-changes/rejecttocomment. This runs beforeACTIONis read, so ACTION is already the rewritten value.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 capscomment-onlyat 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 withconfidence.Suggestion: Re-read the action after the downgrade paths, or gate the annotation on a downgrade flag, and either drop
confidencefrom 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:DOWNGRADEDexists only for the protected-path branch, while the severity-filter branch keepsoriginal_actionlocal 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.