fix(review): correlate screenshot-table presence-mode evidence to the PR head SHA

migrations/0156_pull_request_screenshot_table_presence_satisfied.sql

@@ -0,0 +1,6 @@
+-- Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix, follow-up to #2006). JSON
+-- `{headSha, evidenceFingerprint}` of the head SHA and before/after-image-URL fingerprint that last satisfied
+-- screenshotTableGate's presence-mode check. NULL means presence mode has never satisfied the gate for this PR.
+-- Mirrors visual_capture_satisfied_sha's headSha-keying, but also fingerprints the evidence itself since
+-- presence mode (unlike bot-capture) has no independently-verified render to key satisfaction on alone.
+ALTER TABLE pull_requests ADD COLUMN screenshot_table_presence_satisfied_json TEXT;

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

@@ -326,20 +326,44 @@ export const DEFAULT_SCREENSHOT_CONTRACT_MESSAGE =
 export type ScreenshotTableGateResult = {
   violated: boolean;
   reason: string | null;
+  /** Set ONLY when PRESENCE mode (never matrix mode, never bot-capture -- see the staleness comment on
+   *  `evaluateScreenshotTableGate` below) independently satisfied the gate on THIS evaluation. The caller
+   *  should persist this (mirrors `markPullRequestVisualCaptureSatisfied`'s headSha-keyed write) so a LATER
+   *  evaluation on a NEW head SHA can tell whether the same static body evidence is being silently reused
+   *  across a push (stale -- #stale-screenshot-table-fix) or the contributor genuinely re-affirmed it. Absent
+   *  on every other NO_VIOLATION path (disabled/out-of-scope/bot-capture/matrix), and on a violation. */
+  presenceModeSatisfiedState?: ScreenshotTablePresenceEvidence | undefined;
 };
 
+/** One presence-mode "satisfied" checkpoint: the head SHA it was satisfied at, plus a fingerprint of the
+ *  exact evidence (before/after image URLs) that satisfied it -- see {@link evaluateScreenshotTableGate}'s
+ *  staleness check and {@link presenceModeEvidenceFingerprint}. */
+export type ScreenshotTablePresenceEvidence = { headSha: string; evidenceFingerprint: string };
+
 const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null };
 
