From 784b2cd352d34e6c5e44a3bd13d6e68fcfacf90c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 18:20:18 -0700 Subject: [PATCH] feat(review): advisory-only AI-vision analysis of before/after captures Add a text|image content-block union (AiContentBlock) to both AI message-construction paths -- selfhost/ai.ts::toMessages (the live production path) and every services/ai-review.ts content: call site, including the dual-AI tie-break judge, so a split visual verdict can receive the same screenshots the two reviewers saw instead of falling back to text-only reasoning. The HTTP providers (OpenAI-compatible, Anthropic) translate blocks to their native image shape; the subscription CLIs (claude-code/codex) degrade to text-only since they cannot consume inline image bytes through stdin. Add review/visual/visual-findings.ts: pure gating (pixel-diff threshold via the existing diff-overlay URL, submitter reputation, BYOK) plus prompt/response/finding-construction helpers for an advisory-only visual-regression finding. The finding rides the exact same advisory-findings pipeline ai_consensus_defect/ai_review_split use and is not one of the codes isConfiguredGateBlocker recognizes, so it can never become a gate blocker. Wire a new "Visual findings" collapsible into the unified PR comment, recovered from advisoryFindings the same way the consensus defect is recovered, and excluded from the generic Nits list so it renders exactly once. --- src/review/unified-comment-bridge.ts | 44 ++++- src/review/visual/visual-findings.ts | 152 +++++++++++++++++ src/selfhost/ai.ts | 64 +++++-- src/services/ai-review.ts | 68 +++++++- src/types.ts | 17 ++ test/unit/ai-review.test.ts | 116 +++++++++++++ test/unit/selfhost-ai.test.ts | 73 ++++++++ test/unit/unified-comment-bridge.test.ts | 98 +++++++++++ test/unit/visual-findings.test.ts | 204 +++++++++++++++++++++++ 9 files changed, 817 insertions(+), 19 deletions(-) create mode 100644 src/review/visual/visual-findings.ts create mode 100644 test/unit/visual-findings.test.ts diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 1f975dbf21..281a4f1252 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -23,6 +23,7 @@ import type { GateCheckConclusion, GateCheckEvaluation } from "../rules/advisory import type { PublicPrPanelSignalRow } from "../signals/engine"; import { formatManifestValidationNotice } from "../signals/focus-manifest"; import type { CaptureRoute } from "./visual/capture"; +import { VISUAL_REGRESSION_FINDING_CODE } from "./visual/visual-findings"; // Single-source the panel marker from its canonical home (the upsert reads it there); re-export so existing // importers of `PR_PANEL_COMMENT_MARKER` from this module keep working. The unified body MUST prepend this // verbatim or `createOrUpdatePrIntelligenceComment` posts a DUPLICATE instead of updating in place. @@ -227,8 +228,12 @@ export function buildDualReviewNotes(args: { // raw warning findings). Scrub each with the private-term boundary and DROP any that still leaks. See // PRIVATE_FORBIDDEN_TERMS above. (The consensus-defect blocker is already public-safe via toPublicSafe; the // gate blockers above go through the SAME scrub as Nits.) + // `visual_regression_finding` is excluded here the same way `ai_consensus_defect` is excluded from + // gateBlockerLines above — it renders in its OWN "Visual findings" collapsible (see + // `visualFindingsFromFindings`/`buildVisualFindingsCollapsible`), so folding it into generic Nits too would + // render it twice. const gateNits = (args.warnings ?? []) - .filter((warning) => !isBoilerplateNit(warning)) + .filter((warning) => !isBoilerplateNit(warning) && warning.code !== VISUAL_REGRESSION_FINDING_CODE) .map((warning) => `${warning.title}${warning.action ? ` — ${warning.action}` : ""}`.trim()) .filter(Boolean) .map((line) => publicSafeNit(line)) @@ -263,6 +268,21 @@ export function consensusDefectFromFindings(findings: AdvisoryFinding[] | undefi return { title: found.title, detail: found.detail }; } +/** Recover the advisory-only visual-regression findings (#4111 — AI-vision analysis of before/after visual + * captures) from the SAME advisory findings array `consensusDefectFromFindings` reads above — feeding the + * identical pipeline every other AI-judgment finding rides, so a visual finding is suppressible by + * review.memory, audited the same way, and — critically — can NEVER become a gate blocker: + * `visual_regression_finding` is not one of the codes `isConfiguredGateBlocker` (src/rules/advisory.ts) + * recognizes, so it always stays a warning. Formatted `title: detail`, scrubbed through the same + * `publicSafeNit` defense-in-depth boundary as every other bridge-recovered string. */ +export function visualFindingsFromFindings(findings: AdvisoryFinding[] | undefined): string[] { + return (findings ?? []) + .filter((finding) => finding.code === VISUAL_REGRESSION_FINDING_CODE) + .map((finding) => `${finding.title}: ${finding.detail}`.trim()) + .map((line) => publicSafeNit(line)) + .filter((line): line is string => line !== null); +} + function formatConsensusDefectBlocker(defect: { title: string; detail: string }): string { const title = defect.title.trim(); const detail = defect.detail.trim(); @@ -375,6 +395,19 @@ export type UnifiedCommentBridgeArgs = { linkedIssueSatisfaction?: { status: "addressed" | "partial" | "unaddressed"; rationale: string } | undefined; }; +/** + * Build the "Visual findings" collapsible (#4111) from the advisory-only visual-regression observations + * `visualFindingsFromFindings` recovered — one bullet per finding. Rendered ahead of "Visual preview" so the + * AI's read of the screenshots leads the raw before/after table a maintainer would otherwise have to eyeball + * themselves. Returns null when there are none, so the caller can unconditionally chain this alongside the + * other optional collapsibles (byte-identical for every review where no vision call ran). + */ +export function buildVisualFindingsCollapsible(findings: string[]): UnifiedCollapsible | null { + if (findings.length === 0) return null; + const body = findings.map((finding) => `- ${finding}`).join("\n"); + return { title: "Visual findings", body }; +} + /** * Build the "Visual preview" collapsible from the before/after capture routes — a clean table whose cells are * CLICKABLE THUMBNAILS: a small `` (GitHub caps it to the column width) wrapped in an `` to the @@ -761,10 +794,17 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string args.fixHandoffBlocks && args.fixHandoffBlocks.length > 0 ? buildFixHandoffCollapsible(args.fixHandoffBlocks) : null; const withFixHandoff = fixHandoffCollapsible !== null ? [...(withImpactMap ?? []), fixHandoffCollapsible] : withImpactMap; + // Advisory-only AI-vision analysis of visual captures (#4111): recovered from the SAME advisory findings + // array the consensus defect is recovered from, so an untouched (no vision call ran) review is unaffected — + // `visualFindingsFromFindings` returns `[]` unless a caller actually appended a `visual_regression_finding`. + const visualFindings = visualFindingsFromFindings(args.advisoryFindings); + const visualFindingsCollapsible = visualFindings.length > 0 ? buildVisualFindingsCollapsible(visualFindings) : null; + const withVisualFindings = + visualFindingsCollapsible !== null ? [...(withFixHandoff ?? []), visualFindingsCollapsible] : withFixHandoff; // Visual-capture port: when before/after routes are present, append a "Visual preview" collapsible to the // extra sections. Flag-OFF (the processor passes no beforeAfter) ⇒ extraCollapsibles is unchanged. const visualCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildBeforeAfterCollapsible(args.beforeAfter) : null; - const withVisual = visualCollapsible !== null ? [...(withFixHandoff ?? []), visualCollapsible] : withFixHandoff; + const withVisual = visualCollapsible !== null ? [...(withVisualFindings ?? []), visualCollapsible] : withVisualFindings; // #3612: "Scroll preview" renders ALONGSIDE "Visual preview" (never replacing it) — self-host + gif:true // only, so this is null (no section, no behavior change) for every repo that hasn't opted in. const scrollCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildScrollPreviewCollapsible(args.beforeAfter) : null; diff --git a/src/review/visual/visual-findings.ts b/src/review/visual/visual-findings.ts new file mode 100644 index 0000000000..ad491f16b7 --- /dev/null +++ b/src/review/visual/visual-findings.ts @@ -0,0 +1,152 @@ +// Advisory-only AI-vision analysis of before/after visual captures (#4111, part of the visual-capture +// convergence epic #3607). PURE decision + prompt/response logic ONLY — this module never fetches screenshot +// bytes, calls an AI provider, or touches D1; a caller supplies already-resolved images (as +// `AiContentBlock[]`, see `../../types`), a resolved BYOK provider key, and a resolved reputation signal, so +// this file stays testable without network or D1 fixtures. Wiring a live caller — fetch the captured PNG +// bytes, resolve submitter reputation + BYOK, invoke `callAiProvider`/the self-host AI with the images, and +// append the resulting finding to `advisory.findings` — is a deliberately deferred follow-up (see the #4111 +// PR description); this module ships the gating + message-shape + parsing + finding-construction it needs. +// +// STRICTLY ADVISORY: `VISUAL_REGRESSION_FINDING_CODE` is not one of the codes `isConfiguredGateBlocker` +// (src/rules/advisory.ts) recognizes, so a visual finding can NEVER become a gate blocker — it rides the +// identical `advisory.findings` pipeline `ai_consensus_defect`/`ai_review_split` already use, recovered in the +// unified comment exactly like a consensus defect (see `review/unified-comment-bridge.ts`'s +// `visualFindingsFromFindings`), but there is no code path that promotes it to `blockers`. + +import type { AdvisoryFinding } from "../../types"; +import { extractLastJsonObject, toPublicSafe, type AiReviewProviderKey } from "../../services/ai-review"; +import type { ReputationSignal } from "../submitter-reputation"; +import type { CaptureRoute } from "./capture"; + +/** The advisory finding code a visual-regression observation is published under (#4111). Deliberately absent + * from `isConfiguredGateBlocker`'s allowlist (src/rules/advisory.ts) — see this file's header. */ +export const VISUAL_REGRESSION_FINDING_CODE = "visual_regression_finding"; + +/** Bound on how many routes a single review ever sends to vision, independent of how many the capture + * pipeline rendered — a vision call is the most expensive AI request this codebase makes per-route (an + * image attachment, not just text), so an unbounded capture set must never translate into unbounded spend. */ +const MAX_VISION_ROUTES = 2; + +/** + * True when a captured route crossed the EXISTING pixel-diff change threshold (the visual-agent pixel-diff + * module's `changeThresholdPercent`) — surfaced here via the diff-overlay URL, since `uploadDiffImage` + * (`./capture.ts`) only ever populates `diffUrl`/`diffUrlMobile` for a route `compareRouteScreenshots` + * classified `"changed"`. An "unchanged" route (no diff URL on either viewport) is excluded, so a PR that + * touches web-visible files but renders pixel-identical before/after spends zero vision tokens — no NEW + * threshold is introduced here. (Not imported directly — this file only reads the ALREADY-COMPUTED diffUrl + * field, keeping worker-reachable code free of the Node-only pixel-diff dependency; see + * test/unit/worker-entry-boundary.test.ts.) + */ +export function routeHasConfirmedVisualRegression(route: CaptureRoute): boolean { + return Boolean(route.diffUrl || route.diffUrlMobile); +} + +/** The (bounded) subset of captured routes worth a vision call: only those confirmed changed by the existing + * pixel-diff threshold, capped at {@link MAX_VISION_ROUTES}. */ +export function selectRoutesForVisualVision(routes: readonly CaptureRoute[]): CaptureRoute[] { + return routes.filter(routeHasConfirmedVisualRegression).slice(0, MAX_VISION_ROUTES); +} + +/** Why {@link evaluateVisualVisionGate} declined to run the vision call — observability-only; never public. */ +export type VisualVisionSkipReason = "no_confirmed_regression" | "low_reputation" | "byok_not_configured"; + +export type VisualVisionGateResult = + | { run: false; reason: VisualVisionSkipReason } + | { run: true; routes: CaptureRoute[] }; + +/** + * Decide whether a visual-vision call is warranted for this review — ALL THREE must clear: + * 1. pixel-diff threshold — at least one route the capture pipeline already flagged "changed" (see + * {@link selectRoutesForVisualVision}); an all-unchanged capture costs nothing. + * 2. submitter reputation — a "low" windowed reputation signal (`../submitter-reputation.ts`) skips vision + * exactly like the other AI neurons already skip for a low-reputation/burst submitter + * (`shouldSkipAiForReputation`, `../reputation-wire.ts`); checked FIRST so a low-reputation submitter is + * never even told which reason applies to their capture. + * 3. BYOK — vision rides the maintainer's OWN provider key (`providerKey` non-null): Workers AI is fully + * retired (no free vision-capable path exists) and the self-host subscription CLIs (claude-code/codex) + * cannot consume inline image bytes through their stdin-JSON invocation (see `../../selfhost/ai.ts`'s + * `contentText`), so only an HTTP BYOK provider (anthropic/openai) can actually see the screenshots. + * Pure + total: the caller resolves the reputation signal / provider key (D1 + decryption both live outside + * this file) and passes the results in. + */ +export function evaluateVisualVisionGate(input: { + routes: readonly CaptureRoute[]; + reputationSignal: ReputationSignal; + providerKey: AiReviewProviderKey | null; +}): VisualVisionGateResult { + if (input.reputationSignal === "low") return { run: false, reason: "low_reputation" }; + if (!input.providerKey) return { run: false, reason: "byok_not_configured" }; + const routes = selectRoutesForVisualVision(input.routes); + if (routes.length === 0) return { run: false, reason: "no_confirmed_regression" }; + return { run: true, routes }; +} + +/** One vision observation the model reported for a specific route — both fields already public-safe (see + * {@link parseVisualVisionResponse}). */ +export type VisualVisionFinding = { path: string; body: string }; + +/** Cap on findings kept from a single vision response — mirrors `composeAdvisoryNotes`'s selectivity so a + * verbose model can't pad the comment with a long list of minor observations. */ +const MAX_VISUAL_FINDINGS = 3; + +export const VISUAL_VISION_SYSTEM_PROMPT = [ + "You are reviewing a BEFORE (production) vs AFTER (this pull request's preview deploy) screenshot pair for the same route.", + 'Respond with ONLY a JSON object of this exact shape (no prose, no code fence): {"findings": [{"path": string, "body": string}]}.', + "Report a finding ONLY for a genuine, visually-confirmable regression introduced by the AFTER screenshot — broken layout,", + "overlapping/clipped/unstyled content, a missing or misplaced element, unreadable contrast, or obvious placeholder content.", + "Each body is ONE sentence, specific to what you SEE (not what the diff pixels imply). Do NOT report a color/spacing/copy", + "change that still looks like a normal, intentional design update. Return an empty findings array when the AFTER screenshot", + "looks like a legitimate, correctly-rendered page. Never mention rewards, payouts, wallets, hotkeys, coldkeys, or trust scores.", +].join(" "); + +/** Build the user-turn text naming the route(s) under review, ahead of their image content blocks — the + * caller attaches the actual before/after images (see `../../types`'s `AiContentBlock`); this module only + * builds the text half of the request. */ +export function buildVisualVisionUserPrompt(routes: readonly { path: string }[]): string { + const paths = routes.map((route) => `- ${route.path}`).join("\n"); + return `Route(s) under review:\n${paths}\n\nEach route's images are attached in before, after order.`; +} + +/** Parse the model's structured vision response into public-safe findings, dropping anything unparseable, a + * blank path/body, or a body that trips the public/private boundary (`toPublicSafe`). Bounded to + * {@link MAX_VISUAL_FINDINGS}. Never throws — an unparseable response degrades to `[]`, the same fail-safe + * convention `parseModelReview` uses. */ +export function parseVisualVisionResponse(text: string): VisualVisionFinding[] { + const raw = extractLastJsonObject(text); + if (!raw) return []; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return []; + } + const findingsRaw = (parsed as { findings?: unknown } | null)?.findings; + if (!Array.isArray(findingsRaw)) return []; + const out: VisualVisionFinding[] = []; + for (const entry of findingsRaw) { + if (out.length >= MAX_VISUAL_FINDINGS) break; + if (!entry || typeof entry !== "object") continue; + const record = entry as Record; + const path = typeof record.path === "string" ? record.path.trim() : ""; + const rawBody = typeof record.body === "string" ? record.body : ""; + const body = toPublicSafe(rawBody); + if (!path || !body) continue; + out.push({ path, body }); + } + return out; +} + +/** Build the ADVISORY-ONLY findings for the unified comment (#4111) — one per vision observation, feeding the + * SAME `advisory.findings` pipeline `ai_consensus_defect`/`ai_review_split` already ride (see this file's + * header for why `visual_regression_finding` can never become a blocker). `severity: "warning"` is required, + * not incidental — `evaluateGateCheckCore` (src/rules/advisory.ts) only carries `"warning"`-severity findings + * into `gate.warnings` at all, so anything else would silently vanish from the rendered comment. */ +export function buildVisualRegressionFindings(findings: readonly VisualVisionFinding[]): AdvisoryFinding[] { + return findings.map((finding) => ({ + code: VISUAL_REGRESSION_FINDING_CODE, + severity: "warning", + title: `Possible visual regression: ${finding.path}`, + detail: finding.body, + action: "Advisory only — verify against the Visual preview screenshots before deciding.", + })); +} diff --git a/src/selfhost/ai.ts b/src/selfhost/ai.ts index 3ab07a2c41..6b473ea47e 100644 --- a/src/selfhost/ai.ts +++ b/src/selfhost/ai.ts @@ -6,7 +6,7 @@ // review proceeds deterministically. Every path returns `{ response: string }` (or throws → the caller // records an error and degrades — never a silent wrong answer). -import type { CombineStrategy, OnMerge } from "../services/ai-review"; +import type { AiContentBlock, CombineStrategy, OnMerge } from "../services/ai-review"; import { isConfiguredSelfHostProvider, resolveConfiguredProviderNames } from "./ai-config"; export { assertNoLegacySharedAiEnv } from "./ai-config"; import { incr } from "./metrics"; @@ -14,7 +14,12 @@ import { withReviewSpan } from "./tracing"; import { delimiter } from "node:path"; interface AiRunOptions { - messages?: Array<{ role: string; content: string }>; + // Content is a plain string for every message any pre-#4111 caller ever built (byte-identical). A + // pixel-diff-confirmed visual-vision call (review/visual/visual-findings.ts) instead sends a text+image + // content-block array for the user turn — only the two HTTP providers below (createOpenAiCompatibleAi / + // createAnthropicAi) can actually forward an image to the model; the subscription CLIs degrade to text-only + // (see `contentText`). + messages?: Array<{ role: string; content: string | AiContentBlock[] }>; prompt?: string; systemAppend?: string; text?: string[]; // embedding input — the core's embedTexts passes { text: string[] } @@ -57,11 +62,23 @@ export interface SelfHostAi { run(model: string, options: AiRunOptions): Promise; } -function toMessages(options: AiRunOptions): Array<{ role: string; content: string }> { +function toMessages(options: AiRunOptions): Array<{ role: string; content: string | AiContentBlock[] }> { if (Array.isArray(options.messages)) return options.messages; return [{ role: "user", content: String(options.prompt ?? "") }]; } +/** Plain-text projection of a message's content — extracts and joins ONLY the `text` blocks, dropping any + * `image` block. The subscription CLIs (claude-code/codex) build their prompt by piping flattened text to + * stdin (see `toCliPrompt` below), so an image block has nowhere to go in that invocation; a string content + * passes through unchanged (byte-identical to every pre-#4111 call). */ +function contentText(content: string | AiContentBlock[]): string { + if (typeof content === "string") return content; + return content + .filter((block): block is Extract => block.type === "text") + .map((block) => block.text) + .join(""); +} + function normalizedSystemAppend(options: AiRunOptions): string | undefined { const trimmed = options.systemAppend?.trim(); return trimmed ? trimmed : undefined; @@ -75,11 +92,10 @@ function stripSystemAppend(content: string, systemAppend: string): string { function toCliPrompt(options: AiRunOptions, systemAppend: string | undefined): string { return toMessages(options) - .map((message) => - systemAppend && message.role === "system" - ? stripSystemAppend(message.content, systemAppend) - : message.content, - ) + .map((message) => { + const text = contentText(message.content); + return systemAppend && message.role === "system" ? stripSystemAppend(text, systemAppend) : text; + }) .join("\n\n"); } @@ -216,6 +232,18 @@ function resolveOpenAiCompatibleRepoOverride(providerName: string, options: AiRu return options.openaiCompatibleModel; } +/** Translate the generic {@link AiContentBlock} union into OpenAI chat-completions' native content-part shape + * (`{type:"image_url", image_url:{url:"data:;base64,"}}`) — a string message passes through + * unchanged (byte-identical to every pre-#4111 call). */ +function toOpenAiMessageContent(content: string | AiContentBlock[]): string | Array> { + if (typeof content === "string") return content; + return content.map((block) => + block.type === "image" + ? { type: "image_url", image_url: { url: `data:${block.mimeType};base64,${block.data}` } } + : { type: "text", text: block.text }, + ); +} + /** OpenAI-compatible endpoint (Ollama's /v1, OpenAI, vLLM, LM Studio, …) — chat + embeddings. */ export function createOpenAiCompatibleAi(opts: { baseUrl: string; @@ -250,7 +278,7 @@ export function createOpenAiCompatibleAi(opts: { headers: headers(), body: JSON.stringify({ model: resolvedModel, - messages: toMessages(options), + messages: toMessages(options).map((message) => ({ role: message.role, content: toOpenAiMessageContent(message.content) })), max_tokens: options.max_tokens, temperature: options.temperature, }), @@ -264,6 +292,18 @@ export function createOpenAiCompatibleAi(opts: { }; } +/** Translate the generic {@link AiContentBlock} union into Anthropic's native Messages-API content-part shape + * (`{type:"image", source:{type:"base64", media_type, data}}`) — a string message passes through unchanged + * (byte-identical to every pre-#4111 call). */ +function toAnthropicMessageContent(content: string | AiContentBlock[]): string | Array> { + if (typeof content === "string") return content; + return content.map((block) => + block.type === "image" + ? { type: "image", source: { type: "base64", media_type: block.mimeType, data: block.data } } + : { type: "text", text: block.text }, + ); +} + /** Native Anthropic Messages API (BYOK — bills your Anthropic API key; distinct from the claude-code * subscription path). The system message becomes the top-level `system` param; the rest map to user/assistant. */ export function createAnthropicAi(opts: { apiKey: string; model?: string | undefined; baseUrl?: string | undefined }): SelfHostAi { @@ -274,9 +314,11 @@ export function createAnthropicAi(opts: { apiKey: string; model?: string | undef const system = msgs .filter((m) => m.role === "system") - .map((m) => m.content) + .map((m) => contentText(m.content)) .join("\n\n") || undefined; - const messages = msgs.filter((m) => m.role !== "system").map((m) => ({ role: m.role === "assistant" ? "assistant" : "user", content: m.content })); + const messages = msgs + .filter((m) => m.role !== "system") + .map((m) => ({ role: m.role === "assistant" ? "assistant" : "user", content: toAnthropicMessageContent(m.content) })); // Repo override > construction-time env-resolved opts.model (#3902), same priority as the OpenAI-compatible // providers above and the CLI providers' claudeModel/codexModel. const resolvedModel = resolveModel(firstConfigured(options.anthropicModel, opts.model), model, "claude-sonnet-5"); diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 2a92563c1c..2fb2130415 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -34,7 +34,7 @@ import type { ReviewProfile } from "../signals/focus-manifest"; import { isCodeFile } from "../signals/local-branch"; import { isTestPath } from "../signals/test-evidence"; import { isFindingCategory, type FindingCategory } from "../review/finding-category-classify"; -import type { CombineStrategy, OnMerge } from "../types"; +import type { AiContentBlock, CombineStrategy, OnMerge } from "../types"; /** * The legacy free Workers-AI model pair — used ONLY when neither a self-host `AI_REVIEW_PLAN` reviewer @@ -93,7 +93,7 @@ export type AiReviewProviderKey = { // Cloudflare Workers types (`Env`, `D1Database`, …) this file's runtime code depends on — a type-only // `import("../services/ai-review")` reference from either would still drag this whole module graph into the UI's // typecheck and break it (#2567 follow-up fix). See ../types.ts for the full doc comment. -export type { CombineStrategy, OnMerge } from "../types"; +export type { AiContentBlock, CombineStrategy, OnMerge } from "../types"; /** * Resolve the EFFECTIVE `onMerge` rule for a review call, enforcing that a per-repo `.gittensory.yml @@ -442,6 +442,36 @@ function selfHostCliSystemAppend(model: string, systemAppend: string): string | return provider === "claude-code" || provider === "codex" ? trimmed : undefined; } +/** Build a message's `content` — plain text (BYTE-IDENTICAL, the only shape any call site sent before #4111) + * when no images are attached, or a text+image content-block array when the caller supplies pixel-diff- + * confirmed screenshots. See `review/visual/visual-findings.ts` for the gating that decides when `images` is + * ever non-empty; every existing caller of the functions below passes no `images`, so this is inert today. */ +function toContentBlocks(text: string, images?: readonly AiContentBlock[] | undefined): string | AiContentBlock[] { + if (!images || images.length === 0) return text; + return [{ type: "text", text }, ...images]; +} + +/** Translate the generic {@link AiContentBlock} union into Anthropic's native Messages-API content-part shape + * (`{type:"image", source:{type:"base64", media_type, data}}`) — the ONLY provider-specific step, since the + * block's `text`/`data`/`mimeType` fields already carry everything Anthropic's wire format needs. */ +function toAnthropicContentBlocks(blocks: readonly AiContentBlock[]): Array> { + return blocks.map((block) => + block.type === "image" + ? { type: "image", source: { type: "base64", media_type: block.mimeType, data: block.data } } + : { type: "text", text: block.text }, + ); +} + +/** Translate the generic {@link AiContentBlock} union into OpenAI chat-completions' native content-part shape + * (`{type:"image_url", image_url:{url:"data:;base64,"}}`). */ +function toOpenAiContentBlocks(blocks: readonly AiContentBlock[]): Array> { + return blocks.map((block) => + block.type === "image" + ? { type: "image_url", image_url: { url: `data:${block.mimeType};base64,${block.data}` } } + : { type: "text", text: block.text }, + ); +} + // Exported so the sibling AI-advisory features (e.g. the slop advisory in `./ai-slop`) share ONE budget // window + neuron estimator and never drift from the review path's accounting. export function isEnabled(value: string | undefined): boolean { @@ -902,6 +932,10 @@ async function runWorkersOpinion( diagnostics: AiReviewDiagnostic[] = [], systemAppend = "", correlation?: AiRunCorrelation, + // Pixel-diff-confirmed screenshot(s) for a visual-vision pass (#4111). Absent for every existing caller — + // wiring a real caller (source images, invoke with them) is a deliberately deferred follow-up; see + // review/visual/visual-findings.ts. + images?: readonly AiContentBlock[] | undefined, ): Promise { const ai = env.AI as unknown as AiRunner | undefined; if (!ai || typeof ai.run !== "function") return { review: null }; @@ -933,7 +967,7 @@ async function runWorkersOpinion( temperature: 0, messages: [ { role: "system", content: system }, - { role: "user", content: user }, + { role: "user", content: toContentBlocks(user, images) }, ], ...(cliSystemAppend ? { systemAppend: cliSystemAppend } : {}), ...(correlation?.jobId !== undefined ? { jobId: correlation.jobId } : {}), @@ -1146,9 +1180,19 @@ export async function callAiProvider( system: string, user: string, maxTokens: number, + // Pixel-diff-confirmed screenshot(s) for a visual-vision pass (#4111). Absent for every existing caller + // (byte-identical `content: user` string body); vision rides the maintainer's OWN BYOK key since Workers AI + // is retired — see review/visual/visual-findings.ts for the gating that decides when this is ever non-empty. + images?: readonly AiContentBlock[] | undefined, ): Promise<{ text: string | null; usage?: AiReviewActualUsage | undefined; failure?: ProviderFailure }> { const model = providerKey.model || PROVIDER_DEFAULT_MODEL[providerKey.provider]; + const userContent: string | Array> = + images && images.length > 0 + ? providerKey.provider === "anthropic" + ? toAnthropicContentBlocks([{ type: "text", text: user }, ...images]) + : toOpenAiContentBlocks([{ type: "text", text: user }, ...images]) + : user; try { let response: Response; if (providerKey.provider === "anthropic") { @@ -1163,7 +1207,7 @@ export async function callAiProvider( model, max_tokens: maxTokens, system, - messages: [{ role: "user", content: user }], + messages: [{ role: "user", content: userContent }], }), signal: AbortSignal.timeout(AI_PROVIDER_TIMEOUT_MS), }); @@ -1179,7 +1223,7 @@ export async function callAiProvider( max_tokens: maxTokens, messages: [ { role: "system", content: system }, - { role: "user", content: user }, + { role: "user", content: userContent }, ], }), signal: AbortSignal.timeout(AI_PROVIDER_TIMEOUT_MS), @@ -1205,12 +1249,14 @@ async function runProviderReview( system: string, user: string, maxTokens: number, + images?: readonly AiContentBlock[] | undefined, ): Promise { const { text, usage, failure } = await callAiProvider( providerKey, system, user, maxTokens, + images, ); const model = providerKey.model || PROVIDER_DEFAULT_MODEL[providerKey.provider]; if (failure) return { review: null, failure, diagnostic: { model, attempt: 0, status: "provider_error", error: failure } }; @@ -1576,6 +1622,10 @@ async function runDualAiTieBreakJudgeCall( swapped: boolean, diagnostics: AiReviewDiagnostic[], correlation?: AiRunCorrelation, + // Pixel-diff-confirmed screenshot(s) (#4111): when the two reviewers SPLIT on a visual-capture PR, the judge + // gets the SAME images the reviewers saw so its verdict isn't text-only reasoning about a visual defect. + // Absent for every existing caller — byte-identical `content: user` string. + images?: readonly AiContentBlock[] | undefined, ): Promise<{ verdict: DualAiTieBreakVerdict; consensusTitle?: string | undefined } | null> { const ai = env.AI as unknown as AiRunner | undefined; if (!ai || typeof ai.run !== "function") return null; @@ -1598,7 +1648,7 @@ async function runDualAiTieBreakJudgeCall( temperature: 0, messages: [ { role: "system", content: TIE_BREAK_JUDGE_SYSTEM_PROMPT }, - { role: "user", content: user }, + { role: "user", content: toContentBlocks(user, images) }, ], ...(correlation?.jobId !== undefined ? { jobId: correlation.jobId } : {}), ...(correlation?.repoFullName !== undefined @@ -1656,6 +1706,10 @@ async function resolveDualAiTieBreakWithOrderStability(input: { reviewB: ModelReview; diagnostics: AiReviewDiagnostic[]; correlation?: AiRunCorrelation | undefined; + // Pixel-diff-confirmed screenshot(s) (#4111), handed to BOTH the normal- and swapped-order judge calls so + // the order-swap stability check still compares the SAME visual evidence either way. Absent for every + // existing caller — byte-identical to today. + images?: readonly AiContentBlock[] | undefined; }): Promise<{ stable: boolean; verdict: DualAiTieBreakVerdict; @@ -1672,6 +1726,7 @@ async function resolveDualAiTieBreakWithOrderStability(input: { false, input.diagnostics, input.correlation, + input.images, ); const swappedOrder = await runDualAiTieBreakJudgeCall( input.env, @@ -1682,6 +1737,7 @@ async function resolveDualAiTieBreakWithOrderStability(input: { true, input.diagnostics, input.correlation, + input.images, ); if (!normalOrder || !swappedOrder) { return { stable: false, verdict: "inconclusive", orderUnstable: false }; diff --git a/src/types.ts b/src/types.ts index 93b0a22570..20cb9d00f2 100644 --- a/src/types.ts +++ b/src/types.ts @@ -636,6 +636,23 @@ export type CombineStrategy = "single" | "consensus" | "synthesis"; * {@link CombineStrategy} for why the canonical definition lives here rather than `services/ai-review.ts`. */ export type OnMerge = "either" | "both"; +/** + * A multimodal content block for an AI provider message (#4111 — advisory-only AI-vision analysis of + * before/after visual captures). Canonical definition lives here for the same UI-safety reason as + * {@link CombineStrategy} above: this type carries no ambient Workers/Node types, so any file that needs it + * (including the UI workspace) can import it without dragging in `Env`/`D1Database`. + * • `text` — plain prompt text. The only content kind any message ever carried before this issue, so a + * message whose content is a plain `string` (never an array) is byte-identical to today. + * • `image` — a base64-encoded screenshot (`data`, no `data:` URI prefix) + its MIME type. Attached ONLY for + * a route the EXISTING pixel-diff threshold already confirmed changed (see + * `review/visual/visual-findings.ts`'s gating) — an unchanged route never costs a vision token. A + * provider that cannot consume images (the self-host subscription CLIs — see `selfhost/ai.ts`'s + * `contentText`) drops image blocks and sends the text blocks alone rather than failing the call. + */ +export type AiContentBlock = + | { type: "text"; text: string } + | { type: "image"; data: string; mimeType: string }; + export const MAX_CONTRIBUTOR_OPEN_ITEM_CAP = 100; export type RepositorySettings = { diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index de0becb43c..8b3eafcc77 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3,9 +3,11 @@ import { __aiReviewInternals, BEST_REVIEW_MODELS, buildTestEvidencePromptSection, + callAiProvider, resolveEffectiveAiReviewOnMerge, resolveEffectiveAiReviewPlan, runGittensoryAiReview, + type AiContentBlock, type GittensoryAiReviewInput, } from "../../src/services/ai-review"; import { createTestEnv } from "../helpers/d1"; @@ -1221,6 +1223,58 @@ describe("BYOK provider dispatch", () => { }); }); +describe("callAiProvider content-block union (#4111 — advisory-only visual-vision analysis)", () => { + const image: AiContentBlock = { type: "image", data: "QUJD", mimeType: "image/png" }; + + it("sends a plain string user message when no images are supplied (byte-identical to today)", async () => { + let body: Record | undefined; + vi.stubGlobal( + "fetch", + vi.fn(async (_url: RequestInfo | URL, init?: RequestInit) => { + body = JSON.parse(init?.body as string) as Record; + return new Response(JSON.stringify({ content: [{ type: "text", text: "ok" }] }), { status: 200 }); + }), + ); + await callAiProvider({ provider: "anthropic", key: "sk-ant" }, "sys", "user text", 256); + const messages = body?.messages as Array<{ content: unknown }>; + expect(messages[0]?.content).toBe("user text"); + }); + + it("attaches an image content block to the Anthropic user message, in Anthropic's native shape", async () => { + let body: Record | undefined; + vi.stubGlobal( + "fetch", + vi.fn(async (_url: RequestInfo | URL, init?: RequestInit) => { + body = JSON.parse(init?.body as string) as Record; + return new Response(JSON.stringify({ content: [{ type: "text", text: "ok" }] }), { status: 200 }); + }), + ); + await callAiProvider({ provider: "anthropic", key: "sk-ant" }, "sys", "user text", 256, [image]); + const messages = body?.messages as Array<{ content: unknown }>; + expect(messages[0]?.content).toEqual([ + { type: "text", text: "user text" }, + { type: "image", source: { type: "base64", media_type: "image/png", data: "QUJD" } }, + ]); + }); + + it("attaches an image content block to the OpenAI user message, in OpenAI's native shape", async () => { + let body: Record | undefined; + vi.stubGlobal( + "fetch", + vi.fn(async (_url: RequestInfo | URL, init?: RequestInit) => { + body = JSON.parse(init?.body as string) as Record; + return new Response(JSON.stringify({ choices: [{ message: { content: "ok" } }] }), { status: 200 }); + }), + ); + await callAiProvider({ provider: "openai", key: "sk-secret" }, "sys", "user text", 256, [image]); + const messages = body?.messages as Array<{ role: string; content: unknown }>; + expect(messages[1]?.content).toEqual([ + { type: "text", text: "user text" }, + { type: "image_url", image_url: { url: "data:image/png;base64,QUJD" } }, + ]); + }); +}); + describe("Workers AI fallback + degraded output", () => { it("tries the per-slot fallback model then withholds unparseable output from public notes", async () => { const run = vi.fn(async (_model: string) => ({ @@ -2221,6 +2275,50 @@ describe("pure helpers", () => { }); }); + it("REGRESSION (#4111): runDualAiTieBreakJudgeCall attaches supplied images to the judge's user message; omits them (plain string) when absent", async () => { + const seenContents: unknown[] = []; + const run = vi.fn(async (_model: string, payload: { messages?: Array<{ role: string; content: unknown }> }) => { + seenContents.push(payload.messages?.[1]?.content); + return { response: JSON.stringify({ favored: "reviewer_0" }) }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const images = [{ type: "image" as const, data: "QUJD", mimeType: "image/png" }]; + await runDualAiTieBreakJudgeCall(env, "primary-model", "", blockedA, clean, false, [], undefined, images); + expect(seenContents[0]).toEqual([ + { type: "text", text: buildDualAiTieBreakJudgeUserPrompt(blockedA, clean, false) }, + { type: "image", data: "QUJD", mimeType: "image/png" }, + ]); + await runDualAiTieBreakJudgeCall(env, "primary-model", "", blockedA, clean, false, []); + expect(typeof seenContents[1]).toBe("string"); + }); + + it("REGRESSION (#4111): a SPLIT verdict's tie-break judge receives the SAME images on both the normal- and swapped-order calls", async () => { + const seenContents: unknown[] = []; + const run = vi.fn(async (_model: string, payload: { messages?: Array<{ role: string; content: unknown }> }) => { + seenContents.push(payload.messages?.[1]?.content); + return { response: JSON.stringify({ favored: "reviewer_0" }) }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const images = [{ type: "image" as const, data: "QUJD", mimeType: "image/png" }]; + await resolveDualAiTieBreakWithOrderStability({ + env, + model: "primary-model", + fallback: "primary-model", + reviewA: blockedA, + reviewB: clean, + diagnostics: [], + images, + }); + // One call for the normal order, one for the swapped order — BOTH must have seen the image. + expect(seenContents).toHaveLength(2); + for (const content of seenContents) { + expect(Array.isArray(content)).toBe(true); + expect(content).toEqual( + expect.arrayContaining([{ type: "image", data: "QUJD", mimeType: "image/png" }]), + ); + } + }); + it("swap-stable consensus tie-break resolves conflicting blockers via judge title", async () => { resetMetrics(); let aiCalls = 0; @@ -2809,6 +2907,24 @@ describe("pure helpers", () => { expect(run).toHaveBeenCalledTimes(1); }); + it("REGRESSION (#4111): runWorkersOpinion attaches supplied images to the user message; omits them (plain string) when absent", async () => { + const seenContents: unknown[] = []; + const run = vi.fn(async (_model: string, options: Record) => { + const messages = options.messages as Array<{ content: unknown }>; + seenContents.push(messages[1]?.content); + return { response: reviewJson() }; + }); + const env = createTestEnv({ AI: { run } as unknown as Ai }); + const images = [{ type: "image" as const, data: "QUJD", mimeType: "image/png" }]; + await runWorkersOpinion(env, "m", "m", "sys", "user text", 256, [], "", undefined, images); + expect(seenContents[0]).toEqual([ + { type: "text", text: "user text" }, + { type: "image", data: "QUJD", mimeType: "image/png" }, + ]); + await runWorkersOpinion(env, "m", "m", "sys", "user text", 256); + expect(seenContents[1]).toBe("user text"); + }); + it("runWorkersOpinion stops retrying a model after ONE subscription_cli_timeout, but the fallback still gets its full retry budget (#gaming-tactic-draft-cycle)", async () => { let primaryAttempts = 0; const run = vi.fn(async (model: string) => { diff --git a/test/unit/selfhost-ai.test.ts b/test/unit/selfhost-ai.test.ts index bcf61e2f3a..0192faa04a 100644 --- a/test/unit/selfhost-ai.test.ts +++ b/test/unit/selfhost-ai.test.ts @@ -265,6 +265,79 @@ describe("createAnthropicAi (#979 native BYOK)", () => { }); }); +describe("content-block union (#4111 — text|image messages, advisory-only visual-vision analysis)", () => { + it("createOpenAiCompatibleAi translates an image content block to OpenAI's image_url shape, alongside the text block", async () => { + let body: { messages: unknown } | undefined; + vi.stubGlobal("fetch", vi.fn(async (_u: string, init: { body: string }) => { + body = JSON.parse(init.body) as { messages: unknown }; + return new Response(JSON.stringify({ choices: [{ message: { content: "ok" } }] }), { status: 200 }); + })); + await createOpenAiCompatibleAi({ baseUrl: "http://o/v1" }).run("m", { + messages: [{ role: "user", content: [{ type: "text", text: "look at this" }, { type: "image", data: "QUJD", mimeType: "image/png" }] }], + }); + expect(body?.messages).toEqual([ + { role: "user", content: [{ type: "text", text: "look at this" }, { type: "image_url", image_url: { url: "data:image/png;base64,QUJD" } }] }, + ]); + }); + + it("createOpenAiCompatibleAi passes a plain string content through unchanged (no images attached — byte-identical)", async () => { + let body: { messages: unknown } | undefined; + vi.stubGlobal("fetch", vi.fn(async (_u: string, init: { body: string }) => { + body = JSON.parse(init.body) as { messages: unknown }; + return new Response(JSON.stringify({ choices: [{ message: { content: "ok" } }] }), { status: 200 }); + })); + await createOpenAiCompatibleAi({ baseUrl: "http://o/v1" }).run("m", { messages: [{ role: "user", content: "plain text" }] }); + expect(body?.messages).toEqual([{ role: "user", content: "plain text" }]); + }); + + it("createAnthropicAi translates an image content block to Anthropic's base64 image shape, alongside the text block", async () => { + let body: Record | undefined; + vi.stubGlobal("fetch", vi.fn(async (_u: string, init: { body: string }) => { + body = JSON.parse(init.body) as Record; + return new Response(JSON.stringify({ content: [{ type: "text", text: "ok" }] }), { status: 200 }); + })); + await createAnthropicAi({ apiKey: "sk-ant" }).run("m", { + messages: [ + { role: "system", content: "be terse" }, + { role: "user", content: [{ type: "text", text: "look at this" }, { type: "image", data: "QUJD", mimeType: "image/png" }] }, + ], + }); + expect(body?.system).toBe("be terse"); + expect(body?.messages).toEqual([ + { role: "user", content: [{ type: "text", text: "look at this" }, { type: "image", source: { type: "base64", media_type: "image/png", data: "QUJD" } }] }, + ]); + }); + + it("createAnthropicAi extracts only the text blocks when the SYSTEM message is itself a content-block array", async () => { + let body: Record | undefined; + vi.stubGlobal("fetch", vi.fn(async (_u: string, init: { body: string }) => { + body = JSON.parse(init.body) as Record; + return new Response(JSON.stringify({ content: [{ type: "text", text: "ok" }] }), { status: 200 }); + })); + await createAnthropicAi({ apiKey: "sk-ant" }).run("m", { + messages: [ + { role: "system", content: [{ type: "text", text: "be terse" }, { type: "image", data: "ZZZ", mimeType: "image/png" }] }, + { role: "user", content: "go" }, + ], + }); + expect(body?.system).toBe("be terse"); + expect(body?.messages).toEqual([{ role: "user", content: "go" }]); + }); + + it("the CLI subscription providers (codex/claude-code) degrade an image content block to text-only for the stdin prompt (images have nowhere to go through stdin JSON)", async () => { + let capturedInput = ""; + const ok: StubSpawn = async (_cmd, _args, opts) => { + capturedInput = opts.input ?? ""; + return { stdout: JSON.stringify({ type: "result", result: "ok" }), code: 0 }; + }; + await createCodexAi({ GITTENSORY_ENABLE_UNSAFE_CODEX_REVIEWER: "1" }, ok, noAuthCheck).run("", { + messages: [{ role: "user", content: [{ type: "text", text: "look at this" }, { type: "image", data: "QUJD", mimeType: "image/png" }] }], + }); + expect(capturedInput).toBe("look at this"); + expect(capturedInput).not.toContain("QUJD"); + }); +}); + describe("createChainAi (fallback)", () => { it("falls through to the next provider on failure, returns the first success", async () => { const failing = { name: "a", ai: { run: async () => { throw new Error("down"); } } }; diff --git a/test/unit/unified-comment-bridge.test.ts b/test/unit/unified-comment-bridge.test.ts index 46256aa3de..f26a9449c5 100644 --- a/test/unit/unified-comment-bridge.test.ts +++ b/test/unit/unified-comment-bridge.test.ts @@ -3,6 +3,7 @@ import { buildClosedUnifiedCommentBody, buildDualReviewNotes, buildUnifiedCommentBody, + buildVisualFindingsCollapsible, consensusDefectFromFindings, gateConclusionToVerdict, isUnifiedReviewCommentEnabled, @@ -11,7 +12,9 @@ import { PR_PANEL_COMMENT_MARKER, splitAiReviewNits, verdictToRecommendation, + visualFindingsFromFindings, } from "../../src/review/unified-comment-bridge"; +import { VISUAL_REGRESSION_FINDING_CODE } from "../../src/review/visual/visual-findings"; import { PR_PANEL_COMMENT_MARKER as MARKER_FROM_COMMENTS } from "../../src/github/comments"; import { deriveUnifiedStatus, type MergeReadiness, type UnifiedCollapsible, type UnifiedCommentStatus } from "../../src/review/unified-comment"; import type { GateCheckEvaluation } from "../../src/rules/advisory"; @@ -92,6 +95,46 @@ describe("consensusDefectFromFindings", () => { }); }); +describe("visualFindingsFromFindings (#4111 — advisory-only AI-vision analysis)", () => { + it("recovers only visual_regression_finding entries, formatted 'title: detail', ignoring other codes", () => { + const findings: AdvisoryFinding[] = [ + { code: "missing_linked_issue", severity: "warning", title: "No linked issue", detail: "..." }, + { code: VISUAL_REGRESSION_FINDING_CODE, severity: "warning", title: "Possible visual regression: /pricing", detail: "The third column lost its border." }, + ]; + expect(visualFindingsFromFindings(findings)).toEqual([ + "Possible visual regression: /pricing: The third column lost its border.", + ]); + expect(visualFindingsFromFindings([])).toEqual([]); + expect(visualFindingsFromFindings(undefined)).toEqual([]); + }); + + it("scrubs a private term out of a visual finding before it reaches the public comment (privacy invariant)", () => { + const findings: AdvisoryFinding[] = [ + { code: VISUAL_REGRESSION_FINDING_CODE, severity: "warning", title: "Possible visual regression: /pricing", detail: "Your trust score looks broken here." }, + ]; + const [line] = visualFindingsFromFindings(findings); + expect(line).not.toMatch(/trust score/i); + expect(line).toContain("[context]"); + }); +}); + +describe("buildVisualFindingsCollapsible (#4111)", () => { + it("renders one bullet per finding", () => { + const c = buildVisualFindingsCollapsible([ + "Possible visual regression: /pricing: The third column lost its border.", + "Possible visual regression: /about: The hero image is missing.", + ]); + expect(c?.title).toBe("Visual findings"); + expect(c?.body).toBe( + "- Possible visual regression: /pricing: The third column lost its border.\n- Possible visual regression: /about: The hero image is missing.", + ); + }); + + it("returns null when there are no findings (no empty section)", () => { + expect(buildVisualFindingsCollapsible([])).toBeNull(); + }); +}); + describe("buildDualReviewNotes", () => { it("folds the advisory notes (assessment), the consensus defect (blocker), and warnings (nits) into one note", () => { const reviews = buildDualReviewNotes({ @@ -716,6 +759,45 @@ describe("gate blockers render in 'Why this is blocked' (FIX D1)", () => { }); }); +describe("buildUnifiedCommentBody: visual findings render in their OWN section, never duplicated as a generic Nit (#4111)", () => { + it("renders the 'Visual findings' collapsible from advisoryFindings and stays advisory-only (merge verdict unaffected)", () => { + const visualFinding: AdvisoryFinding = { + code: VISUAL_REGRESSION_FINDING_CODE, + severity: "warning", + title: "Possible visual regression: /pricing", + detail: "The third column lost its border.", + action: "Advisory only — verify against the Visual preview screenshots before deciding.", + }; + const body = buildUnifiedCommentBody({ + // A real evaluateGateCheck run would carry this "warning"-severity finding into gate.warnings too — + // simulated here so the exclusion-from-generic-Nits behavior is exercised the same way it is live. + gate: gate({ warnings: [visualFinding] }), + advisoryFindings: [visualFinding], + panelRows, + readinessTotal: 80, + changedFiles: 2, + footerMarkdown: footer, + }); + expect(body).toContain("Visual findings"); + expect(body).toContain("Possible visual regression: /pricing: The third column lost its border."); + // Never duplicated into the generic Nits collapsible. + expect(body.split("Possible visual regression: /pricing").length - 1).toBe(1); + // Strictly advisory: a "warning"-severity, non-blocker finding never turns a passing gate into anything else. + expect(body).toContain("Suggested Action - Approve/Merge"); + }); + + it("omits the 'Visual findings' section entirely when no visual finding is present (byte-identical to today)", () => { + const body = buildUnifiedCommentBody({ + gate: gate(), + panelRows, + readinessTotal: 80, + changedFiles: 2, + footerMarkdown: footer, + }); + expect(body).not.toContain("Visual findings"); + }); +}); + describe("verdictReason on a held/blocked headline (FIX D2)", () => { it("appends the gate summary to a BLOCKED (close) verdict headline", () => { const body = buildUnifiedCommentBody({ @@ -967,6 +1049,22 @@ describe("buildDualReviewNotes — public-safe Nit scrub (privacy-critical, gate expect(nit, `"${term}" must not leak`).not.toContain(term); } }); + + it("excludes a visual_regression_finding warning from Nits (#4111 — it renders in its own 'Visual findings' collapsible instead)", () => { + const reviews = buildDualReviewNotes({ + aiReview: { notes: "Reviewer assessment." }, + warnings: [ + { code: VISUAL_REGRESSION_FINDING_CODE, severity: "warning", title: "Possible visual regression: /pricing", detail: "The third column lost its border." }, + { code: "w2", severity: "warning", title: "Add a unit test", detail: "...", action: "Cover the new branch." }, + ], + recommendation: "manual_review", + verdict: "manual", + }); + const nits = reviews[0]?.notes?.nits ?? []; + expect(nits).toHaveLength(1); + expect(nits).not.toContain(expect.stringContaining("Possible visual regression")); + expect(nits).toContain("Add a unit test — Cover the new branch."); + }); }); describe("buildClosedUnifiedCommentBody (closed/skipped PR through the unified renderer)", () => { diff --git a/test/unit/visual-findings.test.ts b/test/unit/visual-findings.test.ts new file mode 100644 index 0000000000..e80117dea0 --- /dev/null +++ b/test/unit/visual-findings.test.ts @@ -0,0 +1,204 @@ +import { describe, expect, it } from "vitest"; +import { + buildVisualRegressionFindings, + buildVisualVisionUserPrompt, + evaluateVisualVisionGate, + parseVisualVisionResponse, + routeHasConfirmedVisualRegression, + selectRoutesForVisualVision, + VISUAL_REGRESSION_FINDING_CODE, +} from "../../src/review/visual/visual-findings"; +import type { CaptureRoute } from "../../src/review/visual/capture"; +import type { AiReviewProviderKey } from "../../src/services/ai-review"; +import { evaluateGateCheck } from "../../src/rules/advisory"; +import type { Advisory } from "../../src/types"; + +const changedRoute = (path: string): CaptureRoute => ({ + path, + beforeUrl: `https://api.example.dev/gittensory/shot?key=before-${path}`, + afterUrl: `https://api.example.dev/gittensory/shot?key=after-${path}`, + diffUrl: `https://api.example.dev/gittensory/shot?key=diff-${path}`, +}); +const unchangedRoute = (path: string): CaptureRoute => ({ + path, + beforeUrl: `https://api.example.dev/gittensory/shot?key=before-${path}`, + afterUrl: `https://api.example.dev/gittensory/shot?key=after-${path}`, +}); +const providerKey: AiReviewProviderKey = { provider: "anthropic", key: "sk-ant" }; + +describe("routeHasConfirmedVisualRegression", () => { + it("is true when the route has a desktop diff URL", () => { + expect(routeHasConfirmedVisualRegression(changedRoute("/pricing"))).toBe(true); + }); + + it("is true when ONLY the mobile diff URL is present", () => { + expect(routeHasConfirmedVisualRegression({ path: "/", diffUrlMobile: "https://x/shot?key=d" })).toBe(true); + }); + + it("is false for an unchanged route (no diff URL on either viewport)", () => { + expect(routeHasConfirmedVisualRegression(unchangedRoute("/about"))).toBe(false); + expect(routeHasConfirmedVisualRegression({ path: "/" })).toBe(false); + }); +}); + +describe("selectRoutesForVisualVision", () => { + it("filters out unchanged routes, keeping only pixel-diff-confirmed ones", () => { + const routes = [changedRoute("/a"), unchangedRoute("/b"), changedRoute("/c")]; + expect(selectRoutesForVisualVision(routes).map((r) => r.path)).toEqual(["/a", "/c"]); + }); + + it("caps the result at MAX_VISION_ROUTES even when more routes are confirmed changed", () => { + const routes = [changedRoute("/a"), changedRoute("/b"), changedRoute("/c")]; + expect(selectRoutesForVisualVision(routes).map((r) => r.path)).toEqual(["/a", "/b"]); + }); + + it("returns [] when no route is confirmed changed", () => { + expect(selectRoutesForVisualVision([unchangedRoute("/a")])).toEqual([]); + expect(selectRoutesForVisualVision([])).toEqual([]); + }); +}); + +describe("evaluateVisualVisionGate", () => { + it("skips for a low-reputation submitter, even with a confirmed regression and BYOK configured (checked FIRST)", () => { + expect( + evaluateVisualVisionGate({ routes: [changedRoute("/a")], reputationSignal: "low", providerKey }), + ).toEqual({ run: false, reason: "low_reputation" }); + }); + + it("skips when BYOK is not configured, even with a confirmed regression and good reputation", () => { + expect( + evaluateVisualVisionGate({ routes: [changedRoute("/a")], reputationSignal: "neutral", providerKey: null }), + ).toEqual({ run: false, reason: "byok_not_configured" }); + expect( + evaluateVisualVisionGate({ routes: [changedRoute("/a")], reputationSignal: "trusted", providerKey: null }), + ).toEqual({ run: false, reason: "byok_not_configured" }); + }); + + it("skips when no route crossed the pixel-diff threshold, even with good reputation and BYOK configured", () => { + expect( + evaluateVisualVisionGate({ routes: [unchangedRoute("/a")], reputationSignal: "neutral", providerKey }), + ).toEqual({ run: false, reason: "no_confirmed_regression" }); + }); + + it("runs, returning the bounded confirmed-regression routes, for a neutral- or trusted-reputation submitter with BYOK configured", () => { + const routes = [changedRoute("/a"), unchangedRoute("/b")]; + expect(evaluateVisualVisionGate({ routes, reputationSignal: "neutral", providerKey })).toEqual({ + run: true, + routes: [changedRoute("/a")], + }); + expect(evaluateVisualVisionGate({ routes, reputationSignal: "trusted", providerKey })).toEqual({ + run: true, + routes: [changedRoute("/a")], + }); + }); +}); + +describe("buildVisualVisionUserPrompt", () => { + it("renders one bullet per route path", () => { + const prompt = buildVisualVisionUserPrompt([{ path: "/pricing" }, { path: "/about" }]); + expect(prompt).toContain("- /pricing"); + expect(prompt).toContain("- /about"); + expect(prompt).toContain("before, after order"); + }); +}); + +describe("parseVisualVisionResponse", () => { + it("parses a valid findings array into public-safe entries", () => { + const text = JSON.stringify({ findings: [{ path: "/pricing", body: "The third column lost its border." }] }); + expect(parseVisualVisionResponse(text)).toEqual([{ path: "/pricing", body: "The third column lost its border." }]); + }); + + it("drops an entry with a blank path", () => { + const text = JSON.stringify({ findings: [{ path: " ", body: "Something broke." }] }); + expect(parseVisualVisionResponse(text)).toEqual([]); + }); + + it("drops an entry with a blank/empty body (fails toPublicSafe's emptiness guard)", () => { + const text = JSON.stringify({ findings: [{ path: "/pricing", body: "" }] }); + expect(parseVisualVisionResponse(text)).toEqual([]); + }); + + it("drops a non-object entry and a findings value that isn't an array", () => { + expect(parseVisualVisionResponse(JSON.stringify({ findings: ["just a string"] }))).toEqual([]); + expect(parseVisualVisionResponse(JSON.stringify({ findings: "not an array" }))).toEqual([]); + }); + + it("drops an entry whose path is not a string (coerces to the empty-string fallback, then fails the blank guard)", () => { + const text = JSON.stringify({ findings: [{ path: 123, body: "Something broke." }] }); + expect(parseVisualVisionResponse(text)).toEqual([]); + }); + + it("drops an entry whose body is missing/not a string (coerces to the empty-string fallback, then fails toPublicSafe)", () => { + const text = JSON.stringify({ findings: [{ path: "/pricing" }] }); + expect(parseVisualVisionResponse(text)).toEqual([]); + }); + + it("returns [] for text with no JSON object at all", () => { + expect(parseVisualVisionResponse("not json, just prose")).toEqual([]); + }); + + it("returns [] for a balanced-brace object that is still invalid JSON (e.g. a trailing comma)", () => { + // extractLastJsonObject only brace-matches — it happily extracts this SYNTACTICALLY invalid JSON (a + // trailing comma), so JSON.parse itself must throw and be caught. + expect(parseVisualVisionResponse('{"findings": [1,]}')).toEqual([]); + }); + + it("caps the result at MAX_VISUAL_FINDINGS even when the model returns more", () => { + const findings = Array.from({ length: 5 }, (_, i) => ({ path: `/r${i}`, body: `Issue ${i}.` })); + expect(parseVisualVisionResponse(JSON.stringify({ findings }))).toHaveLength(3); + }); +}); + +describe("buildVisualRegressionFindings", () => { + it("maps each vision finding into an advisory-only, non-blocking AdvisoryFinding", () => { + const findings = buildVisualRegressionFindings([{ path: "/pricing", body: "The third column lost its border." }]); + expect(findings).toEqual([ + { + code: VISUAL_REGRESSION_FINDING_CODE, + severity: "warning", + title: "Possible visual regression: /pricing", + detail: "The third column lost its border.", + action: "Advisory only — verify against the Visual preview screenshots before deciding.", + }, + ]); + }); + + it("returns [] for an empty findings list", () => { + expect(buildVisualRegressionFindings([])).toEqual([]); + }); +}); + +describe("REGRESSION (#4111): a visual-regression finding can NEVER become a gate blocker", () => { + it("stays in gate.warnings (never gate.blockers) and the gate conclusion stays 'success' regardless of policy", () => { + const advisory: Advisory = { + id: "advisory-visual", + targetType: "pull_request", + targetKey: "owner/repo#9", + repoFullName: "owner/repo", + pullNumber: 9, + headSha: "sha9", + conclusion: "neutral", + severity: "warning", + title: "Gittensory advisory available", + summary: "1 advisory finding generated.", + findings: buildVisualRegressionFindings([{ path: "/pricing", body: "The third column lost its border." }]), + generatedAt: "2026-07-07T00:00:00.000Z", + }; + // Even a maximally permissive/aggressive policy (every optional gate mode set to "block") must not promote + // visual_regression_finding — it simply is not one of the codes isConfiguredGateBlocker recognizes. + const result = evaluateGateCheck(advisory, { + confirmedContributor: true, + linkedIssueGateMode: "block", + duplicatePrGateMode: "block", + aiReviewGateMode: "block", + manifestPolicyGateMode: "block", + selfAuthoredLinkedIssueGateMode: "block", + linkedIssueSatisfactionGateMode: "block", + lockfileIntegrityGateMode: "block", + claGateMode: "block", + }); + expect(result.conclusion).toBe("success"); + expect(result.blockers).toEqual([]); + expect(result.warnings).toEqual(advisory.findings); + }); +});