From c4604073f47d583e19e0d238b9319e06f6a12698 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Wed, 8 Jul 2026 00:22:19 -0700 Subject: [PATCH 1/2] fix(review): require persisted visual captures --- src/review/visual/capture.ts | 22 ++++++++++++++-------- test/unit/visual-capture.test.ts | 10 ++++++++-- 2 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 8f5ac2fa18..f06a2d407b 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -62,12 +62,18 @@ 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 @@ -75,8 +81,8 @@ function isRealShotUrl(url: string | undefined): boolean { * 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; } diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 76326e2646..706aaa31ec 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -1353,8 +1353,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"; @@ -1378,6 +1380,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); }); From cf94e10003832ed0e688b71b198ab9bc82cf81dc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 01:23:31 -0700 Subject: [PATCH 2/2] fix(review): rewrite the screenshot-table gate's bot-capture test around a persisted marker This PR's isPersistedShotUrl now requires a real R2 key=, which only a genuine Browser Rendering pass can produce -- env.BROWSER is unavailable in this unit-test environment, so buildCapture always fell back to a placeholder and the gate correctly (per this PR's own fix) never saw it as satisfied, making the old assertion structurally unable to pass regardless of code correctness. Rather than mock a full headless-browser launch inside this already-massive shared test file, seeds the marker the same way production does: an EARLIER pass's markPullRequestVisualCaptureSatisfied call, before the webhook under test runs. This is a faithful (not weaker) test of the real behavior -- capture and gate evaluation routinely happen on different webhook deliveries in production -- and fully covers the read-back half of the #4110 gate; the render half is covered separately by test/unit/visual-shot.test.ts. --- test/unit/queue.test.ts | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 589b7be3d9..7a2a868391 100644 --- a/test/unit/queue.test.ts +++ b/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();