fix(review): stop labeling an advisory-only AI consensus defect a "Blocker" - #2673
Merged
Merged
Conversation
…ocker" Investigated #2592 end-to-end: the gate already has a real, config-driven way to make an AI consensus defect actually block merge (aiReviewGateMode: "block", src/rules/advisory.ts) -- off by default. The real bug is narrower: runGittensoryAiReview unconditionally pushes ai_consensus_defect into advisory.findings whenever the two AI reviewers agree on a critical defect, REGARDLESS of aiReviewGateMode, and the unified-comment bridge unconditionally recovered it into the "Blockers" section. So by default (mode unset/advisory), a PR could show a check-run conclusion of "success" (auto-merging) while its own comment listed a "critical defect" under "Blockers" -- confusing at best, actively misleading at worst, since the check-run conclusion (which drives GitHub's real merge decision) is completely independent of this comment-only rendering path. Chose option 3 from the issue (confirm the gate's block/advisory decision is correct as designed -- a blanket default change risks fleet-wide false- positive closes, and a comment-only "held" veto would be worse than today given the check-run/comment split: the comment would claim manual review is needed while the check-run still auto-merges), plus a scoped accuracy fix: only surface the defect under "Blockers" when the gate itself promoted it (present in gate.blockers, i.e. aiReviewGateMode: "block" is actually configured). Otherwise fold it into the non-blocking Nits, clearly labeled "advisory only — not configured to block merge", so the comment never claims a merge is blocked when it will not be. Closes #2592
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2673 +/- ##
=======================================
Coverage 96.10% 96.10%
=======================================
Files 237 237
Lines 26538 26542 +4
Branches 9624 9625 +1
=======================================
+ Hits 25505 25509 +4
Misses 424 424
Partials 609 609
🚀 New features to boost your workflow:
|
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
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
Investigated #2592 end-to-end (supersedes #2002):
aiReviewGateMode(src/rules/advisory.ts) already exists and already provides a real, config-driven way for a maintainer to make an AI consensus defect actually block merge —blockmode promotesai_consensus_defect/ai_review_splitinto the gate's own hard blockers. It's off by default.gateEvaluation.conclusion, posted via the check-run) is completely independent ofderiveUnifiedStatus's comment-only rendering — confirmed by tracing both paths insrc/queue/processors.ts.runGittensoryAiReview(src/queue/processors.ts:5378) unconditionally pushes anai_consensus_defectfinding intoadvisory.findingswhenever the two AI reviewers agree on a critical defect, regardless ofaiReviewGateMode. The unified-comment bridge (consensusDefectFromFindingsinsrc/review/unified-comment-bridge.ts) unconditionally recovered it into the comment's "Blockers" section. Net effect: by default (mode unset/advisory), a PR's check-run reportssuccess(auto-merges) while the SAME PR's own comment lists a "critical defect" under "Blockers" — internally contradictory.Decision
Chose option 3 from the issue (confirm the gate's block/advisory decision is correct as designed) —
aiReviewGateMode: blockalready exists for maintainers who want this to be real; a blanket default change (option 1) risks fleet-wide false-positive closes, and a comment-only veto (option 2, downgradingderiveUnifiedStatusto "held") would be worse than today given the check-run/comment split: the comment would claim manual review is needed while the check-run still silently auto-merges.Plus a scoped accuracy fix, distinct from the three enumerated options: only surface the AI consensus defect under "Blockers" when the gate itself actually promoted it (present in
gate.blockers, i.e.aiReviewGateMode: "block"is configured). Otherwise fold it into the non-blocking Nits section, clearly labeled(advisory only — not configured to block merge), so the comment never claims a merge is blocked when it will not be. The gate's own decision logic is completely untouched.Closes #2592
Test plan
npm run typechecknpx vitest run test/unit/unified-comment-bridge.test.ts— 59/59 pass, including 3 updated tests (now passgateBlockersto keep asserting the real-blocker path) and 3 new tests covering the advisory-only path (routes to Nits, survives as the sole reviewer-side content, coexists with an unrelated real gate blocker)npx vitest run test/unit/queue.test.ts— 368/368 pass unmodified (full processors.ts wiring unaffected)npm run test:ci(full local gate, unsharded)npm audit --audit-level=moderate— 0 vulnerabilities