Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/review/visual/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,9 @@ export async function buildCapture(
let previewFailed = target.previewFailed === true;
let previewUnobtainable = false;
let previewPending = false;
// Whether the deployments read below threw. The structurally-unobtainable flag's contract (#9881) is that
// the deployments read SUCCEEDED and found none — so a call where it threw must never set the flag (#10059).
let deploymentsReadFailed = false;
// Hoisted above the discovery block below (was previously computed after it) so the eternal-"loading"-
// placeholder fix's `buildState === "absent"` branch can consult it -- seeing this whole file top to
// bottom, its own later use (guarding the actions_fallback dispatch) is unchanged.
Expand All @@ -721,8 +724,13 @@ export async function buildCapture(
const status = await getLatestDeploymentStatus({ token, repo, sha: target.headSha, ref: target.headRef, apiVersion, rateLimitAdmissionKey });
previewBase = status.url ?? "";
previewFailed = status.failed;
// getLatestDeploymentStatus reports a read failure (403/rate-limit/5xx) via `error: true` rather than
// throwing — that call did NOT prove there is no deployment, so it must suppress previewUnobtainable
// exactly as a thrown read does (#10059).
if (status.error === true) deploymentsReadFailed = true;
} catch {
previewBase = "";
deploymentsReadFailed = true;
}
if (!previewBase && !previewFailed && target.previewFromChecks && target.headSha) {
previewBase = (await findPreviewUrlFromChecks({ token, repo, sha: target.headSha, apiVersion, rateLimitAdmissionKey })) ?? "";
Expand All @@ -745,7 +753,7 @@ export async function buildCapture(
await recordPreviewPollAttempt(env, target.headSha);
previewPending = true;
}
} else if (buildState === "absent" && !actionsFallbackEnabled) {
} else if ((buildState === "absent" || buildState === "unreadable") && !actionsFallbackEnabled) {
// Eternal-"loading"-placeholder fix: 'absent' means no Workers-Builds-named check-run was found
// AT ALL, not "still building" -- previously this fell through as a silent no-op, leaving
// previewPending/previewFailed both false, so the caller's afterPlaceholder always resolved to
Expand All @@ -763,7 +771,11 @@ export async function buildCapture(
// #9881: `absent` means no preview check-run was ever found, and the budget is now spent -- so
// this is not a late or broken deploy, it is a repo with no preview pipeline. Recording it is
// what lets the screenshot-table gate decline to CLOSE on evidence it could never obtain.
previewUnobtainable = true;
// #10059: record it ONLY on a PROVEN absence — the check-run read succeeded (`absent`, not the
// `unreadable` catch) AND the deployments read succeeded. A transient read failure is not proof.
if (buildState === "absent" && !deploymentsReadFailed) {
previewUnobtainable = true;
}
} else {
await recordPreviewPollAttempt(env, target.headSha);
previewPending = true;
Expand Down
10 changes: 6 additions & 4 deletions src/review/visual/preview-url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,16 +307,18 @@ export async function findPreviewUrlFromPrComments(params: {
/**
* State of the per-PR preview BUILD (Cloudflare Workers Builds check-run) for a head SHA, so capture can tell
* "still building / its URL-comment is just lagging" (keep polling) apart from "failed" (show the terminal
* failed card) and "no preview build at all" (don't poll). Returns 'absent' on any read failure (fail-safe:
* never an infinite poll on a transient error).
* failed card) and "no preview build at all" (don't poll). A SUCCESSFUL read that finds no matching check-run
* returns 'absent'; a read that THREW returns 'unreadable' (still a value — the "never an infinite poll on a
* transient error" fail-safe is preserved — but a distinct one, so a caller can tell a proven "no build here"
* apart from "couldn't check", e.g. before recording structurally-unobtainable capture, #10059).
*/
export async function getPreviewBuildState(params: {
token: string;
repo: GitHubRepo;
sha: string;
apiVersion?: string | undefined;
rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined;
}): Promise<"building" | "succeeded" | "failed" | "absent"> {
}): Promise<"building" | "succeeded" | "failed" | "absent" | "unreadable"> {
const base = `https://api.github.com/repos/${params.repo.owner}/${params.repo.repo}`;
const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey };
try {
Expand All @@ -333,7 +335,7 @@ export async function getPreviewBuildState(params: {
);
return state ?? "absent";
} catch {
return "absent";
return "unreadable";
}
}

Expand Down
7 changes: 5 additions & 2 deletions test/unit/preview-url.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,9 +204,10 @@ describe("preview-url pagination (#7450)", () => {
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "s3" })).resolves.toBe("absent");
});

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

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

Expand Down
65 changes: 65 additions & 0 deletions test/unit/visual-capture.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,71 @@ describe("visual capture preview discovery", () => {
await expect(previewPollAttemptCount(env, "budget-head-2")).resolves.toBe(MAX_PREVIEW_POLL_ATTEMPTS);
});

const captureAt = (env: Env, headSha: string, prNumber: number) =>
buildCapture(env, "installation-token", { repoFullName: "owner/repo", prNumber, headSha, previewFromChecks: true }, ["apps/loopover-ui/src/routes/app.index.tsx"]);

it("REGRESSION (#10059): a rejecting check-runs read at an exhausted budget yields previewUnobtainable false, not a proven absence", async () => {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/deployments?")) return Response.json([]);
if (url.includes("/status")) return Response.json({ statuses: [] });
if (url.includes("/check-runs")) throw new Error("simulated GitHub read failure");
if (url.includes("/comments")) return Response.json([]);
return new Response("not found", { status: 404 });
});
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) await recordPreviewPollAttempt(env, "unreadable-head");
const result = await captureAt(env, "unreadable-head", 21);
expect(result.previewUnobtainable).toBe(false);
expect(result.previewPending).toBe(false);
});

