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
25 changes: 18 additions & 7 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" };
});
}

Expand Down Expand Up @@ -746,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 @@ -757,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 @@ -815,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
88 changes: 80 additions & 8 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 @@ -203,6 +217,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, …). */
Expand Down Expand Up @@ -587,27 +607,59 @@ 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
? `${input.reviewerCount} reviewers, synthesized`
: 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");
}
Expand Down Expand Up @@ -705,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 All @@ -726,7 +787,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
Expand Down Expand Up @@ -783,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 @@ -803,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
5 changes: 3 additions & 2 deletions test/unit/queue-4.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2874,8 +2874,9 @@ describe("queue processors", () => {
expect(postedBody).toContain("<!-- gittensory-pr-panel:v1 -->");
// 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
Expand Down
Loading