diff --git a/src/review/fix-handoff-render.ts b/src/review/fix-handoff-render.ts index 155a4a485c..2c3e9df18f 100644 --- a/src/review/fix-handoff-render.ts +++ b/src/review/fix-handoff-render.ts @@ -11,6 +11,10 @@ // already produced through the public-safe filter (InlineFinding.body/suggestion are sanitized upstream by // composeInlineFindings before they ever reach here — this module adds no new free text of its own beyond the // fixed label/marker strings below). +// +// Also renders the AGGREGATE flavor (#5102): buildFixHandoffAggregateBlock combines every finding into ONE +// block for a single agent run over the whole PR, instead of one run per finding — same rendering contract, +// still unwired (see that function's doc comment for why). import { LOCAL_WRITE_BOUNDARY } from "../mcp/local-write-tools"; import type { InlineFinding } from "../services/ai-review"; @@ -80,3 +84,48 @@ export function buildFixHandoffBlock(finding: InlineFinding): FixHandoffBlock { export function buildFixHandoffBlocks(findings: InlineFinding[]): FixHandoffBlock[] { return findings.map((finding) => buildFixHandoffBlock(finding)); } + +/** A whole PR's findings rendered as ONE fix-handoff block, for a single local-agent run instead of one run per + * finding (#5102). */ +export type FixHandoffAggregateBlock = { + findingCount: number; + /** The rendered, machine-readable markdown block (fenced items + an HTML comment marker a harness can grep for). */ + body: string; + boundary: string; +}; + +/** The HTML comment marker prefixing the rendered aggregate block, distinct from FIX_HANDOFF_MARKER so a + * harness can tell a per-finding block from the aggregate one. */ +const FIX_HANDOFF_AGGREGATE_MARKER = ""; + +/** One numbered list item for the aggregate block: same location/label/suggestion composition as + * buildFixHandoffBlock, just indented under a shared numbered list instead of standing alone. */ +function fixHandoffAggregateItem(finding: InlineFinding, index: number): string { + const hasLine = Number.isInteger(finding.line) && finding.line > 0; + const safePath = markdownPathCodeText(finding.path); + const location = hasLine ? `${safePath}:${finding.line}` : `${safePath} (no specific line)`; + const label = finding.severity === "blocker" ? "Blocker" : "Nit"; + const suggestion = finding.suggestion?.trim(); + const suggestionBlock = suggestion ? `\n \`\`\`\n ${suggestion.replace(/\n/g, "\n ")}\n \`\`\`` : ""; + return `${index + 1}. **${label} at \`${location}\`** — ${finding.body}${suggestionBlock}`; +} + +/** PURE: combine every current finding into ONE fix-handoff block for a single local-agent run across the + * whole PR (#5102) — the aggregate sibling of buildFixHandoffBlock/buildFixHandoffBlocks, mirroring + * CodeRabbit's split between a per-finding "Prompt for AI Agents" collapsible and an aggregate "Fix all + * issues" prompt (confirmed against live CodeRabbit-reviewed PRs — see #5102). Same boundary-safe, + * content-only contract as the per-finding block: no server-side write, no execution, public-safe by + * construction (every field rendered here was already made public-safe upstream by composeInlineFindings). + * Empty in ⇒ null out — nothing to hand off. Render-only, like buildFixHandoffBlock was before its own + * wiring PR (#4053) — NOT wired into the unified comment here; #5102 leaves per-finding vs aggregate vs + * both as an open placement question for the wiring PR to resolve. */ +export function buildFixHandoffAggregateBlock(findings: InlineFinding[]): FixHandoffAggregateBlock | null { + if (findings.length === 0) return null; + const body = [ + FIX_HANDOFF_AGGREGATE_MARKER, + `**Fix handoff — ${findings.length} finding${findings.length === 1 ? "" : "s"} across this PR**`, + ...findings.map((finding, index) => fixHandoffAggregateItem(finding, index)), + `\n_${LOCAL_WRITE_BOUNDARY}_`, + ].join("\n"); + return { findingCount: findings.length, body, boundary: LOCAL_WRITE_BOUNDARY }; +} diff --git a/test/unit/fix-handoff-render.test.ts b/test/unit/fix-handoff-render.test.ts index 69eb966cef..c7684454c5 100644 --- a/test/unit/fix-handoff-render.test.ts +++ b/test/unit/fix-handoff-render.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { buildFixHandoffBlock, buildFixHandoffBlocks } from "../../src/review/fix-handoff-render"; +import { buildFixHandoffAggregateBlock, buildFixHandoffBlock, buildFixHandoffBlocks } from "../../src/review/fix-handoff-render"; import { LOCAL_WRITE_BOUNDARY } from "../../src/mcp/local-write-tools"; import type { InlineFinding } from "../../src/services/ai-review"; @@ -83,3 +83,55 @@ describe("buildFixHandoffBlocks (#2175)", () => { expect(buildFixHandoffBlocks([])).toEqual([]); }); }); + +describe("buildFixHandoffAggregateBlock (#5102)", () => { + it("returns null for no findings (no-op)", () => { + expect(buildFixHandoffAggregateBlock([])).toBeNull(); + }); + + it("combines a single finding into one block with singular wording", () => { + const block = buildFixHandoffAggregateBlock([finding()]); + expect(block?.findingCount).toBe(1); + expect(block?.body).toContain("**Fix handoff — 1 finding across this PR**"); + expect(block?.body).toContain("1. **Blocker at `src/a.ts:12`** — Null check missing before dereference."); + }); + + it("combines multiple findings into one numbered block with plural wording", () => { + const block = buildFixHandoffAggregateBlock([ + finding({ path: "a.ts", line: 1, severity: "blocker" }), + finding({ path: "b.ts", line: 2, severity: "nit" }), + ]); + expect(block?.findingCount).toBe(2); + expect(block?.body).toContain("**Fix handoff — 2 findings across this PR**"); + expect(block?.body).toContain("1. **Blocker at `a.ts:1`**"); + expect(block?.body).toContain("2. **Nit at `b.ts:2`**"); + }); + + it("renders a path-only location when a finding has no commentable line", () => { + const block = buildFixHandoffAggregateBlock([finding({ line: 0 })]); + expect(block?.body).toContain("src/a.ts (no specific line)"); + expect(block?.body).not.toContain("src/a.ts:0"); + }); + + it("includes a fenced suggestion block, indented under its list item, when present", () => { + const block = buildFixHandoffAggregateBlock([finding({ suggestion: "if (!value) return null;" })]); + expect(block?.body).toContain("```\n if (!value) return null;\n ```"); + }); + + it("omits the suggestion block entirely when absent or whitespace-only", () => { + expect(buildFixHandoffAggregateBlock([finding()])?.body).not.toContain("```"); + expect(buildFixHandoffAggregateBlock([finding({ suggestion: " " })])?.body).not.toContain("```"); + }); + + it("always includes the exact LOCAL_WRITE_BOUNDARY text (boundary-safe)", () => { + const block = buildFixHandoffAggregateBlock([finding()]); + expect(block?.boundary).toBe(LOCAL_WRITE_BOUNDARY); + expect(block?.body).toContain(LOCAL_WRITE_BOUNDARY); + }); + + it("includes the aggregate HTML comment marker, distinct from the per-finding marker", () => { + const block = buildFixHandoffAggregateBlock([finding()]); + expect(block?.body).toContain(""); + expect(block?.body).not.toContain(""); + }); +});