diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9b03e5afe..b0f2a7143 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -654,7 +654,7 @@ import { } from "../review/linked-issue-hard-rules"; import { DEFAULT_UNLINKED_ISSUE_GUARDRAIL } from "../review/unlinked-issue-guardrail-config"; import { resolveUnlinkedIssueMatchDisposition } from "../review/unlinked-issue-guardrail"; -import { DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, extractTableRowImageUrls, type ScreenshotTableGateConfig } from "../review/screenshot-table-gate"; +import { CAPTURE_UNOBTAINABLE_REASON, DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, extractTableRowImageUrls, type ScreenshotTableGateConfig } from "../review/screenshot-table-gate"; import { isSafeHttpUrl } from "../review/content-lane/safe-url"; import { buildScreenshotTableVisionFindings, @@ -3549,7 +3549,7 @@ async function runAgentMaintenancePlanAndExecute( presenceModeSatisfied: pr.screenshotTablePresenceSatisfied, // #9881: proved-unobtainable for THIS head only -- a later push re-arms the requirement, and a repo // that gains preview deploys stops matching on its very next commit. - captureUnobtainable: Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === pr.headSha, + captureUnobtainable: screenshotCaptureUnobtainableForHead(pr), }); // #9030: a visual-capture pipeline ERROR (browserless down, timeout, GitHub hiccup) or a still-building // preview looked IDENTICAL to "capture concluded normally, no visual evidence found" -- both left @@ -8760,15 +8760,26 @@ export async function maybeAddLockfileTamperFinding( } } +/** #9881/#10060: whether the bot has proved visual capture is structurally unobtainable for THIS pr's current + * head. Shared by BOTH `evaluateScreenshotTableGate` call sites (the enforcement pass in + * `runAgentMaintenancePlanAndExecute` and the advisory pass below) so the two evaluations of the same fact + * cannot disagree -- a later push re-arms the requirement, since the persisted marker is head-scoped. */ +export function screenshotCaptureUnobtainableForHead(pr: { headSha?: string | null | undefined; visualCaptureUnobtainableSha?: string | null | undefined }): boolean { + return Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === pr.headSha; +} + /** - * Screenshot-table gate advisory visibility (#2006 follow-up). `action: "close"` already communicates via its - * own templated close comment (see planAgentMaintenanceActions/screenshotTableCloseMessage), so a violation - * there never needs a SEPARATE advisory finding -- this only ever fires for `action: "advisory"`, which + * Screenshot-table gate advisory visibility (#2006 follow-up, #10060). `action: "close"` already communicates + * via its own templated close comment (see planAgentMaintenanceActions/screenshotTableCloseMessage), so a + * violation there never needs a SEPARATE advisory finding -- this fires for `action: "advisory"` (which * previously had NO visible effect at all: the live gate's only other `evaluateScreenshotTableGate` call site - * (`runAgentMaintenancePlanAndExecute`) discards the result entirely once `action !== "close"`. Mirrors - * `maybeAddLockfileTamperFinding` immediately above: off/out-of-scope is free, a violation appends ONE - * warning-severity, non-blocking finding (unrecognized by `isConfiguredGateBlocker`, so it can never gate), - * and any evaluation error is swallowed so it can never destabilize the gate. + * (`runAgentMaintenancePlanAndExecute`) discards the result entirely once `action !== "close"`) AND, per + * `captureUnobtainable`, for a `close`/`block` gate whose enforcement #9881 degraded -- otherwise that + * degrade was silent: `runAgentMaintenancePlanAndExecute` (the only place that evaluated it) never appended + * a finding for the degraded path. Mirrors `maybeAddLockfileTamperFinding` immediately above: off/out-of-scope + * is free, a violation appends ONE warning-severity, non-blocking finding (unrecognized by + * `isConfiguredGateBlocker`, so it can never gate), and any evaluation error is swallowed so it can never + * destabilize the gate. */ export async function maybeAddScreenshotTableAdvisoryFinding( env: Env, @@ -8780,10 +8791,19 @@ export async function maybeAddScreenshotTableAdvisoryFinding( prBody: string | null | undefined; prLabels: string[]; botCaptureSatisfied: boolean; + captureUnobtainable: boolean; files: Awaited> | null; }, ): Promise { - if (!args.screenshotTableGateConfig.enabled || args.screenshotTableGateConfig.action !== "advisory") return; + if (!args.screenshotTableGateConfig.enabled) return; + // #9881 degrade: a captureUnobtainable gate must still be evaluated even when action !== "advisory" -- ONLY + // the evaluator (not this early return) can tell whether the degrade actually applies, since it also + // requires `violated: true`. Every other action !== "advisory" case stays the pre-#10060 free early-out. + // (Nothing further down needs to re-check `action !== "advisory"`: reaching past this line with a + // non-advisory action is only possible via captureUnobtainable, and evaluateScreenshotTableGate sets + // `enforcementDegradedReason` on every `violated: true` result whose `captureUnobtainable` was true -- + // so a non-advisory action that gets this far is always the degraded case.) + if (args.screenshotTableGateConfig.action !== "advisory" && !args.captureUnobtainable) return; try { const files = args.files ?? @@ -8794,9 +8814,12 @@ export async function maybeAddScreenshotTableAdvisoryFinding( prLabels: args.prLabels, changedFiles: files.map((file) => file.path), botCaptureSatisfied: args.botCaptureSatisfied, + captureUnobtainable: args.captureUnobtainable, }); if (!result.violated) return; - const detail = result.reason ?? DEFAULT_SCREENSHOT_CONTRACT_MESSAGE; + const enforcementDegraded = result.enforcementDegradedReason !== undefined; + const reason = result.reason ?? DEFAULT_SCREENSHOT_CONTRACT_MESSAGE; + const detail = enforcementDegraded ? `${reason}\n\n${CAPTURE_UNOBTAINABLE_REASON}` : reason; args.advisory.findings.push({ code: "screenshot_table_missing", severity: "warning", @@ -12138,9 +12161,10 @@ async function maybePublishPrPublicSurface( files: await getReviewFiles(), }); - // Screenshot-table gate advisory visibility (#2006 follow-up): `action: "advisory"` previously had no - // visible effect at all (see maybeAddScreenshotTableAdvisoryFinding's own doc comment). No-op for `off`, - // out-of-scope, or `action: "close"` (which already communicates via its own close comment). + // Screenshot-table gate advisory visibility (#2006 follow-up, #10060): `action: "advisory"` previously had + // no visible effect at all, and a `close`/`block` gate whose enforcement #9881 degraded went completely + // silent (see maybeAddScreenshotTableAdvisoryFinding's own doc comment). No-op for `off`, out-of-scope, or + // a non-degraded `close`/`block` (which already communicates via its own close/hold comment). await maybeAddScreenshotTableAdvisoryFinding(env, { advisory, repoFullName, @@ -12149,6 +12173,9 @@ async function maybePublishPrPublicSurface( prBody: pr.body, prLabels: pr.labels, botCaptureSatisfied: Boolean(pr.headSha) && pr.visualCaptureSatisfiedSha === pr.headSha, + // #9881: the SAME shared helper the enforcement call site (this file) calls -- the two evaluations of + // the same pure check cannot disagree. + captureUnobtainable: screenshotCaptureUnobtainableForHead(pr), files: await getReviewFiles(), }); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index a329e296c..d48713606 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -17,7 +17,10 @@ import { CAPTURE_UNOBTAINABLE_REASON, type ScreenshotMatrixPair, } from "../../src/review/screenshot-table-gate"; -import type { ScreenshotTableGateConfig } from "../../src/types"; +import type { Advisory, PullRequestFileRecord, ScreenshotTableGateConfig } from "../../src/types"; +import { maybeAddScreenshotTableAdvisoryFinding, screenshotCaptureUnobtainableForHead } from "../../src/queue/processors"; +import { planAgentMaintenanceActions, type AgentActionPlanInput } from "../../src/settings/agent-actions"; +import { createTestEnv } from "../helpers/d1"; function config(overrides: Partial = {}): ScreenshotTableGateConfig { return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], ...overrides }; @@ -999,3 +1002,172 @@ describe("enforcement degrade when capture is unobtainable (#9881)", () => { expect(before).toEqual(evaluateScreenshotTableGate({ ...violatingInput, captureUnobtainable: undefined })); }); }); + +// #9881/#10060: the conjunction shared by both evaluateScreenshotTableGate call sites in processors.ts. +describe("screenshotCaptureUnobtainableForHead (#9881/#10060)", () => { + it("false when headSha is absent, regardless of the persisted marker", () => { + expect(screenshotCaptureUnobtainableForHead({ headSha: null, visualCaptureUnobtainableSha: null })).toBe(false); + expect(screenshotCaptureUnobtainableForHead({ headSha: undefined, visualCaptureUnobtainableSha: "sha-1" })).toBe(false); + }); + + it("true when the persisted marker matches the current head", () => { + expect(screenshotCaptureUnobtainableForHead({ headSha: "sha-1", visualCaptureUnobtainableSha: "sha-1" })).toBe(true); + }); + + it("false when the persisted marker is for a DIFFERENT head — a later push re-arms the requirement", () => { + expect(screenshotCaptureUnobtainableForHead({ headSha: "sha-2", visualCaptureUnobtainableSha: "sha-1" })).toBe(false); + expect(screenshotCaptureUnobtainableForHead({ headSha: "sha-2", visualCaptureUnobtainableSha: null })).toBe(false); + }); +}); + +// #10060: the degrade decided by evaluateScreenshotTableGate above only reaches a maintainer if +// maybeAddScreenshotTableAdvisoryFinding (src/queue/processors.ts) actually appends a finding for it. Before +// this fix `action !== "advisory"` always early-returned, so a degraded close/block gate went completely +// silent -- the exact gap #9881's own comments claimed did not exist. +describe("maybeAddScreenshotTableAdvisoryFinding wiring the #9881 degrade to a finding (#10060)", () => { + const REPO = "acme/widgets"; + const PR = 7; + + function advisory(): Advisory { + return { + id: "adv-10060", + targetType: "pull_request", + targetKey: `${REPO}#${PR}`, + repoFullName: REPO, + pullNumber: PR, + headSha: "sha-10060", + conclusion: "neutral", + severity: "info", + title: "LoopOver advisory available", + summary: "ok", + findings: [], + generatedAt: "2026-07-31T00:00:00.000Z", + }; + } + + function scopedFiles(): PullRequestFileRecord[] { + return [ + { + repoFullName: REPO, + pullNumber: PR, + path: "apps/web/src/routes/home.tsx", + status: "modified", + additions: 1, + deletions: 0, + changes: 1, + payload: {}, + }, + ]; + } + + function args(overrides: Partial[1]> = {}) { + return { + advisory: advisory(), + repoFullName: REPO, + pullNumber: PR, + screenshotTableGateConfig: config({ enabled: true, action: "close" as const, whenPaths: ["apps/web/src/**"] }), + prBody: "no screenshots here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: false, + files: scopedFiles(), + ...overrides, + }; + } + + // Mirrors what processors.ts actually computes for the planner when the gate is degraded: + // screenshotTableMatch/screenshotEvidenceHold stay undefined and screenshotTableEvidenceUnresolved stays + // false (all three gated on `!screenshotTableEnforcementDegraded`), so the plan built from the same facts + // falls straight through to ordinary disposition instead of closing or holding. + function degradedPlanInput(overrides: Partial = {}): AgentActionPlanInput { + return { + blockerTitles: [], + autonomy: { close: "auto", approve: "auto", merge: "auto" }, + autoMaintain: { requireApprovals: 1, mergeMethod: "squash" }, + slopGateMinScore: 60, + changedPaths: [], + hardGuardrailGlobs: [], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + ciState: "passed", + pr: { labels: [] }, + conclusion: "success", + ...overrides, + }; + } + + it("gate disabled: no finding regardless of captureUnobtainable", async () => { + const env = createTestEnv(); + const callArgs = args({ + screenshotTableGateConfig: config({ enabled: false, action: "close" as const, whenPaths: ["apps/web/src/**"] }), + captureUnobtainable: true, + }); + await maybeAddScreenshotTableAdvisoryFinding(env, callArgs); + expect(callArgs.advisory.findings).toEqual([]); + }); + + it("action close, violated, captureUnobtainable true: appends exactly one finding naming the maintainer remedy, and the plan built from the same degraded facts has no close and no hold", async () => { + const env = createTestEnv(); + const callArgs = args({ captureUnobtainable: true }); + await maybeAddScreenshotTableAdvisoryFinding(env, callArgs); + expect(callArgs.advisory.findings).toHaveLength(1); + const finding = callArgs.advisory.findings[0]; + expect(finding?.code).toBe("screenshot_table_missing"); + expect(finding?.detail).toContain(CAPTURE_UNOBTAINABLE_REASON); + + const plan = planAgentMaintenanceActions(degradedPlanInput()); + expect(plan.map((a) => a.actionClass)).not.toContain("close"); + expect(plan.some((a) => a.actionClass === "label" && a.autonomyClass === "close")).toBe(false); + }); + + it("action block, violated, captureUnobtainable true: same finding appended, no hold", async () => { + const env = createTestEnv(); + const callArgs = args({ + screenshotTableGateConfig: config({ enabled: true, action: "block", whenPaths: ["apps/web/src/**"] }), + captureUnobtainable: true, + }); + await maybeAddScreenshotTableAdvisoryFinding(env, callArgs); + expect(callArgs.advisory.findings).toHaveLength(1); + expect(callArgs.advisory.findings[0]?.detail).toContain(CAPTURE_UNOBTAINABLE_REASON); + + const plan = planAgentMaintenanceActions(degradedPlanInput()); + expect(plan.map((a) => a.actionClass)).not.toContain("close"); + expect(plan.some((a) => a.actionClass === "label" && a.autonomyClass === "close")).toBe(false); + }); + + it("action close, violated, captureUnobtainable false: pins today's behaviour — NO advisory finding", async () => { + const env = createTestEnv(); + const callArgs = args({ captureUnobtainable: false }); + await maybeAddScreenshotTableAdvisoryFinding(env, callArgs); + expect(callArgs.advisory.findings).toEqual([]); + }); + + it("action advisory, violated, captureUnobtainable false: byte-identical finding to today", async () => { + const env = createTestEnv(); + const callArgs = args({ + screenshotTableGateConfig: config({ enabled: true, action: "advisory", whenPaths: ["apps/web/src/**"] }), + captureUnobtainable: false, + }); + await maybeAddScreenshotTableAdvisoryFinding(env, callArgs); + expect(callArgs.advisory.findings).toHaveLength(1); + const finding = callArgs.advisory.findings[0]; + expect(finding).toMatchObject({ + code: "screenshot_table_missing", + severity: "warning", + title: "Missing before/after screenshot table", + action: + "Add a before/after screenshot table to the pull request description (advisory only — this does not block merge).", + }); + expect(finding?.detail).not.toContain(CAPTURE_UNOBTAINABLE_REASON); + }); + + // #10060 regression: the exact defect reported -- a degraded `action: "close"` evaluation must never be a + // completely silent pass. + it("regression: a degraded close-action gate never produces a silent pass", async () => { + const env = createTestEnv(); + const callArgs = args({ captureUnobtainable: true }); + await maybeAddScreenshotTableAdvisoryFinding(env, callArgs); + expect(callArgs.advisory.findings).not.toEqual([]); + }); +});