fix(ai-review): verify reviewers actually agree; strip blocker authority from a bailed review - #9114
Merged
Merged
Conversation
…thority from a bailed review #9074 -- consensusDefectOf returned a defect whenever BOTH reviewers had a non-empty blockers list and NEVER compared the texts, while the doc comment two lines above asserted the property the code did not implement ("Requiring two independent models to AGREE is itself the precision mechanism"). Reviewer A's "SQL injection in src/db.ts" plus reviewer B's "the new helper lacks a doc comment" produced a critical finding published on the contributor's PR as "AI reviewers agree on a likely critical defect: SQL injection in src/db.ts" -- a false claim of agreement, given as the reason their work was auto-closed under one-shot rules. Agreement is now established by comparing blocker texts (normalized significant-token overlap, with a lower bar when both cite the same path and share another term), across ANY pair from the two lists since reviewers order findings differently. Two reviewers flagging DIFFERENT defects is neither consensus nor silence -- it is now a SPLIT, gating on the weaker of the two confidences. Previously `split` required exactly one side to have flagged, so both flagging produced no finding at all: strictly weaker than one doing so. A blank blocker entry no longer counts as "flagged" anywhere, which also removes the case where two empty strings manufactured a critical closing finding under the generic "AI reviewers agree" title. #9087 -- the system prompt tells a model that cannot read the diff to bail with INCOHERENT_DIFF_ASSESSMENT and return empty blockers. A model that bailed, violated that instruction, and happened to write a >=40-char rationale had its blockers promoted into a usable review by #8789's reclassification. Under `combine: "single"` (our live claude-code+ollama config) a lone blocker becomes a full ai_consensus_defect -- severity critical, published as agreement -- and a close under aiReviewGateMode: block. Blockers are now dropped on reclassification; the valueAssessment (#8789's actual purpose), nits, and suggestions are kept. The published title also stops claiming agreement when only one reviewer ran. #9085 (partial) -- adds slop_risk_above_threshold and surface_lane_reject to CONFIGURED_GATE_BLOCKER_SIGNAL_CODES. Both gate real closes but were absent, so their reversals recorded under a different id (or not at all) and the per-rule precision check in downgradeCloseToHold could never apply to them -- the same drift shape as the backtest_regression omission this list's doc comment was written about. NOT done, and the issue's premise is wrong: #9085 claims severity "never determines consequence" and is read only by presentational code. It is not -- conclusionForSeverity maps critical -> action_required, so raising these findings' severity to match their consequence flipped advisory-mode repos from neutral to action_required and broke four gate tests. Reverted; the severity mismatch needs that coupling decoupled first, which is its own change. Closes #9074 Closes #9087 Tests: 11 consensus/split cases (unrelated defects are not consensus, same defect in different words is, any-pair matching, weaker-confidence rule, disagreement becomes a split with min confidence, one-sided split unchanged, clean pair yields nothing, unverifiable text never agrees, shared-path booster, single shared token is not agreement), a reclassification test proving blockers are dropped while the valueAssessment survives, and 3 taxonomy tests. 100% line+branch coverage on all added lines; 985/985 across 9 suites.
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Contributor
Logic backtestReplayed 0 historical case(s) for Backtest comparison:
|
❌ 16 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 1 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
This was referenced Jul 27, 2026
JSONbored
added a commit
that referenced
this pull request
Jul 28, 2026
…d attribute votes to the producing model (#9477, #9478) (#9513) * fix(ai): make prompt drift a cache miss, bound the Ollama context, and attribute votes to the producing model (#9477, #9478) getCachedAiReview reuses a cacheable row with NO maxAgeMs and replays its findings verbatim, including a severity:critical ai_consensus_defect. AI_REVIEW_CACHE_INPUT_VERSION was hand-bumped and last moved for #8364, but #8789, #8791, #8833, #8845, #8961, #9035, #9074, #9087, #9114, #9145 and #9445 all changed prompt text or verdict logic without one -- so a PR closed or held under the pre-#9074 false-consensus rule re-gated at the same head to the SAME stale finding and the corrected logic never ran. The fingerprint now folds in REVIEW_PROMPT_VERSION and a digest of the canonical judge prompt, both pure and available before the call, so drift is an automatic miss rather than something a human must remember. Bumped to v7 to kill rows written under the incomplete fingerprint. Ollama silently left-truncates past num_ctx (typically 4k-8k by default) and then answers confidently over whatever survived -- the TAIL of the prompt, i.e. the context sections rather than the diff. The review path sends up to 120k chars of diff plus a 240k-char context budget, so on the fallback provider a review could come from a fraction of the change while carrying the same blocker authority and confidence semantics as the primary. An explicit num_ctx is now sent for the ollama provider only, since other OpenAI-compatible servers may reject an unknown options key, and an explicit caller providerOptions still wins. runWorkersOpinion iterates [primary, fallback] internally and its outcome carried no model identity, so a fallback-produced review was recorded as a PRIMARY vote -- poisoning the reviewer_vote audit events, #8229's routing track records, and scoreJudgmentAgreement's contribution to decision-record confidence. The doc's claim that slot-to-model is unambiguous by construction holds for the tie-break swap, not for in-slot fallback. * chore(selfhost): regenerate the env reference for the new env reads
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.
Summary
Two ways the gate publicly claimed something it never checked — and closed contributor PRs on that claim.
#9074 — "AI reviewers agree" was never verified.
consensusDefectOfreturned a defect whenever both reviewers had a non-empty blockers list, and never compared the texts. The doc comment two lines above asserted the property the code didn't implement. So reviewer A's "SQL injection insrc/db.ts" plus reviewer B's "the new helper lacks a doc comment" produced a critical finding, published on the contributor's PR as:…as the stated reason their work was auto-closed under one-shot rules.
Agreement is now established by actually comparing blocker texts (normalized significant-token overlap, with a lower bar when both cite the same path and share another term), matched across any pair from the two lists since reviewers order findings differently.
Two reviewers flagging different defects is neither consensus nor silence — it's now a split, gating on the weaker confidence. Worth calling out:
splitpreviously required exactly one side to have flagged, so both flagging produced no finding at all — strictly weaker than one reviewer flagging. A blank blocker entry also no longer counts as "flagged" anywhere, which removes the case where two empty strings manufactured a critical closing finding under the generic "AI reviewers agree on a likely blocking defect" title.#9087 — a model that said it couldn't read the diff kept blocker authority over it. The system prompt instructs a model that can't read the diff to bail and return empty blockers. A model that bailed, violated that instruction, and happened to write a ≥40-char rationale had its blockers promoted into a usable review by #8789's reclassification. Under
combine: "single"(our liveclaude-code,ollamaconfig) a lone blocker becomes a fullai_consensus_defect— critical, published as agreement — and a close underaiReviewGateMode: block. Blockers are now dropped on reclassification; thevalueAssessment(#8789's actual purpose), nits, and suggestions are kept. The published title also stops claiming agreement when only one reviewer ran.Closes #9074
Closes #9087
#9085 — partially done, and its premise turned out to be wrong
Done: added
slop_risk_above_thresholdandsurface_lane_rejecttoCONFIGURED_GATE_BLOCKER_SIGNAL_CODES. Both gate real closes but were absent, so their reversals recorded under a different id (or not at all) and the per-rule precision check indowngradeCloseToHoldcould never apply to them — the same drift shape as thebacktest_regressionomission that list's own doc comment was written about.Not done, deliberately. The issue states severity "never determines consequence anywhere on the decision path" and is read only by two presentational functions. That's incorrect:
conclusionForSeveritymapscritical → action_required. I implemented the severity fix, and it flipped advisory-mode repos fromneutraltoaction_required, breaking four gate tests — i.e. raising these findings to match their consequence would have changed behavior for repos that deliberately run them advisory-only. Reverted.The mismatch is real and still worth fixing, but it needs that coupling decoupled first (severity should describe the finding; consequence should come from the gate mode). That's its own change, so I've left #9085 open rather than half-fixing it.
Test plan
valueAssessment/nits/suggestions survivetsc --noEmit, migrations/schema-drift/docs/engine-parity gates green; 985/985 across 9 suites