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
18 changes: 13 additions & 5 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,11 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string
// "all checks passed" wording rather than being overwritten by the gate's "no blocker found" summary.
const gateReason = gateVerdictReason(args.gate);
const verdictReason = verdict !== "merge" ? gateReason : undefined;
// #6068: split the fix-handoff blocks by severity -- blocker-severity ones render per-blocker (right after
// each blocker, via blockerFixContext below); nit-severity ones stay in the combined "Fix handoff"
// collapsible (buildFixHandoffCollapsible, further down) exactly as before. Same source data
// (buildFixHandoffBlocks(aiReview.inlineFindings), src/queue/processors.ts), just routed by severity.
const blockerFixHandoffBlocks = args.fixHandoffBlocks?.filter((block) => block.severity === "blocker") ?? [];
const input = buildUnifiedReviewInput({
changedFiles: args.changedFiles,
reviews,
Expand All @@ -760,6 +765,7 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string
...(args.maxFindingsCaps !== undefined ? { maxFindingsCaps: args.maxFindingsCaps } : {}),
...(args.findingCategories !== undefined ? { inlineFindings: args.findingCategories } : {}),
...(args.linkedIssueSatisfaction !== undefined ? { linkedIssueSatisfaction: args.linkedIssueSatisfaction } : {}),
...(blockerFixHandoffBlocks.length > 0 ? { blockerFixContext: blockerFixHandoffBlocks } : {}),
});
// The gate already produced 0/1 reviewer notes from a synthesis of the model pair; reflect the caller's
// actual reviewer count (for the chip + the "N reviewers, synthesized" evidence) without re-deriving it.
Expand Down Expand Up @@ -818,11 +824,13 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string
const withImpactMap =
impactMapCollapsible !== null ? [...(withFindingCategories ?? []), impactMapCollapsible] : withFindingCategories;
// review.fixHandoff emission (#1962): when the operator flag AND the manifest opt in, the processor hands us
// the pre-rendered fix-handoff blocks here; append the "Fix handoff" collapsible after Impact map (another
// structural, no-AI section) and ahead of the visual preview. Flag-OFF (the processor passes undefined) ⇒
// extraCollapsibles is unchanged.
const fixHandoffCollapsible =
args.fixHandoffBlocks && args.fixHandoffBlocks.length > 0 ? buildFixHandoffCollapsible(args.fixHandoffBlocks) : null;
// the pre-rendered fix-handoff blocks here. #6068: blocker-severity blocks now render per-blocker instead
// (blockerFixHandoffBlocks above, threaded into `input.blockerFixContext`) -- this collapsible carries only
// the nit-severity remainder, after Impact map (another structural, no-AI section) and ahead of the visual
// preview. Flag-OFF, or every block was blocker-severity, (the processor passes undefined / an empty
// remainder) ⇒ extraCollapsibles is unchanged.
const nitFixHandoffBlocks = args.fixHandoffBlocks?.filter((block) => block.severity === "nit") ?? [];
const fixHandoffCollapsible = nitFixHandoffBlocks.length > 0 ? buildFixHandoffCollapsible(nitFixHandoffBlocks) : null;
const withFixHandoff =
fixHandoffCollapsible !== null ? [...(withImpactMap ?? []), fixHandoffCollapsible] : withImpactMap;
// Advisory-only AI-vision analysis of visual captures (#4111): recovered from the SAME advisory findings
Expand Down
25 changes: 25 additions & 0 deletions src/review/unified-comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,20 @@ export interface UnifiedReviewInput {
summary: string;
/** Consensus blocking issues (shown expanded when present). */
blockers?: string[];
/** Structured, per-finding fix context for blocker-severity inline findings (#6068) — one entry per finding
* with a commentable location, each already rendered (by `buildFixHandoffBlock`,
* src/review/fix-handoff-render.ts) into a copy-paste-ready markdown block (location + instruction +
* suggested diff). Rendered as its own "Copy AI fix context" collapsible right after each blocker,
* mirroring CodeRabbit's per-finding "Prompt for AI Agents" pattern — the whole-PR "Copy for AI agents"
* block above stays as the aggregate option. Structural shape (just `.body`) so the host can pass
* `FixHandoffBlock[]` without this renderer importing that type — stays self-contained. NOT correlated
* with the `blockers` strings above (they come from separate sources — gate hard-blockers, review-thread
* findings, and this AI-findings source do not share one array) — rendered as its own supplementary group
* after the blockers list, not matched 1:1 to a specific bullet. Absent/empty (default; the host only
* passes these when `review.fixHandoff` is on AND a fresh review produced blocker-severity inline
* findings) ⇒ no section, byte-identical. `path`/`line` are only used to label each collapsible so
* multiple entries stay distinguishable while collapsed. */
blockerFixContext?: ReadonlyArray<{ path: string; line?: number; body: string }>;
/** Non-blocking suggestions (collapsed). */
nits?: string[];
/** CI + merge-state readiness. */
Expand Down Expand Up @@ -743,6 +757,15 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi
blocks.push(buildAiContextBlock(blockersAll, collapsiblesOpen));
}

