Skip to content

Commit d1c105e

Browse files
RealDiligentRealDiligent
andauthored
fix(visual): only mark capture structurally-unobtainable on a proven, not a failed, read (#10129)
previewUnobtainable (#9881) records a PROVEN fact — the deployments read succeeded, the check-run read succeeded and found no preview build at all, and the poll budget is spent — and the screenshot-table gate degrades its CLOSE to advisory on it. But getPreviewBuildState returns "absent" on ANY read failure (fail-safe), and getLatestDeploymentStatus reports a non-404 failure as { error: true }. So a transient GitHub read failure was recorded as "this repo has no preview pipeline", degrading the gate on evidence the bot never actually obtained. getPreviewBuildState now returns a distinct "unreadable" on a thrown read (still a value — the never-infinite-poll contract holds — but distinguishable from a genuine empty read). buildCapture treats "unreadable" exactly like "absent" for polling/previewFailed, tracks whether getLatestDeploymentStatus reported error:true (or threw), and sets previewUnobtainable only when the check-run read genuinely succeeded empty AND the deployments read succeeded. Poll budget, previewPending/previewFailed/renderFailed for the both-reads-succeeded cases, and the gate/repository writer are byte-identical. Closes #10059 Co-authored-by: RealDiligent <nft.gold.eth@gmail.com>
1 parent cc6db92 commit d1c105e

4 files changed

Lines changed: 90 additions & 8 deletions

File tree

src/review/visual/capture.ts

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -707,6 +707,9 @@ export async function buildCapture(
707707
let previewFailed = target.previewFailed === true;
708708
let previewUnobtainable = false;
709709
let previewPending = false;
710+
// Whether the deployments read below threw. The structurally-unobtainable flag's contract (#9881) is that
711+
// the deployments read SUCCEEDED and found none — so a call where it threw must never set the flag (#10059).
712+
let deploymentsReadFailed = false;
710713
// Hoisted above the discovery block below (was previously computed after it) so the eternal-"loading"-
711714
// placeholder fix's `buildState === "absent"` branch can consult it -- seeing this whole file top to
712715
// bottom, its own later use (guarding the actions_fallback dispatch) is unchanged.
@@ -721,8 +724,13 @@ export async function buildCapture(
721724
const status = await getLatestDeploymentStatus({ token, repo, sha: target.headSha, ref: target.headRef, apiVersion, rateLimitAdmissionKey });
722725
previewBase = status.url ?? "";
723726
previewFailed = status.failed;
727+
// getLatestDeploymentStatus reports a read failure (403/rate-limit/5xx) via `error: true` rather than
728+
// throwing — that call did NOT prove there is no deployment, so it must suppress previewUnobtainable
729+
// exactly as a thrown read does (#10059).
730+
if (status.error === true) deploymentsReadFailed = true;
724731
} catch {
725732
previewBase = "";
733+
deploymentsReadFailed = true;
726734
}
727735
if (!previewBase && !previewFailed && target.previewFromChecks && target.headSha) {
728736
previewBase = (await findPreviewUrlFromChecks({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey })) ?? "";
@@ -745,7 +753,7 @@ export async function buildCapture(
745753
await recordPreviewPollAttempt(env, target.headSha);
746754
previewPending = true;
747755
}
748-
} else if (buildState === "absent" && !actionsFallbackEnabled) {
756+
} else if ((buildState === "absent" || buildState === "unreadable") && !actionsFallbackEnabled) {
749757
// Eternal-"loading"-placeholder fix: 'absent' means no Workers-Builds-named check-run was found
750758
// AT ALL, not "still building" -- previously this fell through as a silent no-op, leaving
751759
// previewPending/previewFailed both false, so the caller's afterPlaceholder always resolved to
@@ -763,7 +771,11 @@ export async function buildCapture(
763771
// #9881: `absent` means no preview check-run was ever found, and the budget is now spent -- so
764772
// this is not a late or broken deploy, it is a repo with no preview pipeline. Recording it is
765773
// what lets the screenshot-table gate decline to CLOSE on evidence it could never obtain.
766-
previewUnobtainable = true;
774+
// #10059: record it ONLY on a PROVEN absence — the check-run read succeeded (`absent`, not the
775+
// `unreadable` catch) AND the deployments read succeeded. A transient read failure is not proof.
776+
if (buildState === "absent" && !deploymentsReadFailed) {
777+
previewUnobtainable = true;
778+
}
767779
} else {
768780
await recordPreviewPollAttempt(env, target.headSha);
769781
previewPending = true;

src/review/visual/preview-url.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -307,16 +307,18 @@ export async function findPreviewUrlFromPrComments(params: {
307307
/**
308308
* State of the per-PR preview BUILD (Cloudflare Workers Builds check-run) for a head SHA, so capture can tell
309309
* "still building / its URL-comment is just lagging" (keep polling) apart from "failed" (show the terminal
310-
* failed card) and "no preview build at all" (don't poll). Returns 'absent' on any read failure (fail-safe:
311-
* never an infinite poll on a transient error).
310+
* failed card) and "no preview build at all" (don't poll). A SUCCESSFUL read that finds no matching check-run
311+
* returns 'absent'; a read that THREW returns 'unreadable' (still a value — the "never an infinite poll on a
312+
* transient error" fail-safe is preserved — but a distinct one, so a caller can tell a proven "no build here"
313+
* apart from "couldn't check", e.g. before recording structurally-unobtainable capture, #10059).
312314
*/
313315
export async function getPreviewBuildState(params: {
314316
token: string;
315317
repo: GitHubRepo;
316318
sha: string;
317319
apiVersion?: string | undefined;
318320
rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined;
319-
}): Promise<"building" | "succeeded" | "failed" | "absent"> {
321+
}): Promise<"building" | "succeeded" | "failed" | "absent" | "unreadable"> {
320322
const base = `https://api.github.com/repos/${params.repo.owner}/${params.repo.repo}`;
321323
const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey };
322324
try {
@@ -333,7 +335,7 @@ export async function getPreviewBuildState(params: {
333335
);
334336
return state ?? "absent";
335337
} catch {
336-
return "absent";
338+
return "unreadable";
337339
}
338340
}
339341

test/unit/preview-url.test.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -204,9 +204,10 @@ describe("preview-url pagination (#7450)", () => {
204204
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "s3" })).resolves.toBe("absent");
205205
});
206206

207-
it("getPreviewBuildState is bounded and degrades to absent on a later-page failure", async () => {
207+
it("getPreviewBuildState is bounded, returns absent when the read completes empty, and unreadable when a later-page read fails (#10059)", async () => {
208208
const spin = vi.fn(async () => Response.json({ check_runs: [] }, { headers: { link: NEXT_LINK } }));
209209
vi.stubGlobal("fetch", spin);
210+
// A read that COMPLETED across every page and found no build is a genuine absence.
210211
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "spin" })).resolves.toBe("absent");
211212
expect(spin).toHaveBeenCalledTimes(10); // PREVIEW_LIST_MAX_PAGES
212213

@@ -215,7 +216,9 @@ describe("preview-url pagination (#7450)", () => {
215216
return Response.json({ check_runs: [] }, { headers: { link: NEXT_LINK } });
216217
});
217218
vi.stubGlobal("fetch", failLater);
218-
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("absent");
219+
// A read that THREW (here, mid-pagination) never proved absence — it is unreadable, still a value so the
220+
// caller never infinite-polls, but distinct from a genuine empty read (#10059).
221+
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("unreadable");
219222
expect(failLater).toHaveBeenCalledTimes(2);
220223
});
221224

test/unit/visual-capture.test.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,71 @@ describe("visual capture preview discovery", () => {
342342
await expect(previewPollAttemptCount(env, "budget-head-2")).resolves.toBe(MAX_PREVIEW_POLL_ATTEMPTS);
343343
});
344344

345+
const captureAt = (env: Env, headSha: string, prNumber: number) =>
346+
buildCapture(env, "installation-token", { repoFullName: "owner/repo", prNumber, headSha, previewFromChecks: true }, ["apps/loopover-ui/src/routes/app.index.tsx"]);
347+
348+
it("REGRESSION (#10059): a rejecting check-runs read at an exhausted budget yields previewUnobtainable false, not a proven absence", async () => {
349+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
350+
const url = input.toString();
351+
if (url.includes("/deployments?")) return Response.json([]);
352+
if (url.includes("/status")) return Response.json({ statuses: [] });
353+
if (url.includes("/check-runs")) throw new Error("simulated GitHub read failure");
354+
if (url.includes("/comments")) return Response.json([]);
355+
return new Response("not found", { status: 404 });
356+
});
357+
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
358+
for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) await recordPreviewPollAttempt(env, "unreadable-head");
359+
const result = await captureAt(env, "unreadable-head", 21);
360+
expect(result.previewUnobtainable).toBe(false);
361+
expect(result.previewPending).toBe(false);
362+
});
363+
364+
it("#9881 pinned: a SUCCEEDING check-runs read that finds no preview build at an exhausted budget yields previewUnobtainable true", async () => {
365+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
366+
const url = input.toString();
367+
if (url.includes("/deployments?")) return Response.json([]);
368+
if (url.includes("/status")) return Response.json({ statuses: [] });
369+
if (url.includes("/check-runs")) return Response.json({ check_runs: [{ name: "lint", status: "completed", conclusion: "success" }] });
370+
if (url.includes("/comments")) return Response.json([]);
371+
return new Response("not found", { status: 404 });
372+
});
373+
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
374+
for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) await recordPreviewPollAttempt(env, "absent-head");
375+
const result = await captureAt(env, "absent-head", 22);
376+
expect(result.previewUnobtainable).toBe(true);
377+
});
378+
379+
it("REGRESSION (#10059): a rejecting deployments read suppresses previewUnobtainable even when the check-runs read succeeds empty", async () => {
380+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
381+
const url = input.toString();
382+
if (url.includes("/deployments?")) throw new Error("simulated deployments read failure");
383+
if (url.includes("/status")) return Response.json({ statuses: [] });
384+
if (url.includes("/check-runs")) return Response.json({ check_runs: [{ name: "lint", status: "completed", conclusion: "success" }] });
385+
if (url.includes("/comments")) return Response.json([]);
386+
return new Response("not found", { status: 404 });
387+
});
388+
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
389+
for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) await recordPreviewPollAttempt(env, "deploy-fail-head");
390+
const result = await captureAt(env, "deploy-fail-head", 23);
391+
expect(result.previewUnobtainable).toBe(false);
392+
});
393+
394+
it("REGRESSION (#10059): a sustained GitHub read failure across the whole poll budget never marks capture structurally unobtainable", async () => {
395+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
396+
const url = input.toString();
397+
if (url.includes("/deployments?")) throw new Error("read failure");
398+
if (url.includes("/check-runs")) throw new Error("read failure");
399+
if (url.includes("/comments")) return Response.json([]);
400+
if (url.includes("/status")) return Response.json({ statuses: [] });
401+
return new Response("not found", { status: 404 });
402+
});
403+
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
404+
for (let i = 0; i <= MAX_PREVIEW_POLL_ATTEMPTS; i += 1) {
405+
const result = await captureAt(env, "sustained-fail-head", 24);
406+
expect(result.previewUnobtainable).toBe(false);
407+
}
408+
});
409+
345410
it("eternal-loading-placeholder fix: marks the capture pending (not silently ignored) when no matching preview check run exists at all (buildState 'absent') and no actions_fallback is configured", async () => {
346411
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
347412
const url = input.toString();

0 commit comments

Comments
 (0)