it("#9881 pinned: a SUCCEEDING check-runs read that finds no preview build at an exhausted budget yields previewUnobtainable true", async () => {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/deployments?")) return Response.json([]);
if (url.includes("/status")) return Response.json({ statuses: [] });
if (url.includes("/check-runs")) return Response.json({ check_runs: [{ name: "lint", status: "completed", conclusion: "success" }] });
if (url.includes("/comments")) return Response.json([]);
return new Response("not found", { status: 404 });
});
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) await recordPreviewPollAttempt(env, "absent-head");
const result = await captureAt(env, "absent-head", 22);
expect(result.previewUnobtainable).toBe(true);
});

it("REGRESSION (#10059): a rejecting deployments read suppresses previewUnobtainable even when the check-runs read succeeds empty", async () => {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/deployments?")) throw new Error("simulated deployments read failure");
if (url.includes("/status")) return Response.json({ statuses: [] });
if (url.includes("/check-runs")) return Response.json({ check_runs: [{ name: "lint", status: "completed", conclusion: "success" }] });
if (url.includes("/comments")) return Response.json([]);
return new Response("not found", { status: 404 });
});
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
for (let i = 0; i < MAX_PREVIEW_POLL_ATTEMPTS; i += 1) await recordPreviewPollAttempt(env, "deploy-fail-head");
const result = await captureAt(env, "deploy-fail-head", 23);
expect(result.previewUnobtainable).toBe(false);
});

it("REGRESSION (#10059): a sustained GitHub read failure across the whole poll budget never marks capture structurally unobtainable", async () => {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
if (url.includes("/deployments?")) throw new Error("read failure");
if (url.includes("/check-runs")) throw new Error("read failure");
if (url.includes("/comments")) return Response.json([]);
if (url.includes("/status")) return Response.json({ statuses: [] });
return new Response("not found", { status: 404 });
});
const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() });
for (let i = 0; i <= MAX_PREVIEW_POLL_ATTEMPTS; i += 1) {
const result = await captureAt(env, "sustained-fail-head", 24);
expect(result.previewUnobtainable).toBe(false);
}
});

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 () => {
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
const url = input.toString();
Expand Down