+/** A deterministic fingerprint of the presence-mode EVIDENCE in `body` -- the before/after image URL pairs a
+ *  contributor's table actually contributes as proof, not the surrounding prose/caption text (which can churn
+ *  harmlessly without the evidence itself changing). Reuses {@link extractTableRowImageUrls} (the same
+ *  >=2-images-per-row extraction the matrix-mode row check already treats as "a real before/after pair") so a
+ *  caption edit or table reflow that doesn't touch the actual image URLs still fingerprints identically. */
+function presenceModeEvidenceFingerprint(body: string | null | undefined): string {
+  return JSON.stringify(extractTableRowImageUrls(body));
+}
+
 /** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation.
  *  `botCaptureSatisfied` ⇒ no violation regardless of mode (an automated capture is equivalent to a
  *  hand-authored table, and the bot doesn't (yet) shoot a full viewport/theme matrix -- see #4535's scope note).
  *
  *  Two modes, chosen by whether `config.requireViewports` is non-empty (#4535):
  *  - MATRIX mode: every required (viewport, theme) pair (`requiredScreenshotMatrixPairs`) must have a labeled
  *    before/after row. Violated ⇒ the reason names exactly which pairs are still missing.
- *  - PRESENCE mode (the original #2006 behavior, unchanged): in scope AND (no image-bearing table in the body
- *    OR an image pasted outside a table OR a committed image file under a scoped path) ⇒ violated, with the
- *    configured (or default) templated message as the reason. */
+ *  - PRESENCE mode (the original #2006 behavior): in scope AND (no image-bearing table in the body OR an image
+ *    pasted outside a table OR a committed image file under a scoped path) ⇒ violated, with the configured (or
+ *    default) templated message as the reason. #stale-screenshot-table-fix: ALSO violated when the body's
+ *    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: {
   config: ScreenshotTableGateConfig;
   prBody: string | null | undefined;
@@ -352,6 +376,16 @@ export function evaluateScreenshotTableGate(input: {
    *  help, which doesn't apply once the bot has already proven the change visually. Absent/false ⇒
    *  byte-identical to pre-#4110 behavior (body-table evidence only). */
   botCaptureSatisfied?: boolean | undefined;
+  /** The PR's current head SHA, for PRESENCE-mode staleness correlation only (matrix mode and bot-capture
+   *  already have their own head-SHA-correct evidence paths -- see the staleness comment below). Absent/empty
+   *  ⇒ byte-identical to pre-fix behavior (no correlation possible without it), matching this function's
+   *  existing "malformed/missing input degrades gracefully" convention. */
+  headSha?: string | null | undefined;
+  /** The (headSha, evidenceFingerprint) checkpoint PRESENCE mode was last confirmed satisfied at for this PR,
+   *  persisted by the caller from a PRIOR call's `presenceModeSatisfiedState` (mirrors how `botCaptureSatisfied`
+   *  above is itself derived by the caller from a persisted `visualCaptureSatisfiedSha === headSha` check).
+   *  `null`/undefined ⇒ never satisfied before (or the caller has no persistence wired up yet). */
+  presenceModeSatisfied?: ScreenshotTablePresenceEvidence | null | undefined;
 }): ScreenshotTableGateResult {
   const { config } = input;
   if (!config.enabled) return NO_VIOLATION;
@@ -368,6 +402,34 @@ export function evaluateScreenshotTableGate(input: {
   const hasTable = hasImageBearingMarkdownTable(input.prBody);
   const outsideTable = hasImageOutsideTable(input.prBody);
   const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths);
-  if (hasTable && !outsideTable && !committedImage) return NO_VIOLATION;
+  if (hasTable && !outsideTable && !committedImage) {
+    // #stale-screenshot-table-fix: unlike botCaptureSatisfied above (keyed to headSha by construction), this
+    // presence check is pure regex/string matching over `prBody` with NO tie to the PR's live head SHA at all
+    // -- a table pasted on push #1 keeps matching byte-for-byte on pushes #2..#N even after they ship a real
+    // visual regression the table never pictured. FAIL CLOSED (this codebase's existing philosophy for
+    // ambiguous review states, e.g. linked-issue hard rules): if we know the current head SHA and this EXACT
+    // evidence (the extracted before/after image URLs, not surrounding prose) already satisfied the gate for a
+    // DIFFERENT, older head, treat it as still-violated -- the contributor must either genuinely re-affirm
+    // (edit the body so the extracted evidence differs, even by re-uploading to the same table position -- a
+    // fresh GitHub upload gets a fresh URL) or let the bot's own capture pipeline take over for the new head.
+    // A headSha we've never seen satisfied before (first table ever, or the caller has no persistence wired up)
+    // is NOT stale -- there is nothing to be stale relative to.
+    const headSha = input.headSha;
+    const priorSatisfied = input.presenceModeSatisfied;
+    const evidenceFingerprint = presenceModeEvidenceFingerprint(input.prBody);
+    const staleForNewHead =
+      typeof headSha === "string" &&
+      headSha.length > 0 &&
+      priorSatisfied != null &&
+      priorSatisfied.headSha !== headSha &&
+      priorSatisfied.evidenceFingerprint === evidenceFingerprint;
+    if (!staleForNewHead) {
+      return {
+        violated: false,
+        reason: null,
+        ...(typeof headSha === "string" && headSha.length > 0 ? { presenceModeSatisfiedState: { headSha, evidenceFingerprint } } : {}),
+      };
+    }
+  }
   return { violated: true, reason: config.message ?? appendSkillLink(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, config.skillFileUrl) };
 }

src/db/repositories.ts

@@ -3994,6 +3994,24 @@ export async function markPullRequestVisualCaptureSatisfied(env: Env, fullName:
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
 }
 
+/** Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix): record the (headSha,
+ *  evidenceFingerprint) checkpoint at which evaluateScreenshotTableGate's presence-mode check just satisfied
+ *  the gate for this PR (see that function's `presenceModeSatisfiedState` result field and staleness comment).
+ *  Mirrors markPullRequestVisualCaptureSatisfied's headSha-scoped WHERE (a live head that advanced between
+ *  evaluation and this write makes the UPDATE no-op rather than stamp a stale head). */
+export async function markPullRequestScreenshotTablePresenceSatisfied(
+  env: Env,
+  fullName: string,
+  number: number,
+  state: { headSha: string; evidenceFingerprint: string },
+): Promise<void> {
+  const db = getDb(env.DB);
+  await db
+    .update(pullRequests)
+    .set({ screenshotTablePresenceSatisfiedJson: jsonString(state), updatedAt: nowIso() })
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, state.headSha)));
+}
+
 /** Sweep convergence: stamp the timestamp the scheduled re-gate sweep just recomputed this PR. A plain D1 UPDATE
  *  — NOT routed through the agent-action-executor chokepoint (#1258) — so it advances even when GitHub writes are
  *  suppressed (dry-run / paused). selectRegateCandidates orders the sweep by last_regated_at, so a just-regated PR
@@ -6399,6 +6417,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull
     linkedIssueHardRuleViolatedAt: row.linkedIssueHardRuleViolatedAt,
     linkedIssueHardRuleViolationReason: row.linkedIssueHardRuleViolationReason,
     visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha,
+    screenshotTablePresenceSatisfied: parseJson<{ headSha: string; evidenceFingerprint: string } | null>(row.screenshotTablePresenceSatisfiedJson, null),
   };
 }
 

src/db/schema.ts

@@ -527,6 +527,15 @@ export const pullRequests = sqliteTable(
     // new head. loopover-computed (publish-written), omitted from the GitHub-sync SET clause so a later sync
     // cannot clobber it.
     visualCaptureSatisfiedSha: text("visual_capture_satisfied_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).
+    // Unlike visual_capture_satisfied_sha above, presence mode has no bot-verified render to key on, so this
+    // stores BOTH the head it was satisfied at AND a fingerprint of the exact evidence -- a later push (new
+    // head) carrying the SAME UNCHANGED evidence is stale and must re-violate; a genuinely different fingerprint
+    // (the contributor re-affirmed) refreshes it. loopover-computed (planner-written), omitted from the
+    // GitHub-sync SET clause so a later sync cannot clobber it.
+    screenshotTablePresenceSatisfiedJson: text("screenshot_table_presence_satisfied_json"),
     createdAt: text("created_at").notNull().$defaultFn(() => nowIso()),
     updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()),
   },

src/queue/processors.ts

@@ -55,6 +55,7 @@ import {
   markPullRequestReviewsInvalidated,
   markPullRequestSurfacePublished,
   markPullRequestVisualCaptureSatisfied,
+  markPullRequestScreenshotTablePresenceSatisfied,
   getLatestRegatedAt,
   getLatestBacklogConvergenceRegatedAt,
   claimRegateFanoutSlot,
@@ -2753,11 +2754,31 @@ async function runAgentMaintenancePlanAndExecute(
     prLabels: pr.labels,
     changedFiles: changedPaths,
     botCaptureSatisfied,
+    headSha: pr.headSha,
+    presenceModeSatisfied: pr.screenshotTablePresenceSatisfied,
   });
   const screenshotTableMatch =
     screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close"
       ? { matched: true, reason: screenshotTableGateResult.reason }
       : undefined;
+  // #stale-screenshot-table-fix: presence mode just independently re-confirmed the gate for THIS head SHA --
+  // persist the (headSha, evidenceFingerprint) checkpoint so a LATER push that carries the SAME UNCHANGED
+  // evidence correctly re-violates instead of silently staying green forever (see evaluateScreenshotTableGate's
+  // staleness comment). Best-effort, mirrors markPullRequestVisualCaptureSatisfied's call site: a write failure
+  // here just means the next evaluation can't tell this evidence was already checked, never blocks the rest of
+  // the maintenance pass.
+  if (screenshotTableGateResult.presenceModeSatisfiedState) {
+    await markPullRequestScreenshotTablePresenceSatisfied(env, repoFullName, pr.number, screenshotTableGateResult.presenceModeSatisfiedState).catch((error) => {
+      console.log(
+        JSON.stringify({
+          event: "screenshot_table_presence_satisfied_mark_failed",
+          repoFullName,
+          pull: pr.number,
+          message: errorMessage(error).slice(0, 200),
+        }),
+      );
+    });
+  }
 
   // Account-age throttle (#2561, anti-abuse): a friction/visibility signal for the classic ban-evasion pattern
   // (a banned login gets a fresh account the same day) — NEVER an automatic close on account age alone. Off

src/types.ts

@@ -605,6 +605,11 @@ 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;
+  /** Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix): the (headSha,
+   *  evidenceFingerprint) checkpoint the screenshotTableGate's presence-mode check last satisfied for this PR
+   *  (see evaluateScreenshotTableGate's staleness comment). `null`/absent = presence mode has never satisfied
+   *  the gate for this PR yet. Planner-written; read straight from the row. */
+  screenshotTablePresenceSatisfied?: { headSha: string; evidenceFingerprint: string } | null | undefined;
   /** File paths changed by this open PR, when the caller has already resolved them (e.g. from the
    *  `pull_request_files` cache). Absent/undefined when not resolved — callers must not assume an empty array
    *  means "no files changed". Mirrors {@link RecentMergedPullRequestRecord.changedFiles} so the same