fix(review): stop restating blockers twice in the PR review comment - #5378
Merged
Conversation
gateVerdictReason() fell back to gate.summary for the top "Suggested
Action" reason line -- but evaluateGateCheckCore's summary for a
"failure" conclusion is literally gate.blockers restated as one joined
string (title + action per finding). The "Why this is blocked" section
a few lines below independently renders those SAME gate.blockers
(folded in via buildDualReviewNotes), so every blocked/reject PR
comment printed the identical blocker text twice.
The existing test suite's gate() mock always hand-set a short generic
summary ("A hard blocker was found."), never the realistic
blockers-restated value evaluateGateCheckCore actually produces, so
this never got caught locally -- it surfaced live on PR #5347.
Only fall back to gate.summary/title when gate.blockers is empty (the
neutral/held case, which has no "Why this is blocked" section to
duplicate against and is the only state that still needs a top-level
reason).
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5378 +/- ##
==========================================
- Coverage 94.41% 94.37% -0.04%
==========================================
Files 551 551
Lines 44168 44169 +1
Branches 14632 14633 +1
==========================================
- Hits 41702 41686 -16
- Misses 1791 1808 +17
Partials 675 675
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This was referenced Jul 12, 2026
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
gateVerdictReason()(src/review/unified-comment-bridge.ts) fell back togate.summaryfor the top-level reason line.evaluateGateCheckCore'ssummaryfor a"failure"conclusion is literallygate.blockersrestated as one joined string (title+—+actionper finding, joined with"; "). The "Why this is blocked" section a few lines below independently renders those samegate.blockers(folded in viabuildDualReviewNotes'sgateBlockersparam) — so a blocked/reject-close comment printed the identical blocker text twice.gate()helper always hand-set a short, genericsummarystring ("A hard blocker was found.") rather than the realistic blockers-restated valueevaluateGateCheckCoreactually produces in production, so this slipped past every existing test.Fix
gateVerdictReasonnow only falls back togate.summary/gate.titlewhengate.blockersis empty. Structurally,evaluateGateCheckCoreonly ever returns a non-emptyblockersarray on a"failure"conclusion (every neutral/success branch explicitly setsblockers: []), so this is scoped precisely to the case that duplicates — the held/neutral case (which has no "Why this is blocked" section to duplicate against) is unaffected and still shows its own top-level reason exactly as before.Test plan
npm run typecheck— cleangate.summaryshape (blockers restated), proving the blocker text now appears exactly oncenpx vitest run test/unit/unified-comment-bridge.test.ts test/unit/unified-comment.test.ts test/unit/unified-comment-parity.test.ts— 170/170 passingnpx vitest run test/unit/queue.test.ts test/unit/queue-2.test.ts test/unit/queue-3.test.ts test/unit/queue-4.test.ts test/unit/queue-5.test.ts(downstream comment-building callers) — 671/671 passingsrc/review/unified-comment-bridge.tsis 100% lines / 99.6% branches (the one uncovered branch is on an untouched, pre-existing line, not part of this diff)npm run test:engine-parity— clean (this file isn't part of any hand-duplicated twin pair, confirmed)