From 6b99f6bf924ce81ed132e5b1fda8e043e95c38f5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:47:22 -0700 Subject: [PATCH] review: wire per-blocker AI fix-context, CodeRabbit-style The structured fix-context data (path/line/instruction/suggested diff) already existed via buildFixHandoffBlock, but only ever rendered into its own disconnected, all-severities-combined "Fix handoff" collapsible -- never attached to the blockers a reader is actually looking at. Split fixHandoffBlocks by severity: blocker-severity findings now render as their own "Copy AI fix context" collapsible directly under each blocker (still gated by the existing review.fixHandoff toggle); nit-severity findings keep going into the combined "Fix handoff" collapsible as before. The whole-PR "Copy for AI agents" prompt is unchanged and stays available as the aggregate option. Closes #6068 --- src/review/unified-comment-bridge.ts | 18 +++-- src/review/unified-comment.ts | 25 +++++++ test/unit/unified-comment-bridge.test.ts | 83 ++++++++++++++++++++++++ test/unit/unified-comment.test.ts | 47 ++++++++++++++ 4 files changed, 168 insertions(+), 5 deletions(-) diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 9c8f213fcf..e658d3ab84 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -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, @@ -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. @@ -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 diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 6a60e8a032..240bf7a907 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -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. */ @@ -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. @@ -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; @@ -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 } : {}), }; } diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 3d6ed45eb4..bbd4e2b100 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -543,6 +543,89 @@ describe("buildUnifiedCommentBody", () => { expect(body).toContain("
Signal definitions"); // extraCollapsibles }); + describe("fixHandoffBlocks severity split (#6068)", () => { + const blockerBlock = { + path: "src/foo.ts", + line: 10, + severity: "blocker" as const, + instruction: "Null check missing.", + body: "\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: "\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("
🔧 Copy AI fix context — src/foo.ts:10"); + expect(body).toContain("Null check missing."); + expect(body).toContain("
Fix handoff"); + expect(body).toContain("Consider renaming."); + // The blocker-severity instruction must NOT also leak into the combined "Fix handoff" collapsible body. + const fixHandoffIndex = body.indexOf("
Fix handoff"); + const fixHandoffEnd = body.indexOf("
", 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("
Fix handoff"); + }); + + 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("
Fix handoff"); + }); + + 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("
🔧 Copy AI fix context — src/foo.ts"); + 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 diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 21dc583f6e..ac2eb15903 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -591,6 +591,53 @@ describe("renderUnifiedReviewComment", () => { expect(md).not.toContain("Safe summary
"); expect(md).not.toContain("Body "); }); + + 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("
🔧 Copy AI fix context — src/foo.ts:10"); + expect(md).toContain("
🔧 Copy AI fix context — src/bar.ts:20"); + 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"); + 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"); + 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
" }], + }); + expect(md).toContain("Suggested fix </details><!-- hidden -->"); + expect(md).not.toContain("Suggested fix
"); + }); + }); }); describe("'Copy for AI agents' block", () => {