From e0b3b0f35f81523ce93ef88046759cfeef4d66d6 Mon Sep 17 00:00:00 2001 From: St0rmz1 Date: Fri, 17 Jul 2026 13:29:33 -0700 Subject: [PATCH] fix(code-review): group council findings by line instead of lossy dedup --- .../prompts/council-prompt.test.ts | 20 +++++++++++++++++++ .../code-reviews/prompts/council-prompt.ts | 9 ++++++++- .../src/code-review-council.test.ts | 3 +++ .../worker-utils/src/code-review-council.ts | 1 + 4 files changed, 32 insertions(+), 1 deletion(-) diff --git a/apps/web/src/lib/code-reviews/prompts/council-prompt.test.ts b/apps/web/src/lib/code-reviews/prompts/council-prompt.test.ts index 8872a0b671..577dfaf864 100644 --- a/apps/web/src/lib/code-reviews/prompts/council-prompt.test.ts +++ b/apps/web/src/lib/code-reviews/prompts/council-prompt.test.ts @@ -122,6 +122,26 @@ describe('buildCouncilOrchestratorPrompt', () => { expect(prompt).toContain('see the Council Review decision above'); }); + it('instructs the coordinator to GROUP same-line findings (no dedup, keep max severity, drop nothing)', () => { + const prompt = buildCouncilOrchestratorPrompt({ + basePrompt: 'BASE', + specialists: [ + specialist({ id: 'security', name: 'Security' }), + specialist({ id: 'correctness', name: 'Correctness', role: 'correctness' }), + ], + aggregationStrategy: 'unanimous', + }); + + // Distinct findings that share a line must NOT be deduped away; group and keep every lens. + expect(prompt).toContain('GROUP findings by file:line'); + expect(prompt).toContain('do NOT dedup, merge away, or drop any finding'); + expect(prompt).toContain('lists'); + expect(prompt).toContain('HIGHEST severity among the grouped findings'); + expect(prompt).toContain('EVERY specialist finding must'); + // The old lossy instruction must be gone. + expect(prompt).not.toContain('Merge duplicate findings'); + }); + it('instructs the coordinator to narrate progress (startup, per-specialist, done)', () => { const prompt = buildCouncilOrchestratorPrompt({ basePrompt: 'BASE', diff --git a/apps/web/src/lib/code-reviews/prompts/council-prompt.ts b/apps/web/src/lib/code-reviews/prompts/council-prompt.ts index 5e546a733e..7485c705cf 100644 --- a/apps/web/src/lib/code-reviews/prompts/council-prompt.ts +++ b/apps/web/src/lib/code-reviews/prompts/council-prompt.ts @@ -101,7 +101,14 @@ export function buildCouncilOrchestratorPrompt(params: { "- Treat the specialists' combined findings as the review result and follow the", ' publication instructions in the review context below EXACTLY as a standard review does', ' (in provider mode: post the inline comments AND the summary to the PR; in kilo mode:', - ' return them). Merge duplicate findings that land on the same file and line.', + ' return them).', + '- GROUP findings by file:line — do NOT dedup, merge away, or drop any finding. When two or', + ' more specialists flag the SAME file:line, post ONE inline comment for that line that lists', + " EACH specialist separately (the specialist's name, its severity, and its rationale) — they", + ' are distinct findings that happen to share a line, not duplicates, so no lens is lost. Set', + " that comment's header severity to the HIGHEST severity among the grouped findings; never", + ' downgrade. Findings on different lines stay separate comments. EVERY specialist finding must', + ' appear in the posted review — never discard one because another specialist flagged that line.', '- Keep the summary body EXACTLY as the base instructions require: the required leading', ' marker (e.g. ``) and the standard summary heading, unchanged.', '- Do NOT write a "Council Review" section, a per-specialist table, or any votes/decision', diff --git a/packages/worker-utils/src/code-review-council.test.ts b/packages/worker-utils/src/code-review-council.test.ts index 2fecb2b6c5..590597c101 100644 --- a/packages/worker-utils/src/code-review-council.test.ts +++ b/packages/worker-utils/src/code-review-council.test.ts @@ -590,6 +590,9 @@ describe('buildCouncilReviewSection', () => { expect(section).toContain('| Specialist | Model | Highest severity | Findings |'); expect(section).toContain('_Governance mode: Unanimous —'); expect(section).toContain('a single blocking vote blocks the merge'); + // Note so the per-specialist raw counts are not misread against the grouped inline-comment count. + expect(section).toContain('counted per specialist'); + expect(section).toContain('grouped into one inline comment'); }); it('phrases a gating (automated) block decision as a hard merge block', () => { diff --git a/packages/worker-utils/src/code-review-council.ts b/packages/worker-utils/src/code-review-council.ts index c45eacfc15..29a3fcb960 100644 --- a/packages/worker-utils/src/code-review-council.ts +++ b/packages/worker-utils/src/code-review-council.ts @@ -609,6 +609,7 @@ export function buildCouncilReviewSection( '|------------|-------|------------------|----------|', tableRows, '', + '_Findings are counted per specialist; when specialists flag the same line their findings are grouped into one inline comment, so the inline comment count is lower than this total._', `_Governance mode: ${governanceLabel} — ${GOVERNANCE_EXPLANATIONS[result.aggregationStrategy]}._`, ].join('\n'); }