review: aggregate comment-size budget guard

src/review/unified-comment-bridge.ts

@@ -43,6 +43,8 @@ import {
   type ReviewNotes,
   type ReviewRecommendation,
   type UnifiedCollapsible,
+  type UnifiedCommentContext,
+  type UnifiedReviewInput,
   type UnifiedSignalRow,
   type Verdict,
 } from "./unified-comment";
@@ -720,6 +722,47 @@ export function buildFindingCategoryCollapsible(findings: FindingCategoryInput[]
   return { title: "Finding categories", body };
 }
 
+// #6069: a safe upper bound for the RENDERED comment body (before the marker prefix in buildUnifiedCommentBody
+// below), comfortably under GitHub's real ~65536-character issue/PR comment cap (see MAX_STORED_BODY_CHARS,
+// src/db/repositories.ts, for the same real limit applied to INCOMING bodies) -- leaves headroom for that
+// marker plus a margin against undercounting. A maximally-featured PR (every optional collapsible active,
+// plus e.g. a large changed-files table) had no aggregate size guard before this -- each section only ever
+// capped ITSELF, so a comment could silently exceed GitHub's limit and fail to post at all.
+const COMMENT_BODY_BUDGET_CHARS = 60_000;
+
+/**
+ * Render the comment, dropping the LOWEST-priority optional collapsibles -- from the end of
+ * `extraCollapsibles` -- one at a time until the body fits COMMENT_BODY_BUDGET_CHARS or none are left to
+ * drop. The construction order in `buildUnifiedCommentBody` already puts the heaviest/most decorative
+ * sections last (Visual preview / Scroll preview's embedded image tables, Changed files' up-to-200-row
+ * table), ahead of nothing but each other, so trimming from the end removes bulk before reference material.
+ * Disposition-relevant content -- headline, chips, verdict, blockers, per-blocker AI fix context, decision
+ * drivers -- is rendered directly by `renderUnifiedReviewComment` and is never part of `extraCollapsibles`,
+ * so this loop can never drop it. When trimming happens, a final note is appended so the omission is visible
+ * IN the comment (a maintainer reading it sees why detail is missing), not just an ops log they'd never see.
+ */
+function renderWithinBudget(
+  input: UnifiedReviewInput,
+  ctxBase: Omit<UnifiedCommentContext, "extraCollapsibles">,
+  extraCollapsibles: UnifiedCollapsible[] | undefined,
+): string {
+  const all = extraCollapsibles ?? [];
+  let kept = all;
+  for (;;) {
+    const body = renderUnifiedReviewComment(input, { ...ctxBase, ...(kept.length > 0 ? { extraCollapsibles: kept } : {}) });
+    if (body.length <= COMMENT_BODY_BUDGET_CHARS || kept.length === 0) {
+      if (kept.length === all.length) return body;
+      const omittedCount = all.length - kept.length;
+      const note: UnifiedCollapsible = {
+        title: "Some detail omitted",
+        body: `${omittedCount} additional section${omittedCount === 1 ? "" : "s"} were left out of this comment to stay within GitHub's comment size limit.`,
+      };
+      return renderUnifiedReviewComment(input, { ...ctxBase, extraCollapsibles: [...kept, note] });
+    }
+    kept = kept.slice(0, -1);
+  }
+}
+
 /**
  * Build the unified PR-review comment body from loopover's live data. Returns a string that STARTS with
  * the panel marker (so the existing upsert updates in place) followed by the rendered unified comment.
@@ -849,20 +892,23 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string
   const scrollCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildScrollPreviewCollapsible(args.beforeAfter) : null;
   const extraCollapsibles = scrollCollapsible !== null ? [...(withVisual ?? []), scrollCollapsible] : withVisual;
 
-  const body = renderUnifiedReviewComment(input, {
-    brand: args.brand ?? "LoopOver review",
-    readinessScore: args.readinessTotal,
-    signals,
-    footerMarkdown: args.footerMarkdown,
-    reviewedAt: args.reviewedAt ?? new Date(),
-    ...(args.reRunLabel !== undefined ? { reRunLabel: args.reRunLabel } : {}),
-    ...(args.generateTestsLabel !== undefined ? { generateTestsLabel: args.generateTestsLabel } : {}),
-    ...(extraCollapsibles !== undefined ? { extraCollapsibles } : {}),
-    ...(args.heldForReview ? { heldForReview: true } : {}),
-    ...(args.neverClosed ? { neverClosed: true } : {}),
-    ...(args.preflightHeld ? { preflightHeld: true } : {}),
-    commentVerbosity: args.commentVerbosity,
-  });
+  const body = renderWithinBudget(
+    input,
+    {
+      brand: args.brand ?? "LoopOver review",
+      readinessScore: args.readinessTotal,
+      signals,
+      footerMarkdown: args.footerMarkdown,
+      reviewedAt: args.reviewedAt ?? new Date(),
+      ...(args.reRunLabel !== undefined ? { reRunLabel: args.reRunLabel } : {}),
+      ...(args.generateTestsLabel !== undefined ? { generateTestsLabel: args.generateTestsLabel } : {}),
+      ...(args.heldForReview ? { heldForReview: true } : {}),
+      ...(args.neverClosed ? { neverClosed: true } : {}),
+      ...(args.preflightHeld ? { preflightHeld: true } : {}),
+      commentVerbosity: args.commentVerbosity,
+    },
+    extraCollapsibles,
+  );
 
   // Prepend the marker verbatim (matching the legacy body, which leads with the marker then a blank line)
   // so `createOrUpdatePrIntelligenceComment` finds and updates the SAME comment in place.

test/unit/unified-comment-bridge.test.ts

@@ -1323,3 +1323,59 @@ describe("isUnifiedReviewCommentEnabled (flag-OFF selects the legacy path)", ()
     }
   });
 });
+
+describe("comment size-budget guard (#6069)", () => {
+  it("leaves a normal-sized comment completely untouched (no trimming, no note)", () => {
+    const body = buildUnifiedCommentBody({
+      gate: gate(),
+      aiReview: { notes: "Clean change." },
+      panelRows,
+      readinessTotal: 88,
+      changedFiles: 3,
+      footerMarkdown: footer,
+      extraCollapsibles: [{ title: "Signal definitions", body: "Readiness signals describe public-metadata readiness." }],
+    });
+    expect(body).toContain("<details><summary><b>Signal definitions</b></summary>");
+    expect(body).not.toContain("Some detail omitted");
+    expect(body.length).toBeLessThan(60_000);
+  });
+
+  it("trims the lowest-priority (last) optional collapsibles until the body fits the budget, and notes the omission", () => {
+    // Five ~15KB collapsibles (75KB total) comfortably exceed the 60,000-char budget on their own --
+    // every OTHER optional section (auto-merge summary, changed files, impact map, fix handoff, visual
+    // preview, scroll preview) is left unset, so these five sit at the effective END of the chain and are
+    // exactly what the trim-from-the-end loop should remove first.
+    const huge = (label: string) => ({ title: label, body: "x".repeat(15_000) });
+    const body = buildUnifiedCommentBody({
+      gate: gate({ conclusion: "failure" }),
+      panelRows,
+      readinessTotal: 40,
+      changedFiles: 2,
+      footerMarkdown: footer,
+      extraCollapsibles: [huge("Section A"), huge("Section B"), huge("Section C"), huge("Section D"), huge("Section E")],
+    });
+    expect(body.length).toBeLessThanOrEqual(60_000 + 500); // budget + the small trailing omission note
+    expect(body).toContain("Some detail omitted");
+    // Disposition-relevant content survives trimming intact -- it's never part of extraCollapsibles.
+    expect(body).toContain("**Decision drivers**");
+    expect(body).toContain("Suggested Action");
+    // At least one of the huge sections was actually dropped (otherwise the note wouldn't be honest).
+    const survivingSections = ["Section A", "Section B", "Section C", "Section D", "Section E"].filter((label) => body.includes(label));
+    expect(survivingSections.length).toBeLessThan(5);
+  });
+
+  it("still returns a body (never throws/empties) even when trimming every optional section isn't enough", () => {
+    // A single collapsible bigger than the whole budget by itself -- trimming it away still leaves
+    // core content, which is the correct, only-safe behavior (core content is never droppable).
+    const body = buildUnifiedCommentBody({
+      gate: gate(),
+      panelRows,
+      readinessTotal: 90,
+      changedFiles: 1,
+      footerMarkdown: footer,
+      extraCollapsibles: [{ title: "Massive", body: "x".repeat(200_000) }],
+    });
+    expect(body).toContain("LoopOver review result");
+    expect(body).toContain("**Decision drivers**");
+  });
+});