test(eval): measure review findings, not just review outcomes - #1003
Open
guyoron1 wants to merge 9 commits into
Open
test(eval): measure review findings, not just review outcomes#1003guyoron1 wants to merge 9 commits into
guyoron1 wants to merge 9 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 |
Author
`gh pr view --json comments` returns only the issue-comment timeline —
the review agent's sticky write-up — never the positioned review
comments that carry the individual findings. Judges could therefore see
that a review happened, but not what it found.
capture-fixture.sh's pull_request branch now also fetches
GET /repos/{repo}/pulls/{n}/comments into review_comments as
{path, line, body}. --paginate is load-bearing: the REST default is 30
per page, so a review posting 40 findings would be silently truncated,
hiding exactly the over-flagging that a precision judge exists to
measure. --jq runs per page under --paginate, so `jq -s 'add // []'`
concatenates the per-page arrays back into one.
A fetch failure records review_comments: null plus
review_comments_fetch_failed: true rather than an empty array. An empty
array reads as "the agent posted no findings" when the truth is "we
could not look", which would silently pass a forbidden-findings check.
Signed-off-by: guy oron <goron@redhat.com>
The review suite's existing judges measure the outcome: which labels
landed, what the decision was, whether the budget held. None of them
can tell a review that found the three seeded bugs from one that
approved blindly, or a review that considered and dismissed FP bait
from one that promoted it to a vulnerability. Measuring review
precision means inspecting the findings themselves.
Two deterministic judges read the review_comments capture:
- required_findings: every {file, category, min_severity} entry in
annotations must be satisfied by at least one posted finding.
- forbidden_findings: no posted finding may match an FP-bait entry at
or above its floor. That floor defaults to high, so a low/info
"considered and dismissed" note stays legal — only promoting the
bait to a real vulnerability is a false positive.
Findings are recognised by the "**[severity]** category" header that
formatFindingComment renders. The regex searches rather than anchors,
because postreview.go's file-level fallback prefixes the body with
"_Line N_ · " when GitHub rejects a positioned comment.
`category` is matched against the whole finding body, not the category
token alone: the agent's categories are fine-grained kebab-case
(injection-vuln, logic-error), the output schema constrains them only
to a non-empty string, and SKILL.md routes unrecognised categories to
the nearest dimension. Pinning the token would fail a correct review
on a synonym.
Matching is word-boundary aware. Case-insensitive substring matching
is the bug class review already flagged on eval/code's removed_symbols
judge, where VerboseLoggingEnabled satisfied a VerboseLogging
requirement. Entries are validated up front — a non-list value, a null
file or category, an out-of-vocabulary min_severity, or a typo'd key
fails the case with a message instead of degrading to a weaker check.
Unknown severities fail closed in both directions: never satisfying a
requirement, always tripping a prohibition.
Signed-off-by: guy oron <goron@redhat.com>
The judges are Python embedded in YAML with no import site of their own, so the test extracts the shipped check bodies straight from eval.yaml — the same approach as removed-symbols-judge-test.py, and for the same reason: CI installs neither pyyaml nor ruamel, and a copied body would drift from the code it claims to cover. 54 cases cover exact matches, the word-boundary near-misses a substring matcher would wrongly accept (injection/injections, hash/hashicorp, discount/discounted), severity floors at and one notch below the boundary, unknown severities in both fail-closed directions, case-sensitive file paths, the "_Line N_ ·" fallback body, malformed and missing annotations, and every capture failure mode. Verified by breaking the shipped judge twice: replacing the boundary match with a substring match fails 4 cases, and dropping the min_severity validation fails 1 with a KeyError. Both restored green. Signed-off-by: guy oron <goron@redhat.com>
Four PR fixtures over one shared webapp fixture repo, symlinked in as
each case's repo/ the way eval/triage's cases share python-webapp.
Numbered from 004 so they do not collide with
003-protected-path-downgrade.
- 004-seeded-bugs: three real, independent bugs (SQL injection via an
f-string, hmac.compare_digest downgraded to ==, apply_discount
missing its / 100) whose PR body actively misdirects on all three.
- 005-fp-bait: a correct PR carrying MD5-as-cache-key and a
static-argv subprocess.run. Neither may be raised at high/critical.
- 006-docs-only: a docs clarification that is true of the base repo,
so nothing above info is a genuine finding.
- 007-dependency-bump: a one-line patch bump with no high/critical
surface.
The base repo holds the safe version of each file, so the seeded bugs
appear in the diff as removals of the protection rather than as
pre-existing code the reviewer has to go hunting for.
required_findings match on the substantive claim ("injection",
"timing") rather than on a category token, since the category
vocabulary is a convention the agent may deviate from. The clean cases
use an empty category to forbid any finding on the file at or above
the floor, which asserts "invent nothing here" without having to
enumerate what might be invented.
Signed-off-by: guy oron <goron@redhat.com>
pass_rate is sum(values)/len(values) over every case that produced a value, and a case declaring no findings ground truth returns True. With one recall case out of five, required_findings at 0.70 could not fail even if that case missed every seeded bug — the recall gate was decorative. Both findings judges now carry an `if:`, so the harness skips them for cases with no ground truth rather than counting a trivial pass, and the rates are over the cases that actually assert something. The threshold comment claimed this was already true; it now describes what the numbers really do, including that 0.9 across three precision cases means all three must pass rather than allowing one failure. Signed-off-by: guy oron <goron@redhat.com>
The existing cases share their ground truth with the review guidance: each seeded bug and each bait construct appears verbatim as an example in skills/pr-review. A suite built that way measures instruction-following, and it cannot detect a prompt change that over-suppresses, because the rule and the case were written together. 008 is the deliberate mirror of 005. Both PRs hash a secret-prefixed payload with md5; in 005 the digest is a cache key and flagging it is a false positive, here it is compared against an inbound X-Signature header and missing it ships an authentication bypass. The surface pattern is identical, so neither case can be passed by recognising the construct — only by following the value to its use. Both prose channels lie: the docstring calls it 'not a security boundary' and the PR body repeats it, while the function that uses it as a signature sits three lines below in the same diff. That makes this the case that fails if a reviewer ever treats an author's description of their own code as evidence about it. Signed-off-by: guy oron <goron@redhat.com>
Two ways the precision judge could report a clean pass without having seen anything. forbidden_findings returned success whenever no inline comment parsed as a finding. postreview.go folds findings into the review body when GitHub 422s the positioned comments, and drops any finding with no file or line — and the review guidance tells the agent to omit the line rather than guess one. In those cases precision is unverifiable, which is not the same as verified clean: a promoted bait would pass. It now fails when comments exist but none parse, and says why. fetch_review_comments discarded jq's exit status, so a jq failure returned success with empty output; the caller then passed "" to --argjson, which aborts jq under set -e before fixture-state.json is written at all. That loses every judge for the case, including the label and budget ones, rather than just this field. The existing pr_fetch_failed path exists precisely to avoid that. Signed-off-by: guy oron <goron@redhat.com>
When GitHub rejects the positioned comments, post-review retries without them and embeds each finding as a bullet in the review body (buildFallbackReviewBody). The findings are real; only the delivery changed. Reading inline comments alone made the recall judge report a miss the agent never made, and left the precision judge unable to see a promoted bait at all — a silent pass on exactly the case the suite exists to catch. Both judges now also parse the fallback bullets, anchored on the fixed note post-review writes, so an ordinary review body is never mined for things that look like findings. The unverifiable guard accounts for it too: a fallback note whose bullets do not parse fails rather than reporting a clean review. This is what agents#209 was waiting on. The 422 bug itself (agents#193) is still open, but the eval no longer has to wait for it: a review delivered through the fallback is now judged on its findings rather than on how GitHub happened to accept them. Signed-off-by: guy oron <goron@redhat.com>
guyoron1
force-pushed
the
eval/review-precision-cases
branch
from
August 26, 2026 08:31
ef4e0ac to
eb45eae
Compare
guyoron1
marked this pull request as ready for review
August 26, 2026 14:34
PR Summary by Qodotest(eval): score review findings for recall and precision
AI Description
Diagram
High-Level Assessment
Files changed (26)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Three review findings from qodo on fullsend-ai#1003: The forbidden_findings judge treated zero inline comments as a verified clean review, but postreview.go omits findings with no file or line from the inline comments entirely — they remain in the sticky comment body. A review whose findings all lacked a position posted nothing inline, left no 422 marker, and passed as clean. The judge now scans the sticky/issue comments and review bodies for finding headers before trusting an empty review_comments, and fails closed when findings are visible somewhere it cannot grade them. Both findings judges gated on truthiness (if: annotations.get(...)), so a malformed-but-falsy ground truth ({}, "", 0) was silently skipped — excluded from the pass-rate denominator — instead of reaching the check body's fail-loud validation. The gate now skips only for absent or [], and the check bodies type-check before the trivial-pass so the falsy malformed values fail with a message. Case 007 called requests 2.31.0 -> 2.32.3 a patch-level bump; that is a minor bump. The fixture now bumps 2.32.2 -> 2.32.3, which is genuinely patch-only with no breaking changes, so the case premise and the forbidden_findings floor it justifies are accurate. Judge tests cover all three: empty-mapping/empty-string annotations fail loudly, sticky-only findings fail closed, a chatty no-findings sticky still passes, and sticky duplicates of positioned findings do not trip the guard. Signed-off-by: guy oron <goron@redhat.com>
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.
The review eval judges outcome labels only, so a review can apply the right label while reporting the wrong things — or nothing at all. These cases judge the findings themselves.
What changes
capture-fixture.shnow snapshots inline PR review comments. That is wherepostreview.goputs structured findings, andgh pr view --jsondoes not expose them. Two deterministic judges read them:required_findings— a seeded bug must be reported, at or above a stated severity.forbidden_findings— a safe construct must not be promoted to a real vulnerability.Five cases: seeded bugs, false-positive bait, docs-only, a dependency bump, and
008-reassuring-docstring.Why 008 exists
It is the deliberate mirror of the bait case. Both PRs hash a secret-prefixed payload with md5 — in
005the digest is a receipt cache key, in008it is compared against an inboundX-Signatureheader. The surface pattern is identical, so neither case can be passed by recognising the construct, only by following the value to its use.In
008both prose channels lie: the docstring says "not a security boundary" and the PR body repeats it, while the function using the digest as a signature sits three lines below in the same diff.Without a case like this, a suite whose ground truth overlaps the review guidance measures instruction-following, and a prompt change that over-suppresses reads as a precision win.
Two ways the judges could have lied
They could not fail.
pass_rateissum(values)/len(values)over every case that produced a value, and a case declaring no ground truth returnsTrue. With one recall case out of five,required_findingsat0.70could not have failed even if that case missed every seeded bug. Both findings judges now carry anif:so the harness skips them where there is no ground truth, and the rates are over the cases that actually assert something.They could pass without seeing anything.
forbidden_findingsreturned success whenever no inline comment parsed as a finding. A 422 folds findings into the review body, and findings with no file or line are dropped before the inline comments — so precision was unverifiable, which is not the same as verified clean. It now fails closed and says why. Separately,fetch_review_commentsdiscarded jq's exit status, so a jq failure returned success with empty output and then aborted the script beforefixture-state.jsonwas written at all, losing every judge for that case.The 422 path
agents#209was parked pending the 422 fix, because a review whose positioned comments GitHub rejects has its findings folded into the review body instead. Reading inline comments alone would report a miss the agent never made, and would hide a promoted bait from the precision judge entirely — a silent pass on the case the suite exists to catch.Both judges now also parse those fallback bullets, anchored on the fixed note
post-reviewwrites, so an ordinary review body is never mined for things that merely look like findings. The eval no longer waits onagents#193: a review delivered through the fallback is judged on its findings rather than on how GitHub happened to accept them.Testing
eval/scripts/review-findings-judge-test.pyextracts the judge bodies from the shippedeval.yaml— no second copy to drift — and runs 56 synthetic payloads through them. Wired intomake script-test. Negative-checked: disabling the fail-closed guard fails the suite; replacing the word-boundary match with a substring match fails four boundary cases../eval/lint-cases.shpasses for review, triage, code and fix;pre-commit run --all-filesis clean.Notes for review
SKILL.md's vocabulary is per-dimension kebab-case and unrecognised categories route to the nearest dimension — pinning the token fails a correct review on a synonym.008is the only anti-bait case; letting it fail while the suite stays green would make it decorative, which is the defect theif:gates were added to fix.Why this lands first
The review agent is growing a second implementation: the pi runtime runs
review on Grok 4.6 without sub-agents (first live run:
fullsend-ai/pi-xai-vertex#4). These judges score the posted output —
inline review comments and the review body — not the architecture that
produced it, so the same cases can score both implementations
head-to-head. That comparison needs the eval to exist before either
implementation changes.