diff --git a/migrations/0156_pull_request_screenshot_table_presence_satisfied.sql b/migrations/0156_pull_request_screenshot_table_presence_satisfied.sql new file mode 100644 index 0000000000..23e470ec6b --- /dev/null +++ b/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; diff --git a/packages/loopover-engine/src/review/screenshot-table-gate.ts b/packages/loopover-engine/src/review/screenshot-table-gate.ts index c5a7f570fe..54cab138be 100644 --- a/packages/loopover-engine/src/review/screenshot-table-gate.ts +++ b/packages/loopover-engine/src/review/screenshot-table-gate.ts @@ -326,10 +326,31 @@ 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). @@ -337,9 +358,12 @@ const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null * 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) }; } diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 4715da2fbb..f488d15b0e 100644 --- a/src/db/repositories.ts +++ b/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 { + 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), }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index f43e8e4600..4bc9639435 100644 --- a/src/db/schema.ts +++ b/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()), }, diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 63d516fff9..0a0e2a4285 100644 --- a/src/queue/processors.ts +++ b/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 diff --git a/src/types.ts b/src/types.ts index 4bce741f31..bd07a8055f 100644 --- a/src/types.ts +++ b/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 diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index f4fdf80611..8d1c95ebf6 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -1465,6 +1465,95 @@ describe("queue processors", () => { expect(closeAudit?.n).toBe(0); }); + // #stale-screenshot-table-fix: the exact audited failure scenario -- a contributor pastes a genuine + // before/after table on push #1 (gate passes, PR stays open), then pushes new commits (push #2, a NEW head + // SHA) WITHOUT touching the PR body at all. Pre-fix, presence mode is pure regex matching over `prBody` with + // no tie to the live head SHA, so the SAME unchanged table silently satisfies the gate forever, even though it + // no longer proves anything about the code now on the new head. Post-fix, the gate must correlate presence-mode + // evidence to the head SHA it was last satisfied at and re-violate on push #2 since the evidence never changed. + it("screenshot-table gate (#stale-screenshot-table-fix): a 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", + commentMode: "all_prs", + publicSurface: "comment_only", + checkRunMode: "off", + reviewCheckMode: "required", + autonomy: { close: "auto", merge: "auto", label: "auto" }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"] } } }, "repo_file"); + // The SAME body/evidence is reused verbatim across both pushes -- the contributor never re-edits it. + const unchangedBody = + "Changed the button color.\n\n| Before | After |\n| --- | --- |\n| ![before](https://x/before.png) | ![after](https://x/after.png) |\n\nCloses #1"; + let currentHeadSha = "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/9001/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/9001/reviews")) return Response.json([]); + if (url.includes("/pulls/9001/commits")) return Response.json([]); + if (url.endsWith("/pulls/9001") && method === "PATCH") { + if (JSON.parse(String(init?.body ?? "{}")).state === "closed") { seen.closed = true; seen.closeCount += 1; } + return Response.json({ number: 9001, state: "closed" }); + } + if (url.endsWith("/pulls/9001")) return Response.json({ number: 9001, 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/9001/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/9001/labels") && method === "POST") return Response.json([]); + if (url.includes("/issues/9001/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.includes("/issues/9001/comments")) return Response.json([]); + return Response.json({}); + }); + + // Push #1: the table is present and correlated to THIS head for the first time ever -- passes, stays open. + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-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: 9001, title: "New button color", state: "open", user: { login: "visual-contributor" }, head: { sha: currentHeadSha }, labels: [{ name: "visual" }], body: unchangedBody, mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + expect(seen.closed).toBe(false); + // The presence-mode checkpoint was persisted for push #1's head, keyed to the evidence fingerprint. + const afterPush1 = await getPullRequest(env, "JSONbored/gittensory", 9001); + expect(afterPush1?.screenshotTablePresenceSatisfied?.headSha).toBe("stale-push-1"); + + // Push #2: new commits land (a NEW head SHA) but the contributor never edits the body -- the SAME evidence + // that satisfied push #1 is now stale. Pre-fix this would still pass (pure string match, no SHA awareness); + // post-fix it must violate and the PR must be closed. + currentHeadSha = "stale-push-2"; + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-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: 9001, title: "New button color", state: "open", user: { login: "visual-contributor" }, head: { sha: currentHeadSha }, labels: [{ name: "visual" }], body: unchangedBody, 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