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); + }); +});