feat(review): vision-verify screenshot-table PRs with the local VLM

packages/gittensory-engine/src/review/screenshot-table-gate.ts

@@ -186,7 +186,7 @@ const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|<img\b[^>]*>/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 `<img
+// src="...">` 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+"[^"]*")?\)|<img\b[^>]*\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 };

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<void> {
+  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<void> {
+  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

src/review/screenshot-table-gate.ts

@@ -186,7 +186,7 @@ const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|<img\b[^>]*>/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 `<img
+// src="...">` 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+"[^"]*")?\)|<img\b[^>]*\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 };