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: 16 additions & 0 deletions migrations/0207_visual_capture_unobtainable.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
-- Visual capture that is structurally UNOBTAINABLE for a repo (#9881).
--
-- `screenshotTableGate.action: "close"` closes a PR when no visual evidence is found. That is correct when
-- evidence is merely absent. It is wrong when the pipeline could never have produced it: a repo with
-- `review.visual.enabled` but NO preview deployments at all (JSONbored/awesome-claude is the live case)
-- exhausts its preview-poll budget, never sets `visual_capture_satisfied_sha`, and every contributor PR
-- touching a visual-scoped path is then closed one-shot for evidence no contributor action could supply.
--
-- This column records the head SHA at which the bot PROVED the distinction: the deployments read succeeded,
-- reported no deployment whatsoever (not a failed one, not an API error), and the poll budget is spent.
-- The gate degrades its CLOSE to advisory for exactly that head and says why, rather than destroying a PR
-- over a pipeline gap the maintainer was never told about.
--
-- Scoped to head SHA like its siblings (visual_capture_satisfied_sha, visual_capture_retry_pending_sha), so
-- a later commit re-arms the requirement and a repo that gains preview deploys stops matching immediately.
ALTER TABLE pull_requests ADD COLUMN visual_capture_unobtainable_sha TEXT;
32 changes: 31 additions & 1 deletion packages/loopover-engine/src/review/screenshot-table-gate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,12 @@ export type ScreenshotTableGateResult = {
* across a push (stale -- #stale-screenshot-table-fix / #8866) or the contributor genuinely re-affirmed it.
* Absent on every other NO_VIOLATION path (disabled/out-of-scope/bot-capture), and on a violation. */
presenceModeSatisfiedState?: ScreenshotTablePresenceEvidence | undefined;
/** #9881: set when the violation stands but ENFORCEMENT must not. The bot proved this repo produces no
* preview deployment at all, so `review.visual.enabled` can never satisfy the gate here and a CLOSE would
* destroy PRs over evidence no contributor action could supply. The finding is still raised -- the PR
* really does lack visual evidence -- but the caller degrades `action: "close"` to advisory and says this.
* Absent whenever enforcement may proceed normally. */
enforcementDegradedReason?: string | undefined;
};

/** One presence/matrix-mode "satisfied" checkpoint: the head SHA it was satisfied at, plus a fingerprint of the
Expand Down Expand Up @@ -395,7 +401,26 @@ function evidenceFreshnessForHead(
* evidence otherwise passes but is STALE -- the exact same before/after evidence already satisfied the gate
* for a prior, different head SHA (see `headSha`/`presenceModeSatisfied` below and the inline comment at the
* check itself) -- a screenshot table from push #1 must not silently keep passing through pushes #2..#N. */
export function evaluateScreenshotTableGate(input: {
/** #9881: the reason attached when enforcement is degraded. Stated in the contributor-visible comment, so
* the PR author learns the gate could not be satisfied here rather than being left to guess. */
export const CAPTURE_UNOBTAINABLE_REASON =
"This repository has `review.visual.enabled` but produces no preview deployment, so the bot cannot capture the AFTER screenshot — the screenshot-table requirement is reported here but not enforced. A maintainer needs to either enable preview deploys or set `requireScreenshotTable.action: advisory`.";

/**
* The gate, with #9881's enforcement degrade applied.
*
* The violation itself is decided by the pure evaluator below and is UNCHANGED by the degrade: a PR with no
* visual evidence still has no visual evidence, and the finding still says so. What changes is whether that
* finding may be acted on. When the bot has proved the repo cannot produce a capture at all, a CLOSE would
* punish a contributor for a maintainer-side pipeline gap that no contributor action can close.
*/
export function evaluateScreenshotTableGate(input: Parameters<typeof evaluateScreenshotTableGateViolation>[0]): ScreenshotTableGateResult {
const result = evaluateScreenshotTableGateViolation(input);
if (!result.violated || input.captureUnobtainable !== true) return result;
return { ...result, enforcementDegradedReason: CAPTURE_UNOBTAINABLE_REASON };
}

function evaluateScreenshotTableGateViolation(input: {
config: ScreenshotTableGateConfig;
prBody: string | null | undefined;
prLabels: string[];
Expand All @@ -417,6 +442,11 @@ export function evaluateScreenshotTableGate(input: {
* `visualCaptureSatisfiedSha === headSha` check). `null`/undefined ⇒ never satisfied before (or the caller
* has no persistence wired up yet). */
presenceModeSatisfied?: ScreenshotTablePresenceEvidence | null | undefined;
/** #9881: true when the bot PROVED visual capture is structurally unobtainable for this repo at this head
* -- the deployments read succeeded, found none at all, and the poll budget is spent. Does NOT suppress
* the violation (the PR genuinely has no visual evidence); it degrades ENFORCEMENT, so a maintainer still
* sees the finding but a contributor does not lose their PR to a pipeline gap. */
captureUnobtainable?: boolean | undefined;
}): ScreenshotTableGateResult {
const { config } = input;
if (!config.enabled) return NO_VIOLATION;
Expand Down
21 changes: 20 additions & 1 deletion src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4687,7 +4687,7 @@ export async function markPullRequestVisualCaptureSatisfied(env: Env, fullName:
// #9030: a proven-successful capture for this head supersedes any earlier "retry pending" marker recorded
// for the SAME head (an error or a still-building preview on an earlier attempt) -- clearing it here keeps
// the row's state minimal instead of leaving a now-moot marker sitting alongside a satisfied one.
.set({ visualCaptureSatisfiedSha: headSha, visualCaptureRetryPendingSha: null, visualCaptureRetryPendingAt: null, updatedAt: nowIso() })
.set({ visualCaptureSatisfiedSha: headSha, visualCaptureRetryPendingSha: null, visualCaptureRetryPendingAt: null, visualCaptureUnobtainableSha: null, updatedAt: nowIso() })
.where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
}

Expand All @@ -4712,6 +4712,25 @@ export async function markPullRequestVisualCaptureRetryPending(env: Env, fullNam
.where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
}

/** Structurally unobtainable capture (#9881): record that for `headSha` the bot PROVED the repo produces no
* preview deployment at all -- the deployments read SUCCEEDED (not an API error, not a failed deploy) and
* returned nothing, and the preview-poll budget for this head is spent.
*
* That distinction is the whole point. "No evidence found" is a legitimate reason to close a visual PR;
* "evidence was never obtainable" is not, because no contributor action other than hand-authoring the table
* could ever change it, and nothing told the maintainer their configuration was unsatisfiable. The
* screenshotTableGate degrades its CLOSE to advisory while this equals the current head.
*
* Scoped to headSha like its siblings, so a later commit re-arms the requirement and a repo that gains
* preview deploys stops matching on its very next push. */
export async function markPullRequestVisualCaptureUnobtainable(env: Env, fullName: string, number: number, headSha: string): Promise<void> {
const db = getDb(env.DB);
await db
.update(pullRequests)
.set({ visualCaptureUnobtainableSha: headSha, updatedAt: nowIso() })
.where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
}

/** Release the retry latch: the retry chain that justified it has ended without a successful capture, so the
* screenshotTableGate must stop deferring and evaluate the evidence actually present.
*
Expand Down
5 changes: 5 additions & 0 deletions src/db/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,11 @@ export const pullRequests = sqliteTable(
// a sha with no timestamp reads as EXPIRED, which is both honest (the row predates the column) and what
// releases the PRs already stuck behind one. loopover-computed, written with the sha, cleared with it.
visualCaptureRetryPendingAt: text("visual_capture_retry_pending_at"),
// #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable for this repo --
// the deployments read succeeded, found none at all, and the poll budget is spent. The screenshot-table
// gate degrades its CLOSE to advisory for that head rather than closing a PR over evidence no
// contributor action could produce.
visualCaptureUnobtainableSha: text("visual_capture_unobtainable_sha"),
// Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix, follow-up to #2006).
// JSON `{headSha, evidenceFingerprint}` -- the head SHA and before/after-image-URL fingerprint that last
// satisfied screenshotTableGate's presence-mode check (see evaluateScreenshotTableGate's staleness comment).
Expand Down
47 changes: 44 additions & 3 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import {
markPullRequestVisualCaptureSatisfied,
clearPullRequestVisualCaptureRetryPending,
markPullRequestVisualCaptureRetryPending,
markPullRequestVisualCaptureUnobtainable,
markPullRequestScreenshotTablePresenceSatisfied,
getLatestRegatedAt,
getLatestBacklogConvergenceRegatedAt,
Expand Down Expand Up @@ -3525,6 +3526,9 @@ async function runAgentMaintenancePlanAndExecute(
botCaptureSatisfied,
headSha: pr.headSha,
presenceModeSatisfied: pr.screenshotTablePresenceSatisfied,
// #9881: proved-unobtainable for THIS head only -- a later push re-arms the requirement, and a repo
// that gains preview deploys stops matching on its very next commit.
captureUnobtainable: Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === pr.headSha,
});
// #9030: a visual-capture pipeline ERROR (browserless down, timeout, GitHub hiccup) or a still-building
// preview looked IDENTICAL to "capture concluded normally, no visual evidence found" -- both left
Expand Down Expand Up @@ -3561,14 +3565,35 @@ async function runAgentMaintenancePlanAndExecute(
`so the screenshot-table gate is evaluating on the evidence actually present instead of deferring again`,
});
}
// #9881: the bot proved this repo produces no preview deployment at all, so `review.visual.enabled` can
// never satisfy this gate here. The violation stands and still surfaces as a finding, but a CLOSE would
// destroy a contributor's PR over a maintainer-side pipeline gap no contributor action can close, and
// which nothing had ever told the maintainer about. Degrade to advisory and say so.
const screenshotTableEnforcementDegraded = screenshotTableGateResult.enforcementDegradedReason !== undefined;
const screenshotTableMatch =
screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && !botCaptureRetryPending
screenshotTableGateResult.violated &&
screenshotTableGateConfig.action === "close" &&
!botCaptureRetryPending &&
!screenshotTableEnforcementDegraded
? { matched: true, reason: screenshotTableGateResult.reason }
: undefined;
if (screenshotTableEnforcementDegraded) {
await recordAuditEvent(env, {
eventType: "github_app.screenshot_table_close_degraded_capture_unobtainable",
actor: null,
targetKey: `${repoFullName}#${pr.number}`,
outcome: "completed",
detail: screenshotTableGateResult.enforcementDegradedReason ?? null,
metadata: { headSha: pr.headSha ?? null, configuredAction: screenshotTableGateConfig.action },
}).catch(() => undefined);
}
// #9462: deferring the CLOSE is only half a deferral -- on its own it let the plan fall through to a MERGE.
// Thread the unresolved state into the planner so it holds the PR instead of silently skipping the gate.
const screenshotTableEvidenceUnresolved =
screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && botCaptureRetryPending;
screenshotTableGateResult.violated &&
screenshotTableGateConfig.action === "close" &&
botCaptureRetryPending &&
!screenshotTableEnforcementDegraded;
if (screenshotTableEvidenceUnresolved) {
await recordAuditEvent(env, {
eventType: "github_app.screenshot_table_close_deferred_capture_retry",
Expand Down Expand Up @@ -12902,7 +12927,7 @@ async function maybePublishPrPublicSurface(
// ordinary "nothing found" capture would, with no separate code path to maintain.
const capture =
reviewVisualConfig.enabled === false
? { routes: [], interactions: [], previewPending: false, renderFailed: false }
? { routes: [], interactions: [], previewPending: false, renderFailed: false, previewUnobtainable: false }
: await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig, changedCssFiles);
beforeAfter = capture.routes;
interactionPreviews = capture.interactions;
Expand Down Expand Up @@ -12953,6 +12978,22 @@ async function maybePublishPrPublicSurface(
previewPollAttempt,
});
} else if (pr.headSha) {
// #9881: this capture concluded, and it concluded because the repo has no preview pipeline at all
// (build state `absent` across every poll, budget now spent) -- not because a deploy was late or
// a renderer blipped. Record it so the screenshot-table gate degrades its CLOSE instead of
// destroying a PR over evidence no contributor action could produce.
if (capture.previewUnobtainable) {
await markPullRequestVisualCaptureUnobtainable(env, repoFullName, pr.number, pr.headSha).catch((error) => {
console.log(
JSON.stringify({
event: "visual_capture_unobtainable_mark_failed",
repoFullName,
pull: pr.number,
message: errorMessage(error).slice(0, 200),
}),
);
});
}
// Conclusive: release any latch this head still carries. Best-effort and idempotent -- the common
// case is that there is no latch to clear, and a failed clear only leaves the age bound in
// visual-capture-retry-latch.ts to end the deferral instead of ending it now.
Expand Down
12 changes: 11 additions & 1 deletion src/review/visual/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ export interface CaptureResult {
* browserless outage.
*/
renderFailed: boolean;
/** #9881: the preview is not late or broken -- this repo has NO preview pipeline at all. Set only when the
* build state was `absent` (no preview check-run found whatsoever, across every poll) AND the per-head
* poll budget is spent. That pair is the proof the screenshot-table gate needs to tell "no evidence found"
* apart from "evidence was never obtainable" before it closes anyone's PR. */
previewUnobtainable: boolean;
}

/** True when `url` is a persisted rendered shot. `capturePage` can also return an on-demand `?url=`
Expand Down Expand Up @@ -700,6 +705,7 @@ export async function buildCapture(
// building) so we can show a terminal "deploy failed" card instead of a spinner.
let previewBase = "";
let previewFailed = target.previewFailed === true;
let previewUnobtainable = false;
let previewPending = 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
Expand Down Expand Up @@ -754,6 +760,10 @@ export async function buildCapture(
const attempts = await previewPollAttemptCount(env, target.headSha);
if (attempts >= MAX_PREVIEW_POLL_ATTEMPTS) {
previewFailed = true;
// #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;
} else {
await recordPreviewPollAttempt(env, target.headSha);
previewPending = true;
Expand Down Expand Up @@ -958,5 +968,5 @@ export async function buildCapture(
}
}

return { routes: captureRoutes, interactions: interactionRoutes, previewPending, renderFailed };
return { routes: captureRoutes, interactions: interactionRoutes, previewPending, renderFailed, previewUnobtainable };
}
4 changes: 4 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,10 @@ export type PullRequestRecord = {
* screenshotTableGate treats visualCaptureSatisfiedSha === headSha as evidence equivalent to a hand-authored
* before/after table. Publish-written; read straight from the row. */
visualCaptureSatisfiedSha?: string | null | undefined;
/** #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable here -- the
* deployments read succeeded, found none at all, and the poll budget is spent. The screenshotTableGate
* degrades its CLOSE to advisory while this equals the current head. */
visualCaptureUnobtainableSha?: string | null | undefined;
/** False-positive close guard (#9030): the head SHA a bounded visual-capture recapture retry is currently
* scheduled/in-flight for -- set only when the capture pipeline errored, or the preview is still building,
* AND a retry budget attempt remains. While this equals the PR's current headSha, the screenshotTableGate's
Expand Down
Loading