diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index e658d3ab84..8c7fbb57ce 100644 --- a/src/review/unified-comment-bridge.ts +++ b/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, + 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. diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index bbd4e2b100..b10bc517dc 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/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("
Signal definitions"); + 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**"); + }); +});