feat(review): wire up pixel-diff into a Diff column (self-host)

scripts/build-selfhost.mjs

@@ -47,6 +47,11 @@ await esbuild.build({
         build.onResolve({ filter: /^cloudflare:workers$/ }, () => ({ path: resolve(root, "src/selfhost/cf-workers-shim.ts") }));
         build.onResolve({ filter: /^@cloudflare\/puppeteer$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/puppeteer.ts") }));
         build.onResolve({ filter: /^agents\/mcp$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/agents-mcp.ts") }));
+        // Worker-safe no-op → real pixel-diff (pixelmatch/pngjs need Node Buffer, forbidden in the Worker
+        // bundle by test/unit/worker-entry-boundary.test.ts). Exact match: capture.ts is the only value
+        // importer, always as "./pixel-diff" (same-directory sibling) — the stub's own `import type` back to
+        // the original is erased before bundling and never reaches this resolver.
+        build.onResolve({ filter: /^\.\/pixel-diff$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/pixel-diff.ts") }));
       },
     },
   ],

src/review/unified-comment-bridge.ts

@@ -338,10 +338,13 @@ export type UnifiedCommentBridgeArgs = {
  * CLICKABLE THUMBNAILS: a small `<img>` (GitHub caps it to the column width) wrapped in an `<a href>` to the
  * SAME full-resolution shot, so a click opens the screenshot full-size. One row per route per viewport
  * (desktop / mobile), with the route path as the caption and a before (production) vs after (this PR's preview)
- * column. Emitted as TRUSTED raw HTML (`rawHtml: true`) so the `<a>/<img>` survive — public-safe by
- * construction: every value is a first-party minted /gittensory/shot URL or a route path (no private rubric /
- * scoring terms), and a stray `"` in a URL is neutralized so it can't break out of the attribute. Returns null
- * when nothing is renderable (no route has any shot URL), so the section is omitted rather than shown empty.
+ * column, plus a Diff column (#3674, self-host only) highlighting exactly what changed when a pixel-diff
+ * provider is available and finds a real visual difference — absent on hosted builds and any unchanged/no-diff
+ * cell, which render as a dash like every other missing shot. Emitted as TRUSTED raw HTML (`rawHtml: true`) so
+ * the `<a>/<img>` survive — public-safe by construction: every value is a first-party minted /gittensory/shot
+ * URL or a route path (no private rubric / scoring terms), and a stray `"` in a URL is neutralized so it can't
+ * break out of the attribute. Returns null when nothing is renderable (no route has any shot URL), so the
+ * section is omitted rather than shown empty.
  */
 export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedCollapsible | null {
   const attr = (value: string): string =>
@@ -355,22 +358,27 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl
   const cell = (url: string | undefined, label: string): string =>
     url ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(url)}"></a>` : "—";
   const rows: string[] = [];
+  let hasAnyDiff = false;
   for (const route of routes) {
     const path = markdownCode(route.path);
     if (route.beforeUrl || route.afterUrl) {
-      rows.push(`| ${path} | desktop | ${cell(route.beforeUrl, `before ${route.path}`)} | ${cell(route.afterUrl, `after ${route.path}`)} |`);
+      if (route.diffUrl) hasAnyDiff = true;
+      rows.push(`| ${path} | desktop | ${cell(route.beforeUrl, `before ${route.path}`)} | ${cell(route.afterUrl, `after ${route.path}`)} | ${cell(route.diffUrl, `diff ${route.path}`)} |`);
     }
     if (route.beforeUrlMobile || route.afterUrlMobile) {
-      rows.push(`| ${path} | mobile | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)`)} |`);
+      if (route.diffUrlMobile) hasAnyDiff = true;
+      rows.push(`| ${path} | mobile | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)`)} | ${cell(route.diffUrlMobile, `diff ${route.path} (mobile)`)} |`);
     }
   }
   if (rows.length === 0) return null;
   const body = [
-    "| Route | Viewport | Before (production) | After (this PR's preview) |",
-    "| --- | --- | --- | --- |",
+    "| Route | Viewport | Before (production) | After (this PR's preview) | Diff |",
+    "| --- | --- | --- | --- | --- |",
     ...rows,
     "",
-    "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy._",
+    hasAnyDiff
+      ? "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy · Diff highlights exactly what changed._"
+      : "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy._",
   ].join("\n");
   return { title: "Visual preview", body, rawHtml: true };
 }

src/review/visual/capture.ts

@@ -20,6 +20,7 @@ import {
   parseRepo,
 } from "./preview-url";
 import { captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type Viewport } from "./shot";
+import { compareCapturedScreenshots, isVisualDiffAvailable, type VisualDiffOutcome } from "./pixel-diff";
 
 const NAMESPACE = "gittensory";
 const DEFAULT_ROUTES = ["/"];
@@ -28,13 +29,17 @@ const DEFAULT_ROUTE_FILE = /apps\/gittensory-ui\/src\/routes\/(.+?)\.(?:tsx|jsx)
 // wall-clock — Browser Rendering is the costliest binding.
 const MAX_ROUTES = 2;
 
-/** A single captured route's before/after shot URLs (desktop + mobile). undefined slot ⇒ a dash cell. */
+/** A single captured route's before/after shot URLs (desktop + mobile), plus an optional pixel-diff overlay
+ *  per viewport (#3674) — self-host only (isVisualDiffAvailable), and only when the diff clears the visual-
+ *  diff module's own noise threshold; undefined slot ⇒ a dash cell either way. */
 export interface CaptureRoute {
   path: string;
   beforeUrl?: string | undefined;
   beforeUrlMobile?: string | undefined;
   afterUrl?: string | undefined;
   afterUrlMobile?: string | undefined;
+  diffUrl?: string | undefined;
+  diffUrlMobile?: string | undefined;
 }
 
 /** The capture pipeline's result: the rendered routes, plus whether a preview build is still pending. */
@@ -143,7 +148,12 @@ async function capturePage(
   slot: "before" | "after",
   viewportName: "desktop" | "mobile",
   viewport: Viewport,
-): Promise<{ url?: string | undefined }> {
+  // #3674: when true, ALSO resolve the raw PNG bytes (not just the URL) so the caller can pixel-diff
+  // before+after — including on a cache hit, which is the COMMON case for "before" (the same production
+  // shot is reused across many PR reviews). Costs one extra read on a cache hit; false (every existing
+  // caller) skips it entirely, so this is zero-cost unless a caller opts in.
+  includeBytes = false,
+): Promise<{ url?: string | undefined; png?: Uint8Array | undefined }> {
   if (!page) return {};
   const shotBase = env.PUBLIC_API_ORIGIN; // this worker's public origin (serves /gittensory/shot)
   const onDemand = shotBase ? `${shotBase}/${NAMESPACE}/shot?url=${encodeURIComponent(page)}&w=${viewport.width}&h=${viewport.height}` : page;
@@ -154,7 +164,11 @@ async function capturePage(
     const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}.png`;
     const url = shotBase ? `${shotBase}/${NAMESPACE}/shot?key=${encodeURIComponent(key)}` : onDemand;
     const cached = await env.REVIEW_AUDIT.get(key).catch(() => null);
-    if (cached) return { url };
+    if (cached) {
+      if (!includeBytes) return { url };
+      const bytes = await new Response(cached.body).arrayBuffer().then((buf) => new Uint8Array(buf)).catch(() => undefined);
+      return { url, ...(bytes ? { png: bytes } : {}) };
+    }
     const { png, authWalled } = await captureShot(env, page, viewport).catch(() => ({ png: null, authWalled: false }));
     // A protected route that redirected to a sign-in wall: show an honest "requires authentication"
     // placeholder rather than caching/serving a screenshot of the login screen.
@@ -163,12 +177,31 @@ async function capturePage(
     }
     if (png) {
       await env.REVIEW_AUDIT.put(key, png, { httpMetadata: { contentType: "image/png" } }).catch(() => undefined);
-      return { url };
+      return { url, ...(includeBytes ? { png } : {}) };
     }
   }
   return { url: onDemand };
 }
 
+/** Upload a computed diff-overlay PNG to the same store `capturePage` uses, returning its shot URL — or
+ *  undefined when there's no diff image (unchanged/new/removed/no-diff-provider), storage is unavailable, or
+ *  the upload fails. Mirrors capturePage's own key/URL scheme so the diff shares its caching story. */
+async function uploadDiffImage(
+  env: Env,
+  target: CaptureTarget,
+  path: string,
+  viewportName: "desktop" | "mobile",
+  diff: VisualDiffOutcome | null,
+): Promise<string | undefined> {
+  if (!diff?.diffImagePng) return undefined;
+  const shotBase = env.PUBLIC_API_ORIGIN;
+  if (!env.REVIEW_AUDIT || !shotBase) return undefined;
+  const fingerprint = await sha256Hex(`${target.headSha ?? target.prNumber}:diff:${viewportName}:${path}`);
+  const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}-diff.png`;
+  await env.REVIEW_AUDIT.put(key, diff.diffImagePng, { httpMetadata: { contentType: "image/png" } }).catch(() => undefined);
+  return `${shotBase}/${NAMESPACE}/shot?key=${encodeURIComponent(key)}`;
+}
+
 /** Per-repo `review.visual` config, as resolved by the caller from the manifest (#3609 / #3610). Absent ⇒
  *  byte-identical to today (GitHub-native discovery, automatic route inference, built-in route cap). */
 export type VisualCaptureConfig = { preview?: VisualPreviewInput | null | undefined; routes?: VisualRoutesInput | null | undefined };
@@ -229,24 +262,41 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge
   const failedPlaceholder = shotBase ? `${shotBase}/${NAMESPACE}/shot?placeholder=failed` : undefined;
   const afterPlaceholder = previewFailed ? failedPlaceholder : loadingPlaceholder;
 
+  // #3674: resolved ONCE per call, not per route/viewport — false in every hosted build (see pixel-diff.ts),
+  // so capturePage never pays the extra cached-bytes-read cost unless self-host's real diff module is active.
+  const diffAvailable = isVisualDiffAvailable();
   const routes = resolveVisualRoutes(visualFiles, visualConfig?.routes);
   const captureRoutes: CaptureRoute[] = [];
   for (const path of routes) {
     const beforePage = prodBase ? joinUrl(prodBase, path) : "";
     const afterPage = previewBase ? joinUrl(previewBase, path) : "";
     // Render desktop + mobile for each slot in parallel (4 PNGs/route) to bound wall-clock.
     const [beforeShot, beforeMobileShot, afterShot, afterMobileShot] = await Promise.all([
-      capturePage(env, target, beforePage, "before", "desktop", DESKTOP_VIEWPORT),
-      capturePage(env, target, beforePage, "before", "mobile", MOBILE_VIEWPORT),
-      afterPage ? capturePage(env, target, afterPage, "after", "desktop", DESKTOP_VIEWPORT) : Promise.resolve<{ url?: string | undefined }>({ url: afterPlaceholder }),
-      afterPage ? capturePage(env, target, afterPage, "after", "mobile", MOBILE_VIEWPORT) : Promise.resolve<{ url?: string | undefined }>({ url: afterPlaceholder }),
+      capturePage(env, target, beforePage, "before", "desktop", DESKTOP_VIEWPORT, diffAvailable),
+      capturePage(env, target, beforePage, "before", "mobile", MOBILE_VIEWPORT, diffAvailable),
+      afterPage ? capturePage(env, target, afterPage, "after", "desktop", DESKTOP_VIEWPORT, diffAvailable) : Promise.resolve<{ url?: string | undefined; png?: Uint8Array | undefined }>({ url: afterPlaceholder }),
+      afterPage ? capturePage(env, target, afterPage, "after", "mobile", MOBILE_VIEWPORT, diffAvailable) : Promise.resolve<{ url?: string | undefined; png?: Uint8Array | undefined }>({ url: afterPlaceholder }),
+    ]);
+    // A diff needs BOTH sides' real bytes — a placeholder/dash slot (no preview yet, auth-walled, render
+    // failure) has no `png`, so compareCapturedScreenshots degrades to null exactly like a missing shot does.
+    const [desktopDiff, mobileDiff] = diffAvailable
+      ? await Promise.all([
+          compareCapturedScreenshots(beforeShot.png, afterShot.png),
+          compareCapturedScreenshots(beforeMobileShot.png, afterMobileShot.png),
+        ])
+      : [null, null];
+    const [diffUrl, diffUrlMobile] = await Promise.all([
+      uploadDiffImage(env, target, path, "desktop", desktopDiff),
+      uploadDiffImage(env, target, path, "mobile", mobileDiff),
     ]);
     captureRoutes.push({
       path,
       beforeUrl: beforeShot.url,
       beforeUrlMobile: beforeMobileShot.url,
       afterUrl: afterShot.url,
       afterUrlMobile: afterMobileShot.url,
+      ...(diffUrl ? { diffUrl } : {}),
+      ...(diffUrlMobile ? { diffUrlMobile } : {}),
     });
   }
   return { routes: captureRoutes, previewPending };

src/review/visual/pixel-diff.ts

@@ -0,0 +1,33 @@
+// Pixel-comparison provider seam for the before/after capture pipeline (#3674). WORKER-SAFE DEFAULT: a no-op.
+//
+// The real screenshot-comparison logic (the self-host-only module under `src/visual-agent/`) depends on
+// Node's `Buffer` and a native-leaning PNG-decode step, which the Cloudflare Workers runtime doesn't
+// guarantee — that's why `test/unit/worker-entry-boundary.test.ts` forbids importing (or even naming, in
+// worker-reachable file content) that module from the Worker entry (`src/index.ts`). This file is the seam:
+// `capture.ts` (which IS Worker-reachable) imports ONLY this file, never the self-host module directly.
+// `scripts/build-selfhost.mjs`'s esbuild plugin swaps this exact specifier for a real implementation when
+// bundling the self-host entry (`src/server.ts`) — the SAME module-substitution pattern already used for
+// `@cloudflare/puppeteer` in that same build. The Worker's own (wrangler) bundle never applies that swap, so
+// hosted mode always uses this no-op — zero behavior change, zero added cost, until a Workers-compatible
+// pixel-comparison path exists.
+export type VisualDiffOutcome = {
+  status: "changed" | "unchanged" | "new" | "removed";
+  changedPixelPercent: number | null;
+  diffImagePng: Uint8Array | null;
+};
+
+/** True when this build can actually compute a pixel diff (self-host only, see module header). Callers use
+ *  this to decide whether it's worth paying the extra cost of holding/fetching screenshot bytes at all —
+ *  always false here, so nothing about the existing capture path changes in hosted mode. */
+export function isVisualDiffAvailable(): boolean {
+  return false;
+}
+
+/** Compare two screenshots. Always null in the Worker-safe default — self-host's swapped-in implementation
+ *  does the real comparison. Callers must treat null as "no diff available for this cell", never a failure. */
+export async function compareCapturedScreenshots(
+  _before: Uint8Array | null | undefined,
+  _after: Uint8Array | null | undefined,
+): Promise<VisualDiffOutcome | null> {
+  return null;
+}

src/selfhost/stubs/pixel-diff.ts

@@ -0,0 +1,33 @@
+// Self-host replacement for src/review/visual/pixel-diff.ts (#3674). Swapped in by
+// scripts/build-selfhost.mjs's esbuild plugin, the same mechanism used for @cloudflare/puppeteer — this
+// file is only ever bundled into dist/server.mjs, never the Worker entry, so it's safe to depend on
+// pixelmatch/pngjs (Node `Buffer` + PNG decode) here. Unlike puppeteer-core, pixelmatch/pngjs are
+// unconditional package.json dependencies (no INSTALL_VISUAL_REVIEW-style opt-in), so a plain static
+// import is fine — no lazy runtime import needed.
+import { compareRouteScreenshots } from "../../visual-agent/visual-diff";
+import type { VisualDiffOutcome } from "../../review/visual/pixel-diff";
+
+export function isVisualDiffAvailable(): boolean {
+  return true;
+}
+
+export async function compareCapturedScreenshots(
+  before: Uint8Array | null | undefined,
+  after: Uint8Array | null | undefined,
+): Promise<VisualDiffOutcome | null> {
+  if (!before && !after) return null;
+  try {
+    const result = compareRouteScreenshots({
+      route: "",
+      before: before ? Buffer.from(before) : null,
+      after: after ? Buffer.from(after) : null,
+    });
+    return {
+      status: result.status,
+      changedPixelPercent: result.changedPixelPercent,
+      diffImagePng: result.diffImagePng ? new Uint8Array(result.diffImagePng) : null,
+    };
+  } catch {
+    return null;
+  }
+}

test/unit/pixel-diff.test.ts

@@ -0,0 +1,17 @@
+import { describe, expect, it } from "vitest";
+import { compareCapturedScreenshots, isVisualDiffAvailable } from "../../src/review/visual/pixel-diff";
+
+describe("pixel-diff Worker-safe default (#3674)", () => {
+  it("reports diffing as unavailable", () => {
+    expect(isVisualDiffAvailable()).toBe(false);
+  });
+
+  it("always resolves to null regardless of input, since the real implementation is self-host only", async () => {
+    const before = new Uint8Array([1, 2, 3]);
+    const after = new Uint8Array([4, 5, 6]);
+    await expect(compareCapturedScreenshots(before, after)).resolves.toBeNull();
+    await expect(compareCapturedScreenshots(before, undefined)).resolves.toBeNull();
+    await expect(compareCapturedScreenshots(null, after)).resolves.toBeNull();
+    await expect(compareCapturedScreenshots(null, null)).resolves.toBeNull();
+  });
+});