// Per-finding "Copy AI fix context" (#6068): one collapsible per blocker-severity inline finding, each a
// self-contained copy-paste-ready block (location + instruction + suggested diff) for a contributor's own
// local coding agent -- the CodeRabbit-style per-finding companion to the whole-PR block above. Never
// gated by verbosity, same rationale as the blockers section itself.
for (const entry of input.blockerFixContext ?? []) {
const location = entry.line && entry.line > 0 ? `${entry.path}:${entry.line}` : entry.path;
blocks.push(details("🔧 Copy AI fix context", entry.body, location, collapsiblesOpen));
}

// Category breakdown (#2150): a compact, deterministic one-liner of the finding mix (e.g. "2 correctness ·
// 1 security"). Omitted entirely when no finding carries a category (default) ⇒ byte-identical. Pure tally, no
// AI, no gate impact.
Expand Down Expand Up @@ -830,6 +853,7 @@ export function buildUnifiedReviewInput(opts: {
maxFindingsCaps?: { blockers: number | null; nits: number | null };
linkedIssueSatisfaction?: { status: "addressed" | "partial" | "unaddressed"; rationale: string };
inlineFindings?: ReadonlyArray<{ category?: UnifiedFindingCategory | undefined }>;
blockerFixContext?: ReadonlyArray<{ path: string; line?: number; body: string }>;
}): UnifiedReviewInput {
const ex = extractReviewSummary(opts.reviews);
const changedFiles = typeof opts.changedFiles === "number" ? opts.changedFiles : opts.changedFiles.length;
Expand All @@ -850,6 +874,7 @@ export function buildUnifiedReviewInput(opts: {
...(opts.maxFindingsCaps !== undefined ? { maxFindingsCaps: opts.maxFindingsCaps } : {}),
...(opts.linkedIssueSatisfaction !== undefined ? { linkedIssueSatisfaction: opts.linkedIssueSatisfaction } : {}),
...(opts.inlineFindings !== undefined ? { inlineFindings: opts.inlineFindings } : {}),
...(opts.blockerFixContext !== undefined ? { blockerFixContext: opts.blockerFixContext } : {}),
};
}

Expand Down
83 changes: 83 additions & 0 deletions test/unit/unified-comment-bridge.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,89 @@ describe("buildUnifiedCommentBody", () => {
expect(body).toContain("<details><summary><b>Signal definitions</b></summary>"); // extraCollapsibles
});

describe("fixHandoffBlocks severity split (#6068)", () => {
const blockerBlock = {
path: "src/foo.ts",
line: 10,
severity: "blocker" as const,
instruction: "Null check missing.",
body: "<!-- loopover:fix-handoff -->\n**Fix handoff — Blocker at `src/foo.ts:10`**\nNull check missing.",
boundary: "Local execution only.",
};
const nitBlock = {
path: "src/bar.ts",
line: 20,
severity: "nit" as const,
instruction: "Consider renaming.",
body: "<!-- loopover:fix-handoff -->\n**Fix handoff — Nit at `src/bar.ts:20`**\nConsider renaming.",
boundary: "Local execution only.",
};

it("renders a blocker-severity block as its own 'Copy AI fix context' collapsible, and a nit-severity block inside 'Fix handoff'", () => {
const body = buildUnifiedCommentBody({
gate: gate(),
panelRows,
readinessTotal: 70,
changedFiles: 2,
footerMarkdown: footer,
fixHandoffBlocks: [blockerBlock, nitBlock],
});
expect(body).toContain("<details><summary><b>🔧 Copy AI fix context</b> — src/foo.ts:10</summary>");
expect(body).toContain("Null check missing.");
expect(body).toContain("<details><summary><b>Fix handoff</b></summary>");
expect(body).toContain("Consider renaming.");
// The blocker-severity instruction must NOT also leak into the combined "Fix handoff" collapsible body.
const fixHandoffIndex = body.indexOf("<details><summary><b>Fix handoff</b></summary>");
const fixHandoffEnd = body.indexOf("</details>", fixHandoffIndex);
expect(body.slice(fixHandoffIndex, fixHandoffEnd)).not.toContain("Null check missing.");
});

it("omits the 'Fix handoff' collapsible entirely when every block is blocker-severity", () => {
const body = buildUnifiedCommentBody({
gate: gate(),
panelRows,
readinessTotal: 70,
changedFiles: 2,
footerMarkdown: footer,
fixHandoffBlocks: [blockerBlock],
});
expect(body).toContain("🔧 Copy AI fix context");
expect(body).not.toContain("<details><summary><b>Fix handoff</b></summary>");
});

it("omits the per-blocker 'Copy AI fix context' collapsible entirely when every block is nit-severity", () => {
const body = buildUnifiedCommentBody({
gate: gate(),
panelRows,
readinessTotal: 70,
changedFiles: 2,
footerMarkdown: footer,
fixHandoffBlocks: [nitBlock],
});
expect(body).not.toContain("🔧 Copy AI fix context");
expect(body).toContain("<details><summary><b>Fix handoff</b></summary>");
});

it("renders neither section when fixHandoffBlocks is absent (default, byte-identical)", () => {
const body = buildUnifiedCommentBody({ gate: gate(), panelRows, readinessTotal: 70, changedFiles: 2, footerMarkdown: footer });
expect(body).not.toContain("🔧 Copy AI fix context");
expect(body).not.toContain("Fix handoff");
});

it("labels the collapsible with just the path when the finding has no commentable line (line: 0 sentinel)", () => {
const body = buildUnifiedCommentBody({
gate: gate(),
panelRows,
readinessTotal: 70,
changedFiles: 2,
footerMarkdown: footer,
fixHandoffBlocks: [{ ...blockerBlock, line: 0 }],
});
expect(body).toContain("<details><summary><b>🔧 Copy AI fix context</b> — src/foo.ts</summary>");
expect(body).not.toContain("src/foo.ts:0");
});
});

// #4589: generateTestsLabel is a SEPARATE explicit field on BuildUnifiedCommentBodyArgs (not implicitly
// forwarded) — a prior version of this bridge silently dropped it because only reRunLabel was allowlisted
// here, so the checkbox never appeared in a real webhook-posted comment despite the renderer itself
Expand Down
47 changes: 47 additions & 0 deletions test/unit/unified-comment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,53 @@ describe("renderUnifiedReviewComment", () => {
expect(md).not.toContain("Safe summary </details>");
expect(md).not.toContain("Body <!-- comment -->");
});

describe("blockerFixContext (#6068)", () => {
it("renders one 'Copy AI fix context' collapsible per entry, labeled with its path:line", () => {
const md = renderUnifiedReviewComment({
...base,
decision: "close",
blockers: ["Null check missing."],
blockerFixContext: [
{ path: "src/foo.ts", line: 10, body: "**Fix handoff — Blocker at `src/foo.ts:10`**\nNull check missing." },
{ path: "src/bar.ts", line: 20, body: "**Fix handoff — Blocker at `src/bar.ts:20`**\nAnother defect." },
],
});
expect(md).toContain("<details><summary><b>🔧 Copy AI fix context</b> — src/foo.ts:10</summary>");
expect(md).toContain("<details><summary><b>🔧 Copy AI fix context</b> — src/bar.ts:20</summary>");
expect(md.match(/🔧 Copy AI fix context/g)?.length).toBe(2);
});

it("labels the collapsible with just the path when line is absent or the 0 no-line sentinel", () => {
const withoutLine = renderUnifiedReviewComment({ ...base, decision: "close", blockerFixContext: [{ path: "src/foo.ts", body: "x" }] });
expect(withoutLine).toContain("— src/foo.ts</summary>");
expect(withoutLine).not.toContain("src/foo.ts:0");
const zeroLine = renderUnifiedReviewComment({ ...base, decision: "close", blockerFixContext: [{ path: "src/foo.ts", line: 0, body: "x" }] });
expect(zeroLine).toContain("— src/foo.ts</summary>");
expect(zeroLine).not.toContain("src/foo.ts:0");
});

it("renders independently of the plain-text blockers list — present even with zero string blockers", () => {
const md = renderUnifiedReviewComment({ ...base, decision: "merge", blockerFixContext: [{ path: "src/foo.ts", line: 1, body: "x" }] });
expect(md).not.toContain("Why this is blocked");
expect(md).toContain("🔧 Copy AI fix context");
});

it("omits every 'Copy AI fix context' collapsible when absent (default, byte-identical)", () => {
const md = renderUnifiedReviewComment({ ...base, decision: "close", blockers: ["x"] });
expect(md).not.toContain("🔧 Copy AI fix context");
});

it("angle-escapes blockerFixContext body content (public-safe)", () => {
const md = renderUnifiedReviewComment({
...base,
decision: "close",
blockerFixContext: [{ path: "src/foo.ts", line: 1, body: "Suggested fix </details><!-- hidden -->" }],
});
expect(md).toContain("Suggested fix &lt;/details&gt;&lt;!-- hidden --&gt;");
expect(md).not.toContain("Suggested fix </details>");
});
});
});

describe("'Copy for AI agents' block", () => {
Expand Down