Skip to content

Commit 105da06

Browse files
committed
fix(gate): never close a PR for visual evidence the pipeline cannot produce (#9881)
`screenshotTableGate.action: "close"` closed PRs when no visual evidence was found. Correct when evidence is merely absent; wrong when it was never obtainable. A repo with `review.visual.enabled` but no preview deploys at all (JSONbored/awesome-claude) exhausts its poll budget, never sets visualCaptureSatisfiedSha, and every contributor PR touching a visual-scoped path is closed one-shot -- for evidence no contributor action could ever supply, against a misconfiguration nobody was told about. The proof was already in the capture: build state `absent` means no preview check-run was found AT ALL, and the file already noted this was "confirmed live on a repo whose UI has no preview-deploy CI configured". Paired with a spent poll budget, that distinguishes "no pipeline" from a late deploy or a renderer blip, which is exactly the distinction the close was missing. The violation is UNCHANGED -- the PR really does lack visual evidence and the finding still says so. Only enforcement degrades, and the contributor-visible reason names the maintainer-side remedy, since no contributor action can resolve it. Marked per head SHA like its siblings, so a later push re-arms the requirement and a repo that gains preview deploys stops matching on its very next commit.
1 parent badf9fb commit 105da06

9 files changed

Lines changed: 179 additions & 10 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
-- Visual capture that is structurally UNOBTAINABLE for a repo (#9881).
2+
--
3+
-- `screenshotTableGate.action: "close"` closes a PR when no visual evidence is found. That is correct when
4+
-- evidence is merely absent. It is wrong when the pipeline could never have produced it: a repo with
5+
-- `review.visual.enabled` but NO preview deployments at all (JSONbored/awesome-claude is the live case)
6+
-- exhausts its preview-poll budget, never sets `visual_capture_satisfied_sha`, and every contributor PR
7+
-- touching a visual-scoped path is then closed one-shot for evidence no contributor action could supply.
8+
--
9+
-- This column records the head SHA at which the bot PROVED the distinction: the deployments read succeeded,
10+
-- reported no deployment whatsoever (not a failed one, not an API error), and the poll budget is spent.
11+
-- The gate degrades its CLOSE to advisory for exactly that head and says why, rather than destroying a PR
12+
-- over a pipeline gap the maintainer was never told about.
13+
--
14+
-- Scoped to head SHA like its siblings (visual_capture_satisfied_sha, visual_capture_retry_pending_sha), so
15+
-- a later commit re-arms the requirement and a repo that gains preview deploys stops matching immediately.
16+
ALTER TABLE pull_requests ADD COLUMN visual_capture_unobtainable_sha TEXT;

packages/loopover-engine/src/review/screenshot-table-gate.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -339,6 +339,12 @@ export type ScreenshotTableGateResult = {
339339
* across a push (stale -- #stale-screenshot-table-fix / #8866) or the contributor genuinely re-affirmed it.
340340
* Absent on every other NO_VIOLATION path (disabled/out-of-scope/bot-capture), and on a violation. */
341341
presenceModeSatisfiedState?: ScreenshotTablePresenceEvidence | undefined;
342+
/** #9881: set when the violation stands but ENFORCEMENT must not. The bot proved this repo produces no
343+
* preview deployment at all, so `review.visual.enabled` can never satisfy the gate here and a CLOSE would
344+
* destroy PRs over evidence no contributor action could supply. The finding is still raised -- the PR
345+
* really does lack visual evidence -- but the caller degrades `action: "close"` to advisory and says this.
346+
* Absent whenever enforcement may proceed normally. */
347+
enforcementDegradedReason?: string | undefined;
342348
};
343349

344350
/** One presence/matrix-mode "satisfied" checkpoint: the head SHA it was satisfied at, plus a fingerprint of the
@@ -395,7 +401,26 @@ function evidenceFreshnessForHead(
395401
* evidence otherwise passes but is STALE -- the exact same before/after evidence already satisfied the gate
396402
* for a prior, different head SHA (see `headSha`/`presenceModeSatisfied` below and the inline comment at the
397403
* check itself) -- a screenshot table from push #1 must not silently keep passing through pushes #2..#N. */
398-
export function evaluateScreenshotTableGate(input: {
404+
/** #9881: the reason attached when enforcement is degraded. Stated in the contributor-visible comment, so
405+
* the PR author learns the gate could not be satisfied here rather than being left to guess. */
406+
export const CAPTURE_UNOBTAINABLE_REASON =
407+
"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`.";
408+
409+
/**
410+
* The gate, with #9881's enforcement degrade applied.
411+
*
412+
* The violation itself is decided by the pure evaluator below and is UNCHANGED by the degrade: a PR with no
413+
* visual evidence still has no visual evidence, and the finding still says so. What changes is whether that
414+
* finding may be acted on. When the bot has proved the repo cannot produce a capture at all, a CLOSE would
415+
* punish a contributor for a maintainer-side pipeline gap that no contributor action can close.
416+
*/
417+
export function evaluateScreenshotTableGate(input: Parameters<typeof evaluateScreenshotTableGateViolation>[0]): ScreenshotTableGateResult {
418+
const result = evaluateScreenshotTableGateViolation(input);
419+
if (!result.violated || input.captureUnobtainable !== true) return result;
420+
return { ...result, enforcementDegradedReason: CAPTURE_UNOBTAINABLE_REASON };
421+
}
422+
423+
function evaluateScreenshotTableGateViolation(input: {
399424
config: ScreenshotTableGateConfig;
400425
prBody: string | null | undefined;
401426
prLabels: string[];
@@ -417,6 +442,11 @@ export function evaluateScreenshotTableGate(input: {
417442
* `visualCaptureSatisfiedSha === headSha` check). `null`/undefined ⇒ never satisfied before (or the caller
418443
* has no persistence wired up yet). */
419444
presenceModeSatisfied?: ScreenshotTablePresenceEvidence | null | undefined;
445+
/** #9881: true when the bot PROVED visual capture is structurally unobtainable for this repo at this head
446+
* -- the deployments read succeeded, found none at all, and the poll budget is spent. Does NOT suppress
447+
* the violation (the PR genuinely has no visual evidence); it degrades ENFORCEMENT, so a maintainer still
448+
* sees the finding but a contributor does not lose their PR to a pipeline gap. */
449+
captureUnobtainable?: boolean | undefined;
420450
}): ScreenshotTableGateResult {
421451
const { config } = input;
422452
if (!config.enabled) return NO_VIOLATION;

src/db/repositories.ts

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4687,7 +4687,7 @@ export async function markPullRequestVisualCaptureSatisfied(env: Env, fullName:
46874687
// #9030: a proven-successful capture for this head supersedes any earlier "retry pending" marker recorded
46884688
// for the SAME head (an error or a still-building preview on an earlier attempt) -- clearing it here keeps
46894689
// the row's state minimal instead of leaving a now-moot marker sitting alongside a satisfied one.
4690-
.set({ visualCaptureSatisfiedSha: headSha, visualCaptureRetryPendingSha: null, visualCaptureRetryPendingAt: null, updatedAt: nowIso() })
4690+
.set({ visualCaptureSatisfiedSha: headSha, visualCaptureRetryPendingSha: null, visualCaptureRetryPendingAt: null, visualCaptureUnobtainableSha: null, updatedAt: nowIso() })
46914691
.where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
46924692
}
46934693

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

4715+
/** Structurally unobtainable capture (#9881): record that for `headSha` the bot PROVED the repo produces no
4716+
* preview deployment at all -- the deployments read SUCCEEDED (not an API error, not a failed deploy) and
4717+
* returned nothing, and the preview-poll budget for this head is spent.
4718+
*
4719+
* That distinction is the whole point. "No evidence found" is a legitimate reason to close a visual PR;
4720+
* "evidence was never obtainable" is not, because no contributor action other than hand-authoring the table
4721+
* could ever change it, and nothing told the maintainer their configuration was unsatisfiable. The
4722+
* screenshotTableGate degrades its CLOSE to advisory while this equals the current head.
4723+
*
4724+
* Scoped to headSha like its siblings, so a later commit re-arms the requirement and a repo that gains
4725+
* preview deploys stops matching on its very next push. */
4726+
export async function markPullRequestVisualCaptureUnobtainable(env: Env, fullName: string, number: number, headSha: string): Promise<void> {
4727+
const db = getDb(env.DB);
4728+
await db
4729+
.update(pullRequests)
4730+
.set({ visualCaptureUnobtainableSha: headSha, updatedAt: nowIso() })
4731+
.where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
4732+
}
4733+
47154734
/** Release the retry latch: the retry chain that justified it has ended without a successful capture, so the
47164735
* screenshotTableGate must stop deferring and evaluate the evidence actually present.
47174736
*

src/db/schema.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,11 @@ export const pullRequests = sqliteTable(
494494
// a sha with no timestamp reads as EXPIRED, which is both honest (the row predates the column) and what
495495
// releases the PRs already stuck behind one. loopover-computed, written with the sha, cleared with it.
496496
visualCaptureRetryPendingAt: text("visual_capture_retry_pending_at"),
497+
// #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable for this repo --
498+
// the deployments read succeeded, found none at all, and the poll budget is spent. The screenshot-table
499+
// gate degrades its CLOSE to advisory for that head rather than closing a PR over evidence no
500+
// contributor action could produce.
501+
visualCaptureUnobtainableSha: text("visual_capture_unobtainable_sha"),
497502
// Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix, follow-up to #2006).
498503
// JSON `{headSha, evidenceFingerprint}` -- the head SHA and before/after-image-URL fingerprint that last
499504
// satisfied screenshotTableGate's presence-mode check (see evaluateScreenshotTableGate's staleness comment).

src/queue/processors.ts

Lines changed: 44 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ import {
5858
markPullRequestVisualCaptureSatisfied,
5959
clearPullRequestVisualCaptureRetryPending,
6060
markPullRequestVisualCaptureRetryPending,
61+
markPullRequestVisualCaptureUnobtainable,
6162
markPullRequestScreenshotTablePresenceSatisfied,
6263
getLatestRegatedAt,
6364
getLatestBacklogConvergenceRegatedAt,
@@ -3525,6 +3526,9 @@ async function runAgentMaintenancePlanAndExecute(
35253526
botCaptureSatisfied,
35263527
headSha: pr.headSha,
35273528
presenceModeSatisfied: pr.screenshotTablePresenceSatisfied,
3529+
// #9881: proved-unobtainable for THIS head only -- a later push re-arms the requirement, and a repo
3530+
// that gains preview deploys stops matching on its very next commit.
3531+
captureUnobtainable: Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === pr.headSha,
35283532
});
35293533
// #9030: a visual-capture pipeline ERROR (browserless down, timeout, GitHub hiccup) or a still-building
35303534
// preview looked IDENTICAL to "capture concluded normally, no visual evidence found" -- both left
@@ -3561,14 +3565,35 @@ async function runAgentMaintenancePlanAndExecute(
35613565
`so the screenshot-table gate is evaluating on the evidence actually present instead of deferring again`,
35623566
});
35633567
}
3568+
// #9881: the bot proved this repo produces no preview deployment at all, so `review.visual.enabled` can
3569+
// never satisfy this gate here. The violation stands and still surfaces as a finding, but a CLOSE would
3570+
// destroy a contributor's PR over a maintainer-side pipeline gap no contributor action can close, and
3571+
// which nothing had ever told the maintainer about. Degrade to advisory and say so.
3572+
const screenshotTableEnforcementDegraded = screenshotTableGateResult.enforcementDegradedReason !== undefined;
35643573
const screenshotTableMatch =
3565-
screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && !botCaptureRetryPending
3574+
screenshotTableGateResult.violated &&
3575+
screenshotTableGateConfig.action === "close" &&
3576+
!botCaptureRetryPending &&
3577+
!screenshotTableEnforcementDegraded
35663578
? { matched: true, reason: screenshotTableGateResult.reason }
35673579
: undefined;
3580+
if (screenshotTableEnforcementDegraded) {
3581+
await recordAuditEvent(env, {
3582+
eventType: "github_app.screenshot_table_close_degraded_capture_unobtainable",
3583+
actor: null,
3584+
targetKey: `${repoFullName}#${pr.number}`,
3585+
outcome: "completed",
3586+
detail: screenshotTableGateResult.enforcementDegradedReason ?? null,
3587+
metadata: { headSha: pr.headSha ?? null, configuredAction: screenshotTableGateConfig.action },
3588+
}).catch(() => undefined);
3589+
}
35683590
// #9462: deferring the CLOSE is only half a deferral -- on its own it let the plan fall through to a MERGE.
35693591
// Thread the unresolved state into the planner so it holds the PR instead of silently skipping the gate.
35703592
const screenshotTableEvidenceUnresolved =
3571-
screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && botCaptureRetryPending;
3593+
screenshotTableGateResult.violated &&
3594+
screenshotTableGateConfig.action === "close" &&
3595+
botCaptureRetryPending &&
3596+
!screenshotTableEnforcementDegraded;
35723597
if (screenshotTableEvidenceUnresolved) {
35733598
await recordAuditEvent(env, {
35743599
eventType: "github_app.screenshot_table_close_deferred_capture_retry",
@@ -12902,7 +12927,7 @@ async function maybePublishPrPublicSurface(
1290212927
// ordinary "nothing found" capture would, with no separate code path to maintain.
1290312928
const capture =
1290412929
reviewVisualConfig.enabled === false
12905-
? { routes: [], interactions: [], previewPending: false, renderFailed: false }
12930+
? { routes: [], interactions: [], previewPending: false, renderFailed: false, previewUnobtainable: false }
1290612931
: await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig, changedCssFiles);
1290712932
beforeAfter = capture.routes;
1290812933
interactionPreviews = capture.interactions;
@@ -12953,6 +12978,22 @@ async function maybePublishPrPublicSurface(
1295312978
previewPollAttempt,
1295412979
});
1295512980
} else if (pr.headSha) {
12981+
// #9881: this capture concluded, and it concluded because the repo has no preview pipeline at all
12982+
// (build state `absent` across every poll, budget now spent) -- not because a deploy was late or
12983+
// a renderer blipped. Record it so the screenshot-table gate degrades its CLOSE instead of
12984+
// destroying a PR over evidence no contributor action could produce.
12985+
if (capture.previewUnobtainable) {
12986+
await markPullRequestVisualCaptureUnobtainable(env, repoFullName, pr.number, pr.headSha).catch((error) => {
12987+
console.log(
12988+
JSON.stringify({
12989+
event: "visual_capture_unobtainable_mark_failed",
12990+
repoFullName,
12991+
pull: pr.number,
12992+
message: errorMessage(error).slice(0, 200),
12993+
}),
12994+
);
12995+
});
12996+
}
1295612997
// Conclusive: release any latch this head still carries. Best-effort and idempotent -- the common
1295712998
// case is that there is no latch to clear, and a failed clear only leaves the age bound in
1295812999
// visual-capture-retry-latch.ts to end the deferral instead of ending it now.

src/review/visual/capture.ts

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,11 @@ export interface CaptureResult {
101101
* browserless outage.
102102
*/
103103
renderFailed: boolean;
104+
/** #9881: the preview is not late or broken -- this repo has NO preview pipeline at all. Set only when the
105+
* build state was `absent` (no preview check-run found whatsoever, across every poll) AND the per-head
106+
* poll budget is spent. That pair is the proof the screenshot-table gate needs to tell "no evidence found"
107+
* apart from "evidence was never obtainable" before it closes anyone's PR. */
108+
previewUnobtainable: boolean;
104109
}
105110

106111
/** True when `url` is a persisted rendered shot. `capturePage` can also return an on-demand `?url=`
@@ -700,6 +705,7 @@ export async function buildCapture(
700705
// building) so we can show a terminal "deploy failed" card instead of a spinner.
701706
let previewBase = "";
702707
let previewFailed = target.previewFailed === true;
708+
let previewUnobtainable = false;
703709
let previewPending = false;
704710
// Hoisted above the discovery block below (was previously computed after it) so the eternal-"loading"-
705711
// placeholder fix's `buildState === "absent"` branch can consult it -- seeing this whole file top to
@@ -754,6 +760,10 @@ export async function buildCapture(
754760
const attempts = await previewPollAttemptCount(env, target.headSha);
755761
if (attempts >= MAX_PREVIEW_POLL_ATTEMPTS) {
756762
previewFailed = true;
763+
// #9881: `absent` means no preview check-run was ever found, and the budget is now spent -- so
764+
// this is not a late or broken deploy, it is a repo with no preview pipeline. Recording it is
765+
// what lets the screenshot-table gate decline to CLOSE on evidence it could never obtain.
766+
previewUnobtainable = true;
757767
} else {
758768
await recordPreviewPollAttempt(env, target.headSha);
759769
previewPending = true;
@@ -958,5 +968,5 @@ export async function buildCapture(
958968
}
959969
}
960970

961-
return { routes: captureRoutes, interactions: interactionRoutes, previewPending, renderFailed };
971+
return { routes: captureRoutes, interactions: interactionRoutes, previewPending, renderFailed, previewUnobtainable };
962972
}

src/types.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -762,6 +762,10 @@ export type PullRequestRecord = {
762762
* screenshotTableGate treats visualCaptureSatisfiedSha === headSha as evidence equivalent to a hand-authored
763763
* before/after table. Publish-written; read straight from the row. */
764764
visualCaptureSatisfiedSha?: string | null | undefined;
765+
/** #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable here -- the
766+
* deployments read succeeded, found none at all, and the poll budget is spent. The screenshotTableGate
767+
* degrades its CLOSE to advisory while this equals the current head. */
768+
visualCaptureUnobtainableSha?: string | null | undefined;
765769
/** False-positive close guard (#9030): the head SHA a bounded visual-capture recapture retry is currently
766770
* scheduled/in-flight for -- set only when the capture pipeline errored, or the preview is still building,
767771
* AND a retry budget attempt remains. While this equals the PR's current headSha, the screenshotTableGate's

0 commit comments

Comments
 (0)