Repository navigation
feat(code-review): verify every candidate finding before posting - #714
TheGreatAxios merged 1 commit into
Conversation
594b1a9 to
02a3988
Compare
TheGreatAxios
left a comment
There was a problem hiding this comment.
Independent review at 02a3988.
Findings
- The issue's eval outcome is not met, but the PR closes the issue. CL-11634's outcome and AGENTS.md ("a change to how a workflow judges or reviews states its package's eval result before and after") need precision before and after. The body says "pending the owner's eval run". Fill it in before the PR is marked ready. The plan's chain scenario is deferred too; the body says so.
- Any verdict other than
reallands indropped, but the contract accepts only'not_real'.writeInputfiltersj.verdict !== "real", and the structured director checks only that the reply parses as JSON, not that it matches the schema. A reply such as{"verdict":"unsure"}or one withoutreasonwould failCodeReviewOutputatreport(I checked:dropped[0].verdict must be "not_real" (was "unsure")). The whole review would then show as not finished, with no retry, becausewriteInputis an action. This is an unverified hypothesis on reachability: Anthropic's json-schema response format should enforce the enum. Suggested fix: validate the verdict with arktype where the check's reply is read, so a bad answer fails a step that retries. - Nit:
WriteInput.verdictsis now redundant. Nothing reads it, anddroppedcarries the verdicts. Remove it. - Nit: docs/ARCHITECTURE.md formatting and wording. The added line "every consolidated group gets one more model call…" is 110 columns, while the paragraph wraps at about 80. "The check step is always on" describes the code against its past state; "The check step reads every consolidated group…" says what exists now.
Verified
- Order and effort:
checksorders the groups withorderedGroups(severity, then agreement).checkis a stockmapover them, and stock runs map items one at a time in order. Effort comes frominference.check(request, else profile; medium for Anthropic). - Not posted on either host: the manager sends only
review.commentstosendCodeReview(GitHub and Bitbucket DC). The hub'scode-review-runs.tsmaps the comment fields one by one.droppedreaches neither the posted comment nor the portal. - A check that errors or times out: I made the stub refuse every check call. The check step was tried 3 times, the run ended
failedwith nowriteInput,writeorreviewoutput, and no write call was made. The reviewer then reports "the review did not finish". The group is neither posted nor dropped without notice. One failed check fails the whole review, which matches the README's "a step that fails for good fails the review". - No
checkoption is left in the contracts, hub, eval, sweep or docs. Sweep replay: a run saved without--checkhascheck: [], which takes theorderedbranch, the same as before. The 2026-10-21e2e.jsonis not on disk, so I checked this by reasoning only. - The not_real test fails with the filter removed (
findings = judged.map((j) => j.finding)gives 8 pass, 1 fail). - Model calls per review: plan, diff finder (one turn), consolidate and write are one call each, plus the repository finder's turns. The change adds one check call per group. The body's count is right.
bun run lint(warnings only),bun run check, tests for pr-code-review, pr-code-review-anthropic, triage-contracts, triage-manager and the hub pull-request reader (370 pass), andmanager-intake.postgres.test.ts(16 pass). The commit is signed (G) and follows Conventional Commits, with no attribution.
The check step now runs on every review, one call per consolidated group, and the request's check option is gone. A group the check finds not real is left out of the review and kept in the reported result's dropped with the verdict and reason. The eval prints each stage's precision. Closes CL-11634
02a3988 to
e6d91cb
Compare
|
Addressed at e6d91cb:
|
TheGreatAxios
left a comment
There was a problem hiding this comment.
Finding 1 pending the owner's eval run; no other findings.
TheGreatAxios
left a comment
There was a problem hiding this comment.
Re-review at e6d91cb: the one open finding (eval outcome) is resolved by the owner retiring the 52-case suite; the measurement moves to CL-11683. No findings.
There was a problem hiding this comment.
Makes the per-finding verification check run on every code review instead of only when a check request field is set, removes that field, and moves rejected findings into a dropped list in the reported result. Only findings the check confirms are posted. The main risks are that unread files are judged without their code, and that a missing check verdict silently hides a finding.
| function check({ finding, excerpt }: (typeof ordered)[number]) { | ||
| return { text: checkMessage(request, finding, excerpt), inference: inference.check }; | ||
| } |
There was a problem hiding this comment.
P1 · Unverified groups are checked with an empty file excerpt and can be dropped as not real
Every group now goes through the check. A group whose file the consolidate step could not read has an empty excerpt, because consolidate-director builds it from linesOf(site.path) and marks the finding in unreadFindings. checkMessage still sends an empty <file> block, so the check model judges the defect from the quoted line alone. Before this change such a finding was posted with the note 'Not checked against the file'. Now it can be dropped with a not_real verdict that was reached without its code.
Trigger: a candidate whose file was missing from the consolidate step's reads, so it appears in unreadFindings and has excerpt === "". The check answers not_real, the finding goes to dropped, and it never reaches the posted review.
Fix: when the excerpt is empty, skip the check and keep the finding with its existing 'Not checked against the file' marking, or send the check an explicit note that no file text is available and do not let a not_real verdict drop it. Add a test with an unread file.
| const judged = checked.map((finding, i) => ({ finding, ...verdicts[i]! })); | ||
| const findings = judged.flatMap((j) => (j.verdict === "real" ? [j.finding] : [])); | ||
| const dropped = judged.filter((j) => j.verdict !== "real"); |
There was a problem hiding this comment.
P2 · Missing check verdict yields a dropped entry with undefined verdict and reason
verdicts[i]! only silences the compiler. If there are fewer check outputs than consolidated findings, spreading undefined adds no verdict field. The finding then fails the === "real" test and lands in dropped with verdict and reason undefined. That breaks the CodeReviewDropped contract (verdict: 'not_real', reason: string), and a finding nobody checked is hidden as though the check had rejected it.
Trigger: a check output list shorter than checks.findings, for example a partially recorded or replayed run, or a workflow where the check step returns fewer outputs than groups.
Fix: look up the verdict explicitly and treat a missing one as an error or as unchecked, not as dropped. For example, throw when verdicts[i] is undefined, or keep the finding and mark it unchecked. Add a test where the check output list is shorter than the group list.
Every code review now checks each candidate finding before it is posted. The check step used to run only when the request set
check, which the hub never did. It now runs on every review, and thecheckrequest field is removed. For each consolidated finding, the check reads the finding, the code it quotes and the file lines around it, and answers real or not real with a reason. Only confirmed findings are written up and posted. The dropped ones are kept in the review's reported result underdropped, in the finder's words with the verdict and reason. The check prompt is unchanged, and its effort comes from the provider profile like the other steps (medium for Anthropic).ghandANTHROPIC_API_KEY):bun run eval:review -- --e2e --baseline. The eval checks every finding, as every review now does. It prints precision for every stage, so theconsolidatedrow (what was posted before this change) and thefinalrow (what is posted now) give precision and recall without and with the check from one run.--baselinealso rewrites the committed baseline file. The--checkflag is gone. The posting-threshold sweep still replays runs saved before this change, which have no check outputs.droppedwith its verdict and reason. A code-review chain scenario is tracked separately: no isolated sidecar place exists in the chain stack, so code review never runs there.Verified with
bun run lint,bun run check, the pr-code-review, pr-code-review-anthropic, triage-contracts and triage-manager tests, the hub's pull-request reader test andmanager-intake.postgres.test.ts.Closes CL-11634