From 8ed4f573084a1a5978209dbe3c130696f1ab3b8d Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 29 Jul 2026 15:55:30 +0900 Subject: [PATCH] fix(review): stop silently dropping blockers/nits past 12 in the unified comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dedupeLines` defaulted to a hard cap of 12, so the blockers and nits lists were truncated to 12 items *before* the disclosed truncation stage. Two consequences: the human list was cut with no "+N more" footer (the overflow looked like it did not exist), and the "Copy for AI agents" block — which promises *every* blocker — never received findings 13+. The `changedFiles` status chip and the code-review signal row also counted the raw, un-deduped `input.blockers`, so a duplicate blocker could inflate the chip past what the rendered section actually lists. Make `dedupeLines` un-capped by default and keep the 12-item display cap at the `truncateFindingsForDisplay` call site (the disclosed-truncation stage), so overflow now renders a real "+N more" footer while the AI-context block stays complete. Derive both blocker counts from the deduped set. Closes #9670 --- src/review/unified-comment.ts | 28 ++++++++++++++---- test/unit/unified-comment.test.ts | 48 +++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 7714a81e5..c9c7695fa 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -431,7 +431,9 @@ function plural(n: number, one: string): string { function statusChips(input: UnifiedReviewInput, ctx: UnifiedCommentContext, status: UnifiedCommentStatus): string { const chips: string[] = [`\`${plural(input.changedFiles, "file")}\``]; if (input.reviewerCount > 0) chips.push(`\`${plural(input.reviewerCount, "AI reviewer")}\``); - const blockerCount = (input.blockers ?? []).length; + // Count the DEDUPED blockers -- the same set the "Why this is blocked" section renders from -- so the chip can + // never claim a number the section does not account for (e.g. when input.blockers carries duplicates) (#9670). + const blockerCount = dedupeLines(input.blockers ?? []).length; chips.push(blockerCount ? `\`${plural(blockerCount, "blocker")}\`` : "`no blockers`"); // The readiness score is advisory-only and NEVER feeds the gate (see deriveUnifiedStatus's own comments) — // showing it next to a non-"ready" verdict reads as contradictory (e.g. "readiness 93/100" beside "fixes @@ -490,8 +492,11 @@ function verdictLine(status: UnifiedCommentStatus, input: UnifiedReviewInput, ct } } -/** Dedupe + cap a list of lines (case-insensitive), so blockers/nits never balloon the comment. */ -function dedupeLines(items: string[], cap = 12): string[] { +/** Dedupe a list of lines (case-insensitive). `cap` defaults to unlimited (#9670): the blockers/nits callers + * pass the FULL deduped set to truncateFindingsForDisplay, which is the disclosed-truncation stage -- capping + * here instead silently dropped items 13+ from the AI-context block and hid the "+N more" footer. Callers that + * genuinely want a hard, undisclosed cap (e.g. buildAiContextBlock's reasons) still pass one explicitly. */ +function dedupeLines(items: string[], cap = Number.POSITIVE_INFINITY): string[] { const seen = new Set(); const out: string[] = []; for (const raw of items) { @@ -507,6 +512,11 @@ function dedupeLines(items: string[], cap = 12): string[] { } /** Truncate a findings list for display-only rendering. Null/undefined cap ⇒ unchanged. */ +// The human-scannable display cap applied when a repo's maxFindingsCaps supplies nothing. Moved here from +// dedupeLines' old default so the cap is now disclosed truncation (with a "+N more" footer) rather than silent +// data loss upstream of the AI-context block and the more-footer (#9670). +export const DEFAULT_FINDINGS_DISPLAY_CAP = 12; + export function truncateFindingsForDisplay( items: string[], cap: number | null | undefined, @@ -638,7 +648,9 @@ function nonRequiredFailingChecksBlock(readiness: MergeReadiness | undefined): s * found something, but produced no write-up -- not "AI review never ran; a separate check is what's * blocking this." */ function codeReviewRow(input: UnifiedReviewInput): UnifiedSignalRow { - const blockerCount = (input.blockers ?? []).length; + // Count the DEDUPED blockers so this row's evidence phrasing keys off the same population the blockers + // section renders (a duplicate blocker must not tip reviewerEvidence into the "blocker is from ..." arm) (#9670). + const blockerCount = dedupeLines(input.blockers ?? []).length; const reviewerEvidence = input.reviewerCount > 1 ? `${input.reviewerCount} reviewers, synthesized` @@ -764,7 +776,7 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi if (input.summary.trim()) blocks.push(`**Review summary**\n${escapePublicHtmlAngles(input.summary.trim())}`); const nitsAll = dedupeLines(input.nits ?? []); - const nitsTrunc = truncateFindingsForDisplay(nitsAll, input.maxFindingsCaps?.nits); + const nitsTrunc = truncateFindingsForDisplay(nitsAll, input.maxFindingsCaps?.nits ?? DEFAULT_FINDINGS_DISPLAY_CAP); if (nitsAll.length && verbosity !== "quiet") { const nitsBody = nitsTrunc.shown.length ? appendMoreFooter(taskList(nitsTrunc.shown), nitsTrunc.hiddenCount) @@ -772,8 +784,12 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi blocks.push(details("Nits", nitsBody, `${nitsAll.length} non-blocking`, collapsiblesOpen)); } + // dedupe uncapped so blockersAll is the FULL set: truncateFindingsForDisplay (the disclosed-truncation stage) + // then sees a real hiddenCount, and buildAiContextBlock below gets every blocker as its comment promises. The + // 12-item DISPLAY default is preserved here (not in dedupeLines) so an unset maxFindingsCaps still caps the + // human list, now with a "+N more" footer rather than silent loss (#9670). const blockersAll = dedupeLines(input.blockers ?? []); - const blockersTrunc = truncateFindingsForDisplay(blockersAll, input.maxFindingsCaps?.blockers); + const blockersTrunc = truncateFindingsForDisplay(blockersAll, input.maxFindingsCaps?.blockers ?? DEFAULT_FINDINGS_DISPLAY_CAP); if (blockersAll.length) { const heading = status === "blocked" ? "Why this is blocked" : "Concerns raised — review before merging"; const blockersBody = blockersTrunc.shown.length diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 75c642cba..cf271cc16 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -927,6 +927,54 @@ describe("review.max_findings display caps (#2049)", () => { expect(capped).toContain("only nit"); expect(capped).not.toMatch(/\+1 more/); }); + + describe("un-capped dedupe + default 12-item display cap (#9670)", () => { + // Before #9670 dedupeLines' default cap was 12, so it silently dropped findings 13+ BEFORE the disclosed + // truncation stage: the human list was cut with no "+N more" footer, and the "Copy for AI agents" block + // (which promises *every* blocker) never saw them. Now dedupeLines is un-capped and the 12-item default + // lives at the truncateFindingsForDisplay call, so overflow is disclosed and the AI block stays complete. + const thirteenBlockers = Array.from({ length: 13 }, (_, i) => `blocker ${i + 1}`); + + it("caps the human blocker list at 12 with a +1 more footer while the AI block keeps all 13", () => { + const md = renderUnifiedReviewComment({ + ...base, + recommendations: ["request_changes"], + blockers: thirteenBlockers, + // maxFindingsCaps intentionally unset -> the default 12-item display cap applies. + }); + // The 12th blocker is the last one shown in the human list; the 13th is hidden behind the footer. + expect(md).toContain("- blocker 12"); + const humanSection = md.split("📋 Copy for AI agents")[0]!; + expect(humanSection).not.toContain("- blocker 13"); + expect(md).toContain("_+1 more_"); + // The AI-context block lists the FULL set, including the 13th that the human list dropped. + const aiSection = md.split("📋 Copy for AI agents")[1]!; + expect(aiSection).toContain("13. blocker 13"); + }); + + it("caps the nits list at 12 with a +1 more footer by default", () => { + const md = renderUnifiedReviewComment({ + ...base, + nits: Array.from({ length: 13 }, (_, i) => `nit ${i + 1}`), + }); + expect(md).toContain("- [ ] nit 12"); + expect(md).not.toContain("- [ ] nit 13"); + expect(md).toContain("_+1 more_"); + // The section label counts every (deduped) nit, not just the shown 12. + expect(md).toContain("13 non-blocking"); + }); + + it("derives the blocker chip count from the deduped set, so duplicates never inflate it", () => { + const md = renderUnifiedReviewComment({ + ...base, + recommendations: ["request_changes"], + // Three raw blockers, but two are case-insensitive duplicates -> two distinct after dedupe. + blockers: ["Real defect", "real defect", "Second defect"], + }); + expect(md).toContain("`2 blockers`"); + expect(md).not.toContain("`3 blockers`"); + }); + }); }); describe("review.comment_verbosity (#2047)", () => {