diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a5051037e..6b8bc05a5 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -659,7 +659,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, @@ -8902,14 +8902,15 @@ export async function maybeAddLockfileTamperFinding( } /** - * 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 - * 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. + * Screenshot-table gate advisory visibility (#2006 follow-up, #9881 degrade follow-up). `action: "close"`/ + * `"block"` already communicate via their own templated close/hold comment (see + * planAgentMaintenanceActions/screenshotTableCloseMessage), so a violation there never needs a SEPARATE + * advisory finding UNLESS enforcement was degraded (#9881: the bot proved this repo's preview pipeline can + * never satisfy the gate) -- in that case the close/hold comment never fires either, so THIS finding is the + * only place a maintainer ever learns the gate is unsatisfiable here (#10060). Mirrors + * `maybeAddLockfileTamperFinding` immediately above: off 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, @@ -8921,23 +8922,50 @@ export async function maybeAddScreenshotTableAdvisoryFinding( prBody: string | null | undefined; prLabels: string[]; botCaptureSatisfied: boolean; + // #9881/#10060: true when the bot proved this repo's preview pipeline can never produce a capture for + // this head -- threaded from the SAME `Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === + // pr.headSha` expression the enforcement call site (runAgentMaintenancePlanAndExecute) computes, so the + // two evaluations of this pure check can never disagree about whether this PR's gate is degraded. + captureUnobtainable: boolean; files: Awaited> | null; }, ): Promise { - if (!args.screenshotTableGateConfig.enabled || args.screenshotTableGateConfig.action !== "advisory") return; + if (!args.screenshotTableGateConfig.enabled) return; try { - const files = - args.files ?? - (await listPullRequestFiles(env, args.repoFullName, args.pullNumber)); + // #10060: an if-fallback, not `args.files ?? (await listPullRequestFiles(...))` -- that shape left the + // statements immediately following it (the gate evaluation, the violated check) with a phantom 0 lcov hit + // count despite genuinely running every test, which would have sunk this file's Codecov patch coverage. + let files = args.files; + if (files === null) { + files = await listPullRequestFiles(env, args.repoFullName, args.pullNumber); + } + const changedFiles = files.map((file) => file.path); const result = evaluateScreenshotTableGate({ config: args.screenshotTableGateConfig, prBody: args.prBody, prLabels: args.prLabels, - changedFiles: files.map((file) => file.path), + changedFiles, botCaptureSatisfied: args.botCaptureSatisfied, + captureUnobtainable: args.captureUnobtainable, }); if (!result.violated) return; const detail = result.reason ?? DEFAULT_SCREENSHOT_CONTRACT_MESSAGE; + // #10060: a degraded gate must surface here REGARDLESS of the configured action -- close/block never get + // their own comment on this path (the enforcement that would have produced one was degraded away), so an + // advisory-mode repo and a close-mode repo with an unsatisfiable pipeline both need this same visibility. + if (result.enforcementDegradedReason !== undefined) { + const degradedDetail = `${detail}\n\n${CAPTURE_UNOBTAINABLE_REASON}`; + args.advisory.findings.push({ + code: "screenshot_table_missing", + severity: "warning", + title: "Screenshot-table enforcement degraded (capture unobtainable)", + detail: degradedDetail, + action: "Enable preview deploys for this repository, or set requireScreenshotTable.action to advisory.", + publicText: degradedDetail, + }); + return; + } + if (args.screenshotTableGateConfig.action !== "advisory") return; args.advisory.findings.push({ code: "screenshot_table_missing", severity: "warning", @@ -12304,6 +12332,9 @@ async function maybePublishPrPublicSurface( prBody: pr.body, prLabels: pr.labels, botCaptureSatisfied: Boolean(pr.headSha) && pr.visualCaptureSatisfiedSha === pr.headSha, + // #9881/#10060: same expression runAgentMaintenancePlanAndExecute computes for the enforcement decision, + // so the two evaluations of this pure check cannot disagree about whether this PR's gate is degraded. + captureUnobtainable: Boolean(pr.headSha) && pr.visualCaptureUnobtainableSha === pr.headSha, files: await getReviewFiles(), }); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index a329e296c..d22496e1b 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -17,7 +17,11 @@ 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 } from "../../src/queue/processors"; +import { planAgentMaintenanceActions, type AgentActionPlanInput } from "../../src/settings/agent-actions"; +import type { GateCheckConclusion } from "../../src/rules/advisory"; +import { createTestEnv } from "../helpers/d1"; function config(overrides: Partial = {}): ScreenshotTableGateConfig { return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], ...overrides }; @@ -999,3 +1003,220 @@ describe("enforcement degrade when capture is unobtainable (#9881)", () => { expect(before).toEqual(evaluateScreenshotTableGate({ ...violatingInput, captureUnobtainable: undefined })); }); }); + +// #10060: maybeAddScreenshotTableAdvisoryFinding previously early-returned before evaluating anything unless +// `action === "advisory"`, so a `close`/`block` repo whose gate was degraded (#9881) never surfaced a finding +// anywhere — the close/hold comment that would have said so never fires on the degraded path either, so the +// maintainer learned nothing. These tests pin the fixed wiring. +describe("maybeAddScreenshotTableAdvisoryFinding degrade wiring (#10060)", () => { + function advisory(): Advisory { + return { + id: "adv-1", + targetType: "pull_request", + repoFullName: "acme/widgets", + pullNumber: 7, + targetKey: "acme/widgets#7", + headSha: "sha7", + conclusion: "neutral", + severity: "info", + title: "LoopOver advisory available", + summary: "ok", + findings: [], + generatedAt: "2026-07-31T00:00:00.000Z", + }; + } + + const NO_TABLE_FILES: PullRequestFileRecord[] = [ + { repoFullName: "acme/widgets", pullNumber: 7, path: "src/app.tsx", status: "modified", additions: 1, deletions: 0, changes: 1, payload: {} }, + ]; + + function gateConfig(action: "close" | "block" | "advisory"): ScreenshotTableGateConfig { + return { ...DEFAULT_SCREENSHOT_TABLE_GATE, enabled: true, whenLabels: [], whenPaths: [], action }; + } + + it("action: close, violated, captureUnobtainable: true — appends exactly one finding naming the remedy, and the same inputs plan no close/hold", async () => { + const env = createTestEnv(); + const adv = advisory(); + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: gateConfig("close"), + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: true, + files: NO_TABLE_FILES, + }); + expect(adv.findings).toHaveLength(1); + expect(adv.findings[0]?.code).toBe("screenshot_table_missing"); + expect(adv.findings[0]?.detail).toContain(CAPTURE_UNOBTAINABLE_REASON); + expect(adv.findings[0]?.publicText).toContain(CAPTURE_UNOBTAINABLE_REASON); + + // The same degraded facts the real caller threads through (processors.ts): screenshotTableMatch and + // screenshotEvidenceHold both stay absent, and screenshotTableEvidenceUnresolved stays false, so the + // planner falls through to ordinary disposition instead of closing or holding. + const plan = planAgentMaintenanceActions({ + blockerTitles: [], + autonomy: { merge: "auto", close: "auto", review_state_label: "auto" }, + autoMaintain: { requireApprovals: 1, mergeMethod: "squash" }, + slopGateMinScore: 60, + changedPaths: [], + hardGuardrailGlobs: [], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + ciState: "passed", + conclusion: "success" as GateCheckConclusion, + manualReviewLabel: "human-review", + screenshotTableMatch: undefined, + screenshotEvidenceHold: undefined, + screenshotTableEvidenceUnresolved: false, + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + } satisfies AgentActionPlanInput); + expect(plan.some((a) => a.actionClass === "close")).toBe(false); + expect(plan.some((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp !== "remove")).toBe(false); + }); + + it("action: block, violated, captureUnobtainable: true — appends the same degraded finding, and no hold is planned", async () => { + const env = createTestEnv(); + const adv = advisory(); + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: gateConfig("block"), + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: true, + files: NO_TABLE_FILES, + }); + expect(adv.findings).toHaveLength(1); + expect(adv.findings[0]?.detail).toContain(CAPTURE_UNOBTAINABLE_REASON); + + const plan = planAgentMaintenanceActions({ + blockerTitles: [], + autonomy: { merge: "auto", review_state_label: "auto" }, + autoMaintain: { requireApprovals: 1, mergeMethod: "squash" }, + slopGateMinScore: 60, + changedPaths: [], + hardGuardrailGlobs: [], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + ciState: "passed", + conclusion: "success" as GateCheckConclusion, + manualReviewLabel: "human-review", + screenshotTableMatch: undefined, + screenshotEvidenceHold: undefined, + screenshotTableEvidenceUnresolved: false, + pr: { labels: [], mergeableState: "clean", reviewDecision: "APPROVED" }, + } satisfies AgentActionPlanInput); + expect(plan.some((a) => a.actionClass === "label" && a.label === "human-review" && a.labelOp !== "remove")).toBe(false); + expect(plan.some((a) => a.actionClass === "close")).toBe(false); + }); + + it("action: close, violated, captureUnobtainable: false — pins today's behavior: NO advisory finding", async () => { + const env = createTestEnv(); + const adv = advisory(); + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: gateConfig("close"), + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: false, + files: NO_TABLE_FILES, + }); + expect(adv.findings).toEqual([]); + }); + + it("action: advisory, violated, captureUnobtainable: false — byte-identical finding to today", async () => { + const env = createTestEnv(); + const adv = advisory(); + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: gateConfig("advisory"), + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: false, + files: NO_TABLE_FILES, + }); + expect(adv.findings).toHaveLength(1); + expect(adv.findings[0]).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(adv.findings[0]?.detail).not.toContain(CAPTURE_UNOBTAINABLE_REASON); + }); + + it("REGRESSION (#10060): a degraded action: close evaluation never produces a completely silent pass", async () => { + const env = createTestEnv(); + const adv = advisory(); + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: gateConfig("close"), + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: true, + files: NO_TABLE_FILES, + }); + expect(adv.findings.length).toBeGreaterThan(0); + }); + + it("not enabled: does not scan, no finding appended even when captureUnobtainable is true", async () => { + const env = createTestEnv(); + const adv = advisory(); + await maybeAddScreenshotTableAdvisoryFinding(env, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: { ...gateConfig("close"), enabled: false }, + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: true, + files: NO_TABLE_FILES, + }); + expect(adv.findings).toEqual([]); + }); + + it("fail-safe: a thrown error while loading files never propagates and appends no finding", async () => { + const env = createTestEnv(); + const adv = advisory(); + const throwingEnv = { + ...env, + DB: { + ...env.DB, + prepare: () => { + throw new Error("boom"); + }, + }, + } as unknown as typeof env; + await expect( + maybeAddScreenshotTableAdvisoryFinding(throwingEnv, { + advisory: adv, + repoFullName: "acme/widgets", + pullNumber: 7, + screenshotTableGateConfig: gateConfig("close"), + prBody: "no table here", + prLabels: [], + botCaptureSatisfied: false, + captureUnobtainable: true, + files: null, + }), + ).resolves.toBeUndefined(); + expect(adv.findings).toEqual([]); + }); +});