From f682675e4d975990b18475feb54dc78ed5ca3249 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 06:54:59 +0000 Subject: [PATCH] fix(review): stop dedupeConcerns silently dropping blockers/nits past 20 dedupeConcerns still capped its output at 20 with an undisclosed slice, one layer above the disclosed-truncation stage dedupeLines got fixed to in #9670. With more than 20 distinct blockers or nits the +N more footer understated the hidden count, the AI-context block never saw items past 20 despite promising every blocker, and the Nits sub-label reported a number below the real deduped total. Remove the cap so extractReviewSummary returns the full set and truncateFindingsForDisplay is the only place truncation happens. --- src/review/unified-comment.ts | 7 +++- test/unit/unified-comment.test.ts | 61 +++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 2 deletions(-) diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index c443caa1c6..8955cc5459 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -117,7 +117,10 @@ export interface ExtractedReviewSummary { consensusBlocker: boolean; } -/** Case-insensitive de-dup of concern lines (two reviewers often raise the same point). Preserves first wording. */ +/** Case-insensitive de-dup of concern lines (two reviewers often raise the same point). Preserves first wording. + * Uncapped, mirroring dedupeLines (#10010): callers pass the FULL deduped set to truncateFindingsForDisplay, + * which is the disclosed-truncation stage -- capping here instead silently dropped items 21+ from the + * AI-context block and understated the "+N more" footer. */ function dedupeConcerns(items: string[]): string[] { const seen = new Set(); const out: string[] = []; @@ -129,7 +132,7 @@ function dedupeConcerns(items: string[]): string[] { seen.add(key); out.push(t); } - return out.slice(0, 20); + return out; } function extractReviewSummary(reviews: DualReviewNote[]): ExtractedReviewSummary { diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 6ab4429aaa..9474f926bd 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -977,6 +977,67 @@ describe("review.max_findings display caps (#2049)", () => { }); }); +describe("dedupeConcerns un-capped (#10010)", () => { + // dedupeConcerns (the extraction-layer dedupe one layer above dedupeLines) used to hard-cap at 20 with + // `slice(0, 20)` -- an UNDISCLOSED cap, unlike the disclosed truncation at render time. That silently dropped + // blockers/nits 21+ before they ever reached truncateFindingsForDisplay, so the "+N more" footer, the AI-context + // block, and the Nits sub-label all understated the real count. #9670 fixed the same defect one layer down in + // dedupeLines; this closes it here too. + const thirtyBlockers = Array.from({ length: 30 }, (_, i) => `blocker ${i + 1}`); + const thirtyNits = Array.from({ length: 30 }, (_, i) => `nit ${i + 1}`); + + it("buildUnifiedReviewInput carries all 30 distinct blockers and all 30 distinct nits, not capped at 20", () => { + const input = buildUnifiedReviewInput({ + changedFiles: 1, + reviews: [reviewNote("close", { blockers: thirtyBlockers, nits: thirtyNits })], + }); + expect(input.blockers).toHaveLength(30); + expect(input.nits).toHaveLength(30); + }); + + it("renders exactly 12 blocker bullets with the true '+18 more' footer for 30 distinct blockers", () => { + const input = buildUnifiedReviewInput({ changedFiles: 1, reviews: [reviewNote("close", { blockers: thirtyBlockers })] }); + const md = renderUnifiedReviewComment(input); + const humanSection = md.split("📋 Copy for AI agents")[0]!; + expect(humanSection.match(/- blocker \d+/g)).toHaveLength(12); + expect(md).toContain("- blocker 12"); + expect(humanSection).not.toContain("- blocker 13"); + expect(md).toContain("_+18 more_"); + expect(md).not.toContain("_+8 more_"); // the old, wrongly-capped-at-20 hidden count + }); + + it("the copy-for-AI-agents block carries every one of the 30 blockers, not just the display-capped 12", () => { + const input = buildUnifiedReviewInput({ changedFiles: 1, reviews: [reviewNote("close", { blockers: thirtyBlockers })] }); + const md = renderUnifiedReviewComment(input); + const aiSection = md.split("📋 Copy for AI agents")[1]!; + expect(aiSection).toContain("1. blocker 1"); + expect(aiSection).toContain("30. blocker 30"); + }); + + it("the Nits collapsible sub-label reports the true deduped count of 30, not 20", () => { + const input = buildUnifiedReviewInput({ changedFiles: 1, reviews: [reviewNote("merge", { nits: thirtyNits })] }); + const md = renderUnifiedReviewComment(input); + expect(md).toContain("30 non-blocking"); + expect(md).not.toContain("20 non-blocking"); + }); + + it("does not silently drop concerns past 20 before the disclosed truncation stage (#9670 follow-up)", () => { + const twentyFiveBlockers = Array.from({ length: 25 }, (_, i) => `blocker ${i + 1}`); + // A display cap equal to the item count (not null -- null is byte-identical to "unset", i.e. the 12-item + // default, per the maxFindingsCaps doc comment) puts truncateFindingsForDisplay on its `items.length <= cap` + // arm: display truncation never kicks in, so a missing bullet here can only be dedupeConcerns' old cap. + const input = buildUnifiedReviewInput({ + changedFiles: 1, + reviews: [reviewNote("close", { blockers: twentyFiveBlockers })], + maxFindingsCaps: { blockers: 25, nits: null }, + }); + const md = renderUnifiedReviewComment(input); + expect(input.blockers).toHaveLength(25); + for (const line of twentyFiveBlockers) expect(md).toContain(`- ${line}`); + expect(md).not.toMatch(/_\+\d+ more_/); + }); +}); + describe("review.comment_verbosity (#2047)", () => { const input: UnifiedReviewInput = { ...base,