diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 61782cebf8..014629184a 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -822,13 +822,14 @@ settings: # minConfidence: 0.85 # Number 0-1. Minimum AI-verifier confidence to treat a candidate as a real match. # Before/after screenshot-table gate (#2006): deterministic check that a visual/frontend PR's body has a - # before/after image table. Scoped to whenLabels OR whenPaths (either matches); both empty = every PR. Off - # by default. + # before/after image table -- OR (#4110) that the bot's own visual-capture pipeline (review.visual.enabled) + # already produced a real before/after render for this PR's head, which satisfies the gate on its own. + # Scoped to whenLabels OR whenPaths (either matches); both empty = every PR. Off by default. # screenshotTableGate: # enabled: false # Default: false. # whenLabels: [frontend, visual] # Default: [] (no label scoping). # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Default: [] (no path scoping). - # action: close # close | request_changes | comment. Default: close. + # action: close # close is the only supported value. Default: close. # message: "Custom close reason..." # Default: null (built-in message). # Maintainer AI review tuning (`.gittensory.yml` top-level `review:` block). These knobs shape the advisory AI diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index 38510522b3..2115890bb9 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -9265,9 +9265,7 @@ "action": { "type": "string", "enum": [ - "close", - "request_changes", - "comment" + "close" ] }, "message": { diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index e6d3414fef..b8ccb611ed 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -835,13 +835,14 @@ settings: # minConfidence: 0.85 # Number 0-1. Minimum AI-verifier confidence to treat a candidate as a real match. # Before/after screenshot-table gate (#2006): deterministic check that a visual/frontend PR's body has a - # before/after image table. Scoped to whenLabels OR whenPaths (either matches); both empty = every PR. Off - # by default. + # before/after image table -- OR (#4110) that the bot's own visual-capture pipeline (review.visual.enabled) + # already produced a real before/after render for this PR's head, which satisfies the gate on its own. + # Scoped to whenLabels OR whenPaths (either matches); both empty = every PR. Off by default. # screenshotTableGate: # enabled: false # Default: false. # whenLabels: [frontend, visual] # Default: [] (no label scoping). # whenPaths: ["apps/ui/**", "src/**/*.tsx"] # Default: [] (no path scoping). - # action: close # close | request_changes | comment. Default: close. + # action: close # close is the only supported value. Default: close. # message: "Custom close reason..." # Default: null (built-in message). # Maintainer AI review tuning (`.gittensory.yml` top-level `review:` block). These knobs shape the advisory AI diff --git a/migrations/0125_pull_request_visual_capture_satisfied_sha.sql b/migrations/0125_pull_request_visual_capture_satisfied_sha.sql new file mode 100644 index 0000000000..84df41b694 --- /dev/null +++ b/migrations/0125_pull_request_visual_capture_satisfied_sha.sql @@ -0,0 +1,13 @@ +-- Visual-capture gate satisfaction (#4110, visual-capture convergence epic #3607). The bot's before/after +-- capture pipeline (review.visual.enabled, #4093) can now satisfy the deterministic screenshotTableGate +-- (#2006) exactly like a hand-authored before/after table -- but the capture is computed and persisted by the +-- public-surface publish pass (maybePublishPrPublicSurface), which runs BEFORE the maintenance/gate pass +-- (maybeRunAgentMaintenance) re-reads this same PR row. Persisting the marker lets the maintenance pass see +-- "did the bot already prove this PR visually?" without re-running the capture or threading a new return value +-- through every caller of either function. +-- +-- visual_capture_satisfied_sha is the head SHA at which the capture pipeline last produced a REAL before+after +-- render pair (not a placeholder/failed/pending shot) -- scoped to head SHA (mirrors approved_head_sha, 0053 / +-- last_published_surface_sha, 0080: a new commit re-arms the requirement until capture succeeds again for the +-- new head). +ALTER TABLE pull_requests ADD COLUMN visual_capture_satisfied_sha TEXT; diff --git a/packages/gittensory-engine/src/review/screenshot-table-gate.ts b/packages/gittensory-engine/src/review/screenshot-table-gate.ts index 12273b2bd7..66eb033b4b 100644 --- a/packages/gittensory-engine/src/review/screenshot-table-gate.ts +++ b/packages/gittensory-engine/src/review/screenshot-table-gate.ts @@ -28,7 +28,7 @@ export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { action: "close", }; -const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "request_changes", "comment"]; +const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close"]; export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); @@ -72,7 +72,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str const action = isScreenshotTableGateAction(record.action) ? record.action : (() => { - if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be one of close, request_changes, comment; using the default "close".`); + if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" (the only supported value; #4110 removed request_changes/comment as dead config surface); using the default "close".`); return DEFAULT_SCREENSHOT_TABLE_GATE.action; })(); const message = typeof record.message === "string" && record.message.trim().length > 0 ? record.message.trim() : undefined; @@ -184,16 +184,25 @@ const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null /** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. 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. */ + * under a scoped path), UNLESS `botCaptureSatisfied` ⇒ violated, with the configured (or default) templated + * message as the reason. */ export function evaluateScreenshotTableGate(input: { config: ScreenshotTableGateConfig; prBody: string | null | undefined; prLabels: string[]; changedFiles: string[]; + /** #4110: true when the bot's own before/after capture pipeline (review.visual.enabled) already produced a + * REAL before+after render pair for this PR's current head — evidence equivalent to a hand-authored table. + * A successful automated capture satisfies the gate on its own, ahead of (and regardless of) the body-table + * anti-gaming checks below — those exist to stop a contributor from FAKING compliance without the bot's + * 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; }): ScreenshotTableGateResult { const { config } = input; if (!config.enabled) return NO_VIOLATION; if (!isScreenshotTableGateInScope(config, input.prLabels, input.changedFiles)) return NO_VIOLATION; + if (input.botCaptureSatisfied === true) return NO_VIOLATION; const hasTable = hasImageBearingMarkdownTable(input.prBody); const outsideTable = hasImageOutsideTable(input.prBody); const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths); diff --git a/packages/gittensory-engine/src/types/manifest-deps-types.ts b/packages/gittensory-engine/src/types/manifest-deps-types.ts index ba64bddd55..6704a90c7f 100644 --- a/packages/gittensory-engine/src/types/manifest-deps-types.ts +++ b/packages/gittensory-engine/src/types/manifest-deps-types.ts @@ -18,7 +18,9 @@ export type CombineStrategy = "single" | "consensus" | "synthesis"; export type OnMerge = "either" | "both"; -export type ScreenshotTableGateAction = "close" | "request_changes" | "comment"; +// #4110: `request_changes`/`comment` were REMOVED (see src/types.ts's mirror of this type for why) -- `"close"` +// is the only value this gate has ever enforced. +export type ScreenshotTableGateAction = "close"; export type ScreenshotTableGateConfig = { enabled: boolean; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index d2dd252dbc..27942f694d 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3657,6 +3657,19 @@ export async function markPullRequestSurfacePublished(env: Env, fullName: string .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); } +/** Visual-capture gate satisfaction (#4110): record the head SHA at which the bot's before/after capture + * pipeline just produced a REAL before+after render pair for this PR (see `hasSuccessfulBotCapture`, + * `review/visual/capture.ts`). The screenshotTableGate evaluator treats `visualCaptureSatisfiedSha === + * headSha` as evidence equivalent to a hand-authored table. Scoped to headSha (mirrors markPullRequestApproved) + * so a later commit re-arms the requirement until capture succeeds again for the new head. */ +export async function markPullRequestVisualCaptureSatisfied(env: Env, fullName: string, number: number, headSha: string): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ visualCaptureSatisfiedSha: headSha, updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, 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 @@ -5799,6 +5812,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull lastPublishedSurfaceSha: row.lastPublishedSurfaceSha, linkedIssueHardRuleViolatedAt: row.linkedIssueHardRuleViolatedAt, linkedIssueHardRuleViolationReason: row.linkedIssueHardRuleViolationReason, + visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha, }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index bd0908e420..f97672b901 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -477,6 +477,14 @@ export const pullRequests = sqliteTable( // pairing with merge_blocked_sha) -- so a later close can still cite the concrete rule even if the live // re-parse can no longer reproduce it (the issue was unlinked or its state changed). linkedIssueHardRuleViolationReason: text("linked_issue_hard_rule_violation_reason"), + // Visual-capture gate satisfaction (#4110): the head SHA at which the bot's before/after capture pipeline + // (review.visual.enabled) last produced a REAL before+after render pair (not a placeholder/failed/pending + // shot) for this PR. Lets the deterministic screenshotTableGate treat a successful automated capture as + // equivalent evidence to a hand-authored before/after table. Keyed to head SHA (mirrors approved_head_sha / + // last_published_surface_sha) -- a new commit re-arms the requirement until capture succeeds again for the + // new head. gittensory-computed (publish-written), omitted from the GitHub-sync SET clause so a later sync + // cannot clobber it. + visualCaptureSatisfiedSha: text("visual_capture_satisfied_sha"), createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }, diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 489c202c55..13abeadd91 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -776,7 +776,7 @@ export const RepositorySettingsSchema = z enabled: z.boolean(), whenLabels: z.array(z.string()), whenPaths: z.array(z.string()), - action: z.enum(["close", "request_changes", "comment"]), + action: z.enum(["close"]), message: z.string().optional(), }) .optional(), diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 41540b2b80..43ab15e1ae 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -56,6 +56,7 @@ import { markPullRequestsRegated, markPullRequestReviewsInvalidated, markPullRequestSurfacePublished, + markPullRequestVisualCaptureSatisfied, getLatestRegatedAt, claimRegateFanoutSlot, recordAgentCommandFeedback, @@ -358,7 +359,7 @@ import { randomUUID } from "node:crypto"; import { isRetryableJobError, RetryableJobError } from "./retryable"; import { screenshotsAllowed } from "../review/visual-wire"; import { isVisualPath } from "../review/visual/paths"; -import { buildCapture, type CaptureRoute } from "../review/visual/capture"; +import { buildCapture, hasSuccessfulBotCapture, type CaptureRoute } from "../review/visual/capture"; import { incr } from "../selfhost/metrics"; import { renderReviewingPlaceholder, @@ -2778,17 +2779,22 @@ async function runAgentMaintenancePlanAndExecute( ); // Screenshot-table gate (#2006): a DETERMINISTIC check (no AI) that an in-scope (label/path-matched) - // contributor visual/frontend PR's body contains a before/after screenshot table. Off by default - // (settings.screenshotTableGate.enabled === false), so the pure evaluator below is effectively free for the - // common case. Only "close" is wired as an enforcement action here (the other configured actions stay - // advisory, matching the issue's phased rollout) -- the ternary below is the ONLY place that reads `.action`. + // contributor visual/frontend PR's body contains a before/after screenshot table -- OR (#4110) that the + // bot's own visual-capture pipeline already produced a real before/after render for this exact head + // (markPullRequestVisualCaptureSatisfied, written earlier in this same webhook by maybePublishPrPublicSurface + // -- see that function's beforeAfter block -- and re-read here on `pr`, which this caller already re-fetched + // fresh from the DB). Off by default (settings.screenshotTableGate.enabled === false), so the pure evaluator + // below is effectively free for the common case. "close" is the only enforcement action this gate has (#4110 + // removed the dead request_changes/comment surface) -- the check below is the ONLY place that reads `.action`. /* v8 ignore next -- defensive: resolveRepositorySettings always populates screenshotTableGate (getRepositorySettings's DB defaults), so this fallback is unreachable in practice. */ const screenshotTableGateConfig = settings.screenshotTableGate ?? DEFAULT_SCREENSHOT_TABLE_GATE; + const botCaptureSatisfied = Boolean(pr.headSha) && pr.visualCaptureSatisfiedSha === pr.headSha; const screenshotTableGateResult = evaluateScreenshotTableGate({ config: screenshotTableGateConfig, prBody: pr.body, prLabels: pr.labels, changedFiles: changedPaths, + botCaptureSatisfied, }); const screenshotTableMatch = screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" @@ -10260,6 +10266,24 @@ async function maybePublishPrPublicSurface( ? { routes: [], previewPending: false } : await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig); beforeAfter = capture.routes; + // Screenshot-table gate satisfaction (#4110): a successful capture (a real before+after render pair + // on at least one route) is evidence equivalent to a hand-authored before/after table -- persist the + // head SHA it was proven at so the LATER maintenance pass (runAgentMaintenancePlanAndExecute, which + // re-reads this PR row fresh) can see it without re-running the capture or threading a new return + // value through every caller of this function. Best-effort: a write failure here just means the gate + // falls back to requiring a body table, never blocks the rest of the review. + if (pr.headSha && hasSuccessfulBotCapture(beforeAfter)) { + await markPullRequestVisualCaptureSatisfied(env, repoFullName, pr.number, pr.headSha).catch((error) => { + console.log( + JSON.stringify({ + event: "visual_capture_satisfied_mark_failed", + repoFullName, + pull: pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + }); + } // Visual self-poll: the FIRST capture returns a "loading" placeholder for the AFTER shot when the // preview deploy isn't live yet (capture.previewPending). Schedule a delayed re-review to re-capture // the now-ready shot — bounded by `attempt` so a never-resolving preview can't loop (the deployment_status diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts index 2ed45b154a..0c1c21c26a 100644 --- a/src/review/screenshot-table-gate.ts +++ b/src/review/screenshot-table-gate.ts @@ -28,7 +28,7 @@ export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { action: "close", }; -const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "request_changes", "comment"]; +const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close"]; export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); @@ -72,7 +72,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str const action = isScreenshotTableGateAction(record.action) ? record.action : (() => { - if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be one of close, request_changes, comment; using the default "close".`); + if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" (the only supported value; #4110 removed request_changes/comment as dead config surface); using the default "close".`); return DEFAULT_SCREENSHOT_TABLE_GATE.action; })(); const message = typeof record.message === "string" && record.message.trim().length > 0 ? record.message.trim() : undefined; @@ -184,16 +184,25 @@ const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null /** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. 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. */ + * under a scoped path), UNLESS `botCaptureSatisfied` ⇒ violated, with the configured (or default) templated + * message as the reason. */ export function evaluateScreenshotTableGate(input: { config: ScreenshotTableGateConfig; prBody: string | null | undefined; prLabels: string[]; changedFiles: string[]; + /** #4110: true when the bot's own before/after capture pipeline (review.visual.enabled) already produced a + * REAL before+after render pair for this PR's current head — evidence equivalent to a hand-authored table. + * A successful automated capture satisfies the gate on its own, ahead of (and regardless of) the body-table + * anti-gaming checks below — those exist to stop a contributor from FAKING compliance without the bot's + * 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; }): ScreenshotTableGateResult { const { config } = input; if (!config.enabled) return NO_VIOLATION; if (!isScreenshotTableGateInScope(config, input.prLabels, input.changedFiles)) return NO_VIOLATION; + if (input.botCaptureSatisfied === true) return NO_VIOLATION; const hasTable = hasImageBearingMarkdownTable(input.prBody); const outsideTable = hasImageOutsideTable(input.prBody); const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths); diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 5a330bace8..d1cba020b9 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -57,6 +57,36 @@ export interface CaptureResult { previewPending: boolean; } +/** True when `url` is a REAL rendered shot — not a missing slot (`undefined`) and not one of `capturePage`'s + * own placeholder cards (`?placeholder=loading|failed|auth`, minted when there's no preview yet, the deploy + * failed, or the route sign-in-walled). An on-demand `?url=` fallback link (no R2 binding configured) still + * counts as real — it resolves to an actual render, just not a cached one. */ +function isRealShotUrl(url: string | undefined): boolean { + return typeof url === "string" && url.length > 0 && !url.includes("placeholder="); +} + +/** True when `route` has a real before+after PAIR on at least one viewport (desktop or mobile) — the + * deterministic signal {@link hasSuccessfulBotCapture} uses per-route. Requiring BOTH sides of the SAME + * viewport (not "any before" + "any after" mixed across viewports) mirrors what a reviewer actually sees in + * the "Visual preview" table: one comparable pair, not two unrelated renders. */ +function routeHasRealBeforeAfterPair(route: CaptureRoute): boolean { + const desktopReal = isRealShotUrl(route.beforeUrl) && isRealShotUrl(route.afterUrl); + const mobileReal = isRealShotUrl(route.beforeUrlMobile) && isRealShotUrl(route.afterUrlMobile); + return desktopReal || mobileReal; +} + +/** + * True when at least one captured route has a REAL before+after render pair (#4110) — the deterministic + * signal the screenshot-table gate (`review/screenshot-table-gate.ts`) treats as equivalent to a hand-authored + * before/after table: a bot-rendered pair already proves the reviewer can SEE the change, so demanding a + * manual table on top of it would be redundant friction. A capture whose routes are all placeholders (preview + * still building, deploy failed, auth-walled) or empty (capture never ran / found nothing) does NOT satisfy — + * only a genuinely rendered pair does. + */ +export function hasSuccessfulBotCapture(routes: readonly CaptureRoute[]): boolean { + return routes.some(routeHasRealBeforeAfterPair); +} + /** Inputs the capture pipeline needs about the PR under review (resolved by the caller from gittensory data). */ export interface CaptureTarget { repoFullName: string; diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 3f264892e5..d9562b4b13 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -349,11 +349,13 @@ export type AgentActionPlanInput = { unlinkedIssueMatchClose?: { reason: string; comment: string } | undefined; // Screenshot-table gate (#2006): a DETERMINISTIC verdict (no AI, zero hallucination risk) that an in-scope // visual/frontend PR's body is missing a before/after screenshot table (or has an image outside a table, or - // a screenshot committed to the repo instead of uploaded to the PR). Same zero-hallucination short-circuit - // shape as blacklistMatch — fires ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only, so its close is - // tagged `closeKind: "screenshot_table"`. Absent / not-violated ⇒ no effect. The trigger only ever sets this - // when the repo's `screenshotTableGate.action` is `"close"` (the only enforcement mode this planner wires so - // far) — `"request_changes"`/`"comment"` stay advisory-only, surfaced elsewhere. + // a screenshot committed to the repo instead of uploaded to the PR) AND (#4110) the bot's own visual-capture + // pipeline did not already produce a real before/after render for this head -- either piece of evidence + // satisfies the gate, so `matched` here is already false whenever the bot capture succeeded (see + // evaluateScreenshotTableGate's `botCaptureSatisfied` input). Same zero-hallucination short-circuit shape as + // blacklistMatch — fires ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only, so its close is tagged + // `closeKind: "screenshot_table"`. Absent / not-violated ⇒ no effect. `"close"` is the gate's only + // enforcement action (#4110 removed the dead request_changes/comment surface — see ScreenshotTableGateAction). screenshotTableMatch?: { matched: boolean; reason: string | null } | undefined; pr: { mergeableState?: string | null | undefined; @@ -667,10 +669,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // Screenshot-table gate (#2006): same zero-hallucination short-circuit shape as the blacklist above — fires // ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only. The trigger has already resolved scope (label/ - // path match) and run the deterministic body/diff check before ever setting this input; the planner's only - // job is to build the close plan under the repo's normal autonomy/dry-run/kill-switch gates. No coupled label - // (unlike blacklist/contributor-cap/review-nag) — the templated close comment already IS the full contract, - // so a separate enforcement label would be redundant noise on a PR that's about to be closed anyway. + // path match) and run the deterministic body/diff-OR-bot-capture check (#4110) before ever setting this + // input; the planner's only job is to build the close plan under the repo's normal autonomy/dry-run/kill- + // switch gates. No coupled label (unlike blacklist/contributor-cap/review-nag) — the templated close comment + // already IS the full contract, so a separate enforcement label would be redundant noise on a PR that's + // about to be closed anyway. const screenshotTableContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; if (input.screenshotTableMatch?.matched === true && screenshotTableContributor) { if (acting("close")) { diff --git a/src/types.ts b/src/types.ts index 20cb9d00f2..20f05e0718 100644 --- a/src/types.ts +++ b/src/types.ts @@ -544,6 +544,11 @@ export type PullRequestRecord = { * pairing with mergeBlockedSha) — so a later close can still cite the concrete rule even when the live re-parse * can no longer reproduce it. */ linkedIssueHardRuleViolationReason?: string | null | undefined; + /** Visual-capture gate satisfaction (#4110): the head SHA at which the bot's before/after capture pipeline + * last produced a REAL before+after render pair (not a placeholder/failed/pending shot) for this PR. The + * 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; /** 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 @@ -1059,7 +1064,11 @@ export type RepositorySettings = { updatedAt?: string | null | undefined; }; -export type ScreenshotTableGateAction = "close" | "request_changes" | "comment"; +/** #4110: `request_changes`/`comment` were REMOVED (not just left unused) -- they were fully typed/validated + * but `src/queue/processors.ts` only ever branched on `=== "close"`, so setting either in `.gittensory.yml` + * silently did nothing. `"close"` is the only value this gate has ever enforced; a legacy config with either + * removed value normalizes to the default ("close") with a warning, exactly like any other invalid value. */ +export type ScreenshotTableGateAction = "close"; /** Per-repo config for the before/after screenshot-table gate (#2006). See {@link RepositorySettings.screenshotTableGate} * and `review/screenshot-table-gate.ts` for the normalizer + pure evaluator. */ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 8a5fd1096c..e61e72e8d9 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -11260,6 +11260,183 @@ describe("queue processors", () => { expect(closeAudit?.n).toBe(0); }); + // #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, so the bot's own visual-capture + // pipeline (buildCapture, reached through the SAME webhook via maybePublishPrPublicSurface) renders a REAL + // before+after pair before the maintenance pass evaluates the gate. Proves the capture result is persisted + // (markPullRequestVisualCaptureSatisfied) and read back (evaluateScreenshotTableGate's botCaptureSatisfied) + // within a single webhook, without a hand-authored table. + it("screenshot-table gate (#4110): a successful bot before/after capture satisfies the gate on its own, no body table needed", async () => { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", + GITTENSORY_REVIEW_SCREENSHOTS: "true", + }); + 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", + gateCheckMode: "enabled", + autonomy: { close: "auto", label: "auto" }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"] } } }, "repo_file"); + const seen = { closed: false }; + 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" }); + // A web-visible route file (isVisualPath) — this is what makes screenshotsAllowed's file-touch gate open + // and buildCapture actually run, on TOP of the no-body-table screenshotTableGate scope match (label). + if (url.includes("/pulls/58/files")) return Response.json([{ filename: "apps/gittensory-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/58/reviews")) return Response.json([]); + if (url.includes("/pulls/58/commits")) return Response.json([]); + if (url.endsWith("/pulls/58") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 58, state: "closed" }); } + if (url.endsWith("/pulls/58")) return Response.json({ number: 58, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis58" }, mergeable_state: "clean" }); + // Deployments API: none found -> buildCapture falls through to findPreviewUrlFromChecks below. + if (url.includes("/deployments?")) return Response.json([]); + // Combined status: empty statuses[] (byte-identical to the sibling "closed deterministically" fixture's + // CI stub) -- findPreviewUrlFromChecks' status lookup finds nothing here and falls through to check-runs. + if (url.includes("/commits/vis58/status")) return Response.json({ state: "success", statuses: [] }); + // A completed, successful check-run whose details_url is a real workers.dev preview link -- + // findPreviewUrlFromChecks' SECOND lookup resolves it, and reduceLiveCiAggregate reads it as an ordinary + // green check (no pending/failing signal), so CI still evaluates "passed". + if (url.includes("/commits/vis58/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "preview-deploy", status: "completed", conclusion: "success", details_url: "https://pr-58-preview.workers.dev" }] }); + // Check-suite hardening: reduceLiveCiAggregate only certifies a commit settled once it can ALSO read the + // check-suites (a non-empty check-runs list makes it fetch this as a backstop) -- an unstubbed 404 here + // would fail CLOSED to "pending" and defer the whole review before it ever reaches the publish/maintain + // pass. An empty list means nothing is still running. + if (url.includes("/commits/vis58/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/58/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/58/comments")) return Response.json([]); + // The unified-comment path also creates/patches the "Gittensory Orb Review Agent" check run and applies + // the title-derived type label -- neither is under test here, but both must resolve so the review + // completes normally instead of throwing on an unstubbed 404. + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 }); + if (url.includes("/check-runs/901") && method === "PATCH") return Response.json({ id: 901 }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-bot-capture", + 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: 58, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis58" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + + // The bot's own capture already proved the change visually -- no close, despite no body table at all. + expect(seen.closed).toBe(false); + 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).toBe(0); + // The marker persisted and round-trips through toPullRequestRecordFromRow. + const stored = await getPullRequest(env, "JSONbored/gittensory", 58); + expect(stored?.visualCaptureSatisfiedSha).toBe("vis58"); + }); + + // #4110 fail-safe: same fixture as the sibling "satisfies the gate on its own" test above (successful capture, + // in-scope, no body table), except the persistence write itself fails. Proves (1) the write failure never + // throws / never blocks the rest of the review (the marker write is wrapped in its own .catch), and (2) with + // NOTHING persisted, the screenshot-table gate correctly falls back to requiring a body table -- so this + // particular PR IS closed, unlike its sibling. Together the two tests pin both sides of the write's outcome. + it("screenshot-table gate (#4110): a failed visual-capture-satisfied write is swallowed (fail-safe) -- the gate falls back to requiring a body table", async () => { + const markSpy = vi.spyOn(repositoriesModule, "markPullRequestVisualCaptureSatisfied").mockRejectedValueOnce(new Error("D1 write failed")); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", + GITTENSORY_REVIEW_SCREENSHOTS: "true", + }); + 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", + gateCheckMode: "enabled", + autonomy: { close: "auto", label: "auto" }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { screenshotTableGate: { enabled: true, whenLabels: ["visual"] } } }, "repo_file"); + const seen = { closed: false }; + 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/59/files")) return Response.json([{ filename: "apps/gittensory-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/59/reviews")) return Response.json([]); + if (url.includes("/pulls/59/commits")) return Response.json([]); + if (url.endsWith("/pulls/59") && method === "PATCH") { seen.closed = JSON.parse(String(init?.body ?? "{}")).state === "closed"; return Response.json({ number: 59, state: "closed" }); } + if (url.endsWith("/pulls/59")) return Response.json({ number: 59, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis59" }, mergeable_state: "clean" }); + if (url.includes("/deployments?")) return Response.json([]); + if (url.includes("/commits/vis59/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis59/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "preview-deploy", status: "completed", conclusion: "success", details_url: "https://pr-59-preview.workers.dev" }] }); + if (url.includes("/commits/vis59/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/59/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/59/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.includes("/issues/59/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 }); + if (url.includes("/check-runs/901") && method === "PATCH") return Response.json({ id: 901 }); + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-table-bot-capture-write-fail", + 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: 59, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis59" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + mergeable_state: "clean", + reviewDecision: "APPROVED", + }, + }, + }); + } finally { + markSpy.mockRestore(); + } + + // The write failure never throws / never blocks the review -- but with nothing persisted, the gate has no + // bot-capture evidence and falls back to its ordinary no-table close. + expect(seen.closed).toBe(true); + const stored = await getPullRequest(env, "JSONbored/gittensory", 59); + expect(stored?.visualCaptureSatisfiedSha).toBeNull(); + }); + describe("live migrations/** collision recheck (#2550)", () => { // Full merge-eligible stub set (clean + green + approved), reused across scenarios — a positive test proves // the collision hold actually suppresses what would otherwise merge; a negative test proves the check diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 8fc8977843..0fea5845f4 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -20,10 +20,8 @@ function config(overrides: Partial = {}): ScreenshotT const TABLE_BODY = ["| Before | After |", "| --- | --- |", "| ![before](https://x/before.png) | ![after](https://x/after.png) |"].join("\n"); describe("isScreenshotTableGateAction", () => { - it("accepts every valid action", () => { + it("accepts the only valid action", () => { expect(isScreenshotTableGateAction("close")).toBe(true); - expect(isScreenshotTableGateAction("request_changes")).toBe(true); - expect(isScreenshotTableGateAction("comment")).toBe(true); }); it("rejects a non-string or unknown value", () => { @@ -31,6 +29,11 @@ describe("isScreenshotTableGateAction", () => { expect(isScreenshotTableGateAction(123)).toBe(false); expect(isScreenshotTableGateAction(undefined)).toBe(false); }); + + it("rejects request_changes/comment (#4110 removed as dead config surface)", () => { + expect(isScreenshotTableGateAction("request_changes")).toBe(false); + expect(isScreenshotTableGateAction("comment")).toBe(false); + }); }); describe("hasImageBearingMarkdownTable", () => { @@ -184,10 +187,10 @@ describe("normalizeScreenshotTableGateConfig", () => { it("parses a fully valid object", () => { const result = normalizeScreenshotTableGateConfig( - { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "comment", message: "custom text" }, + { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }, [], ); - expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "comment", message: "custom text" }); + expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }); }); it("rejects a non-boolean enabled with a warning, falling back to false", () => { @@ -202,6 +205,13 @@ describe("normalizeScreenshotTableGateConfig", () => { expect(warnings.some((w) => w.includes("action"))).toBe(true); }); + it("rejects the removed request_changes/comment values (#4110), falling back to close", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig({ action: "request_changes" }, warnings).action).toBe("close"); + expect(normalizeScreenshotTableGateConfig({ action: "comment" }, []).action).toBe("close"); + expect(warnings.some((w) => w.includes("action"))).toBe(true); + }); + it("rejects a non-string/empty message with a warning, falling back to undefined", () => { const warnings: string[] = []; const result = normalizeScreenshotTableGateConfig({ message: " " }, warnings); @@ -310,4 +320,51 @@ describe("evaluateScreenshotTableGate", () => { expect(evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: null, prLabels: [], changedFiles: [] }).violated).toBe(true); expect(evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: undefined, prLabels: [], changedFiles: [] }).violated).toBe(true); }); + + describe("botCaptureSatisfied (#4110)", () => { + it("no violation when the bot's own capture already succeeded, even with no body table at all", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: "Just changed some CSS, trust me.", + prLabels: [], + changedFiles: [], + botCaptureSatisfied: true, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("satisfies the gate even when the body would otherwise fail the anti-gaming checks (image outside table + committed image)", () => { + const gamedBody = `${TABLE_BODY}\n\nAlso here's a bonus shot: ![bonus](https://x/bonus.png)`; + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, whenPaths: ["apps/ui/**"] }), + prBody: gamedBody, + prLabels: [], + changedFiles: ["apps/ui/public/screenshot.png"], + botCaptureSatisfied: true, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("still violates when botCaptureSatisfied is explicitly false and there is no table", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: "no table here", + prLabels: [], + changedFiles: [], + botCaptureSatisfied: false, + }); + expect(result.violated).toBe(true); + }); + + it("does not put an out-of-scope PR into scope just because the bot captured something", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, whenLabels: ["frontend"] }), + prBody: "no table here", + prLabels: ["backend"], + changedFiles: [], + botCaptureSatisfied: true, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + }); }); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index 1516050e8f..e9fd31ca19 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -19,10 +19,8 @@ function config(overrides: Partial = {}): ScreenshotT const TABLE_BODY = ["| Before | After |", "| --- | --- |", "| ![before](https://x/before.png) | ![after](https://x/after.png) |"].join("\n"); describe("isScreenshotTableGateAction", () => { - it("accepts every valid action", () => { + it("accepts the only valid action", () => { expect(isScreenshotTableGateAction("close")).toBe(true); - expect(isScreenshotTableGateAction("request_changes")).toBe(true); - expect(isScreenshotTableGateAction("comment")).toBe(true); }); it("rejects a non-string or unknown value", () => { @@ -30,6 +28,11 @@ describe("isScreenshotTableGateAction", () => { expect(isScreenshotTableGateAction(123)).toBe(false); expect(isScreenshotTableGateAction(undefined)).toBe(false); }); + + it("rejects request_changes/comment (#4110 removed as dead config surface)", () => { + expect(isScreenshotTableGateAction("request_changes")).toBe(false); + expect(isScreenshotTableGateAction("comment")).toBe(false); + }); }); describe("hasImageBearingMarkdownTable", () => { @@ -183,10 +186,10 @@ describe("normalizeScreenshotTableGateConfig", () => { it("parses a fully valid object", () => { const result = normalizeScreenshotTableGateConfig( - { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "comment", message: "custom text" }, + { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }, [], ); - expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "comment", message: "custom text" }); + expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }); }); it("rejects a non-boolean enabled with a warning, falling back to false", () => { @@ -201,6 +204,13 @@ describe("normalizeScreenshotTableGateConfig", () => { expect(warnings.some((w) => w.includes("action"))).toBe(true); }); + it("rejects the removed request_changes/comment values (#4110), falling back to close", () => { + const warnings: string[] = []; + expect(normalizeScreenshotTableGateConfig({ action: "request_changes" }, warnings).action).toBe("close"); + expect(normalizeScreenshotTableGateConfig({ action: "comment" }, []).action).toBe("close"); + expect(warnings.some((w) => w.includes("action"))).toBe(true); + }); + it("rejects a non-string/empty message with a warning, falling back to undefined", () => { const warnings: string[] = []; const result = normalizeScreenshotTableGateConfig({ message: " " }, warnings); @@ -309,4 +319,51 @@ describe("evaluateScreenshotTableGate", () => { expect(evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: null, prLabels: [], changedFiles: [] }).violated).toBe(true); expect(evaluateScreenshotTableGate({ config: config({ enabled: true }), prBody: undefined, prLabels: [], changedFiles: [] }).violated).toBe(true); }); + + describe("botCaptureSatisfied (#4110)", () => { + it("no violation when the bot's own capture already succeeded, even with no body table at all", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: "Just changed some CSS, trust me.", + prLabels: [], + changedFiles: [], + botCaptureSatisfied: true, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("satisfies the gate even when the body would otherwise fail the anti-gaming checks (image outside table + committed image)", () => { + const gamedBody = `${TABLE_BODY}\n\nAlso here's a bonus shot: ![bonus](https://x/bonus.png)`; + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, whenPaths: ["apps/ui/**"] }), + prBody: gamedBody, + prLabels: [], + changedFiles: ["apps/ui/public/screenshot.png"], + botCaptureSatisfied: true, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("still violates when botCaptureSatisfied is explicitly false and there is no table", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true }), + prBody: "no table here", + prLabels: [], + changedFiles: [], + botCaptureSatisfied: false, + }); + expect(result.violated).toBe(true); + }); + + it("does not put an out-of-scope PR into scope just because the bot captured something", () => { + const result = evaluateScreenshotTableGate({ + config: config({ enabled: true, whenLabels: ["frontend"] }), + prBody: "no table here", + prLabels: ["backend"], + changedFiles: [], + botCaptureSatisfied: true, + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + }); }); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 8806b38a36..bf47abaf84 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -4,7 +4,8 @@ import { githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation, } from "../../src/github/client"; -import { buildCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; +import { buildCapture, hasSuccessfulBotCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; +import type { CaptureRoute } from "../../src/review/visual/capture"; import * as pixelDiffModule from "../../src/review/visual/pixel-diff"; import * as previewUrlModule from "../../src/review/visual/preview-url"; import * as scrollGifModule from "../../src/review/visual/scroll-gif"; @@ -1075,3 +1076,56 @@ describe("buildCapture scroll-GIF wiring (#3612)", () => { } }); }); + +describe("hasSuccessfulBotCapture (#4110)", () => { + const REAL_BEFORE = "https://api.example/gittensory/shot?url=https%3A%2F%2Fprod.example%2Fapp&w=1440&h=900"; + const REAL_AFTER = "https://api.example/gittensory/shot?url=https%3A%2F%2Fpreview.example%2Fapp&w=1440&h=900"; + const LOADING_PLACEHOLDER = "https://api.example/gittensory/shot?placeholder=loading"; + const FAILED_PLACEHOLDER = "https://api.example/gittensory/shot?placeholder=failed"; + + function route(overrides: Partial = {}): CaptureRoute { + return { path: "/app", ...overrides }; + } + + it("true when a route has a real before+after pair on desktop", () => { + expect(hasSuccessfulBotCapture([route({ beforeUrl: REAL_BEFORE, afterUrl: REAL_AFTER })])).toBe(true); + }); + + it("true when only the MOBILE pair is real (desktop absent)", () => { + expect(hasSuccessfulBotCapture([route({ beforeUrlMobile: REAL_BEFORE, afterUrlMobile: REAL_AFTER })])).toBe(true); + }); + + it("false when afterUrl is a placeholder (preview still building)", () => { + expect(hasSuccessfulBotCapture([route({ beforeUrl: REAL_BEFORE, afterUrl: LOADING_PLACEHOLDER })])).toBe(false); + }); + + it("false when afterUrl is the failed-deploy placeholder", () => { + expect(hasSuccessfulBotCapture([route({ beforeUrl: REAL_BEFORE, afterUrl: FAILED_PLACEHOLDER })])).toBe(false); + }); + + it("false when beforeUrl is missing (no production render)", () => { + expect(hasSuccessfulBotCapture([route({ afterUrl: REAL_AFTER })])).toBe(false); + }); + + it("false when afterUrl is an empty string", () => { + expect(hasSuccessfulBotCapture([route({ beforeUrl: REAL_BEFORE, afterUrl: "" })])).toBe(false); + }); + + it("false for a route with no shots at all", () => { + expect(hasSuccessfulBotCapture([route()])).toBe(false); + }); + + it("false for an empty routes array (capture never ran / found nothing)", () => { + expect(hasSuccessfulBotCapture([])).toBe(false); + }); + + it("true when only ONE of several routes has a real pair (some() semantics, not every())", () => { + const routes = [route({ path: "/a", afterUrl: LOADING_PLACEHOLDER, beforeUrl: REAL_BEFORE }), route({ path: "/b", beforeUrl: REAL_BEFORE, afterUrl: REAL_AFTER })]; + expect(hasSuccessfulBotCapture(routes)).toBe(true); + }); + + it("false when every route is all-placeholder", () => { + const routes = [route({ path: "/a", beforeUrl: REAL_BEFORE, afterUrl: LOADING_PLACEHOLDER }), route({ path: "/b", beforeUrl: REAL_BEFORE, afterUrl: FAILED_PLACEHOLDER })]; + expect(hasSuccessfulBotCapture(routes)).toBe(false); + }); +});