From a75f5b0185d08e6397353026680d02b1ca6586a3 Mon Sep 17 00:00:00 2001
From: JSONbored <49853598+JSONbored@users.noreply.github.com>
Date: Thu, 2 Jul 2026 20:59:37 -0700
Subject: [PATCH] fix(review): stop labeling an advisory-only AI consensus
 defect a "Blocker"
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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
---
 src/review/unified-comment-bridge.ts     | 22 ++++++++--
 test/unit/unified-comment-bridge.test.ts | 52 ++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts
index bf22ec9396..80a3c59474 100644
--- a/src/review/unified-comment-bridge.ts
+++ b/src/review/unified-comment-bridge.ts
@@ -169,9 +169,24 @@ export function buildDualReviewNotes(args: {
   const { main: assessment, nits: aiNitLines } = splitAiReviewNits(
     args.aiReview?.notes?.trim() ?? "",
   );
-  const consensusBlocker = args.consensusDefect
+  // The consensus defect is a REAL blocker only when the gate itself promoted it (aiReviewGateMode: "block" —
+  // see src/rules/advisory.ts). When aiReviewGateMode is off/advisory (the default), `ai_consensus_defect` is
+  // still unconditionally added to advisory.findings (so it's always recoverable here), but the gate
+  // conclusion stays "success" and the PR still merges — labeling it a "Blocker" then is actively misleading
+  // (a green, auto-merging check-run next to a "1 blocker" chip). Fold it into the non-blocking Nits instead,
+  // clearly framed as advisory-only, so the comment never claims a merge is blocked when it will not be.
+  // (#2592 — the gate's own block/advisory decision is unchanged; this only fixes how the comment labels it.)
+  const consensusIsGateBlocking = (args.gateBlockers ?? []).some(
+    (finding) => finding.code === "ai_consensus_defect",
+  );
+  const consensusBlocker = args.consensusDefect && consensusIsGateBlocking
     ? [formatConsensusDefectBlocker(args.consensusDefect)]
     : [];
+  const consensusAdvisoryNits = args.consensusDefect && !consensusIsGateBlocking
+    ? [publicSafeNit(`${formatConsensusDefectBlocker(args.consensusDefect)} (advisory only — not configured to block merge)`)].filter(
+        (line): line is string => line !== null,
+      )
+    : [];
   // FIX D1: fold the gate's own hard blockers into the reviewer blockers (so a non-AI gate failure populates
   // "Why this is blocked"). Exclude `ai_consensus_defect` (already surfaced via consensusDefect → appears once)
   // and scrub each through the same public-safe boundary as Nits, DROPPING any that still leaks a private term.
@@ -198,8 +213,9 @@ export function buildDualReviewNotes(args: {
   const aiNits = aiNitLines
     .map((line) => publicSafeNit(line))
     .filter((line): line is string => line !== null);
-  const nits = [...aiNits, ...gateNits];
-  if (!assessment && blockers.length === 0) return [];
+  // The advisory-only consensus defect leads the list (it's the most severe item even though non-blocking).
+  const nits = [...consensusAdvisoryNits, ...aiNits, ...gateNits];
+  if (!assessment && blockers.length === 0 && nits.length === 0) return [];
   const notes: ReviewNotes = {
     assessment,
     suggestions: [],
diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts
index 7e333cf585..f35c31ad92 100644
--- a/test/unit/unified-comment-bridge.test.ts
+++ b/test/unit/unified-comment-bridge.test.ts
@@ -97,6 +97,8 @@ describe("buildDualReviewNotes", () => {
     const reviews = buildDualReviewNotes({
       aiReview: { notes: "The refactor looks correct." },
       consensusDefect: { title: "Off-by-one", detail: "Loop bound is wrong." },
+      // Present in gateBlockers ⇒ aiReviewGateMode: "block" actually promoted it — a REAL blocker (#2592).
+      gateBlockers: [{ code: "ai_consensus_defect", severity: "critical", title: "Off-by-one", detail: "Loop bound is wrong." }],
       warnings: [{ code: "w1", severity: "warning", title: "Missing test", detail: "...", action: "Add a test." }],
       recommendation: "close",
       verdict: "close",
@@ -114,6 +116,7 @@ describe("buildDualReviewNotes", () => {
   it("omits the ': detail' and ' — action' suffixes when the defect has no detail and the warning has no action", () => {
     const reviews = buildDualReviewNotes({
       consensusDefect: { title: "Null deref", detail: "" },
+      gateBlockers: [{ code: "ai_consensus_defect", severity: "critical", title: "Null deref", detail: "" }],
       warnings: [{ code: "w1", severity: "warning", title: "No test", detail: "..." }], // no `action`
       recommendation: "close",
       verdict: "close",
@@ -128,12 +131,61 @@ describe("buildDualReviewNotes", () => {
         title: "AI reviewers agree on a likely critical defect: src/types.ts:111 leaves `Finding` unclosed",
         detail: "src/types.ts:111 leaves `Finding` unclosed",
       },
+      gateBlockers: [
+        {
+          code: "ai_consensus_defect",
+          severity: "critical",
+          title: "AI reviewers agree on a likely critical defect: src/types.ts:111 leaves `Finding` unclosed",
+          detail: "src/types.ts:111 leaves `Finding` unclosed",
+        },
+      ],
       recommendation: "close",
       verdict: "close",
     });
     expect(reviews[0]?.notes?.blockers).toEqual(["src/types.ts:111 leaves `Finding` unclosed"]);
   });
 
+  // #2592: aiReviewGateMode defaults to advisory, so a consensus defect DOES NOT reach gate.blockers by
+  // default even though it is unconditionally added to advisory.findings (see queue/processors.ts). The
+  // comment must not then label it a "Blocker" — that claims a merge is blocked when it will not be.
+  describe("consensus defect NOT promoted by the gate (aiReviewGateMode off/advisory — #2592)", () => {
+    it("routes the defect into Nits, clearly labeled advisory-only, instead of Blockers", () => {
+      const reviews = buildDualReviewNotes({
+        aiReview: { notes: "Looks mostly fine." },
+        consensusDefect: { title: "Off-by-one", detail: "Loop bound is wrong." },
+        // No gateBlockers containing ai_consensus_defect ⇒ the gate did NOT promote it (advisory mode).
+        recommendation: "merge",
+        verdict: "merge",
+      });
+      expect(reviews[0]?.notes?.blockers).toEqual([]);
+      expect(reviews[0]?.notes?.nits).toEqual(["Off-by-one: Loop bound is wrong. (advisory only — not configured to block merge)"]);
+    });
+
+    it("still surfaces the note when the defect is the ONLY reviewer-side content (no assessment, no blockers)", () => {
+      // Regression guard: before #2592 the early-return only checked `blockers.length === 0`, so an
+      // advisory-only defect with no aiReview.notes and no gate blockers would silently vanish entirely.
+      const reviews = buildDualReviewNotes({
+        consensusDefect: { title: "Null deref", detail: "src/foo.ts:12" },
+        recommendation: "merge",
+        verdict: "merge",
+      });
+      expect(reviews).toHaveLength(1);
+      expect(reviews[0]?.notes?.blockers).toEqual([]);
+      expect(reviews[0]?.notes?.nits).toEqual(["Null deref: src/foo.ts:12 (advisory only — not configured to block merge)"]);
+    });
+
+    it("a gateBlockers list present but NOT containing ai_consensus_defect still treats the defect as advisory-only", () => {
+      const reviews = buildDualReviewNotes({
+        consensusDefect: { title: "Off-by-one", detail: "Loop bound is wrong." },
+        gateBlockers: [{ code: "missing_linked_issue", severity: "critical", title: "No linked issue", detail: "..." }],
+        recommendation: "merge",
+        verdict: "merge",
+      });
+      expect(reviews[0]?.notes?.blockers).toEqual(["No linked issue"]); // the real (non-AI) gate blocker still blocks
+      expect(reviews[0]?.notes?.nits).toEqual(["Off-by-one: Loop bound is wrong. (advisory only — not configured to block merge)"]);
+    });
+  });
+
   it("demotes self-host environmental/process warnings out of the nits, keeping real code nits (#review-accuracy)", () => {
     const reviews = buildDualReviewNotes({
       aiReview: { notes: "Looks fine." },