You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.
Context
#9670 established the rule for this renderer: a findings list is truncated at exactly ONE stage — the
disclosed one — so the reader always gets a +N more footer and the copy-paste block always gets the full
set. dedupeLines carries that rule in its own doc comment (src/review/unified-comment.ts:518-522):
/** 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. */functiondedupeLines(items: string[],cap=Number.POSITIVE_INFINITY): string[]{
The list dedupeLines receives is produced one layer earlier, by dedupeConcerns, which still carries an
UNDISCLOSED hard cap (src/review/unified-comment.ts:121-133):
extractReviewSummary builds BOTH rendered lists through it (src/review/unified-comment.ts:139 and :141), and buildUnifiedReviewInput copies them straight into UnifiedReviewInput.blockers / .nits
(:942-943). buildUnifiedCommentBody (src/review/unified-comment-bridge.ts:888) is the only production
caller, and the reviewer notes it passes in fold the gate's own hard blockers in as well
(gateBlockers: args.gate.blockers, src/review/unified-comment-bridge.ts:874).
Consequences today, all reachable from one PR review:
renderUnifiedReviewComment computes blockersTrunc.hiddenCount from the ALREADY-capped list
(src/review/unified-comment.ts:814-815), so with 30 distinct blockers the comment renders 12 items and _+8 more_ — the reader is told 20 exist when 30 do. The +N more footer is wrong by construction.
buildAiContextBlock(blockersAll, …) (:825) is documented as receiving "every blocker" (:822-823) but
receives at most 20.
The Nits collapsible's summary line renders ${nitsAll.length} non-blocking (:807), which is capped at
20 for the same reason. Nits are the union of both reviewers' nits AND their free-form suggestions
(:141), so exceeding 20 on a real multi-file PR is ordinary, not pathological.
git log -S'dedupeConcerns' --oneline -- src/review/unified-comment.ts shows this function has not been
touched since the original port (252757587); the #9670/#9766 fix landed on dedupeLines only
(762ed6d4c), leaving the upstream cap in place.
Requirements
dedupeConcerns must no longer truncate. Remove the slice(0, 20) so it returns every distinct concern,
exactly as dedupeLines was changed to do.
extractReviewSummary's returned blockers and nits must therefore be the FULL deduped sets.
The rendered human-facing lists must stay capped at the existing display cap: truncateFindingsForDisplay
with input.maxFindingsCaps?.blockers ?? DEFAULT_FINDINGS_DISPLAY_CAP (src/review/unified-comment.ts:815)
and the nits equivalent (:802) must NOT change, and DEFAULT_FINDINGS_DISPLAY_CAP must stay 12.
appendMoreFooter's hiddenCount must now reflect the true number of hidden items (e.g. 30 distinct
blockers with the default cap ⇒ 12 shown and _+18 more_, not _+8 more_).
buildAiContextBlock must receive every deduped blocker, matching its own doc comment.
The Nits collapsible's ${nitsAll.length} non-blocking sub-label must report the true deduped count.
Behaviour that must NOT change: dedupeConcerns's normalization key (toLowerCase() plus the [\s.,;:!?]+ collapse) and its blank-line skip; the ordering (first-seen wording preserved); dedupeLines'
own default of Number.POSITIVE_INFINITY; actionReasonBullets' explicit dedupeLines(reasons, 8) cap
(:592), which is a deliberate undisclosed cap on a different surface.
⚠️ Required pattern: mirror dedupeLines (src/review/unified-comment.ts:518-535) exactly — an uncapped
dedupe whose callers do the disclosed truncation. What does NOT satisfy this issue: raising 20 to a
larger number (still an undisclosed cap, same defect); adding a second +N more footer inside extractReviewSummary (a parallel disclosure mechanism instead of fixing the single one that exists);
capping inside buildUnifiedCommentBody instead of removing the cap here; a test-only PR that asserts the
current 20-item behaviour.
Deliverables
src/review/unified-comment.ts: dedupeConcerns returns out (no slice), so extractReviewSummary({ reviews }) with a single reviewer carrying 30 distinct blockers strings
returns blockers.length === 30.
A test in test/unit/unified-comment.test.ts asserting buildUnifiedReviewInput with one review note
carrying 30 distinct blockers yields input.blockers.length === 30 (and the same for 30 distinct nits).
A test in test/unit/unified-comment.test.ts asserting renderUnifiedReviewComment for that input
renders exactly 12 blocker bullets and the literal footer _+18 more_.
A test in test/unit/unified-comment.test.ts asserting the copy-paste fenced block emitted by buildAiContextBlock for that same input contains all 30 numbered entries (30. is present).
A test in test/unit/unified-comment.test.ts asserting the Nits collapsible's summary reads 30 non-blocking for 30 distinct nits.
A regression test at test/unit/unified-comment.test.ts named for this bug (e.g. "does not silently drop concerns past 20 before the disclosed truncation stage (#9670 follow-up)")
asserting that with 25 distinct blockers and maxFindingsCaps: { blockers: null, nits: null } the
rendered comment contains all 25 bullets and no _+N more_ footer.
All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
removing the slice(0, 20) without adding the +N more count assertion, or adding tests that pin the
current capped counts — does not resolve this issue.
Test Coverage Requirements
This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's coverage.include
covers src/**/*.ts and packages/loopover-engine/src/**/*.ts; src/review/unified-comment.ts is measured
and gated. The change removes a statement rather than adding a branch, but every branch it makes reachable
must be exercised: truncateFindingsForDisplay's items.length <= cap arm AND its slice arm (both
blockers and nits), appendMoreFooter's hiddenCount > 0 and hiddenCount === 0 arms, and renderUnifiedReviewComment's blockersTrunc.shown.length ? … : … ternary (:818-820) and the nits
equivalent (:804-806). Assert both arms of each.
Expected Outcome
The unified review comment's +N more footer states the real number of hidden concerns, the copy-paste
block carries every blocker as its doc comment already promises, and the Nits sub-label reports the real
count — closing the same silent-truncation class #9670 fixed one layer downstream.
Links & Resources
src/review/unified-comment.ts:121-133 — dedupeConcerns and its slice(0, 20)
src/review/unified-comment.ts:135-148 — extractReviewSummary, the only caller
src/review/unified-comment.ts:518-551 — dedupeLines / DEFAULT_FINDINGS_DISPLAY_CAP / truncateFindingsForDisplay, the disclosed-truncation stage
src/review/unified-comment.ts:801-826 — the render path that computes hiddenCount
src/review/unified-comment-bridge.ts:874-888 — the production caller that folds gate blockers in
Context
#9670established the rule for this renderer: a findings list is truncated at exactly ONE stage — thedisclosed one — so the reader always gets a
+N morefooter and the copy-paste block always gets the fullset.
dedupeLinescarries that rule in its own doc comment (src/review/unified-comment.ts:518-522):The list
dedupeLinesreceives is produced one layer earlier, bydedupeConcerns, which still carries anUNDISCLOSED hard cap (
src/review/unified-comment.ts:121-133):extractReviewSummarybuilds BOTH rendered lists through it (src/review/unified-comment.ts:139and:141), andbuildUnifiedReviewInputcopies them straight intoUnifiedReviewInput.blockers/.nits(
:942-943).buildUnifiedCommentBody(src/review/unified-comment-bridge.ts:888) is the only productioncaller, and the reviewer notes it passes in fold the gate's own hard blockers in as well
(
gateBlockers: args.gate.blockers,src/review/unified-comment-bridge.ts:874).Consequences today, all reachable from one PR review:
renderUnifiedReviewCommentcomputesblockersTrunc.hiddenCountfrom the ALREADY-capped list(
src/review/unified-comment.ts:814-815), so with 30 distinct blockers the comment renders 12 items and_+8 more_— the reader is told 20 exist when 30 do. The+N morefooter is wrong by construction.buildAiContextBlock(blockersAll, …)(:825) is documented as receiving "every blocker" (:822-823) butreceives at most 20.
${nitsAll.length} non-blocking(:807), which is capped at20 for the same reason. Nits are the union of both reviewers'
nitsAND their free-formsuggestions(
:141), so exceeding 20 on a real multi-file PR is ordinary, not pathological.git log -S'dedupeConcerns' --oneline -- src/review/unified-comment.tsshows this function has not beentouched since the original port (
252757587); the#9670/#9766fix landed ondedupeLinesonly(
762ed6d4c), leaving the upstream cap in place.Requirements
dedupeConcernsmust no longer truncate. Remove theslice(0, 20)so it returns every distinct concern,exactly as
dedupeLineswas changed to do.extractReviewSummary's returnedblockersandnitsmust therefore be the FULL deduped sets.truncateFindingsForDisplaywith
input.maxFindingsCaps?.blockers ?? DEFAULT_FINDINGS_DISPLAY_CAP(src/review/unified-comment.ts:815)and the nits equivalent (
:802) must NOT change, andDEFAULT_FINDINGS_DISPLAY_CAPmust stay12.appendMoreFooter'shiddenCountmust now reflect the true number of hidden items (e.g. 30 distinctblockers with the default cap ⇒ 12 shown and
_+18 more_, not_+8 more_).buildAiContextBlockmust receive every deduped blocker, matching its own doc comment.Nitscollapsible's${nitsAll.length} non-blockingsub-label must report the true deduped count.dedupeConcerns's normalization key (toLowerCase()plus the[\s.,;:!?]+collapse) and its blank-line skip; the ordering (first-seen wording preserved);dedupeLines'own default of
Number.POSITIVE_INFINITY;actionReasonBullets' explicitdedupeLines(reasons, 8)cap(
:592), which is a deliberate undisclosed cap on a different surface.Deliverables
src/review/unified-comment.ts:dedupeConcernsreturnsout(noslice), soextractReviewSummary({ reviews })with a single reviewer carrying 30 distinctblockersstringsreturns
blockers.length === 30.test/unit/unified-comment.test.tsassertingbuildUnifiedReviewInputwith one review notecarrying 30 distinct blockers yields
input.blockers.length === 30(and the same for 30 distinct nits).test/unit/unified-comment.test.tsassertingrenderUnifiedReviewCommentfor that inputrenders exactly 12 blocker bullets and the literal footer
_+18 more_.test/unit/unified-comment.test.tsasserting the copy-paste fenced block emitted bybuildAiContextBlockfor that same input contains all 30 numbered entries (30.is present).test/unit/unified-comment.test.tsasserting theNitscollapsible's summary reads30 non-blockingfor 30 distinct nits.test/unit/unified-comment.test.tsnamed for this bug (e.g."does not silently drop concerns past 20 before the disclosed truncation stage (#9670 follow-up)")asserting that with 25 distinct blockers and
maxFindingsCaps: { blockers: null, nits: null }therendered comment contains all 25 bullets and no
_+N more_footer.All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example
removing the
slice(0, 20)without adding the+N morecount assertion, or adding tests that pin thecurrent capped counts — does not resolve this issue.
Test Coverage Requirements
This repo enforces 99%+ Codecov patch coverage, branch-counted.
vitest.config.ts'scoverage.includecovers
src/**/*.tsandpackages/loopover-engine/src/**/*.ts;src/review/unified-comment.tsis measuredand gated. The change removes a statement rather than adding a branch, but every branch it makes reachable
must be exercised:
truncateFindingsForDisplay'sitems.length <= caparm AND itsslicearm (bothblockers and nits),
appendMoreFooter'shiddenCount > 0andhiddenCount === 0arms, andrenderUnifiedReviewComment'sblockersTrunc.shown.length ? … : …ternary (:818-820) and the nitsequivalent (
:804-806). Assert both arms of each.Expected Outcome
The unified review comment's
+N morefooter states the real number of hidden concerns, the copy-pasteblock carries every blocker as its doc comment already promises, and the Nits sub-label reports the real
count — closing the same silent-truncation class
#9670fixed one layer downstream.Links & Resources
src/review/unified-comment.ts:121-133—dedupeConcernsand itsslice(0, 20)src/review/unified-comment.ts:135-148—extractReviewSummary, the only callersrc/review/unified-comment.ts:518-551—dedupeLines/DEFAULT_FINDINGS_DISPLAY_CAP/truncateFindingsForDisplay, the disclosed-truncation stagesrc/review/unified-comment.ts:801-826— the render path that computeshiddenCountsrc/review/unified-comment-bridge.ts:874-888— the production caller that folds gate blockers in