diff --git a/packages/gittensory-engine/src/review/screenshot-table-gate.ts b/packages/gittensory-engine/src/review/screenshot-table-gate.ts index aa90875ceb..60891c9476 100644 --- a/packages/gittensory-engine/src/review/screenshot-table-gate.ts +++ b/packages/gittensory-engine/src/review/screenshot-table-gate.ts @@ -186,7 +186,7 @@ const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|]*>/i; * one needs actual cell contents (not just "does some cell have an image"), so duplicating its short * header+separator detection loop keeps both independently simple instead of risking a regression in either * from a shared-code change. */ -function extractTableRows(body: string | null | undefined): string[][] { +export function extractTableRows(body: string | null | undefined): string[][] { if (!body) return []; const lines = body.split(/\r?\n/); const tableRowPattern = /^\s*\|.*\|\s*$/; @@ -215,6 +215,31 @@ function extractTableRows(body: string | null | undefined): string[][] { return rows; } +// Matches EITHER markdown image syntax (`![alt](url)`, optionally with a trailing `"title"`) OR an `` tag, capturing the URL from whichever alternative matched -- covers a bare `![]()` cell and the +// PR template's own clickable-thumbnail convention (`[![before](url)](url)`, where the OUTER `[...](...)` is +// the click-through link and this pattern correctly targets the INNER `!`-prefixed image markup instead). +const CELL_IMAGE_URL_PATTERN = /!\[[^\]]*\]\(([^)\s]+)(?:\s+"[^"]*")?\)|]*\bsrc=["']([^"']+)["'][^>]*>/i; + +function extractCellImageUrl(cell: string): string | null { + const match = cell.match(CELL_IMAGE_URL_PATTERN); + if (!match) return null; + /* v8 ignore next -- defensive: whichever alternative of CELL_IMAGE_URL_PATTERN matched always captures a + * non-empty group (both require at least one non-`)`/non-`"` character), so this fallback is unreachable. */ + return match[1] ?? match[2] ?? null; +} + +/** The image URLs found in each detected table row (source order), for rows with at least two — a real + * before/after pair worth comparing, not a single decorative image or caption-only row. Reuses + * {@link extractTableRows}'s own header+separator detection rather than re-scanning the body. A row with + * MORE than two images (e.g. a desktop+mobile matrix row) keeps every image; callers that only want a pair + * slice it themselves. */ +export function extractTableRowImageUrls(body: string | null | undefined): string[][] { + return extractTableRows(body) + .map((row) => row.map(extractCellImageUrl).filter((url): url is string => url !== null)) + .filter((urls) => urls.length >= 2); +} + /** One (viewport, theme) combination the matrix must cover. `theme: null` means the theme dimension isn't * required at all (a repo can require viewport coverage without color-mode coverage). */ export type ScreenshotMatrixPair = { viewport: string; theme: string | null }; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a937e2abd4..4fa7aee5c9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -567,7 +567,16 @@ import { } from "../review/linked-issue-hard-rules"; import { DEFAULT_UNLINKED_ISSUE_GUARDRAIL } from "../review/unlinked-issue-guardrail-config"; import { resolveUnlinkedIssueMatchDisposition } from "../review/unlinked-issue-guardrail"; -import { DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate } from "../review/screenshot-table-gate"; +import { DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, extractTableRowImageUrls } from "../review/screenshot-table-gate"; +import { isSafeHttpUrl } from "../review/content-lane/safe-url"; +import { + buildScreenshotTableVisionFindings, + buildScreenshotTableVisionUserPrompt, + evaluateScreenshotTableVisionGate, + parseScreenshotTableVisionResponse, + SCREENSHOT_TABLE_VISION_FINDING_CODE, + SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT, +} from "../review/visual/screenshot-table-vision"; import { isOpsEnabled, runOpsAlerts } from "../review/ops-wire"; import { isRecapEnabled, resolveMaintainerRecapManifestOverride, runMaintainerRecapJob } from "../review/maintainer-recap-wire"; import { isSweepWatchdogEnabled, runSweepLivenessWatchdog } from "../review/sweep-watchdog"; @@ -8748,6 +8757,146 @@ async function recordVisualVisionUsage( }); } +async function recordScreenshotTableVisionUsage( + env: Env, + args: { repoFullName: string; pr: { number: number }; author: string | null }, + providerKey: { provider: string }, + status: string, + detail: string, + usage?: AiReviewActualUsage | undefined, +): Promise { + await recordAiUsageEvent(env, { + feature: "screenshot_table_vision", + actor: args.author ?? null, + route: "github_app.screenshot_table_vision", + model: `byok:${providerKey.provider}`, + status, + estimatedNeurons: 0, + provider: usage?.provider, + effort: usage?.effort, + inputTokens: usage?.inputTokens, + outputTokens: usage?.outputTokens, + totalTokens: usage?.totalTokens, + costUsd: usage?.costUsd, + detail, + metadata: { repoFullName: args.repoFullName, pullNumber: args.pr.number }, + }); +} + +/** + * Vision-verify a contributor-pasted screenshot-table's images (#4366, part of #4325): screenshot-table-gate.ts's + * DETERMINISTIC check only verifies markdown STRUCTURE (a table exists with image-bearing cells), so a + * contributor can satisfy it with two identical images or a screenshot unrelated to the stated change. This + * adds that missing check on top, in the SAME two stages screenshot-table-vision.ts's header documents: + * a free byte-identical pre-check (no AI) here, then a bounded AI-vision call for genuinely different pairs. + * Gated on `settings.screenshotTableGate?.enabled` — this repo must already have opted into the deterministic + * gate at all; there is no separate dedicated toggle, mirroring how #4111's sibling visual-vision check has no + * config field of its own either (gated by AI_VISION/BYOK availability + the existing reputation/aiReviewAllAuthors + * settings). STRICTLY ADVISORY, mirrors `runVisualVisionForAdvisory`'s exact shape (resolve reputation + BYOK, + * gate, call, parse, mutate `args.advisory.findings`) so it can be exercised directly in tests. Never throws: + * any failure (a broken image fetch, a provider error, an unparseable response) degrades to "no finding added". + */ +export async function runScreenshotTableVisionForAdvisory( + env: Env, + args: { + mode: AgentActionMode; + repoFullName: string; + pr: { number: number }; + prBody: string | null | undefined; + prTitle: string | null | undefined; + author: string | null; + confirmedContributor: boolean; + settings: RepositorySettings; + advisory: { findings: AdvisoryFinding[] }; + }, +): Promise { + if (args.mode === "paused" || !args.settings.screenshotTableGate?.enabled) return; + const rawPairs = extractTableRowImageUrls(args.prBody).filter((pair) => pair.every((url) => isSafeHttpUrl(url))); + if (rawPairs.length === 0) return; + try { + const fetchedPairs: Array<{ before: AiContentBlock; after: AiContentBlock }> = []; + const findings: AdvisoryFinding[] = []; + for (const [rowIndex, [beforeUrl, afterUrl]] of rawPairs.slice(0, 2).entries()) { + /* v8 ignore next -- defensive: rawPairs only contains rows with >=2 urls, so both slots exist here. */ + if (!beforeUrl || !afterUrl) continue; + const [beforeBlock, afterBlock] = await Promise.all([ + fetchShotContentBlock(beforeUrl), + fetchShotContentBlock(afterUrl), + ]); + if (!beforeBlock || !afterBlock) continue; + /* v8 ignore next -- defensive: fetchShotContentBlock's only success return shape is {type:"image",...}. */ + if (beforeBlock.type !== "image" || afterBlock.type !== "image") continue; + if (beforeBlock.data === afterBlock.data) { + findings.push({ + code: SCREENSHOT_TABLE_VISION_FINDING_CODE, + severity: "warning", + title: `Possible screenshot-table issue: identical images (row ${rowIndex + 1})`, + detail: "The before and after images for this row are byte-identical — this doesn't look like real before/after evidence.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }); + continue; + } + fetchedPairs.push({ before: beforeBlock, after: afterBlock }); + } + if (fetchedPairs.length > 0) { + const reputation = await getEffectiveSubmitterReputation(env, { repoFullName: args.repoFullName, submitter: args.author ?? undefined }); + const storedKey = + args.confirmedContributor && args.settings.aiReviewByok + ? await getDecryptedRepositoryAiKey(env, args.repoFullName) + : null; + const providerKey = + storedKey && (!args.settings.aiReviewProvider || args.settings.aiReviewProvider === storedKey.provider) + ? { provider: storedKey.provider, key: storedKey.key, model: args.settings.aiReviewModel ?? storedKey.model } + : null; + const selfHostVisionAllowed = args.confirmedContributor || args.settings.aiReviewAllAuthors; + const selfHostVisionAvailable = selfHostVisionAllowed && Boolean(env.AI_VISION); + const gate = evaluateScreenshotTableVisionGate({ + imagePairCount: fetchedPairs.length, + reputationSignal: reputation.signal, + providerKey, + selfHostVisionAvailable, + }); + if (gate.run) { + const images: AiContentBlock[] = fetchedPairs + .slice(0, gate.pairCount) + .flatMap((pair) => [pair.before, pair.after]); + const userPrompt = buildScreenshotTableVisionUserPrompt(args.prTitle, gate.pairCount); + let visionText: string | null; + let visionUsage: AiReviewActualUsage | undefined; + if (providerKey) { + const response = await callAiProvider(providerKey, SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT, userPrompt, 400, images); + visionText = response.text; + visionUsage = response.usage; + await recordScreenshotTableVisionUsage( + env, + args, + providerKey, + visionText ? "ok" : response.failure ? "error" : "ok", + visionText ? `advisory findings check (${gate.pairCount} pairs)` : response.failure ? `provider failure: ${String(response.failure)}` : "no usable output", + visionUsage, + ); + } else { + visionText = await runSelfHostVisualVision(env, SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT, userPrompt, images); + } + if (visionText) { + const parsed = parseScreenshotTableVisionResponse(visionText, gate.pairCount); + findings.push(...buildScreenshotTableVisionFindings(parsed)); + } + } + } + if (findings.length > 0) args.advisory.findings.push(...findings); + } catch (error) { + console.log( + JSON.stringify({ + event: "screenshot_table_vision_error", + repoFullName: args.repoFullName, + pull: args.pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + } +} + /** * Resolve `manifest_missing_tests`' `passedValidationCount` signal (gate-review finding, #4719): a PR-body * validation-note match (`hasValidationNote`) is checked FIRST since it's free; only when that misses, AND @@ -11323,6 +11472,20 @@ async function maybePublishPrPublicSurface( advisory, routes: beforeAfter, }); + // Vision-verify a contributor-pasted screenshot-table (#4366 wiring) — see runScreenshotTableVisionForAdvisory's + // own doc comment. Independent of the bot-capture vision block above: this checks the CONTRIBUTOR's own + // pasted table images, not the bot's rendered before/after pair. + await runScreenshotTableVisionForAdvisory(env, { + mode, + repoFullName, + pr, + prBody: pr.body, + prTitle: pr.title, + author, + confirmedContributor, + settings, + advisory, + }); // review.memory (#2181, apply slice of #1964): before the unified comment renders, suppress/demote // advisory (non-blocking) findings a maintainer already dismissed as false positives for this repo. ONLY // ever applied to `commentGate.warnings` -- NEVER `commentGate.blockers` -- so this can never change the diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts index 32dfb5afc1..f55adbd374 100644 --- a/src/review/screenshot-table-gate.ts +++ b/src/review/screenshot-table-gate.ts @@ -186,7 +186,7 @@ const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|]*>/i; * one needs actual cell contents (not just "does some cell have an image"), so duplicating its short * header+separator detection loop keeps both independently simple instead of risking a regression in either * from a shared-code change. */ -function extractTableRows(body: string | null | undefined): string[][] { +export function extractTableRows(body: string | null | undefined): string[][] { if (!body) return []; const lines = body.split(/\r?\n/); const tableRowPattern = /^\s*\|.*\|\s*$/; @@ -215,6 +215,31 @@ function extractTableRows(body: string | null | undefined): string[][] { return rows; } +// Matches EITHER markdown image syntax (`![alt](url)`, optionally with a trailing `"title"`) OR an `` tag, capturing the URL from whichever alternative matched -- covers a bare `![]()` cell and the +// PR template's own clickable-thumbnail convention (`[![before](url)](url)`, where the OUTER `[...](...)` is +// the click-through link and this pattern correctly targets the INNER `!`-prefixed image markup instead). +const CELL_IMAGE_URL_PATTERN = /!\[[^\]]*\]\(([^)\s]+)(?:\s+"[^"]*")?\)|]*\bsrc=["']([^"']+)["'][^>]*>/i; + +function extractCellImageUrl(cell: string): string | null { + const match = cell.match(CELL_IMAGE_URL_PATTERN); + if (!match) return null; + /* v8 ignore next -- defensive: whichever alternative of CELL_IMAGE_URL_PATTERN matched always captures a + * non-empty group (both require at least one non-`)`/non-`"` character), so this fallback is unreachable. */ + return match[1] ?? match[2] ?? null; +} + +/** The image URLs found in each detected table row (source order), for rows with at least two — a real + * before/after pair worth comparing, not a single decorative image or caption-only row. Reuses + * {@link extractTableRows}'s own header+separator detection rather than re-scanning the body. A row with + * MORE than two images (e.g. a desktop+mobile matrix row) keeps every image; callers that only want a pair + * slice it themselves. */ +export function extractTableRowImageUrls(body: string | null | undefined): string[][] { + return extractTableRows(body) + .map((row) => row.map(extractCellImageUrl).filter((url): url is string => url !== null)) + .filter((urls) => urls.length >= 2); +} + /** One (viewport, theme) combination the matrix must cover. `theme: null` means the theme dimension isn't * required at all (a repo can require viewport coverage without color-mode coverage). */ export type ScreenshotMatrixPair = { viewport: string; theme: string | null }; diff --git a/src/review/visual/screenshot-table-vision.ts b/src/review/visual/screenshot-table-vision.ts new file mode 100644 index 0000000000..59b4469314 --- /dev/null +++ b/src/review/visual/screenshot-table-vision.ts @@ -0,0 +1,137 @@ +// Advisory-only vision verification of a CONTRIBUTOR-pasted screenshot-table (#4366, part of #4325). PURE +// decision + prompt/response logic ONLY, mirroring visual-findings.ts's own separation: this module never +// fetches image bytes, calls an AI provider, or touches D1 -- a caller supplies already-resolved images (as +// `AiContentBlock[]` pairs, see `../../types`) and a resolved BYOK/self-host-vision provider, so this file +// stays testable without network fixtures. +// +// screenshot-table-gate.ts's DETERMINISTIC check only verifies markdown STRUCTURE (a table exists with +// image-bearing cells) -- it has no way to see whether the pasted images are actually two different, +// plausibly-relevant screenshots, or a contributor gaming the gate with a duplicated/unrelated image. This +// module adds that missing check, split into two stages: +// 1. A cheap, deterministic pre-check the LIVE CALLER runs BEFORE reaching this module at all: two fetched +// images with IDENTICAL base64 bytes need no AI call whatsoever -- see `runScreenshotTableVisionForAdvisory` +// in processors.ts. Only genuinely different-bytes pairs reach the vision gate below. +// 2. The AI-vision judgment here: for a real (different-bytes) pair, ask a vision-capable model whether the +// two images still look near-identical (a re-encoded/recompressed duplicate a byte comparison would miss) +// OR plausibly unrelated to the PR's stated change (a screenshot from an unrelated app/page/topic). +// +// STRICTLY ADVISORY: `SCREENSHOT_TABLE_VISION_FINDING_CODE` is not one of the codes `isConfiguredGateBlocker` +// (src/rules/advisory.ts) recognizes, so this finding can NEVER become a gate blocker -- it rides the +// identical `advisory.findings` pipeline `visual_regression_finding`/`ai_consensus_defect` already use. + +import type { AdvisoryFinding } from "../../types"; +import { extractLastJsonObject, toPublicSafe, type AiReviewProviderKey } from "../../services/ai-review"; +import type { ReputationSignal } from "../submitter-reputation"; + +/** The advisory finding code a screenshot-table gaming observation is published under (#4366). Deliberately + * absent from `isConfiguredGateBlocker`'s allowlist (src/rules/advisory.ts) -- see this file's header. */ +export const SCREENSHOT_TABLE_VISION_FINDING_CODE = "screenshot_table_vision_finding"; + +/** Bound on how many table row image-pairs a single review ever sends to vision -- mirrors + * visual-findings.ts's MAX_VISION_ROUTES: a vision call is the most expensive AI request this codebase makes + * per-row (an image attachment, not just text), so a table with many rows must never translate into + * unbounded spend. */ +const MAX_SCREENSHOT_TABLE_VISION_PAIRS = 2; + +/** Why {@link evaluateScreenshotTableVisionGate} declined to run the vision call -- observability-only. */ +export type ScreenshotTableVisionSkipReason = "no_image_pairs" | "low_reputation" | "byok_not_configured"; + +export type ScreenshotTableVisionGateResult = + | { run: false; reason: ScreenshotTableVisionSkipReason } + | { run: true; pairCount: number }; + +/** + * Decide whether a screenshot-table vision call is warranted — mirrors `evaluateVisualVisionGate`'s three-gate + * shape exactly: + * 1. at least one real (different-bytes) image pair survived the caller's byte pre-check. + * 2. submitter reputation — a "low" windowed reputation signal skips vision, same as every other AI neuron. + * 3. a provider that can actually SEE the images — BYOK or self-host local vision (`env.AI_VISION`, #4335). + * Pure + total: the caller resolves the reputation signal / provider key / self-host vision availability and + * the already-byte-deduped pair count; this only decides admission and how many pairs are in play. + */ +export function evaluateScreenshotTableVisionGate(input: { + imagePairCount: number; + reputationSignal: ReputationSignal; + providerKey: AiReviewProviderKey | null; + selfHostVisionAvailable?: boolean; +}): ScreenshotTableVisionGateResult { + if (input.reputationSignal === "low") return { run: false, reason: "low_reputation" }; + if (!input.providerKey && !input.selfHostVisionAvailable) return { run: false, reason: "byok_not_configured" }; + const pairCount = Math.min(input.imagePairCount, MAX_SCREENSHOT_TABLE_VISION_PAIRS); + if (pairCount === 0) return { run: false, reason: "no_image_pairs" }; + return { run: true, pairCount }; +} + +/** One vision observation the model reported for a specific table row (1-indexed among the pairs sent, not + * the row's position in the PR body — the live caller has no cheap way to recover the original row number + * once rows have been filtered down to real pairs, and the number only needs to disambiguate WHICH pair a + * finding is about when more than one was sent). */ +export type ScreenshotTableVisionFinding = { pairIndex: number; body: string }; + +/** Cap on findings kept from a single vision response — mirrors visual-findings.ts's MAX_VISUAL_FINDINGS. */ +const MAX_SCREENSHOT_TABLE_VISION_FINDINGS = 2; + +export const SCREENSHOT_TABLE_VISION_SYSTEM_PROMPT = [ + "You are checking a pull request's before/after screenshot-table evidence for gaming, not for visual regressions.", + "Each pair below is one table row's before image followed by its after image, in that order.", + 'Respond with ONLY a JSON object of this exact shape (no prose, no code fence): {"findings": [{"pairIndex": number, "body": string}]}.', + "Report a finding for a pair ONLY when the two images are effectively the SAME screenshot (a near-identical", + "duplicate, not a genuine before/after difference) OR when either image looks implausible as evidence for the", + "stated change (an unrelated app/website/topic, a blank/broken render, or an obviously irrelevant picture).", + "Do NOT report a pair just because the visual difference is small — a genuine minor style tweak is exactly", + "what real before/after evidence looks like. pairIndex is 1 for the first pair, 2 for the second, and so on.", + "Each body is ONE sentence, specific to what you SEE. Return an empty findings array when every pair looks", + "like genuine, plausible before/after evidence. Never mention rewards, payouts, wallets, hotkeys, coldkeys, or trust scores.", +].join(" "); + +/** Build the user-turn text naming the PR's stated change ahead of the image content blocks — the caller + * attaches the actual before/after image pairs (see `../../types`'s `AiContentBlock`); this module only + * builds the text half of the request. `prTitle` gives the model the change's stated intent to judge + * plausibility against, mirroring how the regular AI review prompt always includes the PR title. */ +export function buildScreenshotTableVisionUserPrompt(prTitle: string | null | undefined, pairCount: number): string { + const titleLine = prTitle && prTitle.trim() ? `Pull request title: ${prTitle.trim()}\n\n` : ""; + return `${titleLine}${pairCount} before/after image pair(s) are attached below, each pair in before, after order.`; +} + +/** Parse the model's structured vision response into public-safe findings, dropping anything unparseable, an + * out-of-range pairIndex, a blank body, or a body that trips the public/private boundary (`toPublicSafe`). + * Bounded to {@link MAX_SCREENSHOT_TABLE_VISION_FINDINGS}. Never throws — an unparseable response degrades to + * `[]`, the same fail-safe convention `parseVisualVisionResponse` uses. */ +export function parseScreenshotTableVisionResponse(text: string, pairCount: number): ScreenshotTableVisionFinding[] { + 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: ScreenshotTableVisionFinding[] = []; + for (const entry of findingsRaw) { + if (out.length >= MAX_SCREENSHOT_TABLE_VISION_FINDINGS) break; + if (!entry || typeof entry !== "object") continue; + const record = entry as Record; + const pairIndex = typeof record.pairIndex === "number" ? record.pairIndex : NaN; + const rawBody = typeof record.body === "string" ? record.body : ""; + const body = toPublicSafe(rawBody); + if (!Number.isInteger(pairIndex) || pairIndex < 1 || pairIndex > pairCount || !body) continue; + out.push({ pairIndex, body }); + } + return out; +} + +/** Build the ADVISORY-ONLY findings for the unified comment (#4366) — one per vision observation, feeding the + * SAME `advisory.findings` pipeline `visual_regression_finding`/`ai_consensus_defect` already ride. + * `severity: "warning"` is required, not incidental — `evaluateGateCheckCore` (src/rules/advisory.ts) only + * carries `"warning"`-severity findings into `gate.warnings` at all. */ +export function buildScreenshotTableVisionFindings(findings: readonly ScreenshotTableVisionFinding[]): AdvisoryFinding[] { + return findings.map((finding) => ({ + code: SCREENSHOT_TABLE_VISION_FINDING_CODE, + severity: "warning", + title: `Possible screenshot-table issue: pair ${finding.pairIndex}`, + detail: finding.body, + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + })); +} diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 05525f2265..4591dcfef0 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -5,6 +5,7 @@ import { DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, + extractTableRowImageUrls, hasCommittedImageFile, hasImageBearingMarkdownTable, hasImageOutsideTable, @@ -666,3 +667,50 @@ describe("evaluateScreenshotTableGate", () => { }); }); }); + +describe("extractTableRowImageUrls (#4366)", () => { + it("extracts the before/after URL pair from a markdown-image table row", () => { + expect(extractTableRowImageUrls(TABLE_BODY)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("extracts the URL from an tag, mixed with markdown syntax in the same row", () => { + const body = ['| Before | After |', '| --- | --- |', '| | ![after](https://x/after.png) |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("extracts the INNER image URL from a clickable-thumbnail cell ([![alt](img-url)](link-url)), not the outer link", () => { + const body = ['| Before | After |', '| --- | --- |', '| [![before](https://x/before.png)](https://x/before.png) | [![after](https://x/after.png)](https://x/after.png) |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("strips a trailing markdown title from an image URL (![alt](url \"title\"))", () => { + const body = ['| Before | After |', '| --- | --- |', '| ![before](https://x/before.png "Before") | ![after](https://x/after.png "After") |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("drops a row with only ONE image cell (not a real before/after pair)", () => { + const body = ['| Before | After |', '| --- | --- |', '| ![before](https://x/before.png) | no image here |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([]); + }); + + it("returns [] for a table with no image markup at all, and for an empty/missing body", () => { + const body = ["| Before | After |", "| --- | --- |", "| nothing | here |"].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([]); + expect(extractTableRowImageUrls("")).toEqual([]); + expect(extractTableRowImageUrls(null)).toEqual([]); + expect(extractTableRowImageUrls(undefined)).toEqual([]); + }); + + it("extracts a pair from EACH qualifying row when the table has multiple rows", () => { + const body = [ + "| Before | After |", + "| --- | --- |", + "| ![before](https://x/1-before.png) | ![after](https://x/1-after.png) |", + "| ![before](https://x/2-before.png) | ![after](https://x/2-after.png) |", + ].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([ + ["https://x/1-before.png", "https://x/1-after.png"], + ["https://x/2-before.png", "https://x/2-after.png"], + ]); + }); +}); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index 695c290db3..8c7dead3d8 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -4,6 +4,7 @@ import { DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, + extractTableRowImageUrls, hasCommittedImageFile, hasImageBearingMarkdownTable, hasImageOutsideTable, @@ -665,3 +666,59 @@ describe("evaluateScreenshotTableGate", () => { }); }); }); + +describe("extractTableRowImageUrls (#4366)", () => { + it("extracts the before/after URL pair from a markdown-image table row", () => { + expect(extractTableRowImageUrls(TABLE_BODY)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("extracts the URL from an tag, mixed with markdown syntax in the same row", () => { + const body = ['| Before | After |', '| --- | --- |', '| | ![after](https://x/after.png) |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("extracts the INNER image URL from a clickable-thumbnail cell ([![alt](img-url)](link-url)), not the outer link", () => { + const body = ['| Before | After |', '| --- | --- |', '| [![before](https://x/before.png)](https://x/before.png) | [![after](https://x/after.png)](https://x/after.png) |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("strips a trailing markdown title from an image URL (![alt](url \"title\"))", () => { + const body = ['| Before | After |', '| --- | --- |', '| ![before](https://x/before.png "Before") | ![after](https://x/after.png "After") |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/before.png", "https://x/after.png"]]); + }); + + it("drops a row with only ONE image cell (not a real before/after pair)", () => { + const body = ['| Before | After |', '| --- | --- |', '| ![before](https://x/before.png) | no image here |'].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([]); + }); + + it("keeps every image URL on a row with more than two (e.g. a desktop+mobile matrix row)", () => { + const body = [ + '| Viewport | Before | After |', + '| --- | --- | --- |', + '| Desktop | ![before](https://x/d-before.png) | ![after](https://x/d-after.png) |', + ].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([["https://x/d-before.png", "https://x/d-after.png"]]); + }); + + it("returns [] for a table with no image markup at all, and for an empty/missing body", () => { + const body = ["| Before | After |", "| --- | --- |", "| nothing | here |"].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([]); + expect(extractTableRowImageUrls("")).toEqual([]); + expect(extractTableRowImageUrls(null)).toEqual([]); + expect(extractTableRowImageUrls(undefined)).toEqual([]); + }); + + it("extracts a pair from EACH qualifying row when the table has multiple rows", () => { + const body = [ + "| Before | After |", + "| --- | --- |", + "| ![before](https://x/1-before.png) | ![after](https://x/1-after.png) |", + "| ![before](https://x/2-before.png) | ![after](https://x/2-after.png) |", + ].join("\n"); + expect(extractTableRowImageUrls(body)).toEqual([ + ["https://x/1-before.png", "https://x/1-after.png"], + ["https://x/2-before.png", "https://x/2-after.png"], + ]); + }); +}); diff --git a/test/unit/screenshot-table-vision-wiring.test.ts b/test/unit/screenshot-table-vision-wiring.test.ts new file mode 100644 index 0000000000..797a6befbe --- /dev/null +++ b/test/unit/screenshot-table-vision-wiring.test.ts @@ -0,0 +1,552 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { runScreenshotTableVisionForAdvisory } from "../../src/queue/processors"; +import * as repositories from "../../src/db/repositories"; +import { upsertRepositoryAiKey } from "../../src/db/repositories"; +import * as submitterReputation from "../../src/review/submitter-reputation"; +import type { AdvisoryFinding, RepositorySettings } from "../../src/types"; +import { createTestEnv } from "../helpers/d1"; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +const pr = { number: 3 }; +const repoFullName = "acme/widgets"; + +function byokEnv() { + return createTestEnv({ TOKEN_ENCRYPTION_SECRET: "screenshot-vision-test-fake-encryption-secret" }); +} + +function gateEnabledSettings(over: Partial = {}): RepositorySettings { + return { + screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] }, + aiReviewByok: true, + ...over, + } as RepositorySettings; +} + +function findingsHolder(): { findings: AdvisoryFinding[] } { + return { findings: [] }; +} + +function tableBody(beforeUrl: string, afterUrl: string): string { + return `## Screenshots\n\n| Before | After |\n| --- | --- |\n| ![before](${beforeUrl}) | ![after](${afterUrl}) |\n`; +} + +const BEFORE_URL = "https://user-images.githubusercontent.com/before.png"; +const AFTER_URL = "https://user-images.githubusercontent.com/after.png"; + +function findingsResponse(findings: Array<{ pairIndex: number; body: string }>) { + return JSON.stringify({ findings }); +} + +function anthropicOk(text: string) { + return new Response(JSON.stringify({ content: [{ type: "text", text }] }), { status: 200 }); +} + +function stubShotsAndProvider(providerResponseText: string | null, bytes: { before: number[]; after: number[] } = { before: [1, 2, 3], after: [4, 5, 6] }) { + vi.stubGlobal("fetch", vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.anthropic.com/v1/messages") { + return providerResponseText === null ? new Response("upstream error", { status: 500 }) : anthropicOk(providerResponseText); + } + if (url === BEFORE_URL) return new Response(new Uint8Array(bytes.before), { status: 200 }); + if (url === AFTER_URL) return new Response(new Uint8Array(bytes.after), { status: 200 }); + return new Response("not found", { status: 404 }); + })); +} + +describe("runScreenshotTableVisionForAdvisory (#4366)", () => { + it("no-ops when the deterministic screenshot-table gate is disabled -- never touches the network", async () => { + const env = byokEnv(); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ screenshotTableGate: { enabled: false, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] } }), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("no-ops on a paused mode, even with a qualifying table and the gate enabled", async () => { + const env = byokEnv(); + stubShotsAndProvider(findingsResponse([{ pairIndex: 1, body: "identical" }])); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "paused", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + }); + + it("no-ops when the PR body has no image-bearing table row at all", async () => { + const env = byokEnv(); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: "Just a plain description, no table.", + prTitle: "Fix a typo", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("drops a pair with an unsafe (non-HTTPS) URL instead of fetching it (#SSRF)", async () => { + const env = byokEnv(); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody("http://user-images.githubusercontent.com/before.png", AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("drops a pair whose URL resolves to a private/local host instead of fetching it (#SSRF)", async () => { + const env = byokEnv(); + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody("https://169.254.169.254/before.png", AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + it("flags byte-identical before/after images WITHOUT calling any AI provider (free deterministic check)", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(null, { before: [9, 9, 9], after: [9, 9, 9] }); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([ + { + code: "screenshot_table_vision_finding", + severity: "warning", + title: "Possible screenshot-table issue: identical images (row 1)", + detail: "The before and after images for this row are byte-identical — this doesn't look like real before/after evidence.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + const fetchCalls = (globalThis.fetch as unknown as ReturnType).mock.calls.map((c: unknown[]) => String(c[0])); + expect(fetchCalls).not.toContain("https://api.anthropic.com/v1/messages"); + }); + + it("calls the BYOK provider for a genuinely different pair and adds its parsed finding", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(findingsResponse([{ pairIndex: 1, body: "The after screenshot shows an unrelated login page." }])); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([ + { + code: "screenshot_table_vision_finding", + severity: "warning", + title: "Possible screenshot-table issue: pair 1", + detail: "The after screenshot shows an unrelated login page.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + }); + + it("adds no finding when the provider returns an empty findings array (genuine evidence)", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(findingsResponse([])); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + }); + + it("degrades to no finding (never throws) when the provider call itself fails", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(null); + const adv = findingsHolder(); + await expect( + runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }), + ).resolves.toBeUndefined(); + expect(adv.findings).toEqual([]); + }); + + it("declines the AI call for a low-reputation submitter, but a byte-identical pair still gets flagged for free", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + vi.spyOn(submitterReputation, "getSubmitterReputation").mockResolvedValueOnce({ + submissions: 6, + merged: 0, + closed: 6, + manual: 0, + closeRate: 1, + signal: "low", + }); + stubShotsAndProvider(null, { before: [7, 7, 7], after: [7, 7, 7] }); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "bob", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([ + { + code: "screenshot_table_vision_finding", + severity: "warning", + title: "Possible screenshot-table issue: identical images (row 1)", + detail: "The before and after images for this row are byte-identical — this doesn't look like real before/after evidence.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + const fetchCalls = (globalThis.fetch as unknown as ReturnType).mock.calls.map((c: unknown[]) => String(c[0])); + expect(fetchCalls).not.toContain("https://api.anthropic.com/v1/messages"); + }); + + it("runs via env.AI_VISION when no BYOK key is configured at all", async () => { + const runMock = vi.fn(async () => ({ response: findingsResponse([{ pairIndex: 1, body: "Looks like a different app entirely." }]) })); + const env = byokEnv(); + (env as unknown as { AI_VISION: unknown }).AI_VISION = { run: runMock }; + stubShotsAndProvider(null); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ aiReviewByok: false }), + advisory: adv, + }); + expect(runMock).toHaveBeenCalledTimes(1); + expect(adv.findings).toEqual([ + { + code: "screenshot_table_vision_finding", + severity: "warning", + title: "Possible screenshot-table issue: pair 1", + detail: "Looks like a different app entirely.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + }); + + it("does not let an unconfirmed contributor spend self-host vision resources unless all-authors is enabled", async () => { + const runMock = vi.fn(async () => ({ response: findingsResponse([{ pairIndex: 1, body: "should not run" }]) })); + const env = byokEnv(); + (env as unknown as { AI_VISION: unknown }).AI_VISION = { run: runMock }; + stubShotsAndProvider(null); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: false, + settings: gateEnabledSettings({ aiReviewByok: false, aiReviewAllAuthors: false }), + advisory: adv, + }); + expect(runMock).not.toHaveBeenCalled(); + expect(adv.findings).toEqual([]); + }); + + it("still declines entirely when neither BYOK nor env.AI_VISION is configured, but byte-identical detection still works", async () => { + const env = byokEnv(); + stubShotsAndProvider(null, { before: [5, 5, 5], after: [5, 5, 5] }); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ aiReviewByok: false }), + advisory: adv, + }); + expect(adv.findings).toEqual([ + { + code: "screenshot_table_vision_finding", + severity: "warning", + title: "Possible screenshot-table issue: identical images (row 1)", + detail: "The before and after images for this row are byte-identical — this doesn't look like real before/after evidence.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + }); + + it("only sends the first two qualifying rows to fetch/vision, bounding cost on a long table", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + const thirdBefore = "https://user-images.githubusercontent.com/third-before.png"; + const thirdAfter = "https://user-images.githubusercontent.com/third-after.png"; + const body = [ + "| Before | After |", + "| --- | --- |", + `| ![before](${BEFORE_URL}) | ![after](${AFTER_URL}) |`, + `| ![before](${BEFORE_URL}) | ![after](${AFTER_URL}) |`, + `| ![before](${thirdBefore}) | ![after](${thirdAfter}) |`, + ].join("\n"); + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === "https://api.anthropic.com/v1/messages") return anthropicOk(findingsResponse([])); + if (url === BEFORE_URL) return new Response(new Uint8Array([1]), { status: 200 }); + if (url === AFTER_URL) return new Response(new Uint8Array([2]), { status: 200 }); + return new Response("not found", { status: 404 }); + }); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: body, + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + const fetchedUrls = fetchMock.mock.calls.map((c) => String(c[0])); + expect(fetchedUrls).not.toContain(thirdBefore); + expect(fetchedUrls).not.toContain(thirdAfter); + }); + + it("silently skips a pair when only ONE of its two images fetches successfully", async () => { + const env = byokEnv(); + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200 }); + return new Response("not found", { status: 404 }); + }); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + }); + + it("adds no finding when the BYOK provider returns 200 with no usable text (distinct from a failure) -- also exercises a null author", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + // An empty string is a genuine 2xx response, unlike stubShotsAndProvider(null)'s 500 -- callAiProvider + // returns { text: "", failure: undefined } here (no "http_error"), exercising the "no usable output" + // fallback in recordScreenshotTableVisionUsage's detail message rather than the provider-failure one. + stubShotsAndProvider(""); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: null, + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }); + expect(adv.findings).toEqual([]); + }); + + it("still resolves BYOK when the declared provider explicitly matches the stored key's provider", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + stubShotsAndProvider(findingsResponse([])); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ aiReviewProvider: "anthropic" }), + advisory: adv, + }); + const fetchCalls = (globalThis.fetch as unknown as ReturnType).mock.calls.map((c: unknown[]) => String(c[0])); + expect(fetchCalls).toContain("https://api.anthropic.com/v1/messages"); + }); + + it("skips BYOK (falls back to nothing, since self-host vision isn't configured either) when the declared provider doesn't match the stored key", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === BEFORE_URL) return new Response(new Uint8Array([1, 2, 3]), { status: 200 }); + if (url === AFTER_URL) return new Response(new Uint8Array([4, 5, 6]), { status: 200 }); + return new Response("not found", { status: 404 }); + }); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ aiReviewProvider: "openai" }), + advisory: adv, + }); + expect(fetchMock.mock.calls.map((c) => String(c[0]))).not.toContain("https://api.anthropic.com/v1/messages"); + expect(adv.findings).toEqual([]); + }); + + it("swallows a thrown error from the BYOK key lookup and never lets it escape (screenshot_table_vision_error)", async () => { + const env = byokEnv(); + await upsertRepositoryAiKey(env, { repoFullName, provider: "anthropic", key: "sk-ant-key", model: null }); + vi.spyOn(repositories, "getDecryptedRepositoryAiKey").mockRejectedValueOnce(new Error("D1 unavailable")); + stubShotsAndProvider(findingsResponse([])); + const adv = findingsHolder(); + await expect( + runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: tableBody(BEFORE_URL, AFTER_URL), + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings(), + advisory: adv, + }), + ).resolves.toBeUndefined(); + expect(adv.findings).toEqual([]); + }); + + it("distinguishes two identical-image rows by row number instead of producing indistinguishable findings", async () => { + const env = byokEnv(); + const secondBefore = "https://user-images.githubusercontent.com/second-before.png"; + const secondAfter = "https://user-images.githubusercontent.com/second-after.png"; + const body = [ + "| Before | After |", + "| --- | --- |", + `| ![before](${BEFORE_URL}) | ![after](${AFTER_URL}) |`, + `| ![before](${secondBefore}) | ![after](${secondAfter}) |`, + ].join("\n"); + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url === BEFORE_URL || url === AFTER_URL) return new Response(new Uint8Array([1, 1, 1]), { status: 200 }); + if (url === secondBefore || url === secondAfter) return new Response(new Uint8Array([2, 2, 2]), { status: 200 }); + return new Response("not found", { status: 404 }); + }); + vi.stubGlobal("fetch", fetchMock); + const adv = findingsHolder(); + await runScreenshotTableVisionForAdvisory(env, { + mode: "live", + repoFullName, + pr, + prBody: body, + prTitle: "Redesign the nav bar", + author: "alice", + confirmedContributor: true, + settings: gateEnabledSettings({ aiReviewByok: false }), + advisory: adv, + }); + expect(adv.findings).toEqual([ + expect.objectContaining({ title: "Possible screenshot-table issue: identical images (row 1)" }), + expect.objectContaining({ title: "Possible screenshot-table issue: identical images (row 2)" }), + ]); + }); +}); diff --git a/test/unit/screenshot-table-vision.test.ts b/test/unit/screenshot-table-vision.test.ts new file mode 100644 index 0000000000..bd0acb7023 --- /dev/null +++ b/test/unit/screenshot-table-vision.test.ts @@ -0,0 +1,170 @@ +import { describe, expect, it } from "vitest"; +import { + buildScreenshotTableVisionFindings, + buildScreenshotTableVisionUserPrompt, + evaluateScreenshotTableVisionGate, + parseScreenshotTableVisionResponse, + SCREENSHOT_TABLE_VISION_FINDING_CODE, +} from "../../src/review/visual/screenshot-table-vision"; +import type { AiReviewProviderKey } from "../../src/services/ai-review"; +import { evaluateGateCheck } from "../../src/rules/advisory"; +import type { Advisory } from "../../src/types"; + +const providerKey: AiReviewProviderKey = { provider: "anthropic", key: "sk-ant" }; + +describe("evaluateScreenshotTableVisionGate", () => { + it("skips for a low-reputation submitter, even with pairs and BYOK configured (checked FIRST)", () => { + expect( + evaluateScreenshotTableVisionGate({ imagePairCount: 2, reputationSignal: "low", providerKey }), + ).toEqual({ run: false, reason: "low_reputation" }); + }); + + it("skips when BYOK is not configured and self-host vision isn't available, even with pairs and good reputation", () => { + expect( + evaluateScreenshotTableVisionGate({ imagePairCount: 2, reputationSignal: "neutral", providerKey: null }), + ).toEqual({ run: false, reason: "byok_not_configured" }); + expect( + evaluateScreenshotTableVisionGate({ imagePairCount: 2, reputationSignal: "trusted", providerKey: null, selfHostVisionAvailable: false }), + ).toEqual({ run: false, reason: "byok_not_configured" }); + }); + + it("skips when there are no image pairs, even with good reputation and BYOK configured", () => { + expect( + evaluateScreenshotTableVisionGate({ imagePairCount: 0, reputationSignal: "neutral", providerKey }), + ).toEqual({ run: false, reason: "no_image_pairs" }); + }); + + it("runs, capping pairCount at the MAX bound, for a neutral- or trusted-reputation submitter with BYOK configured", () => { + expect(evaluateScreenshotTableVisionGate({ imagePairCount: 1, reputationSignal: "neutral", providerKey })).toEqual({ + run: true, + pairCount: 1, + }); + expect(evaluateScreenshotTableVisionGate({ imagePairCount: 5, reputationSignal: "trusted", providerKey })).toEqual({ + run: true, + pairCount: 2, + }); + }); + + it("runs via a self-host local vision provider even with NO BYOK key configured", () => { + expect( + evaluateScreenshotTableVisionGate({ imagePairCount: 1, reputationSignal: "neutral", providerKey: null, selfHostVisionAvailable: true }), + ).toEqual({ run: true, pairCount: 1 }); + }); +}); + +describe("buildScreenshotTableVisionUserPrompt", () => { + it("includes the PR title and pair count when a title is given", () => { + const prompt = buildScreenshotTableVisionUserPrompt("Redesign the nav bar", 2); + expect(prompt).toContain("Pull request title: Redesign the nav bar"); + expect(prompt).toContain("2 before/after image pair(s)"); + expect(prompt).toContain("before, after order"); + }); + + it("omits the title line entirely for a blank/whitespace/undefined title", () => { + expect(buildScreenshotTableVisionUserPrompt(undefined, 1)).not.toContain("Pull request title"); + expect(buildScreenshotTableVisionUserPrompt(null, 1)).not.toContain("Pull request title"); + expect(buildScreenshotTableVisionUserPrompt(" ", 1)).not.toContain("Pull request title"); + }); +}); + +describe("parseScreenshotTableVisionResponse", () => { + it("parses a valid findings array into public-safe entries", () => { + const text = JSON.stringify({ findings: [{ pairIndex: 1, body: "Both images are the same screenshot." }] }); + expect(parseScreenshotTableVisionResponse(text, 2)).toEqual([{ pairIndex: 1, body: "Both images are the same screenshot." }]); + }); + + it("drops an entry with a pairIndex below 1", () => { + const text = JSON.stringify({ findings: [{ pairIndex: 0, body: "Something is off." }] }); + expect(parseScreenshotTableVisionResponse(text, 2)).toEqual([]); + }); + + it("drops an entry whose pairIndex exceeds the number of pairs actually sent", () => { + const text = JSON.stringify({ findings: [{ pairIndex: 3, body: "Something is off." }] }); + expect(parseScreenshotTableVisionResponse(text, 2)).toEqual([]); + }); + + it("drops a non-integer pairIndex", () => { + const text = JSON.stringify({ findings: [{ pairIndex: 1.5, body: "Something is off." }] }); + expect(parseScreenshotTableVisionResponse(text, 2)).toEqual([]); + }); + + it("drops an entry with a blank/empty body (fails toPublicSafe's emptiness guard)", () => { + const text = JSON.stringify({ findings: [{ pairIndex: 1, body: "" }] }); + expect(parseScreenshotTableVisionResponse(text, 2)).toEqual([]); + }); + + it("drops a non-object entry and a findings value that isn't an array", () => { + expect(parseScreenshotTableVisionResponse(JSON.stringify({ findings: ["just a string"] }), 2)).toEqual([]); + expect(parseScreenshotTableVisionResponse(JSON.stringify({ findings: "not an array" }), 2)).toEqual([]); + }); + + it("drops an entry whose pairIndex is not a number and whose body is missing", () => { + expect(parseScreenshotTableVisionResponse(JSON.stringify({ findings: [{ pairIndex: "1", body: "x" }] }), 2)).toEqual([]); + expect(parseScreenshotTableVisionResponse(JSON.stringify({ findings: [{ pairIndex: 1 }] }), 2)).toEqual([]); + }); + + it("returns [] for text with no JSON object at all", () => { + expect(parseScreenshotTableVisionResponse("not json, just prose", 2)).toEqual([]); + }); + + it("returns [] for a balanced-brace object that is still invalid JSON (e.g. a trailing comma)", () => { + expect(parseScreenshotTableVisionResponse('{"findings": [1,]}', 2)).toEqual([]); + }); + + it("caps the result at MAX_SCREENSHOT_TABLE_VISION_FINDINGS even when the model returns more", () => { + const findings = Array.from({ length: 5 }, (_, i) => ({ pairIndex: 1, body: `Issue ${i}.` })); + expect(parseScreenshotTableVisionResponse(JSON.stringify({ findings }), 2)).toHaveLength(2); + }); +}); + +describe("buildScreenshotTableVisionFindings", () => { + it("maps each vision finding into an advisory-only, non-blocking AdvisoryFinding", () => { + const findings = buildScreenshotTableVisionFindings([{ pairIndex: 1, body: "Both images are the same screenshot." }]); + expect(findings).toEqual([ + { + code: SCREENSHOT_TABLE_VISION_FINDING_CODE, + severity: "warning", + title: "Possible screenshot-table issue: pair 1", + detail: "Both images are the same screenshot.", + action: "Advisory only — verify the screenshot-table images against the stated change before deciding.", + }, + ]); + }); + + it("returns [] for an empty findings list", () => { + expect(buildScreenshotTableVisionFindings([])).toEqual([]); + }); +}); + +describe("REGRESSION (#4366): a screenshot-table-vision 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-screenshot-vision", + 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: buildScreenshotTableVisionFindings([{ pairIndex: 1, body: "Both images are the same screenshot." }]), + generatedAt: "2026-07-07T00:00:00.000Z", + }; + 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); + }); +});