From 2e7e3e2f7b88bbb7e84fa0e0405b443ad2835bf6 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 01:38:59 -0700 Subject: [PATCH 1/3] review: collapse the signal table into decision drivers + advisory fold The always-rendered 9-row Signal/Result/Evidence table only ever had two decision-authoritative rows (Code review, Gate result); the other seven already say "No action." / "Advisory only." in their own Evidence/Action text. Split it into an always-visible "Decision drivers" bullet list (only rows marked gates: true, plus the synthetic Code review row) and a collapsed "Context & advisory signals" fold for everything else -- same underlying data, same review.fields visibility, no longer competing for attention with the blockers. Closes #6067 --- src/review/unified-comment-bridge.ts | 7 ++- src/review/unified-comment.ts | 63 +++++++++++++++++++++--- test/unit/unified-comment-bridge.test.ts | 11 ++++- test/unit/unified-comment.test.ts | 31 ++++++++++-- 4 files changed, 96 insertions(+), 16 deletions(-) diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index f9745c61bc..9c8f213fcf 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -128,11 +128,14 @@ function rowResultText(resultCell: string): string { } /** Map the legacy panel signal rows → the unified table's rows (label/state/result/evidence). The - * unified renderer adds its own "Code review" row first; these follow it (loopover's gate row included). */ + * unified renderer adds its own "Code review" row first; these follow it (loopover's gate row included). + * `gates: true` only for the "Gate result" row (#6067) -- the ONLY row among these that can actually move + * the verdict; every other row's own Evidence/Action text already says it's advisory-only. Drives the split + * between the renderer's always-visible "Decision drivers" list and its collapsed advisory-signals fold. */ export function panelRowsToSignalRows(rows: PublicPrPanelSignalRow[]): UnifiedSignalRow[] { return rows.map((row) => { const [label, result, evidence] = row.cells; - return { label, state: rowState(result), result: rowResultText(result), evidence }; + return { label, state: rowState(result), result: rowResultText(result), evidence, gates: row.key === "gateResult" }; }); } diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 6a8c6f4388..6a60e8a032 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -203,6 +203,12 @@ export interface UnifiedSignalRow { result?: string; /** Evidence cell, e.g. "#1372". */ evidence?: string; + /** True only for a row that can actually change the verdict (today: just "Gate result" — see + * panelRowsToSignalRows, which sets this from the row's own `key === "gateResult"`). Every other row is + * advisory context by construction (#6067) — its own Evidence/Action text already says so (e.g. "No + * action.", "Advisory only."). Drives the split between the always-visible "Decision drivers" list and + * the collapsed "Context & advisory signals" fold. Absent/false ⇒ advisory. */ + gates?: boolean; } /** A collapsed section (loopover side: signal definitions, contributor next steps, …). */ @@ -587,7 +593,10 @@ function nonRequiredFailingChecksBlock(readiness: MergeReadiness | undefined): s return lines.join("\n"); } -function signalTable(input: UnifiedReviewInput, ctx: UnifiedCommentContext): string { +/** The synthetic "Code review" row -- derived from the AI reviewers' own blocker count, never from + * `ctx.signals` -- so it always exists and is never subject to `review.fields` visibility (matches its + * pre-#6067 behavior as the signal table's unconditional first row). */ +function codeReviewRow(input: UnifiedReviewInput): UnifiedSignalRow { const blockerCount = (input.blockers ?? []).length; const reviewerEvidence = input.reviewerCount > 1 @@ -595,19 +604,48 @@ function signalTable(input: UnifiedReviewInput, ctx: UnifiedCommentContext): str : input.reviewerCount === 1 ? "1 reviewer" : "No AI review summary"; - const codeRow: UnifiedSignalRow = { + return { label: "Code review", state: blockerCount ? "fail" : "ok", result: blockerCount ? plural(blockerCount, "blocker") : "No blockers", evidence: reviewerEvidence, }; - const rows = [codeRow, ...(ctx.signals ?? [])]; - const lines = rows.map((r, i) => { - const labelText = escapePublicHtmlAngles(r.label); - const label = i === 0 ? `**${labelText}**` : labelText; +} + +/** One "Decision drivers" bullet: `- {icon} {label} — {result} ({evidence})`. `evidence` is parenthesized + * only when present, matching how sparse the underlying data can legitimately be (e.g. an unconfigured gate). */ +function signalRowLine(r: UnifiedSignalRow): string { + const labelText = escapePublicHtmlAngles(r.label); + const resultText = r.result ? escapePublicHtmlAngles(r.result) : ""; + const evidenceText = r.evidence ? ` (${escapePublicHtmlAngles(r.evidence)})` : ""; + return `- ${SIGNAL_ICON[r.state]} ${labelText} — ${resultText}${evidenceText}`; +} + +/** The always-visible "Decision drivers" list (#6067): ONLY the rows that can actually move the verdict -- + * the synthetic Code review row, plus any host-supplied row marked `gates: true` (today: just "Gate + * result", see `panelRowsToSignalRows`). Replaces the old signal table's synthetic-first-row special case + * with an explicit, always-non-empty list (Code review alone is a valid, common case -- e.g. no gate + * configured for the repo). A short bullet list, not a table: this is meant to be scanned in one glance, + * not cross-referenced like the advisory rows below. */ +function decisionDriverBlock(input: UnifiedReviewInput, ctx: UnifiedCommentContext): string { + const rows = [codeReviewRow(input), ...(ctx.signals ?? []).filter((r) => r.gates)]; + const lines = rows.map((r) => signalRowLine(r)); + return `**Decision drivers**\n${lines.join("\n")}`; +} + +/** Every host-supplied row that is NOT `gates: true` -- advisory context that never moves the verdict (each + * row's own Evidence/Action text already says so, e.g. "No action.", "Advisory only."). Rendered as the + * same Signal/Result/Evidence table the pre-#6067 signal table used, just scoped to this subset and moved + * behind a fold (see the "Context & advisory signals" collapsible in renderUnifiedReviewComment) instead of + * always-visible -- most of what made the old always-rendered table feel noisy. "" when there is nothing + * advisory to show (e.g. the host passed no signals at all), so the caller can omit the section entirely. */ +function advisorySignalsTable(ctx: UnifiedCommentContext): string { + const rows = (ctx.signals ?? []).filter((r) => !r.gates); + if (rows.length === 0) return ""; + const lines = rows.map((r) => { const resultText = r.result ? ` ${escapePublicHtmlAngles(r.result)}` : ""; const result = `${SIGNAL_ICON[r.state]}${resultText}`; - return `| ${label} | ${result} | ${escapePublicHtmlAngles(r.evidence ?? "")} |`; + return `| ${escapePublicHtmlAngles(r.label)} | ${result} | ${escapePublicHtmlAngles(r.evidence ?? "")} |`; }); return ["| Signal | Result | Evidence |", "|---|---|---|", ...lines].join("\n"); } @@ -726,7 +764,16 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi blocks.push(details("Flagged checks (non-blocking)", nonRequiredFailingChecks, undefined, collapsiblesOpen)); } - blocks.push(signalTable(input, ctx)); + // #6067: the old always-rendered 9-row table is split into an always-visible "Decision drivers" list + // (only rows that can move the verdict) and a collapsed "Context & advisory signals" fold (everything + // else). Like the table it replaces, NEITHER is gated by `review.comment_verbosity: quiet` -- these are + // gate-relevant/context signals, not decorative detail (matches the file's existing verbosity contract: + // only Nits + extraCollapsibles are ever dropped by `quiet`). + blocks.push(decisionDriverBlock(input, ctx)); + const advisoryBody = advisorySignalsTable(ctx); + if (advisoryBody) { + blocks.push(details("Context & advisory signals", advisoryBody, "never blocks the verdict", collapsiblesOpen)); + } // Linked-issue satisfaction advisory (#2174): additive, collapsed section — omitted entirely when the host // never resolved a result (default) or `review.comment_verbosity: quiet` trims decorative detail, exactly diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 9b1f27b4f6..3d6ed45eb4 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -71,12 +71,21 @@ describe("panelRowsToSignalRows", () => { it("derives ok/warn/fail from the leading icon and strips it from the result text", () => { const rows = panelRowsToSignalRows(panelRows); const linked = rows.find((row) => row.label === "Linked issue"); - expect(linked).toEqual({ label: "Linked issue", state: "ok", result: "Linked", evidence: "#42" }); + // gates: false — only the "Gate result" row is ever decision-authoritative (#6067). + expect(linked).toEqual({ label: "Linked issue", state: "ok", result: "Linked", evidence: "#42", gates: false }); const reviewLoad = rows.find((row) => row.label === "Change scope"); expect(reviewLoad?.state).toBe("warn"); expect(reviewLoad?.result).toBe("14/20"); }); + it("marks ONLY the Gate result row as gates: true (#6067) — every other row is advisory", () => { + const rows = panelRowsToSignalRows(panelRows); + const gateResult = rows.find((row) => row.label === "Gate result"); + expect(gateResult?.gates).toBe(true); + const everythingElse = rows.filter((row) => row.label !== "Gate result"); + expect(everythingElse.every((row) => row.gates === false)).toBe(true); + }); + it("maps a ❌ result cell to fail", () => { const rows = panelRowsToSignalRows([{ key: "linkedIssue", cells: ["Linked issue", "❌ Missing linked issue", "no closes/fixes reference", "Link an issue."] }]); expect(rows[0]?.state).toBe("fail"); diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index c23aa43be7..21dc583f6e 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -153,7 +153,11 @@ describe("renderUnifiedReviewComment", () => { // second `> ` on top of asAlert's own per-line prefix, giving it a visually distinct bordered sub-block. expect(md).toContain("> > **✅ Suggested Action - Approve/Merge**"); expect(md).toContain("**Review summary**"); - expect(md).toContain("| **Code review** | ✅ No blockers | 2 reviewers, synthesized |"); + // #6067: the old always-rendered table is split into an always-visible "Decision drivers" bullet list + // (Code review + any gates:true row) and a collapsed "Context & advisory signals" fold (everything else). + expect(md).toContain("**Decision drivers**"); + expect(md).toContain("- ✅ Code review — No blockers (2 reviewers, synthesized)"); + expect(md).toContain("
Context & advisory signals — never blocks the verdict"); expect(md).toContain("| Linked issue | ✅ Linked | #1372 |"); expect(md).toContain("
Nits — 1 non-blocking"); expect(md).toContain("- [ ] Document the new property."); @@ -167,7 +171,7 @@ describe("renderUnifiedReviewComment", () => { it("does not describe a single reviewer as synthesized", () => { const md = renderUnifiedReviewComment({ ...base, reviewerCount: 1, decision: "manual", recommendations: ["manual_review"] }, ctx); expect(md).toContain("`1 AI reviewer`"); - expect(md).toContain("| **Code review** | ✅ No blockers | 1 reviewer |"); + expect(md).toContain("- ✅ Code review — No blockers (1 reviewer)"); expect(md).not.toContain("1 reviewers, synthesized"); }); @@ -228,7 +232,7 @@ describe("renderUnifiedReviewComment", () => { expect(md).toContain("Suggested Action - Reject/Close"); expect(md).toContain("Why this is blocked"); expect(md).toContain("Introduces a hardcoded secret."); - expect(md).toContain("| **Code review** | ❌ 1 blocker |"); + expect(md).toContain("- ❌ Code review — 1 blocker (2 reviewers, synthesized)"); // #6066: the advisory-only readiness score is hidden on a non-"ready" verdict — "readiness 93/100" next // to "fixes required" reads as contradictory, since the score never feeds the gate either way. expect(md).not.toContain("readiness 93/100"); @@ -529,6 +533,23 @@ describe("renderUnifiedReviewComment", () => { expect(md).toContain("| Bare row | ⚠️ | |"); }); + it("#6067: a gates:true row joins Code review in the always-visible Decision drivers list, not the advisory fold", () => { + const md = renderUnifiedReviewComment( + { ...base, decision: "close", blockers: ["x"] }, + { signals: [{ label: "Gate result", state: "fail", result: "Blocking", evidence: "linked issue required", gates: true }] }, + ); + expect(md).toContain("**Decision drivers**"); + expect(md).toContain("- ❌ Code review — 1 blocker (2 reviewers, synthesized)"); + expect(md).toContain("- ❌ Gate result — Blocking (linked issue required)"); + expect(md).not.toContain("Context & advisory signals"); // nothing advisory was supplied ⇒ the fold is omitted + }); + + it("#6067: a gates:true row with neither a result nor evidence still renders a bare Decision drivers bullet", () => { + const md = renderUnifiedReviewComment({ ...base, decision: "merge" }, { signals: [{ label: "Gate result", state: "ok", gates: true }] }); + expect(md).toContain("- ✅ Gate result — "); + expect(md).not.toContain("Gate result — ("); + }); + it("uses the 'Concerns raised' heading (not 'Why this is blocked') for blockers on a non-blocked status", () => { // a lone request_changes blocker → held, but the concern is still surfaced under the softer heading const md = renderUnifiedReviewComment({ ...base, recommendations: ["request_changes"], blockers: ["Edge case unhandled."], consensusBlocker: false }, {}); @@ -832,12 +853,12 @@ describe("review.comment_verbosity (#2047)", () => { }; const extraCtx: UnifiedCommentContext = { extraCollapsibles: [{ title: "Changed files", body: "src/a.ts +5" }] }; - it("quiet drops the Nits collapsible and every extra collapsible, but keeps blockers/signal table", () => { + it("quiet drops the Nits collapsible and every extra collapsible, but keeps blockers/decision drivers", () => { const md = renderUnifiedReviewComment(input, { ...extraCtx, commentVerbosity: "quiet" }); expect(md).not.toContain("Nits"); expect(md).not.toContain("Changed files"); expect(md).toContain("a real blocker"); - expect(md).toContain("**Code review**"); // signal table row always present + expect(md).toContain("**Decision drivers**"); // #6067: decision drivers always present, like the table before it }); it("quiet also drops the linked-issue satisfaction section (#2174)", () => { From 0779b3a6de635f986408425c1e5f359bfba6c8f2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 15 Jul 2026 02:12:44 -0700 Subject: [PATCH 2/3] test: fix queue-4 integration assertion for the Decision drivers format #6067 replaced the always-rendered table's bold "**Code review**" first row with the "Decision drivers" bullet list -- this end-to-end integration test (exercising the real renderer through processGitHubWebhook, not a scoped unit test) still asserted the old bold-table-row text. --- test/unit/queue-4.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/unit/queue-4.test.ts b/test/unit/queue-4.test.ts index 0304d639fe..030262249a 100644 --- a/test/unit/queue-4.test.ts +++ b/test/unit/queue-4.test.ts @@ -2874,8 +2874,9 @@ describe("queue processors", () => { expect(postedBody).toContain(""); // The UNIFIED shape, which the legacy body never emits: a full-comment GitHub alert wrapper… expect(postedBody).toMatch(/> \[!(TIP|NOTE|WARNING|CAUTION)\]/); - // …and the renderer's synthesized "Code review" signal row (bold first table label). - expect(postedBody).toContain("**Code review**"); + // …and the renderer's synthesized "Code review" decision-driver row (#6067: the always-visible + // "Decision drivers" bullet list, not a table row anymore). + expect(postedBody).toContain("- ✅ Code review — No blockers"); // Public-safe by construction — no internal trust/economics fields leak through the unified renderer. expect(postedBody).not.toMatch(/wallet|hotkey|reward|trust score/i); // #review-audit (#4220): the comment reads the LIVE `dirty` merge-state (not the stale stored one), so it must From c1f069543fa5a45c498588bfe53bba9ea16fd010 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 3/3] 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", () => {