From 94f3b69fb7d1d5296dc5eff877e1739910c82dcb Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 08:34:16 -0700 Subject: [PATCH] fix(review): stop restating blockers twice in the PR review comment 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). --- src/review/unified-comment-bridge.ts | 8 ++++ test/unit/unified-comment-bridge.test.ts | 50 +++++++++++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 57e27be579..b7ff238942 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -164,6 +164,14 @@ function gateVerdictReason(gate: GateCheckEvaluation): string | undefined { .map(holdWarningVerdictReason) .filter(Boolean); if (holdReasons.length > 0) return holdReasons.join("; "); + // evaluateGateCheckCore's `summary` for a "failure" conclusion is LITERALLY `gate.blockers` restated as one + // joined string (title + action per finding) -- and buildDualReviewNotes folds those SAME `gate.blockers` + // into the reviewer notes that render as the "Why this is blocked" section a few lines below. Falling back + // to `gate.summary`/`gate.title` here would print the identical blocker text TWICE in one comment (the + // real-world bug behind gittensory PR #5347's screenshot). Only reachable when gate.blockers is non-empty, + // since evaluateGateCheckCore only sets `blockers: []` on a neutral/success conclusion -- so this never + // affects the held/neutral case above, which has no "Why this is blocked" section to duplicate against. + if (gate.blockers.length > 0) return undefined; return gate.summary?.trim() || gate.title?.trim() || undefined; } diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index d6279f4ef1..0e697eadb8 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -814,6 +814,49 @@ describe("gate blockers render in 'Why this is blocked' (FIX D1)", () => { expect(body).not.toMatch(/trust score/i); expect(body).toContain("[context]"); }); + + // gittensory PR #5347: the real-world `summary` evaluateGateCheckCore produces for a "failure" conclusion is + // LITERALLY `blockers.map(f => title + action).join("; ")` -- not the short hand-authored string the tests + // above use. The earlier tests in this describe block never exercise that realistic value, so they never + // caught this. Reproduce it exactly here. + it("does NOT print the blocker text twice when gate.summary is the REALISTIC blockers-restated string (#5347)", () => { + const blockerTitle = "schema-version.js:42 applies multiple migrations before stamping once"; + const blockerAction = "wrap the migration loop and the stamp in one transaction"; + const body = buildUnifiedCommentBody({ + gate: gate({ + conclusion: "failure", + title: `Gittensory Orb Review Agent: ${blockerTitle}`, + // The exact shape evaluateGateCheckCore's `summary` field produces: blockers restated, title + action. + summary: `${blockerTitle} — ${blockerAction}.`, + blockers: [{ code: "ai_consensus_defect", severity: "critical", title: blockerTitle, detail: blockerAction, action: blockerAction }], + }), + advisoryFindings: [{ code: "ai_consensus_defect", severity: "critical", title: blockerTitle, detail: blockerAction }], + panelRows, + readinessTotal: 100, + changedFiles: 10, + footerMarkdown: footer, + }); + // Exactly once: under "Why this is blocked" only, never ALSO restated under "Suggested Action". + expect(body.split(blockerTitle).length - 1).toBe(1); + expect(body).toContain("Why this is blocked"); + }); + + it("a manual-review HOLD (no gate blockers) still shows its own top-level reason, unaffected by the #5347 fix", () => { + const body = buildUnifiedCommentBody({ + gate: gate({ + conclusion: "neutral", + title: "Gittensory Orb Review Agent — held for manual review", + summary: "A repo-configured guardrail path was touched.", + blockers: [], + warnings: [{ code: "guardrail_hold", severity: "warning", title: "Guardrail path touched", detail: "wrangler.jsonc" }], + }), + panelRows, + readinessTotal: 50, + changedFiles: 1, + footerMarkdown: footer, + }); + expect(body).toContain("Guardrail path touched: wrangler.jsonc"); + }); }); describe("buildUnifiedCommentBody: visual findings render in their OWN section, never duplicated as a generic Nit (#4111)", () => { @@ -1037,10 +1080,13 @@ describe("privacy invariant: the private 'Maintainer notes' internals never reac footerMarkdown: footer, }); expect(body).not.toContain("Maintainer notes"); - // Sanity: the new depth IS present (so this isn't passing on an empty body). + // Sanity: the new depth IS present (so this isn't passing on an empty body). Since gate.blockers is + // non-empty here, gateVerdictReason omits the redundant gate.summary fallback (#5347) — the real blocker + // titles below are the actual populated content, not the generic "A hard blocker was found." placeholder. expect(body).toContain("Why this is blocked"); expect(body).toContain("CI checks failing"); - expect(body).toContain("A hard blocker was found."); + expect(body).toContain("Real bug"); + expect(body).toContain("No linked issue"); }); });