fix(engine): re-validate screenshot matrix evidence on re-push (#8866)

.release-please-manifest.json

@@ -1,6 +1,6 @@
 {
   "packages/loopover-mcp": "3.14.1",
-  "packages/loopover-engine": "3.14.1",
+  "packages/loopover-engine": "3.15.0",
   "packages/loopover-miner": "3.14.1",
   "packages/loopover-ui-kit": "1.2.0"
 }

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

@@ -326,38 +326,63 @@ 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
+  /** Set when PRESENCE or MATRIX mode independently satisfied the gate on THIS evaluation (never
+   *  bot-capture -- see the staleness comment on `evaluateScreenshotTableGate` below). 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. */
+   *  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;
 };
 
-/** One presence-mode "satisfied" checkpoint: the head SHA it was satisfied at, plus a fingerprint of the
+/** One presence/matrix-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
+/** A deterministic fingerprint of the table 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. */
+ *  caption edit or table reflow that doesn't touch the actual image URLs still fingerprints identically.
+ *  Shared by presence mode and matrix mode (#8866). */
 function presenceModeEvidenceFingerprint(body: string | null | undefined): string {
   return JSON.stringify(extractTableRowImageUrls(body));
 }
 
+/** Shared head-SHA / evidence-fingerprint staleness check for presence AND matrix modes (#stale-screenshot-table-fix /
+ *  #8866). Returns `{ stale: true }` when this exact evidence already satisfied the gate for a different head;
+ *  otherwise `{ stale: false }` and, when `headSha` is known, the checkpoint the caller should persist. */
+function evidenceFreshnessForHead(
+  headSha: string | null | undefined,
+  priorSatisfied: ScreenshotTablePresenceEvidence | null | undefined,
+  prBody: string | null | undefined,
+): { stale: true } | { stale: false; presenceModeSatisfiedState?: ScreenshotTablePresenceEvidence } {
+  const evidenceFingerprint = presenceModeEvidenceFingerprint(prBody);
+  const staleForNewHead =
+    typeof headSha === "string" &&
+    headSha.length > 0 &&
+    priorSatisfied != null &&
+    priorSatisfied.headSha !== headSha &&
+    priorSatisfied.evidenceFingerprint === evidenceFingerprint;
+  if (staleForNewHead) return { stale: true };
+  return {
+    stale: false,
+    ...(typeof headSha === "string" && headSha.length > 0 ? { presenceModeSatisfiedState: { headSha, evidenceFingerprint } } : {}),
+  };
+}
+
 /** 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.
+ *    before/after row. Violated ⇒ the reason names exactly which pairs are still missing. #8866: ALSO violated
+ *    when the matrix is complete but the evidence is STALE for a new head (same fingerprint checkpoint as
+ *    presence mode — see `headSha`/`presenceModeSatisfied` below).
  *  - 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
@@ -376,15 +401,15 @@ 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. */
+  /** The PR's current head SHA, for presence- and matrix-mode staleness correlation (bot-capture already keys
+   *  its own marker to headSha). 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). */
+  /** The (headSha, evidenceFingerprint) checkpoint presence OR matrix 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;
@@ -395,8 +420,21 @@ export function evaluateScreenshotTableGate(input: {
   const matrixPairs = requiredScreenshotMatrixPairs(config);
   if (matrixPairs.length > 0) {
     const missing = missingScreenshotMatrixPairs(input.prBody, matrixPairs);
-    if (missing.length === 0) return NO_VIOLATION;
-    return { violated: true, reason: config.message ?? appendSkillLink(buildScreenshotMatrixMessage(missing), config.skillFileUrl) };
+    if (missing.length > 0) {
+      return { violated: true, reason: config.message ?? appendSkillLink(buildScreenshotMatrixMessage(missing), config.skillFileUrl) };
+    }
+    // #8866: matrix mode previously returned NO_VIOLATION with no head-SHA correlation — a complete matrix
+    // pasted on push #1 kept matching forever. Reuse the same fingerprint checkpoint as presence mode so an
+    // unchanged matrix cannot silently PASS across pushes after a real visual regression.
+    const freshness = evidenceFreshnessForHead(input.headSha, input.presenceModeSatisfied, input.prBody);
+    if (freshness.stale) {
+      return { violated: true, reason: config.message ?? appendSkillLink(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, config.skillFileUrl) };
+    }
+    return {
+      violated: false,
+      reason: null,
+      ...(freshness.presenceModeSatisfiedState ? { presenceModeSatisfiedState: freshness.presenceModeSatisfiedState } : {}),
+    };
   }
 
   const hasTable = hasImageBearingMarkdownTable(input.prBody);
@@ -414,20 +452,12 @@ export function evaluateScreenshotTableGate(input: {
     // 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) {
+    const freshness = evidenceFreshnessForHead(input.headSha, input.presenceModeSatisfied, input.prBody);
+    if (!freshness.stale) {
       return {
         violated: false,
         reason: null,
-        ...(typeof headSha === "string" && headSha.length > 0 ? { presenceModeSatisfiedState: { headSha, evidenceFingerprint } } : {}),
+        ...(freshness.presenceModeSatisfiedState ? { presenceModeSatisfiedState: freshness.presenceModeSatisfiedState } : {}),
       };
     }
   }

src/queue/processors.ts

@@ -3122,12 +3122,12 @@ async function runAgentMaintenancePlanAndExecute(
     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.
+  // #stale-screenshot-table-fix / #8866: presence or matrix 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(

test/unit/queue-3.test.ts

@@ -1755,6 +1755,103 @@ describe("queue processors", () => {
     expect(closeAudit?.n).toBeGreaterThanOrEqual(1);
   });
 
+  // #8866: same audited failure as #stale-screenshot-table-fix, but for MATRIX mode (requireViewports set).
+  // Pre-fix, matrix mode only re-scanned labeled rows with no head-SHA correlation, so an unchanged matrix
+  // table silently PASSed forever after push #1. Post-fix it must reuse the presence-mode fingerprint
+  // checkpoint and re-violate on push #2.
+  it("screenshot-table gate (#8866): a matrix table that passed on push #1 no longer satisfies the gate on push #2 when the body was never re-edited", async () => {
+    const env = createTestEnv({
+      GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),
+    });
+    await upsertInstallation(env, {
+      installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: { metadata: "read", pull_requests: "write", issues: "write" }, events: ["pull_request"] },
+      repositories: [{ name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }],
+    });
+    await upsertRepositorySettings(env, {
+      repoFullName: "JSONbored/gittensory",
+      autonomy: { close: "auto", merge: "auto", label: "auto" },
+    });
+    await upsertRepoFocusManifest(env, "JSONbored/gittensory", {
+      settings: {
+        commentMode: "all_prs",
+        publicSurface: "comment_only",
+        checkRunMode: "off",
+        screenshotTableGate: {
+          enabled: true,
+          whenLabels: ["visual"],
+          requireViewports: ["Desktop"],
+          requireThemes: ["Light"],
+        },
+        reviewCheckMode: "required",
+      },
+    }, "repo_file");
+    const unchangedMatrixBody = [
+      "Changed the button color.",
+      "",
+      "| Viewport · Theme | Before | After |",
+      "| --- | --- | --- |",
+      "| Desktop · Light | ![before](https://x/before.png) | ![after](https://x/after.png) |",
+      "",
+      "Closes #1",
+    ].join("\n");
+    let currentHeadSha = "matrix-stale-push-1";
+    const seen = { closed: false, closeCount: 0 };
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
+      const url = input.toString();
+      const method = init?.method ?? "GET";
+      if (url === "https://api.gittensor.io/miners") return Response.json([]);
+      if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
+      if (url.includes("/pulls/9002/files")) return Response.json([{ filename: "apps/ui/src/App.tsx", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+const ok = true;" }]);
+      if (url.includes("/pulls/9002/reviews")) return Response.json([]);
+      if (url.includes("/pulls/9002/commits")) return Response.json([]);
+      if (url.endsWith("/pulls/9002") && method === "PATCH") {
+        if (JSON.parse(String(init?.body ?? "{}")).state === "closed") { seen.closed = true; seen.closeCount += 1; }
+        return Response.json({ number: 9002, state: "closed" });
+      }
+      if (url.endsWith("/pulls/9002")) return Response.json({ number: 9002, state: "open", user: { login: "visual-contributor" }, head: { sha: currentHeadSha }, mergeable_state: "clean" });
+      if (url.includes(`/commits/${currentHeadSha}/check-runs`)) return Response.json({ total_count: 0, check_runs: [] });
+      if (url.includes(`/commits/${currentHeadSha}/status`)) return Response.json({ state: "success", statuses: [] });
+      if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } });
+      if (url.includes("/issues/9002/labels") && method === "GET") return Response.json([]);
+      if (url.includes("/issues/9002/labels") && method === "POST") return Response.json([]);
+      if (url.includes("/issues/9002/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 });
+      if (url.includes("/issues/9002/comments")) return Response.json([]);
+      return Response.json({});
+    });
+
+    await processJob(env, {
+      type: "github-webhook",
+      deliveryId: "screenshot-table-matrix-stale-push-1",
+      eventName: "pull_request",
+      payload: {
+        action: "opened",
+        installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
+        repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
+        pull_request: { number: 9002, title: "New button color", state: "open", user: { login: "visual-contributor" }, head: { sha: currentHeadSha }, labels: [{ name: "visual" }], body: unchangedMatrixBody, mergeable_state: "clean", reviewDecision: "APPROVED" },
+      },
+    });
+    expect(seen.closed).toBe(false);
+    const afterPush1 = await getPullRequest(env, "JSONbored/gittensory", 9002);
+    expect(afterPush1?.screenshotTablePresenceSatisfied?.headSha).toBe("matrix-stale-push-1");
+
+    currentHeadSha = "matrix-stale-push-2";
+    await processJob(env, {
+      type: "github-webhook",
+      deliveryId: "screenshot-table-matrix-stale-push-2",
+      eventName: "pull_request",
+      payload: {
+        action: "synchronize",
+        installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } },
+        repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
+        pull_request: { number: 9002, title: "New button color", state: "open", user: { login: "visual-contributor" }, head: { sha: currentHeadSha }, labels: [{ name: "visual" }], body: unchangedMatrixBody, mergeable_state: "clean", reviewDecision: "APPROVED" },
+      },
+    });
+    expect(seen.closed).toBe(true);
+    expect(seen.closeCount).toBe(1);
+    const closeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.close'").first<{ n: number }>();
+    expect(closeAudit?.n).toBeGreaterThanOrEqual(1);
+  });
+
   // #4110: same in-scope, NO-body-table fixture as the "closed deterministically" test above (a hand-authored
   // table would normally be the ONLY way to avoid the close) -- the ONLY difference is that this PR ALSO
   // touches a web-visible route file with a real, resolvable preview deploy. Proves the marker