From 27fad9e1f95e26274cfdeecc777368d52ae3db8b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:43:23 -0700 Subject: [PATCH] fix(queue): guard the verdict derivation instead of the pass, so the webhook path is covered (#10227) right. The record half sits at the single ledger write every verdict passes through -- correct, and its own comment says why. The skip half sat at the ENTRY of the whole publish-and-maintain pass, so it suppressed everything the pass OWED, not just the redundant re-derivation. That made the escape hatch an allowlist. Every bounded retry chain had to remember to add itself, and the default for anything unlisted was "suppress me" -- so the webhook path could never be guarded at all: adding it truncated #10061's recapture chain from MAX_CAPTURE_RETRY_ATTEMPTS (5) to VERDICT_BACKOFF_MIN_REPEATS (3), reproduced here before the fix. The webhook path is where the churn actually is. Measured on the Orb over 24h: upstream_state_change carries 348 decision records across 99 heads (3.52 records_per_head), while every other reason sits at 1.08-1.32. The sweep-only guard fired 11 times in the same window. The skip now sits at the one point a pass derives a verdict, inside maybePublishPrPublicSurface -- the same KIND of place the record half already occupies. Everything the pass owes runs above it and cannot be truncated by construction; the pending gate check-run, the AI review, the gate evaluation, the publish and the maintenance ledger write all sit below it. Both entry points reach a verdict only through that function, so neither has to opt in and the webhook path is covered without a second call site. The recapture chain was the one obligation living below that line, inside the comment publish, so it is extracted (runVisualCaptureObligation) and run explicitly on the backed-off path. Its files resolver is threaded as a thunk rather than a resolved list so a backed-off pass on a repo with screenshots off pays for no diff fetch. The normal publish path is unchanged: same call, same place, same order. explicitlyRequested shrinks to one value, webhook.forceAiReview, and it means exactly one thing -- a human asked for this pass (an operator re-gate, or a consumed "Re-run LoopOver review" click). previewPollAttempt needed an exemption only because the old guard sat above the recapture chain. Two behaviours #10229 established are deliberately reversed, both pinned by name in verdict-stability-wire.test.ts rather than left to be rediscovered: * A poll tick is now backed off. It is machine-paced, not human-asked, and its chain runs above the guard, so it has no business in a hatch whose meaning is "a human asked for this". * A backed-off pass charges the regate repair budget again. onReachedReadiness's documented rule is "count executions, not deferrals", and a pass that clears readiness, decides type labels and advances the capture chain is an execution. It is also the anti-starvation direction: a repair on a permanently settled hold would otherwise reselect every sweep tick forever without exhausting. scripts/check-verdict-backoff-chokepoint.ts keeps the structure honest -- one call site, inside the choke point, above the derivation, and every publish-and-maintain entry point routing through it. The fourth rule is the one the issue asked for: a NEW entry point that derives a verdict some other way fails the build instead of silently leaving the backoff behind. Closes #10227 --- package.json | 3 +- scripts/check-verdict-backoff-chokepoint.ts | 141 +++++ src/queue/processors.ts | 486 ++++++++++-------- .../check-verdict-backoff-chokepoint.test.ts | 145 ++++++ test/unit/queue-3.test.ts | 320 ++++++++++++ test/unit/verdict-stability-wire.test.ts | 236 +++++++-- 6 files changed, 1087 insertions(+), 244 deletions(-) create mode 100644 scripts/check-verdict-backoff-chokepoint.ts create mode 100644 test/unit/check-verdict-backoff-chokepoint.test.ts diff --git a/package.json b/package.json index dcce4b75f..7b7959a27 100644 --- a/package.json +++ b/package.json @@ -103,6 +103,7 @@ "dispatch-gate-reasons:check": "node --experimental-strip-types scripts/check-dispatch-gate-reasons.ts", "dispatch-provenance:check": "tsx scripts/check-dispatch-provenance-stamped.ts", "record-mapper-fields:check": "node --experimental-strip-types scripts/check-record-mapper-fields.ts", + "verdict-backoff-chokepoint:check": "node --experimental-strip-types scripts/check-verdict-backoff-chokepoint.ts", "replay-runner-manifest": "tsx scripts/replay-runner-image-manifest.ts", "replay-runner-manifest:write": "tsx scripts/replay-runner-image-manifest.ts --write", "replay-runner-manifest:check": "tsx scripts/replay-runner-image-manifest.ts --check", @@ -151,7 +152,7 @@ "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node --experimental-strip-types scripts/smoke-ui-browser.ts", "pretest:ci": "npm run check-node-version", - "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run record-mapper-fields:check && npm run dispatch-provenance:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run lint:composite-actions && npm run db:migrations:check && npm run db:migrations:immutable:check && npm run turbo-inputs:check && npm run workspace-dep-ranges:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run miner:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run build --workspace @loopover/engine && npm run build --workspace @loopover/discovery-index && npm run build:mcp && npm run build:miner && npm run build --workspace @loopover/ui-kit && npm run typecheck && npm run test:coverage && npm run test:engine-parity && npm run test:live-gate-parity && npm run test:driver-parity && npm run validate:mcp && npm run test --workspace @loopover/engine && npm run test:workers && npm run test:mcp-pack && npm run test:contract-pack && npm run test:miner-pack && npm run test:engine-pack && npm run test:ui-kit-pack && npm run test:miner-deployment-docs-audit && npm run rees:test && npm run ui:openapi:check && npm run control-plane:contract:check && npm run control-plane:openapi:check && npm run ui:version-audit && npm run docs:drift-check && npm run coverage-boltons:check && npm run import-specifiers:check && npm run ui-derived-types:check && npm run server-manifest:check && npm run dead-source-files:check && npm run dead-exports:check && npm run publishable-deps:check && npm run fixture-clock-races:check && npm run typecheck-coverage:check && npm run test-wiring:check && npm run checkers-wired:check && npm run focus-manifest-fields:check && npm run release-commit-parsing:check && npm run releasable-commit-types:check && npm run regate-sort-key:check && npm run maintainer-associations:check && npm run command-redelivery-guards:check && npm run dispatch-gate-reasons:check && npm run verdict-backoff-chokepoint:check && npm run record-mapper-fields:check && npm run dispatch-provenance:check && npm run validate:no-hand-written-js && npm run replay-runner-manifest:check && npm run coco-dev-versions:check && npm run branding-drift:check && npm run manifest:drift-check && npm run engine-parity:drift-check && npm run engines-nvmrc:check && npm run release-manifest:sync:check && npm run release-linked-versions:check && npm run command-reference:check && npm run mcp:tool-reference:check && npm run mcp:client-config:check && npm run contract:api-schemas:check && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci", "test:watch": "vitest", diff --git a/scripts/check-verdict-backoff-chokepoint.ts b/scripts/check-verdict-backoff-chokepoint.ts new file mode 100644 index 000000000..6a89bbc57 --- /dev/null +++ b/scripts/check-verdict-backoff-chokepoint.ts @@ -0,0 +1,141 @@ +#!/usr/bin/env node +// #10227: the verdict-stability backoff (#10184) must guard the verdict DERIVATION, not a pass entry point. +// +// The bug class this closes, stated once. #10204 put the skip at the entry of the whole publish-and-maintain +// pass, so it suppressed everything the pass OWED -- bounded retry chains, one-shot markers, budgets -- and +// not just the redundant re-derivation. Each bounded retry then needed an entry in an `explicitlyRequested` +// allowlist, and a chain that forgot to join it was silently truncated the day it shipped. It is the shape of +// rule whose correctness depends on every future author remembering an unwritten obligation, and the repo has +// been burned by that shape enough times (#9860, #10061) to stop trusting it. +// +// The redesign removes the obligation instead of documenting it: one guard, at the single point a pass derives +// a verdict, with everything the pass owes running above it. A new retry chain is then safe BY CONSTRUCTION. +// That guarantee is only worth anything while the structure holds, so this checks the structure: +// +// 1. ONE call site. The guard is a choke point or it is nothing; two call sites means it is a per-entry-point +// rule again, and the second one will drift. +// 2. That call site is inside maybePublishPrPublicSurface -- the function both entry points reach a verdict +// through. +// 3. It sits ABOVE the derivation it is meant to skip (the pending gate check-run and the gate evaluation). +// A guard that drifts below them still compiles, still passes every behavioural test that only asserts +// "backed off", and saves nothing. +// 4. Every publish-and-maintain entry point routes through it: a function that runs maybeRunAgentMaintenance +// must get its gate from maybePublishPrPublicSurface. This is the one the issue asked for -- a NEW entry +// point that derives a verdict some other way bypasses the choke point, and nothing else would notice. +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; + +const PROCESSORS = "src/queue/processors.ts"; + +/** The guard, the function that must contain it, and the two derivation steps it must precede. */ +const GUARD_CALL = "await stableVerdictBackoffEngaged(env, {"; +const CHOKE_POINT_FUNCTION = "maybePublishPrPublicSurface"; +const DERIVATION_MARKERS = ["createOrUpdatePendingGateCheckRun(", "gateEvaluation = await withReviewPipelineSpan("] as const; + +/** The call that makes a function a publish-and-maintain entry point, and the call that must accompany it. */ +const MAINTENANCE_CALL = "maybeRunAgentMaintenance(env, {"; +const PUBLISH_CALL = "maybePublishPrPublicSurface("; + +export type Violation = { rule: string; detail: string }; + +/** Byte offsets of every occurrence of `needle` in `source`. */ +export function occurrences(source: string, needle: string): number[] { + const found: number[] = []; + for (let at = source.indexOf(needle); at !== -1; at = source.indexOf(needle, at + 1)) found.push(at); + return found; +} + +/** + * The `[start, end)` byte range of the top-level function named `name`. + * + * Deliberately brace-free: it runs from the declaration to the next TOP-LEVEL declaration, which is what + * "column 0" identifies unambiguously in this file's formatting. A brace counter would have to understand + * strings, regex literals and template literals to be correct over a 13k-line file, and being subtly wrong + * about the range is worse here than not checking, because a checker that cries wolf gets muted. + */ +export function topLevelFunctionRange(source: string, name: string): { start: number; end: number } | null { + const declaration = source.indexOf(`\nasync function ${name}(`); + if (declaration === -1) return null; + const start = declaration + 1; + const next = source.slice(start + 1).search(/\n(?:export )?(?:async )?function /); + return { start, end: next === -1 ? source.length : start + 1 + next }; +} + +/** + * The enclosing top-level function for a byte offset: the last `function` declaration at column 0 before it. + * Used to answer "which entry point is this maybeRunAgentMaintenance call in", so a caller's own body -- not + * the whole file -- is what gets searched for the matching publish call. + */ +export function enclosingFunction(source: string, offset: number): { name: string; start: number } | null { + const before = source.slice(0, offset); + const matches = [...before.matchAll(/\n(?:export )?(?:async )?function (\w+)\(/g)]; + const last = matches.at(-1); + if (!last || last[1] === undefined || last.index === undefined) return null; + return { name: last[1], start: last.index + 1 }; +} + +/** Every violation of the four rules above, in the order they are numbered. */ +export function findViolations(source: string): Violation[] { + const violations: Violation[] = []; + const guards = occurrences(source, GUARD_CALL); + + if (guards.length !== 1) { + violations.push({ + rule: "one call site", + detail: `stableVerdictBackoffEngaged is called ${guards.length} time(s); the choke point is exactly one.`, + }); + // Rules 2 and 3 are about THE call site, so they cannot be evaluated without one. + if (guards.length === 0) return violations; + } + + const chokePoint = topLevelFunctionRange(source, CHOKE_POINT_FUNCTION); + if (chokePoint === null) { + violations.push({ rule: "choke point exists", detail: `${CHOKE_POINT_FUNCTION} was renamed or removed; this checker needs updating with it.` }); + return violations; + } + + const guard = guards[0]!; + if (guard < chokePoint.start || guard >= chokePoint.end) { + violations.push({ rule: "guard is inside the choke point", detail: `the guard is outside ${CHOKE_POINT_FUNCTION}, so it is guarding a pass entry again.` }); + } else { + for (const marker of DERIVATION_MARKERS) { + const derivation = source.indexOf(marker, chokePoint.start); + if (derivation !== -1 && derivation < chokePoint.end && derivation < guard) { + violations.push({ rule: "guard precedes the derivation", detail: `\`${marker}\` runs BEFORE the guard, so a backed-off pass still pays for it.` }); + } + } + } + + for (const call of occurrences(source, MAINTENANCE_CALL)) { + const entry = enclosingFunction(source, call); + if (entry === null) continue; + if (!source.slice(entry.start, call).includes(PUBLISH_CALL)) { + violations.push({ + rule: "entry points route through the choke point", + detail: `${entry.name} runs the maintenance pass without deriving its gate through ${CHOKE_POINT_FUNCTION}, so it bypasses the backoff.`, + }); + } + } + + return violations; +} + +function main(): void { + const violations = findViolations(readFileSync(PROCESSORS, "utf8")); + if (violations.length === 0) { + console.log("check-verdict-backoff-chokepoint: the verdict-stability backoff guards exactly one derivation choke point."); + return; + } + process.stderr.write(`check-verdict-backoff-chokepoint: ${violations.length} violation(s) of the #10227 choke-point invariant in ${PROCESSORS}:\n\n`); + for (const violation of violations) process.stderr.write(` [${violation.rule}] ${violation.detail}\n`); + process.stderr.write( + "\nThe backoff must guard the point a pass DERIVES a verdict, not the entry of a pass. Guarding an entry\n" + + "suppresses everything the pass owed -- bounded retry chains, one-shot markers, budgets -- and then every\n" + + "one of them needs an exemption it has to remember to ask for (#10204 truncated #10061's recapture chain\n" + + "from 5 attempts to 3 exactly this way). Put the work a pass OWES above the guard, and the verdict\n" + + "derivation below it, and no future chain needs to know the guard exists.\n", + ); + process.exit(1); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 041500c61..ebe5641f0 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -4369,27 +4369,6 @@ export async function reReviewStoredPullRequest( )) ) return false; - // #10222: back off AFTER the readiness gate, but BEFORE onReachedReadiness and the retrigger consumption - // just below. #10204 placed it after the actuation-lock claim, past both -- so a backed-off pass had already - // consumed the user's one-shot "Re-run LoopOver review" marker and charged regatePullRequest's repair budget - // for work it never did. It must not move EARLIER than readiness either: readiness legitimately defers a - // pass (rebase fired, CI still running), and the screenshot-table gate's bounded recapture chain (#10061) - // depends on those deferrals continuing to happen, so a pre-readiness guard silently truncates that retry - // budget. Between the two is the only correct place. - // - // `force` is an operator's manual re-gate and `previewPollAttempt` is a visual poll's own next tick -- both - // are explicit requests for THIS pass, and backoff must never suppress a pass a human or a bounded retry - // chain asked for. - if ( - await stableVerdictBackoffEngaged(env, { - repoFullName, - prNumber, - headSha: pr.headSha, - deliveryId, - explicitlyRequested: options.force === true || previewPollAttempt !== undefined, - }) - ) - return false; // Fire BEFORE any further (throwable) work below -- this is the one instant readiness is confirmed, so a // caller learns it even if this call goes on to THROW instead of returning (see the JSDoc above). options.onReachedReadiness?.(); @@ -5164,21 +5143,22 @@ async function consumePendingPrPanelRetrigger( * force-finalize → keeps the safe old defer rather than acting early). */ /** - * #10222: should this publish-and-maintain pass back off, because this PR's verdict has not changed (#10184)? - * - * Shared by BOTH sites that run the unit -- `reReviewStoredPullRequest` (sweep / CI completion) and - * `handlePullRequestWebhookEvent` (the `pull_request` webhook). #10204 guarded only the first, which left the - * DOMINANT source unthrottled: over 24h on the Orb, 293 of 344 repeat evaluations carried - * `upstream_state_change` -- `deriveReevaluationReason`'s mapping for a RAW GitHub delivery, i.e. the webhook - * path. A label write the engine itself caused arrives there, not on the sweep. + * #10227: should this pass stop before DERIVING a verdict, because this PR's answer has not changed (#10184)? * - * Call this BEFORE the readiness gate at either site. Readiness fires `onReachedReadiness` (which charges - * regatePullRequest's repair budget) and consumes the one-shot panel-retrigger marker, and a pass that backs - * off after those has silently eaten a user's "Re-run LoopOver review" click with nothing left to re-trigger - * it. Before the lock claim, too: there is no lock to take or release on a pass that is not going to run. + * ONE caller by design: the verdict-derivation choke point inside `maybePublishPrPublicSurface` (see the + * banner comment there). Both entry points that run the publish-and-maintain unit -- `reReviewStoredPullRequest` + * (sweep / CI completion) and `handlePullRequestWebhookEvent` (the `pull_request` webhook) -- reach a verdict + * only through that function, so guarding it covers both without either having to remember to opt in. That + * matters: the webhook path is the DOMINANT source (over 24h on the Orb, 293 of 344 repeat evaluations carried + * `upstream_state_change` -- `deriveReevaluationReason`'s mapping for a RAW GitHub delivery) and it was never + * guarded at all while the guard lived at the callers' pass entry, because no pass-entry guard could be made + * to spare #10061's recapture chain. `scripts/check-verdict-backoff-chokepoint.ts` fails the build if a second + * call site appears, or if a publish-and-maintain entry point stops routing through the choke point. * * `explicitlyRequested` is the escape hatch and the reason this takes a flag at all -- backoff exists to stop - * the machine asking itself the same question, and must NEVER suppress a pass a human asked for. + * the machine asking itself the same question, and must NEVER suppress a pass a human asked for. It has exactly + * one caller-supplied value now (`webhook.forceAiReview`); a bounded retry chain needs no exemption, because + * every obligation runs ABOVE the choke point. * * Fails OPEN everywhere: no head SHA, no state, unreadable state, no cache, or a throwing read all return * false and evaluate normally (see verdict-stability.ts). The delay is capped, so even a stuck PR is still @@ -10237,6 +10217,190 @@ async function scheduleVisualCaptureRetry( } } +/** The visual before/after capture, and the bounded self-heal recapture chain it schedules (#9030 / #9464 / + * #10061). Extracted from `maybePublishPrPublicSurface`'s comment-publish block so BOTH the ordinary publish + * path and a verdict-backed-off pass (#10227) can run it -- the capture chain is an OBLIGATION of the pass, + * not part of deriving the verdict, and its 5-attempt budget is spent across separate passes. A pass that + * skipped it would silently truncate that budget to VERDICT_BACKOFF_MIN_REPEATS attempts, which is exactly + * what the two `screenshot-table gate (#10061)` cases in test/unit/queue-3.test.ts pin. */ +async function runVisualCaptureObligation( + env: Env, + args: { + installationId: number; + repoFullName: string; + pr: Awaited>; + repo: Awaited>; + mode: Awaited>; + repoFocusManifestForComment: Awaited>; + /** The pass's memoized file resolver, not a resolved list: the converged-feature check below is the cheap + * one, and a backed-off pass (#10227) must not pay for a diff fetch on a repo that has screenshots off. */ + getUnifiedFiles: () => Promise>>; + webhook: { deliveryId: string; previewPollAttempt?: number | undefined }; + }, +): Promise<{ beforeAfter: CaptureRoute[]; interactionPreviews: CaptureInteractionRoute[]; bugAnalysisEnabled: boolean }> { + const { installationId, repoFullName, pr, repo, mode, repoFocusManifestForComment, getUnifiedFiles, webhook } = args; + // Visual before/after capture (visual-capture port). Fires ONLY when (1) the "screenshots" converged + // feature resolves active for this repo (resolveConvergedFeature — the global flag AND (a per-repo + // `features.screenshots` override OR the cutover allowlist default), #4616; the caller threads in the + // manifest it already loaded, so this costs no extra fetch) AND (2) the PR touches WEB-VISIBLE files + // (isVisualPath — frontend pages / public OG images; backend .ts/.md/.json PRs never qualify). Fully + // wrapped in try/catch + defaults to [] so a capture failure (render timeout, missing binding, GitHub + // hiccup) can NEVER sink the review — it just omits the "Visual preview" section. Flag-OFF (default) ⇒ + // this returns empty and the unified comment is byte-identical. + let beforeAfter: CaptureRoute[] = []; + // review.visual.bugAnalysis — resolved inside the try block below alongside the rest of reviewVisualConfig, + // but needed at the runVisualVisionForAdvisory call OUTSIDE that block's scope (that call is deliberately + // independent of the capture try/catch above, see its own comment) — mirrors beforeAfter's own + // declared-outer/assigned-inner pattern. false (unresolved, or resolution never ran) ⇒ byte-identical + // default-prompt behavior, same fail-safe direction as every other config-as-code read here. + let bugAnalysisEnabled = false; + let interactionPreviews: CaptureInteractionRoute[] = []; + if (!resolveConvergedFeature(env, repoFocusManifestForComment, "screenshots", repoFullName)) + return { beforeAfter, interactionPreviews, bugAnalysisEnabled }; + const unifiedFiles = await getUnifiedFiles(); + const visualFiles = unifiedFiles + .map((file) => file.path) + .filter(isVisualPath); + // #auto-interaction-detection: only ever read by buildCapture when review.visual.autoDetectInteractions + // is on for this repo -- carries each changed file's own diff patch text (visualFiles above is bare + // paths), the same file.payload?.patch shape review-diff.ts/grounding-wire.ts already read elsewhere. + const changedCssFiles = unifiedFiles.map((file) => ({ + path: file.path, + patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, + })); + if (visualFiles.length > 0) { + try { + const token = await createInstallationToken(env, installationId); + // review.visual (#3609 / #3610): an explicit per-repo preview-URL template / route list. Absent config + // (the default for every repo today) ⇒ EMPTY_VISUAL_CONFIG ⇒ buildCapture's discovery/inference + // behavior is byte-identical to pre-#3609. + const reviewVisualConfig = await resolveVisualCaptureConfig(env, repoFullName); + bugAnalysisEnabled = reviewVisualConfig.bugAnalysis === true; + const captureTarget = { + repoFullName, + prNumber: pr.number, + ...(pr.headSha ? { headSha: pr.headSha } : {}), + ...(pr.headRef ? { headRef: pr.headRef } : {}), + previewFromChecks: true, + // Pins the actions_fallback dispatch (#4112) to a trusted ref -- see buildCapture. Absent (no + // stored default branch yet) ⇒ that dispatch just never fires, same as leaving it unconfigured. + ...(repo?.defaultBranch ? { defaultBranchRef: repo.defaultBranch } : {}), + // #9067: the actions_fallback dispatch is a real GitHub write and must obey dry_run/paused. + mode, + }; + // review.visual.enabled (#4083): a config-as-code override layered on top of the screenshotsAllowed + // env-var gate resolveConvergedFeature applies, not a replacement for it. Unset/true ⇒ defer to that gate (buildCapture + // runs exactly as before); explicit `false` (global default or per-repo, VPS-only) ⇒ force capture off + // for this repo -- a no-routes, non-pending sentinel result, so every line below behaves exactly as an + // ordinary "nothing found" capture would, with no separate code path to maintain. + const capture = + reviewVisualConfig.enabled === false + ? { routes: [], interactions: [], previewPending: false, renderFailed: false, previewUnobtainable: false } + : await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig, changedCssFiles); + beforeAfter = capture.routes; + interactionPreviews = capture.interactions; + // 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 + // webhook also refills it; this is the backstop when that event is missed/late). + // + // #9464: `capture.renderFailed` joins previewPending here as a SECOND blip class. captureShot + // swallows a renderer error per shot and returns a null PNG, so buildCapture used to return + // normally -- previewPending false, nothing thrown -- and neither the #9030 nor the #9207 guard + // fired. The maintenance pass then read "no evidence, no retry pending" and CLOSED the PR one-shot. + // A browserless outage now degrades to "we could not capture evidence, holding" instead. + // + // #9876: the `else` below is the half that was missing, and it is the half that froze three + // contributor PRs. Reaching it means this capture CONCLUDED -- no successful pair, but also nothing + // still pending and nothing broken -- which is a definite answer to the question the latch was + // deferring. Without it, a latch written by an earlier attempt outlived the chain that justified it: + // the durable per-head poll budget ends that chain by suppressing `previewPending`, so the final + // attempt takes neither branch above, `scheduleVisualCaptureRetry` is never called, and the + // budget-exhausted clear that #9462 put INSIDE it never runs. The release belongs to the capture + // outcome, where every conclusion passes, not to a scheduler that by definition stops being called. + const previewPollAttempt = webhook.previewPollAttempt ?? 0; + if (capture.previewPending || capture.renderFailed) { + await scheduleVisualCaptureRetry(env, { + webhook, + repoFullName, + pr, + installationId, + previewPollAttempt, + }); + } else if (pr.headSha) { + // #9881: this capture concluded, and it concluded because the repo has no preview pipeline at all + // (build state `absent` across every poll, budget now spent) -- not because a deploy was late or + // a renderer blipped. Record it so the screenshot-table gate degrades its CLOSE instead of + // destroying a PR over evidence no contributor action could produce. + if (capture.previewUnobtainable) { + await markPullRequestVisualCaptureUnobtainable(env, repoFullName, pr.number, pr.headSha).catch((error) => { + console.log( + JSON.stringify({ + event: "visual_capture_unobtainable_mark_failed", + repoFullName, + pull: pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + }); + } + // Conclusive: release any latch this head still carries. Best-effort and idempotent -- the common + // case is that there is no latch to clear, and a failed clear only leaves the age bound in + // visual-capture-retry-latch.ts to end the deferral instead of ending it now. + await clearPullRequestVisualCaptureRetryPending(env, repoFullName, pr.number, pr.headSha).catch((error) => { + console.log( + JSON.stringify({ + event: "visual_capture_retry_pending_clear_failed", + repoFullName, + pull: pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + }); + } + } catch (error) { + console.log( + JSON.stringify({ + event: "visual_capture_error", + repoFullName, + pull: pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + // #9030: a capture-pipeline ERROR (browserless down, timeout, a GitHub hiccup fetching a token) must + // not be silently indistinguishable from a legitimate "no visual routes found" result -- the + // screenshotTableGate's CLOSE action would otherwise fire on a false positive purely because an + // internal service blipped. Schedule the SAME bounded self-heal retry previewPending already uses. + await scheduleVisualCaptureRetry(env, { + webhook, + repoFullName, + pr, + installationId, + previewPollAttempt: webhook.previewPollAttempt ?? 0, + }); + } + } + return { beforeAfter, interactionPreviews, bugAnalysisEnabled }; +} + async function maybePublishPrPublicSurface( env: Env, installationId: number, @@ -10689,6 +10853,73 @@ async function maybePublishPrPublicSurface( willCheckRun: false, }; + // The PR's changed files are needed by the slop/manifest gates, the AI review + grounding + RAG, the secret + // scan, the check-run, and the unified comment. Resolve them AT MOST ONCE per review and share across the + // gate phase (inside the try) AND the publish phase (check-run + comment, after the try): memoize the first + // resolve so a repo that needs files anywhere pays a single resolve, and a gate-only repo that never needs + // them pays nothing. resolvePullRequestFilesForReview prefers the stored rows and, when they are empty at + // review time (the webhook beat detail-sync), fetches + persists them inline — so the FIRST review sees the + // real diff instead of "0 files / No diff provided" (FIX B). Fail-safe by construction. + let reviewFiles: Awaited> | null = + null; + const getReviewFiles = async (): Promise< + Awaited> + > => { + if (reviewFiles === null) + reviewFiles = await resolvePullRequestFilesForReview(env, { + installationId, + repoFullName, + pullNumber: pr.number, + }); + return reviewFiles; + }; + // ── THE VERDICT-DERIVATION CHOKE POINT (#10227) ────────────────────────────────────────────────────── + // Everything ABOVE this line is what the pass OWES regardless of whether its answer has changed: the + // type-label decision, the miner/contributor resolution, the readiness bookkeeping its callers already did. + // Everything BELOW it derives and publishes a verdict -- the pending gate check-run, the AI review, the gate + // evaluation, the comment/label publish, and (via the `gate` this returns) the maintenance pass's ledger + // write. So this is the one boundary a settled verdict should stop at, and it is deliberately the same KIND + // of place the `record` half already sits in: the single point every verdict passes through. + // + // #10204/#10229 guarded the ENTRY of the whole publish-and-maintain pass instead, at both call sites. That + // suppressed obligations along with the derivation, so every bounded retry chain needed an entry in an + // `explicitlyRequested` allowlist and any chain that forgot to join it was silently truncated the day it + // shipped -- measurably, the #10061 recapture chain lost 2 of its 5 attempts. Guarding here needs no + // allowlist: a chain that runs before this line cannot be truncated by construction, and one entry point is + // covered instead of two (the webhook path, 293 of 344 repeat evaluations in a 24h Orb window, was never + // guarded at all because there was no way to exempt #10061 from a pass-entry guard). + // + // The recapture chain is the one obligation that lived BELOW this line (inside the comment publish), so it + // is run explicitly here before returning -- see runVisualCaptureObligation. + // + // Returning `undefined` is already the "no verdict this pass" signal: maybeRunAgentMaintenance early-returns + // on `if (!gate) return;`, and the webhook path's own comment states the same contract. No sentinel needed. + // + // `webhook.forceAiReview` is the WHOLE allowlist now, and it means exactly one thing: a human asked for this + // pass. Both of its producers are human-initiated -- an operator's manual re-gate (reReviewStoredPullRequest's + // `options.force`) and a consumed "Re-run LoopOver review" panel click (#7626). `previewPollAttempt` needed + // an exemption only because the old guard sat above the recapture chain; it does not need one here. + if ( + await stableVerdictBackoffEngaged(env, { + repoFullName, + prNumber: pr.number, + headSha: pr.headSha, + deliveryId: webhook.deliveryId, + explicitlyRequested: webhook.forceAiReview === true, + }) + ) { + await runVisualCaptureObligation(env, { + installationId, + repoFullName, + pr, + repo, + mode, + repoFocusManifestForComment: await loadRepoFocusManifest(env, repoFullName), + getUnifiedFiles: getReviewFiles, + webhook, + }); + return undefined; + } // #ops-review-burst: the LAST completed gate-check conclusion recorded for this EXACT head SHA, captured // BEFORE this pass writes anything -- recordPublishedGateCheckSummary upserts keyed on // [repoFullName, headSha, name], so a row only exists here at all if THIS SAME COMMIT was already @@ -10831,26 +11062,6 @@ async function maybePublishPrPublicSurface( } return true; }; - // The PR's changed files are needed by the slop/manifest gates, the AI review + grounding + RAG, the secret - // scan, the check-run, and the unified comment. Resolve them AT MOST ONCE per review and share across the - // gate phase (inside the try) AND the publish phase (check-run + comment, after the try): memoize the first - // resolve so a repo that needs files anywhere pays a single resolve, and a gate-only repo that never needs - // them pays nothing. resolvePullRequestFilesForReview prefers the stored rows and, when they are empty at - // review time (the webhook beat detail-sync), fetches + persists them inline — so the FIRST review sees the - // real diff instead of "0 files / No diff provided" (FIX B). Fail-safe by construction. - let reviewFiles: Awaited> | null = - null; - const getReviewFiles = async (): Promise< - Awaited> - > => { - if (reviewFiles === null) - reviewFiles = await resolvePullRequestFilesForReview(env, { - installationId, - repoFullName, - pullNumber: pr.number, - }); - return reviewFiles; - }; // #6724 (review-burst): contentChanged defaults true so every existing caller (the three early-return call // sites below, none of which reach the comment/label publish steps) keeps recording pr_public_surface_published // exactly as before -- only the final "full" call site computes and passes the real value. @@ -13182,162 +13393,19 @@ async function maybePublishPrPublicSurface( // #4745: same reused slop band as the legacy commentArgs above -- the two panel builders never diverge. slopBand: slopBand ?? undefined, }); - // Visual before/after capture (visual-capture port). Fires ONLY when (1) the "screenshots" converged - // feature resolves active for this repo (resolveConvergedFeature — the global flag AND (a per-repo - // `features.screenshots` override OR the cutover allowlist default), #4616; reuses the manifest this - // pass already loaded above, no extra fetch) AND (2) the PR touches WEB-VISIBLE files (isVisualPath — - // frontend pages / public OG images; backend .ts/.md/.json PRs never qualify). Fully wrapped in - // try/catch + defaults to [] so a capture failure (render timeout, missing binding, GitHub hiccup) can - // NEVER sink the review — it just omits the "Visual preview" section. Flag-OFF (default) ⇒ this block is - // skipped entirely and the unified comment is byte-identical. - let beforeAfter: CaptureRoute[] = []; - // review.visual.bugAnalysis — resolved inside the try block below alongside the rest of reviewVisualConfig, - // but needed at the runVisualVisionForAdvisory call OUTSIDE that block's scope (that call is deliberately - // independent of the capture try/catch above, see its own comment) — mirrors beforeAfter's own - // declared-outer/assigned-inner pattern. false (unresolved, or resolution never ran) ⇒ byte-identical - // default-prompt behavior, same fail-safe direction as every other config-as-code read here. - let bugAnalysisEnabled = false; - let interactionPreviews: CaptureInteractionRoute[] = []; - const visualFiles = unifiedFiles - .map((file) => file.path) - .filter(isVisualPath); - // #auto-interaction-detection: only ever read by buildCapture when review.visual.autoDetectInteractions - // is on for this repo -- carries each changed file's own diff patch text (visualFiles above is bare - // paths), the same file.payload?.patch shape review-diff.ts/grounding-wire.ts already read elsewhere. - const changedCssFiles = unifiedFiles.map((file) => ({ - path: file.path, - patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, - })); - if (resolveConvergedFeature(env, repoFocusManifestForComment, "screenshots", repoFullName) && visualFiles.length > 0) { - try { - const token = await createInstallationToken(env, installationId); - // review.visual (#3609 / #3610): an explicit per-repo preview-URL template / route list. Absent config - // (the default for every repo today) ⇒ EMPTY_VISUAL_CONFIG ⇒ buildCapture's discovery/inference - // behavior is byte-identical to pre-#3609. - const reviewVisualConfig = await resolveVisualCaptureConfig(env, repoFullName); - bugAnalysisEnabled = reviewVisualConfig.bugAnalysis === true; - const captureTarget = { - repoFullName, - prNumber: pr.number, - ...(pr.headSha ? { headSha: pr.headSha } : {}), - ...(pr.headRef ? { headRef: pr.headRef } : {}), - previewFromChecks: true, - // Pins the actions_fallback dispatch (#4112) to a trusted ref -- see buildCapture. Absent (no - // stored default branch yet) ⇒ that dispatch just never fires, same as leaving it unconfigured. - ...(repo?.defaultBranch ? { defaultBranchRef: repo.defaultBranch } : {}), - // #9067: the actions_fallback dispatch is a real GitHub write and must obey dry_run/paused. - mode, - }; - // review.visual.enabled (#4083): a config-as-code override layered on top of the screenshotsAllowed - // env-var gate above, not a replacement for it. Unset/true ⇒ defer to that gate's decision (buildCapture - // runs exactly as before); explicit `false` (global default or per-repo, VPS-only) ⇒ force capture off - // for this repo -- a no-routes, non-pending sentinel result, so every line below behaves exactly as an - // ordinary "nothing found" capture would, with no separate code path to maintain. - const capture = - reviewVisualConfig.enabled === false - ? { routes: [], interactions: [], previewPending: false, renderFailed: false, previewUnobtainable: false } - : await buildCapture(env, token, captureTarget, visualFiles, githubRateLimitAdmissionKeyForInstallation(installationId), reviewVisualConfig, changedCssFiles); - beforeAfter = capture.routes; - interactionPreviews = capture.interactions; - // 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 - // webhook also refills it; this is the backstop when that event is missed/late). - // - // #9464: `capture.renderFailed` joins previewPending here as a SECOND blip class. captureShot - // swallows a renderer error per shot and returns a null PNG, so buildCapture used to return - // normally -- previewPending false, nothing thrown -- and neither the #9030 nor the #9207 guard - // fired. The maintenance pass then read "no evidence, no retry pending" and CLOSED the PR one-shot. - // A browserless outage now degrades to "we could not capture evidence, holding" instead. - // - // #9876: the `else` below is the half that was missing, and it is the half that froze three - // contributor PRs. Reaching it means this capture CONCLUDED -- no successful pair, but also nothing - // still pending and nothing broken -- which is a definite answer to the question the latch was - // deferring. Without it, a latch written by an earlier attempt outlived the chain that justified it: - // the durable per-head poll budget ends that chain by suppressing `previewPending`, so the final - // attempt takes neither branch above, `scheduleVisualCaptureRetry` is never called, and the - // budget-exhausted clear that #9462 put INSIDE it never runs. The release belongs to the capture - // outcome, where every conclusion passes, not to a scheduler that by definition stops being called. - const previewPollAttempt = webhook.previewPollAttempt ?? 0; - if (capture.previewPending || capture.renderFailed) { - await scheduleVisualCaptureRetry(env, { - webhook, - repoFullName, - pr, - installationId, - previewPollAttempt, - }); - } else if (pr.headSha) { - // #9881: this capture concluded, and it concluded because the repo has no preview pipeline at all - // (build state `absent` across every poll, budget now spent) -- not because a deploy was late or - // a renderer blipped. Record it so the screenshot-table gate degrades its CLOSE instead of - // destroying a PR over evidence no contributor action could produce. - if (capture.previewUnobtainable) { - await markPullRequestVisualCaptureUnobtainable(env, repoFullName, pr.number, pr.headSha).catch((error) => { - console.log( - JSON.stringify({ - event: "visual_capture_unobtainable_mark_failed", - repoFullName, - pull: pr.number, - message: errorMessage(error).slice(0, 200), - }), - ); - }); - } - // Conclusive: release any latch this head still carries. Best-effort and idempotent -- the common - // case is that there is no latch to clear, and a failed clear only leaves the age bound in - // visual-capture-retry-latch.ts to end the deferral instead of ending it now. - await clearPullRequestVisualCaptureRetryPending(env, repoFullName, pr.number, pr.headSha).catch((error) => { - console.log( - JSON.stringify({ - event: "visual_capture_retry_pending_clear_failed", - repoFullName, - pull: pr.number, - message: errorMessage(error).slice(0, 200), - }), - ); - }); - } - } catch (error) { - console.log( - JSON.stringify({ - event: "visual_capture_error", - repoFullName, - pull: pr.number, - message: errorMessage(error).slice(0, 200), - }), - ); - // #9030: a capture-pipeline ERROR (browserless down, timeout, a GitHub hiccup fetching a token) must - // not be silently indistinguishable from a legitimate "no visual routes found" result -- the - // screenshotTableGate's CLOSE action would otherwise fire on a false positive purely because an - // internal service blipped. Schedule the SAME bounded self-heal retry previewPending already uses. - await scheduleVisualCaptureRetry(env, { - webhook, - repoFullName, - pr, - installationId, - previewPollAttempt: webhook.previewPollAttempt ?? 0, - }); - } - } + // Visual before/after capture + its bounded recapture chain -- see runVisualCaptureObligation. Runs on + // every pass that reaches here, including a verdict-backed-off one (#10227), because the chain's budget + // is spent across passes rather than within one. + const { beforeAfter, interactionPreviews, bugAnalysisEnabled } = await runVisualCaptureObligation(env, { + installationId, + repoFullName, + pr, + repo, + mode, + repoFocusManifestForComment, + getUnifiedFiles: getReviewFiles, + webhook, + }); // AI-vision analysis of a confirmed visual regression (#4111 wiring) — see runVisualVisionForAdvisory's // own doc comment. Deliberately independent of the capture block above (its own try/catch there) so a // vision failure can never affect the "Visual preview" section that block already rendered. diff --git a/test/unit/check-verdict-backoff-chokepoint.test.ts b/test/unit/check-verdict-backoff-chokepoint.test.ts new file mode 100644 index 000000000..4ab4c01ca --- /dev/null +++ b/test/unit/check-verdict-backoff-chokepoint.test.ts @@ -0,0 +1,145 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; +import { enclosingFunction, findViolations, occurrences, topLevelFunctionRange } from "../../scripts/check-verdict-backoff-chokepoint"; + +// #10227: the checker is the thing standing between "the choke point holds" and "the choke point held once, +// in the PR that introduced it". Each case below is a real way the structure can rot, written as the mutation +// that produces it -- so the checker is proven to FAIL on drift, not just to pass on the tree as it stands. + +/** A miniature of processors.ts's shape: the two entry points, the choke point, and the derivation markers. */ +const COMPLIANT = ` +export async function reReviewStoredPullRequest(env: Env) { + const gate = await maybePublishPrPublicSurface(env, 1); + await maybeRunAgentMaintenance(env, { gate }); +} + +async function handlePullRequestWebhookEvent(env: Env) { + const gate = await maybePublishPrPublicSurface(env, 2); + await maybeRunAgentMaintenance(env, { gate }); +} + +async function maybePublishPrPublicSurface(env: Env, n: number) { + await runVisualCaptureObligation(env, {}); + if ( + await stableVerdictBackoffEngaged(env, { + repoFullName, + }) + ) { + return undefined; + } + const pendingGateResult = await createOrUpdatePendingGateCheckRun(env, n); + gateEvaluation = await withReviewPipelineSpan("x", {}, () => evaluate()); + return gateEvaluation; +} + +async function unrelatedHelper(env: Env) { + return env; +} +`; + +describe("check-verdict-backoff-chokepoint (#10227)", () => { + it("passes on a compliant shape", () => { + expect(findViolations(COMPLIANT)).toEqual([]); + }); + + it("passes on the real src/queue/processors.ts", () => { + expect(findViolations(readFileSync("src/queue/processors.ts", "utf8"))).toEqual([]); + }); + + it("fails when a SECOND call site appears -- that is a per-entry-point rule again", () => { + const drifted = COMPLIANT.replace( + "async function handlePullRequestWebhookEvent(env: Env) {\n", + "async function handlePullRequestWebhookEvent(env: Env) {\n if (await stableVerdictBackoffEngaged(env, {\n repoFullName,\n })) return;\n", + ); + expect(findViolations(drifted).map((violation) => violation.rule)).toContain("one call site"); + }); + + it("fails when the guard is deleted outright", () => { + const drifted = COMPLIANT.replace( + " if (\n await stableVerdictBackoffEngaged(env, {\n repoFullName,\n })\n ) {\n return undefined;\n }\n", + "", + ); + expect(findViolations(drifted)).toEqual([{ rule: "one call site", detail: "stableVerdictBackoffEngaged is called 0 time(s); the choke point is exactly one." }]); + }); + + it("fails when the guard drifts BELOW the derivation it is meant to skip", () => { + // The quiet rot: still one call site, still inside the choke point, still "backs off" by every behavioural + // assertion -- and saves nothing, because the gate check-run and the evaluation already ran. + const drifted = COMPLIANT.replace( + " const pendingGateResult = await createOrUpdatePendingGateCheckRun(env, n);\n gateEvaluation = await withReviewPipelineSpan(\"x\", {}, () => evaluate());\n", + "", + ).replace( + " return undefined;\n }\n", + " return undefined;\n }\n const pendingGateResult = await createOrUpdatePendingGateCheckRun(env, n);\n gateEvaluation = await withReviewPipelineSpan(\"x\", {}, () => evaluate());\n", + ); + expect(findViolations(drifted), "moving the derivation below the guard is the COMPLIANT direction").toEqual([]); + + const rotted = COMPLIANT.replace( + " await runVisualCaptureObligation(env, {});\n", + " await runVisualCaptureObligation(env, {});\n const pendingGateResult = await createOrUpdatePendingGateCheckRun(env, n);\n", + ); + expect(findViolations(rotted).map((violation) => violation.rule)).toContain("guard precedes the derivation"); + }); + + it("fails when a NEW publish-and-maintain entry point bypasses the choke point", () => { + // The case the issue asked for: a fresh caller derives a verdict its own way and runs the maintenance pass + // on it. Nothing else in the tree notices, and the backoff quietly stops covering the new path. + const drifted = `${COMPLIANT} +async function someNewSweep(env: Env) { + const gate = await deriveTheVerdictSomeOtherWay(env); + await maybeRunAgentMaintenance(env, { gate }); +} +`; + const violations = findViolations(drifted); + expect(violations.map((violation) => violation.rule)).toContain("entry points route through the choke point"); + expect(violations.map((violation) => violation.detail).join()).toContain("someNewSweep"); + }); + + it("fails when the choke-point function is renamed out from under the checker", () => { + const drifted = COMPLIANT.replaceAll("maybePublishPrPublicSurface", "publishPrSurface"); + expect(findViolations(drifted).map((violation) => violation.rule)).toContain("choke point exists"); + }); + + it("reports the guard as outside the choke point when it moves to a caller", () => { + const drifted = COMPLIANT.replace( + " if (\n await stableVerdictBackoffEngaged(env, {\n repoFullName,\n })\n ) {\n return undefined;\n }\n", + "", + ).replace( + "export async function reReviewStoredPullRequest(env: Env) {\n", + "export async function reReviewStoredPullRequest(env: Env) {\n if (await stableVerdictBackoffEngaged(env, {\n repoFullName,\n })) return;\n", + ); + expect(findViolations(drifted).map((violation) => violation.rule)).toContain("guard is inside the choke point"); + }); + + describe("helpers", () => { + it("occurrences finds every offset, and none when absent", () => { + expect(occurrences("abcabc", "bc")).toEqual([1, 4]); + expect(occurrences("abc", "zz")).toEqual([]); + }); + + it("topLevelFunctionRange bounds a function, and returns null for an unknown name", () => { + const range = topLevelFunctionRange(COMPLIANT, "maybePublishPrPublicSurface"); + expect(range).not.toBeNull(); + expect(COMPLIANT.slice(range!.start, range!.end)).toContain("stableVerdictBackoffEngaged"); + expect(COMPLIANT.slice(range!.start, range!.end)).not.toContain("unrelatedHelper"); + expect(topLevelFunctionRange(COMPLIANT, "noSuchFunction")).toBeNull(); + }); + + it("topLevelFunctionRange runs to end-of-file for the last function", () => { + const source = "\nasync function only(env: Env) {\n return env;\n}\n"; + expect(topLevelFunctionRange(source, "only")?.end).toBe(source.length); + }); + + it("enclosingFunction names the caller, and returns null before any declaration", () => { + const offset = COMPLIANT.indexOf("maybeRunAgentMaintenance(env, {"); + expect(enclosingFunction(COMPLIANT, offset)?.name).toBe("reReviewStoredPullRequest"); + expect(enclosingFunction(COMPLIANT, 1)).toBeNull(); + }); + + it("a maintenance call outside any function is skipped rather than misattributed", () => { + expect(findViolations("await maybeRunAgentMaintenance(env, {\n})\n")).toEqual([ + { rule: "one call site", detail: "stableVerdictBackoffEngaged is called 0 time(s); the choke point is exactly one." }, + ]); + }); + }); +}); diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 2504c0f2d..b0eeaa69a 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -3061,6 +3061,326 @@ describe("queue processors", () => { // PR still exists afterwards. }); + // #10227: the visual-capture block is now a named obligation (runVisualCaptureObligation) that a + // verdict-backed-off pass runs before it returns, so the three outcomes below are reachable from two call + // sites rather than one. They were never directly covered while the block was inline; each is a real branch + // of the capture pipeline, and each decides something the screenshot-table gate downstream acts on. + + it("screenshot-table gate: a PR with NO web-visible files never reaches the capture pipeline at all", async () => { + // The cheap half of the capture guard. Screenshots are ON for this repo, so only isVisualPath's verdict + // stands between a backend-only PR and a browserless render nobody asked for. + const seen90 = { commented: false }; + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture"); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true" }); + env.JOBS = { async send() {} } as unknown as Queue; + 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", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + 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" }); + // Backend-only diff: isVisualPath rejects every path here. + if (url.includes("/pulls/90/files")) return Response.json([{ filename: "src/queue/processors.ts", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/90/reviews")) return Response.json([]); + if (url.includes("/pulls/90/commits")) return Response.json([]); + if (url.endsWith("/pulls/90") && method === "PATCH") return Response.json({ number: 90, state: "closed" }); + if (url.endsWith("/pulls/90")) return Response.json({ number: 90, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis90" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis90/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis90/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis90/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/90/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/90/comments")) { if (method === "POST") seen90.commented = true; return Response.json([]); } + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 960 }, { status: 201 }); + if (url.includes("/check-runs/960") && method === "PATCH") return Response.json({ id: 960 }); + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-no-visual-files-90", + 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: 90, title: "Refactor the queue processor", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis90" }, labels: [{ name: "visual" }], body: "Backend only, no table here.", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + } finally { + buildCaptureSpy.mockRestore(); + } + + // Asserted first, and deliberately: without it the buildCapture assertion below passes for the wrong + // reason -- any setup that never reaches the comment publish at all would satisfy it vacuously. + expect(seen90.commented, "the pass must reach the comment publish, or the assertion below proves nothing").toBe(true); + expect(buildCaptureSpy, "a backend-only PR must not pay for a render").not.toHaveBeenCalled(); + }); + + it("screenshot-table gate (#4110/#4112): a successful capture marks the head satisfied and pins the fallback dispatch to the stored default branch", async () => { + // Two things at once because one capture proves both: a REAL before+after pair is the evidence the gate + // treats as equivalent to a hand-authored table (#4110), and the stored default branch is what the + // actions_fallback dispatch is pinned to (#4112) -- a capture on a repo whose default branch was never + // stored simply never fires that dispatch, so only a stored one exercises the threading. + const capturedTargets: unknown[] = []; + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockImplementation(async (_e, _t, target) => { capturedTargets.push(target); return { + routes: [{ path: "/app", beforeUrl: "https://worker.example/loopover/shot?key=before-90", afterUrl: "https://worker.example/loopover/shot?key=after-90" }], + interactions: [], + previewPending: false, + renderFailed: false, + previewUnobtainable: false, + }; }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true" }); + env.JOBS = { async send() {} } as unknown as Queue; + 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 upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" }, default_branch: "trunk" }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", autonomy: { close: "auto", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + 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/91/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/91/reviews")) return Response.json([]); + if (url.includes("/pulls/91/commits")) return Response.json([]); + if (url.endsWith("/pulls/91")) return Response.json({ number: 91, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis91" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis91/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis91/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis91/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/91/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/91/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 961 }, { status: 201 }); + if (url.includes("/check-runs/961") && method === "PATCH") return Response.json({ id: 961 }); + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-successful-capture-91", + 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: 91, title: "Update the app index route", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis91" }, labels: [{ name: "visual" }], body: "Changed the route layout, no table here.", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + } finally { + buildCaptureSpy.mockRestore(); + } + + expect((await getPullRequest(env, "JSONbored/gittensory", 91))?.visualCaptureSatisfiedSha).toBe("vis91"); + // #4112: captured here rather than off the spy, because mockRestore() in the finally above clears mock.calls. + expect(capturedTargets[0]).toMatchObject({ defaultBranchRef: "trunk" }); + }); + + it("screenshot-table gate (#9881): a capture that proves the repo has NO preview pipeline marks the head unobtainable", async () => { + // The third capture conclusion, and the one that keeps the gate from destroying a PR over evidence no + // contributor action could ever produce: nothing pending, nothing broken, and no preview deployment to + // render against at all. + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ + routes: [], + interactions: [], + previewPending: false, + renderFailed: false, + previewUnobtainable: true, + }); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true" }); + env.JOBS = { async send() {} } as unknown as Queue; + 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", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + 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/92/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (url.includes("/pulls/92/reviews")) return Response.json([]); + if (url.includes("/pulls/92/commits")) return Response.json([]); + if (url.endsWith("/pulls/92") && method === "PATCH") return Response.json({ number: 92, state: "closed" }); + if (url.endsWith("/pulls/92")) return Response.json({ number: 92, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis92" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis92/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis92/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis92/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/92/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/92/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 962 }, { status: 201 }); + if (url.includes("/check-runs/962") && method === "PATCH") return Response.json({ id: 962 }); + return new Response("not found", { status: 404 }); + }); + + try { + await processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-preview-unobtainable-92", + 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: 92, title: "Update the app index route", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis92" }, labels: [{ name: "visual" }], body: "Changed the route layout, no table here.", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }); + } finally { + buildCaptureSpy.mockRestore(); + } + + // Asserted against the COLUMN, not the record: the row->record mapper never surfaces this field, so + // `pr.visualCaptureUnobtainableSha` reads undefined at every reader and #9881's degrade has never fired. + // That is a separate bug (#10270); reading the record here would pin the bug instead of the write. + const unobtainable = await env.DB.prepare("select visual_capture_unobtainable_sha as sha from pull_requests where repo_full_name = ? and number = ?") + .bind("JSONbored/gittensory", 92) + .first<{ sha: string | null }>(); + expect(unobtainable?.sha).toBe("vis92"); + }); + + // Both mark writes are best-effort by design -- the capture already happened, and losing the bookkeeping + // must degrade the gate's evidence, never crash the pass that produced it. Mirrors the fail-safe pairs the + // clear path already has (#9462 / #9876). + + it("screenshot-table gate (#4110): a failed capture-satisfied write after a REAL capture is swallowed -- the pass still completes", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ + routes: [{ path: "/app", beforeUrl: "https://worker.example/loopover/shot?key=before-93", afterUrl: "https://worker.example/loopover/shot?key=after-93" }], + interactions: [], + previewPending: false, + renderFailed: false, + previewUnobtainable: false, + }); + const markSpy = vi.spyOn(repositoriesModule, "markPullRequestVisualCaptureSatisfied").mockRejectedValue(new Error("D1 write failed")); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true" }); + env.JOBS = { async send() {} } as unknown as Queue; + 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", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + 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/93/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\\n+const ok = true;" }]); + if (url.includes("/pulls/93/reviews")) return Response.json([]); + if (url.includes("/pulls/93/commits")) return Response.json([]); + if (url.endsWith("/pulls/93") && method === "PATCH") return Response.json({ number: 93, state: "closed" }); + if (url.endsWith("/pulls/93")) return Response.json({ number: 93, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis93" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis93/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis93/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis93/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/93/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/93/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 963 }, { status: 201 }); + if (url.includes("/check-runs/963") && method === "PATCH") return Response.json({ id: 963 }); + return new Response("not found", { status: 404 }); + }); + + try { + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-satisfied-write-failed-93", + 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: 93, title: "Update the app index route", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis93" }, labels: [{ name: "visual" }], body: "Changed the route layout, no table here.", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }), + ).resolves.not.toThrow(); + expect(markSpy).toHaveBeenCalled(); + } finally { + markSpy.mockRestore(); + buildCaptureSpy.mockRestore(); + } + + // The write lost, so the head is NOT recorded as satisfied and the gate falls back to requiring a table. + expect((await getPullRequest(env, "JSONbored/gittensory", 93))?.visualCaptureSatisfiedSha).toBeNull(); + }); + + it("screenshot-table gate (#9881): a failed capture-unobtainable write is swallowed -- the pass still completes", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ + routes: [], + interactions: [], + previewPending: false, + renderFailed: false, + previewUnobtainable: true, + }); + const markSpy = vi.spyOn(repositoriesModule, "markPullRequestVisualCaptureUnobtainable").mockRejectedValue(new Error("D1 write failed")); + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), LOOPOVER_REVIEW_SCREENSHOTS: "true" }); + env.JOBS = { async send() {} } as unknown as Queue; + 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", label: "auto" } }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true, whenLabels: ["visual"] }, reviewCheckMode: "required" } }, "repo_file"); + 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/94/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\\n+const ok = true;" }]); + if (url.includes("/pulls/94/reviews")) return Response.json([]); + if (url.includes("/pulls/94/commits")) return Response.json([]); + if (url.endsWith("/pulls/94") && method === "PATCH") return Response.json({ number: 94, state: "closed" }); + if (url.endsWith("/pulls/94")) return Response.json({ number: 94, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis94" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis94/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis94/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis94/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/94/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/94/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 964 }, { status: 201 }); + if (url.includes("/check-runs/964") && method === "PATCH") return Response.json({ id: 964 }); + return new Response("not found", { status: 404 }); + }); + + try { + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "screenshot-unobtainable-write-failed-94", + 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: 94, title: "Update the app index route", state: "open", user: { login: "visual-contributor" }, head: { sha: "vis94" }, labels: [{ name: "visual" }], body: "Changed the route layout, no table here.", mergeable_state: "clean", reviewDecision: "APPROVED" }, + }, + }), + ).resolves.not.toThrow(); + expect(markSpy).toHaveBeenCalled(); + } finally { + markSpy.mockRestore(); + buildCaptureSpy.mockRestore(); + } + + const unobtainable94 = await env.DB.prepare("select visual_capture_unobtainable_sha as sha from pull_requests where repo_full_name = ? and number = ?") + .bind("JSONbored/gittensory", 94) + .first<{ sha: string | null }>(); + expect(unobtainable94?.sha).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/verdict-stability-wire.test.ts b/test/unit/verdict-stability-wire.test.ts index f89630270..5d069cca3 100644 --- a/test/unit/verdict-stability-wire.test.ts +++ b/test/unit/verdict-stability-wire.test.ts @@ -1,16 +1,19 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { listAuditEventsByType, upsertInstallation, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertRepositorySettings } from "../../src/db/repositories"; -import { reReviewStoredPullRequest } from "../../src/queue/processors"; +import * as visualCaptureModule from "../../src/review/visual/capture"; +import { MAX_CAPTURE_RETRY_ATTEMPTS } from "../../src/review/visual/preview-poll-budget"; +import { processJob, reReviewStoredPullRequest } from "../../src/queue/processors"; import { verdictStabilityKey, writeVerdictStability } from "../../src/review/verdict-stability"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import { normalizeRegistryPayload } from "../../src/registry/normalize"; import { persistRegistrySnapshot } from "../../src/registry/sync"; import { asCloudEnv, createTestEnv } from "../helpers/d1"; import { generatePrivateKeyPem } from "../helpers/github-app-key"; -// #10222: the wiring #10204 shipped without a test. The backoff logic itself is covered by -// verdict-stability.test.ts; what was never covered is WHERE the guard sits, and #10204 put it after the -// readiness gate -- which fires onReachedReadiness and consumes the one-shot panel-retrigger marker. These -// tests pin the three properties that placement got wrong. +// #10222/#10227: the wiring #10204 shipped without a test. The backoff logic itself is covered by +// verdict-stability.test.ts; what was never covered is WHERE the guard sits. #10204 put it after the readiness +// gate, #10229 moved it one step earlier, and #10227 moved it OFF the pass entry entirely and onto the verdict- +// derivation choke point inside maybePublishPrPublicSurface. These tests pin what that placement must mean. const REPO = "JSONbored/gittensory"; const HEAD = "sha-settled"; @@ -41,17 +44,16 @@ async function seedSettledVerdict(env: ReturnType) { }); } -function stubGitHub(onReadiness: () => void) { +function stubGitHub() { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); if (url.includes("/access_tokens")) return Response.json({ token: "t" }); - // A files read means the pass got PAST the guard into the publish unit. Readiness itself deliberately - // runs before the guard (#10061), so it is not the probe. - if (url.includes("/files")) onReadiness(); if (url.endsWith("/pulls/77")) return Response.json({ number: 77, title: "settled", state: "open", user: { login: "contributor" }, head: { sha: HEAD }, labels: [], body: "Closes #1", mergeable_state: "clean" }); if (url.includes("/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "t", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); if (url.includes("/status")) return Response.json({ state: "success", statuses: [] }); if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + // The webhook path refreshes PR details before the publish unit, and those readers page over ARRAYS. + if (/\/(files|reviews|commits|comments|labels|issues)(\?|$)/.test(url)) return Response.json([]); return Response.json({}); }); } @@ -60,6 +62,15 @@ async function skippedEvents(env: ReturnType) { return listAuditEventsByType(env, "github_app.review_skipped_stable_verdict", "2000-01-01T00:00:00Z"); } +/** Did the pass DERIVE a verdict? #10227 moved the guard below readiness, the advisory build, miner detection + * and the type-label decision -- all of which a backed-off pass still owes and still runs -- so "did GitHub + * get read" is no longer a probe for anything. The maintenance pass is: it runs only on a DEFINED gate, and + * it is where persistDecisionRecord writes the ledger row `records_per_head` counts (#10184). So this event + * answers the only question the guard is about, and it is the metric the guard exists to move. */ +async function verdictDerivedEvents(env: ReturnType) { + return listAuditEventsByType(env, "agent.maintenance.disposition_considered", "2000-01-01T00:00:00Z"); +} + /** The same key processors.ts's pendingPrPanelRetriggerKey builds -- written directly because the marker * writer is module-private to processors.ts. */ const RETRIGGER_KEY = `pr-panel-retrigger-pending:${REPO.toLowerCase()}#77:${HEAD}`; @@ -73,11 +84,10 @@ describe("verdict-stability backoff wiring (#10222)", () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seed(env); await seedSettledVerdict(env); - let reviewRan = false; - stubGitHub(() => { reviewRan = true; }); + stubGitHub(); - expect(await reReviewStoredPullRequest(env, "d1", 123, REPO, 77)).toBe(false); - expect(reviewRan, "a backed-off pass must not reach the publish unit").toBe(false); + await reReviewStoredPullRequest(env, "d1", 123, REPO, 77); + expect(await verdictDerivedEvents(env), "a backed-off pass must not derive a verdict or write a decision record").toHaveLength(0); expect(await skippedEvents(env)).toHaveLength(1); }); @@ -86,40 +96,46 @@ describe("verdict-stability backoff wiring (#10222)", () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seed(env); await seedSettledVerdict(env); - let reviewRan = false; - stubGitHub(() => { reviewRan = true; }); + stubGitHub(); await reReviewStoredPullRequest(env, "d2", 123, REPO, 77, undefined, { force: true }); - expect(reviewRan, "a forced pass must proceed into the publish unit").toBe(true); + expect(await verdictDerivedEvents(env), "a forced pass must derive a fresh verdict").toHaveLength(1); expect(await skippedEvents(env)).toHaveLength(0); }); - it("REGRESSION: a visual-preview poll tick is NEVER backed off", async () => { + it("#10227: a visual-preview poll tick no longer needs an exemption -- it is backed off, and its chain still runs", async () => { + // INVERTED from #10229 on purpose, and this is the whole point of the redesign. previewPollAttempt was in + // the allowlist only because the old guard sat at pass ENTRY, above the recapture chain, so exempting the + // tick was the only way to keep the chain alive. A poll tick is machine-paced, not human-asked, so it has + // no business in an escape hatch whose meaning is "a human asked for this pass". With the guard on the + // derivation instead, the chain runs ABOVE it and the tick can be backed off like any other machine pass. + // The chain-still-completes half is what the two `screenshot-table gate (#10061)` cases in + // test/unit/queue-3.test.ts assert, end-to-end and against the real 5-attempt budget. const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seed(env); await seedSettledVerdict(env); - let reviewRan = false; - stubGitHub(() => { reviewRan = true; }); + stubGitHub(); await reReviewStoredPullRequest(env, "d3", 123, REPO, 77, 2); - expect(reviewRan).toBe(true); - expect(await skippedEvents(env)).toHaveLength(0); + expect(await verdictDerivedEvents(env)).toHaveLength(0); + expect(await skippedEvents(env)).toHaveLength(1); }); - it("REGRESSION: a backed-off pass does not eat the one-shot panel-retrigger marker (#7626)", async () => { - // The failure #10204's placement caused: readiness consumed the marker, THEN the guard returned, so the - // user's "Re-run LoopOver review" click vanished with nothing left to re-trigger it. + it("REGRESSION: a consumed panel-retrigger click gets the review it asked for, same pass (#7626)", async () => { + // The failure #10204's placement caused: readiness consumed the one-shot marker, THEN the guard returned, + // so the user's "Re-run LoopOver review" click vanished with nothing left to re-trigger it. #10229 fixed + // that by making the marker SURVIVE the backed-off pass -- correct, but it still cost the user a round + // trip. Consuming the marker threads forceAiReview, which is now exactly what the escape hatch keys on, so + // the click is honoured by the very pass that consumed it. Strictly stronger than "the marker survives". const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await seed(env); await seedSettledVerdict(env); await env.SELFHOST_TRANSIENT_CACHE?.set(RETRIGGER_KEY, "1", 3600); - stubGitHub(() => undefined); - - expect(await reReviewStoredPullRequest(env, "d4", 123, REPO, 77)).toBe(false); + stubGitHub(); - // The marker must still be there for a later pass to consume. - const stillPending = await env.SELFHOST_TRANSIENT_CACHE?.get(RETRIGGER_KEY); - expect(stillPending, "the retrigger marker must survive a backed-off pass").toBeTruthy(); + await reReviewStoredPullRequest(env, "d4", 123, REPO, 77); + expect(await verdictDerivedEvents(env), "the pass the user clicked for must actually run").toHaveLength(1); + expect(await skippedEvents(env)).toHaveLength(0); }); it("never backs off a PR with no head SHA -- there is no key to have settled under", async () => { @@ -130,7 +146,7 @@ describe("verdict-stability backoff wiring (#10222)", () => { number: 78, title: "no head", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", base: { ref: "main" }, labels: [], body: "Closes #1", created_at: "2026-07-31T09:00:00Z", } as never); - stubGitHub(() => undefined); + stubGitHub(); await reReviewStoredPullRequest(env, "d6", 123, REPO, 78); expect(await skippedEvents(env)).toHaveLength(0); @@ -144,11 +160,163 @@ describe("verdict-stability backoff wiring (#10222)", () => { repeats: 1, lastEvaluatedMs: Date.now(), }); - let reviewRan = false; - stubGitHub(() => { reviewRan = true; }); + stubGitHub(); await reReviewStoredPullRequest(env, "d5", 123, REPO, 77); - expect(reviewRan).toBe(true); + expect(await verdictDerivedEvents(env)).toHaveLength(1); + expect(await skippedEvents(env)).toHaveLength(0); + }); + + it("#10227: the WEBHOOK path is backed off too -- it was never guarded before", async () => { + // The whole reason #10227 exists. 293 of 344 repeat evaluations in a 24h Orb window carried + // `upstream_state_change` (deriveReevaluationReason's mapping for a RAW GitHub delivery), so the webhook + // path is the DOMINANT churn source -- and #10204/#10229 guarded only reReviewStoredPullRequest, because + // no pass-entry guard on this path could be made to spare #10061's recapture chain. Neither entry point + // opts in now: both reach a verdict only through maybePublishPrPublicSurface's choke point. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env); + await seedSettledVerdict(env); + stubGitHub(); + + await processJob(env, { + type: "github-webhook", + deliveryId: "wh1", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 77, title: "settled", state: "open", user: { login: "contributor" }, head: { sha: HEAD }, base: { ref: "main" }, labels: [], body: "Closes #1", mergeable_state: "clean" }, + }, + } as never); + + expect(await verdictDerivedEvents(env), "a settled webhook delivery must not re-derive the verdict").toHaveLength(0); + expect(await skippedEvents(env)).toHaveLength(1); + }); + + it("#10227: the same webhook delivery DOES derive a verdict while the answer is still moving", async () => { + // The other side of the branch above: backoff engages on a settled verdict, not on the webhook path as + // such. Without this, the test above would also pass if the guard simply broke the webhook path. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env); + stubGitHub(); + + await processJob(env, { + type: "github-webhook", + deliveryId: "wh2", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 77, title: "settled", state: "open", user: { login: "contributor" }, head: { sha: HEAD }, base: { ref: "main" }, labels: [], body: "Closes #1", mergeable_state: "clean" }, + }, + } as never); + + expect(await verdictDerivedEvents(env)).toHaveLength(1); expect(await skippedEvents(env)).toHaveLength(0); }); + + it("#10227: a backed-off pass still CHARGES the regate repair budget, deliberately", async () => { + // A reversal of one of #10229's three fixes, made on purpose and pinned here so it stays a decision. + // regatePullRequest charges its bounded repair budget on `onReachedReadiness`, whose documented rule is + // "count executions, not deferrals" -- a pass that got PAST prReadyForReview executed. Under #10229's + // pass-entry guard a backed-off pass was a deferral, so not charging was right. Under the choke point it + // is an execution: readiness passed, the type-label decision ran, the capture chain advanced, and only the + // verdict was skipped. Charging is also the anti-starvation direction -- a repair on a PR whose verdict is + // permanently settled (#8886's shape: a hold that can never clear) would otherwise reselect on every sweep + // tick forever without ever exhausting, which is the exact churn #10184 exists to stop. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await seed(env); + await seedSettledVerdict(env); + stubGitHub(); + + let reachedReadiness = false; + await reReviewStoredPullRequest(env, "d7", 123, REPO, 77, undefined, { onReachedReadiness: () => { reachedReadiness = true; } }); + expect(reachedReadiness, "a backed-off pass is an execution, not a deferral").toBe(true); + expect(await verdictDerivedEvents(env)).toHaveLength(0); + expect(await skippedEvents(env)).toHaveLength(1); + }); +}); + +// #10227: the property the redesign is FOR, stated once and generally. A bounded retry chain runs above the +// choke point, so an engaged backoff must not shorten it -- not by one attempt, and not for a chain nobody +// remembered to exempt. The #10061 chain is the instance that exists today and the one #10204's placement +// actually broke (5 attempts truncated to 3), so it is the instance measured here. Unlike the end-to-end cases +// in queue-3.test.ts, the backoff here is seeded ENGAGED before the first pass, so every pass in the loop is a +// backed-off one and the budget is spent entirely by passes that derived no verdict. + +/** A minimal etag-aware in-memory R2 stand-in for the durable capture-retry budget (mirrors queue-3.test.ts's + * and preview-poll-budget.test.ts's own). */ +function memoryBudgetStore(): R2Bucket { + const store = new Map(); + let etagSeq = 0; + return { + async get(key: string) { + const entry = store.get(key); + return entry === undefined ? null : ({ body: new Response(entry.value).body, httpEtag: entry.etag } as unknown as R2ObjectBody); + }, + async put(key: string, value: unknown, putOptions?: R2PutOptions) { + const onlyIf = putOptions?.onlyIf as R2Conditional | undefined; + const current = store.get(key); + if (onlyIf?.etagMatches !== undefined && current?.etag !== onlyIf.etagMatches) return null; + if (onlyIf?.etagDoesNotMatch === "*" && current !== undefined) return null; + etagSeq += 1; + const etag = `etag-${etagSeq}`; + store.set(key, { value: await new Response(value as BodyInit).text(), etag }); + return { key, etag } as unknown as R2Object; + }, + } as unknown as R2Bucket; +} + +describe("bounded retry chains under an engaged backoff (#10227)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("spends its FULL budget even when every single pass is backed off", async () => { + const capture = vi.spyOn(visualCaptureModule, "buildCapture").mockResolvedValue({ + routes: [{ path: "/app", afterUrl: "https://worker.example/loopover/shot?url=x", afterUrlMobile: "https://worker.example/loopover/shot?url=x" }], + interactions: [], + previewPending: false, + renderFailed: true, + previewUnobtainable: false, + }); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + REVIEW_AUDIT: memoryBudgetStore(), + }); + const sentJobs: Array> = []; + env.JOBS = { async send(message: Record) { sentJobs.push(message); } } as unknown as Queue; + await seed(env); + await upsertRepoFocusManifest(env, REPO, { settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", screenshotTableGate: { enabled: true }, reviewCheckMode: "required" } }, "repo_file"); + await seedSettledVerdict(env); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "t" }); + if (url.endsWith("/pulls/77")) return Response.json({ number: 77, title: "settled", state: "open", user: { login: "contributor" }, head: { sha: HEAD }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "t", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); + if (url.includes("/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + if (url.includes("/pulls/77/files")) return Response.json([{ filename: "apps/loopover-ui/src/routes/app.index.tsx", status: "modified", additions: 5, deletions: 1, changes: 6, patch: "@@\n+const ok = true;" }]); + if (/\/(files|reviews|commits|comments|labels|issues)(\?|$)/.test(url)) return Response.json([]); + return Response.json({}); + }); + + try { + for (let attempt = 0; attempt < MAX_CAPTURE_RETRY_ATTEMPTS + 2; attempt += 1) { + await reReviewStoredPullRequest(env, `chain-${attempt}`, 123, REPO, 77); + } + } finally { + capture.mockRestore(); + } + + expect(await skippedEvents(env), "every pass in the loop must have been backed off").toHaveLength(MAX_CAPTURE_RETRY_ATTEMPTS + 2); + expect(await verdictDerivedEvents(env), "and none of them derived a verdict").toHaveLength(0); + expect( + sentJobs.filter((job) => job.type === "recapture-preview"), + "the chain must still complete its full budget -- not the VERDICT_BACKOFF_MIN_REPEATS truncation #10204 caused", + ).toHaveLength(MAX_CAPTURE_RETRY_ATTEMPTS); + }); });