feat(review): advisory-only AI-vision analysis of before/after visual captures

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 `<img>` (GitHub caps it to the column width) wrapped in an `<a href>` 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;

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<string, unknown>;
+    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.",
+  }));
+}