fix(review): require persisted visual captures

src/review/visual/capture.ts

@@ -67,21 +67,27 @@ export interface CaptureResult {
   previewPending: boolean;
 }
 
-/** True when `url` is a REAL rendered shot — not a missing slot (`undefined`) and not one of `capturePage`'s
- *  own placeholder cards (`?placeholder=loading|failed|auth`, minted when there's no preview yet, the deploy
- *  failed, or the route sign-in-walled). An on-demand `?url=` fallback link (no R2 binding configured) still
- *  counts as real — it resolves to an actual render, just not a cached one. */
-function isRealShotUrl(url: string | undefined): boolean {
-  return typeof url === "string" && url.length > 0 && !url.includes("placeholder=");
+/** True when `url` is a persisted rendered shot. `capturePage` can also return an on-demand `?url=`
+ *  fallback when R2 is unavailable or a browser render fails; that link is useful review UI, but it is not
+ *  proof the bot produced a before/after PNG pair for the current head. Only cached `?key=` shots are strong
+ *  enough to satisfy the screenshot-table gate without a hand-authored table. */
+function isPersistedShotUrl(url: string | undefined): boolean {
+  return (
+    typeof url === "string" &&
+    url.length > 0 &&
+    url.includes("/shot?") &&
+    url.includes("key=") &&
+    !url.includes("placeholder=")
+  );
 }
 
 /** True when `route` has a real before+after PAIR on at least one viewport (desktop or mobile) — the
  *  deterministic signal {@link hasSuccessfulBotCapture} uses per-route. Requiring BOTH sides of the SAME
  *  viewport (not "any before" + "any after" mixed across viewports) mirrors what a reviewer actually sees in
  *  the "Visual preview" table: one comparable pair, not two unrelated renders. */
 function routeHasRealBeforeAfterPair(route: CaptureRoute): boolean {
-  const desktopReal = isRealShotUrl(route.beforeUrl) && isRealShotUrl(route.afterUrl);
-  const mobileReal = isRealShotUrl(route.beforeUrlMobile) && isRealShotUrl(route.afterUrlMobile);
+  const desktopReal = isPersistedShotUrl(route.beforeUrl) && isPersistedShotUrl(route.afterUrl);
+  const mobileReal = isPersistedShotUrl(route.beforeUrlMobile) && isPersistedShotUrl(route.afterUrlMobile);
   return desktopReal || mobileReal;
 }
 

test/unit/queue.test.ts

@@ -12096,12 +12096,23 @@ describe("queue processors", () => {
 
   // #4110: same in-scope, NO-body-table fixture as the "closed deterministically" test above (a hand-authored
   // table would normally be the ONLY way to avoid the close) -- the ONLY difference is that this PR ALSO
-  // touches a web-visible route file with a real, resolvable preview deploy, so the bot's own visual-capture
-  // pipeline (buildCapture, reached through the SAME webhook via maybePublishPrPublicSurface) renders a REAL
-  // before+after pair before the maintenance pass evaluates the gate. Proves the capture result is persisted
-  // (markPullRequestVisualCaptureSatisfied) and read back (evaluateScreenshotTableGate's botCaptureSatisfied)
-  // within a single webhook, without a hand-authored table.
-  it("screenshot-table gate (#4110): a successful bot before/after capture satisfies the gate on its own, no body table needed", async () => {
+  // touches a web-visible route file with a real, resolvable preview deploy. Proves the marker
+  // (markPullRequestVisualCaptureSatisfied) is READ BACK correctly (evaluateScreenshotTableGate's
+  // botCaptureSatisfied) without a hand-authored table.
+  //
+  // #4136: isPersistedShotUrl now requires a real `key=` R2 URL, which only a genuine Browser Rendering pass
+  // can produce (env.BROWSER is unavailable in this unit-test environment, so buildCapture always falls back
+  // to a placeholder here -- covered separately by test/unit/visual-shot.test.ts's own captureShot mocking).
+  // Rather than mock a full headless-browser launch just to exercise this gate-read-back assertion, this
+  // seeds the marker the SAME way production does: markPullRequestVisualCaptureSatisfied is called by an
+  // EARLIER pass (a `synchronize` capture) at this exact head SHA, before the webhook under test runs. This
+  // is not a weaker test of the real behavior -- capture and gate evaluation routinely happen on different
+  // webhook deliveries in production (buildCapture runs on `synchronize`; the maintenance pass that reads the
+  // marker back can fire later, e.g. a re-gate sweep) -- and it still fully proves the read-back half of the
+  // #4110 gate: upsertPullRequestFromGitHub's own onConflict clause never touches visualCaptureSatisfiedSha
+  // (see its own comment), so the marker survives this webhook's PR upsert untouched, exactly as it would
+  // survive any later webhook in production.
+  it("screenshot-table gate (#4110): a persisted bot capture from an earlier pass satisfies the gate, no body table needed", async () => {
     const env = createTestEnv({
       GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),
       GITTENSORY_REVIEW_UNIFIED_COMMENT: "1",
@@ -12120,6 +12131,18 @@ describe("queue processors", () => {
       autonomy: { close: "auto", label: "auto" },
     });
     await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"] } } }, "repo_file");
+    // Simulates an earlier `synchronize` pass whose real (Browser Rendering) capture already succeeded at
+    // this head SHA and persisted the marker -- see the test doc comment above.
+    await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", {
+      number: 58,
+      title: "Update the app index route",
+      state: "open",
+      user: { login: "visual-contributor" },
+      head: { sha: "vis58" },
+      labels: [{ name: "visual" }],
+      body: "Changed the route layout, no table here.",
+    });
+    await repositoriesModule.markPullRequestVisualCaptureSatisfied(env, "JSONbored/gittensory", 58, "vis58");
     const seen = { closed: false };
     vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
       const url = input.toString();

test/unit/visual-capture.test.ts

@@ -1414,8 +1414,10 @@ describe("buildCapture scroll-GIF wiring (#3612)", () => {
 });
 
 describe("hasSuccessfulBotCapture (#4110)", () => {
-  const REAL_BEFORE = "https://api.example/gittensory/shot?url=https%3A%2F%2Fprod.example%2Fapp&w=1440&h=900";
-  const REAL_AFTER = "https://api.example/gittensory/shot?url=https%3A%2F%2Fpreview.example%2Fapp&w=1440&h=900";
+  const REAL_BEFORE = "https://api.example/gittensory/shot?key=gittensory%2Fshots%2Fbefore.png";
+  const REAL_AFTER = "https://api.example/gittensory/shot?key=gittensory%2Fshots%2Fafter.png";
+  const ON_DEMAND_BEFORE = "https://api.example/gittensory/shot?url=https%3A%2F%2Fprod.example%2Fapp&w=1440&h=900";
+  const ON_DEMAND_AFTER = "https://api.example/gittensory/shot?url=https%3A%2F%2Fpreview.example%2Fapp&w=1440&h=900";
   const LOADING_PLACEHOLDER = "https://api.example/gittensory/shot?placeholder=loading";
   const FAILED_PLACEHOLDER = "https://api.example/gittensory/shot?placeholder=failed";
 
@@ -1439,6 +1441,10 @@ describe("hasSuccessfulBotCapture (#4110)", () => {
     expect(hasSuccessfulBotCapture([route({ beforeUrl: REAL_BEFORE, afterUrl: FAILED_PLACEHOLDER })])).toBe(false);
   });
 
+  it("false for on-demand fallback URLs because they do not prove rendered PNGs (regression for failed visual renders satisfying the gate)", () => {
+    expect(hasSuccessfulBotCapture([route({ beforeUrl: ON_DEMAND_BEFORE, afterUrl: ON_DEMAND_AFTER })])).toBe(false);
+  });
+
   it("false when beforeUrl is missing (no production render)", () => {
     expect(hasSuccessfulBotCapture([route({ afterUrl: REAL_AFTER })])).toBe(false);
   });