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 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