diff --git a/.release-please-manifest.json b/.release-please-manifest.json index a112e0abc4..effd9a1389 100644 --- a/.release-please-manifest.json +++ b/.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" } diff --git a/packages/loopover-engine/src/review/screenshot-table-gate.ts b/packages/loopover-engine/src/review/screenshot-table-gate.ts index 54cab138be..72ac0febf3 100644 --- a/packages/loopover-engine/src/review/screenshot-table-gate.ts +++ b/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 } : {}), }; } } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 70cc8736be..c32efa8d19 100644 --- a/src/queue/processors.ts +++ b/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( diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index b032aa1131..456d6b1f33 100644 --- a/test/unit/queue-3.test.ts +++ b/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 diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 87c0ae5d8b..a9deed4df9 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -588,6 +588,67 @@ describe("evaluateScreenshotTableGate", () => { expect(result).toEqual({ violated: false, reason: null }); }); + it("REGRESSION (#8866): a complete matrix that passed for head A is stale on head B when the body never changed", () => { + const passOnPush1 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: FULL_MATRIX_BODY, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-1", + }); + expect(passOnPush1).toEqual({ + violated: false, + reason: null, + presenceModeSatisfiedState: { + headSha: "matrix-push-1", + evidenceFingerprint: expect.any(String), + }, + }); + const staleOnPush2 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: FULL_MATRIX_BODY, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-2", + presenceModeSatisfied: passOnPush1.presenceModeSatisfiedState, + }); + expect(staleOnPush2.violated).toBe(true); + expect(staleOnPush2.presenceModeSatisfiedState).toBeUndefined(); + }); + + it("matrix mode is not stale when the contributor re-affirms with different image URLs on the new head (#8866)", () => { + const push1Body = FULL_MATRIX_BODY; + const push1 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: push1Body, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-1", + }); + const reaffirmedBody = FULL_MATRIX_BODY.replaceAll("x.png", "x-v2.png").replaceAll("y.png", "y-v2.png"); + const push2 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: reaffirmedBody, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(push2.violated).toBe(false); + expect(push2.presenceModeSatisfiedState?.headSha).toBe("matrix-push-2"); + }); + + it("matrix mode without a headSha stays byte-identical to pre-#8866 (no checkpoint, no staleness)", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: FULL_MATRIX_BODY, + prLabels: [], + changedFiles: [], + presenceModeSatisfied: { headSha: "old", evidenceFingerprint: "anything" }, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + it("REGRESSION (metagraphed PR #4661 shape): desktop-only before/after (4/12 images) still violates matrix mode", () => { const desktopOnlyBody = [ "| Theme | Before | After |", diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index 1ac43a330b..477f40cb7f 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -587,6 +587,67 @@ describe("evaluateScreenshotTableGate", () => { expect(result).toEqual({ violated: false, reason: null }); }); + it("REGRESSION (#8866): a complete matrix that passed for head A is stale on head B when the body never changed", () => { + const passOnPush1 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: FULL_MATRIX_BODY, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-1", + }); + expect(passOnPush1).toEqual({ + violated: false, + reason: null, + presenceModeSatisfiedState: { + headSha: "matrix-push-1", + evidenceFingerprint: expect.any(String), + }, + }); + const staleOnPush2 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: FULL_MATRIX_BODY, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-2", + presenceModeSatisfied: passOnPush1.presenceModeSatisfiedState, + }); + expect(staleOnPush2.violated).toBe(true); + expect(staleOnPush2.presenceModeSatisfiedState).toBeUndefined(); + }); + + it("matrix mode is not stale when the contributor re-affirms with different image URLs on the new head (#8866)", () => { + const push1Body = FULL_MATRIX_BODY; + const push1 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: push1Body, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-1", + }); + const reaffirmedBody = FULL_MATRIX_BODY.replaceAll("x.png", "x-v2.png").replaceAll("y.png", "y-v2.png"); + const push2 = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: reaffirmedBody, + prLabels: [], + changedFiles: [], + headSha: "matrix-push-2", + presenceModeSatisfied: push1.presenceModeSatisfiedState, + }); + expect(push2.violated).toBe(false); + expect(push2.presenceModeSatisfiedState?.headSha).toBe("matrix-push-2"); + }); + + it("matrix mode without a headSha stays byte-identical to pre-#8866 (no checkpoint, no staleness)", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig(), + prBody: FULL_MATRIX_BODY, + prLabels: [], + changedFiles: [], + presenceModeSatisfied: { headSha: "old", evidenceFingerprint: "anything" }, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + it("REGRESSION (metagraphed PR #4661 shape): desktop-only before/after (4/12 images) still violates matrix mode", () => { const desktopOnlyBody = [ "| Theme | Before | After |",