Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/review/fix-handoff-render.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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 = "<!-- loopover:fix-handoff-aggregate -->";

/** 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 };
}
54 changes: 53 additions & 1 deletion test/unit/fix-handoff-render.test.ts
Original file line number Diff line number Diff line change
@@ -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";

Expand Down Expand Up @@ -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("<!-- loopover:fix-handoff-aggregate -->");
expect(block?.body).not.toContain("<!-- loopover:fix-handoff -->");
});
});