diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 404fe65c52..1dec5a7af4 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -4505,6 +4505,22 @@ export async function markPullRequestVisualCaptureRetryPending(env: Env, fullNam .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); } +/** #9462: clear the retry marker once the bounded recapture budget is EXHAUSTED. Without this the marker set by + * the previous attempt outlives the retries that justified it: `scheduleVisualCaptureRetry` early-returns at the + * budget, which only skips writing it AGAIN, so `visualCaptureRetryPendingSha === headSha` stayed true forever + * for that head. The only other clear is markPullRequestVisualCaptureSatisfied's, which requires a SUCCESSFUL + * capture -- exactly the thing that is not happening. A permanently-marked head silently disabled the + * screenshotTableGate's close (see the botCaptureRetryPending branch in processors.ts), so the deferral, meant + * to be temporary, became a permanent bypass. Not scoped to headSha on the write: the caller has already + * established which head it is finishing, and a row whose head moved on has a stale marker worth clearing too. */ +export async function clearPullRequestVisualCaptureRetryPending(env: Env, fullName: string, number: number, headSha: string): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ visualCaptureRetryPendingSha: null, updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.visualCaptureRetryPendingSha, headSha))); +} + /** Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix): record the (headSha, * evidenceFingerprint) checkpoint at which evaluateScreenshotTableGate's presence-mode check just satisfied * the gate for this PR (see that function's `presenceModeSatisfiedState` result field and staleness comment). diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 166891cce8..701e25c167 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -56,6 +56,7 @@ import { markPullRequestReviewsInvalidated, markPullRequestSurfacePublished, markPullRequestVisualCaptureSatisfied, + clearPullRequestVisualCaptureRetryPending, markPullRequestVisualCaptureRetryPending, markPullRequestScreenshotTablePresenceSatisfied, getLatestRegatedAt, @@ -2759,6 +2760,7 @@ function buildAgentMaintenancePlanInput(args: { requiredContexts: Set | null; blacklistEntry: ReturnType; screenshotTableMatch: AgentActionPlanInput["screenshotTableMatch"]; + screenshotTableEvidenceUnresolved: AgentActionPlanInput["screenshotTableEvidenceUnresolved"]; contributorCapMatch: AgentActionPlanInput["contributorCapMatch"]; linkedIssueHardRule: AgentActionPlanInput["linkedIssueHardRule"]; linkedIssueRulesConfig: Awaited>; @@ -2790,6 +2792,7 @@ function buildAgentMaintenancePlanInput(args: { requiredContexts, blacklistEntry, screenshotTableMatch, + screenshotTableEvidenceUnresolved, contributorCapMatch, linkedIssueHardRule, linkedIssueRulesConfig, @@ -2856,6 +2859,7 @@ function buildAgentMaintenancePlanInput(args: { : {}), copycatGateMode: settings.copycatGateMode, ...(screenshotTableMatch !== undefined ? { screenshotTableMatch } : {}), + ...(screenshotTableEvidenceUnresolved !== undefined ? { screenshotTableEvidenceUnresolved } : {}), ...(contributorCapMatch !== undefined ? { contributorCapMatch } : {}), // Always threaded (the DB layer populates it, default "over-contributor-limit"); the planner applies its // own fallback. @@ -3451,7 +3455,11 @@ async function runAgentMaintenancePlanAndExecute( screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && !botCaptureRetryPending ? { matched: true, reason: screenshotTableGateResult.reason } : undefined; - if (screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && botCaptureRetryPending) { + // #9462: deferring the CLOSE is only half a deferral -- on its own it let the plan fall through to a MERGE. + // Thread the unresolved state into the planner so it holds the PR instead of silently skipping the gate. + const screenshotTableEvidenceUnresolved = + screenshotTableGateResult.violated && screenshotTableGateConfig.action === "close" && botCaptureRetryPending; + if (screenshotTableEvidenceUnresolved) { await recordAuditEvent(env, { eventType: "github_app.screenshot_table_close_deferred_capture_retry", actor: null, @@ -3592,6 +3600,7 @@ async function runAgentMaintenancePlanAndExecute( requiredContexts, blacklistEntry, screenshotTableMatch, + screenshotTableEvidenceUnresolved, contributorCapMatch, linkedIssueHardRule, linkedIssueRulesConfig, @@ -9476,7 +9485,26 @@ async function scheduleVisualCaptureRetry( previewPollAttempt: number; }, ): Promise { - if (args.previewPollAttempt >= MAX_PREVIEW_POLL_ATTEMPTS) return; + if (args.previewPollAttempt >= MAX_PREVIEW_POLL_ATTEMPTS) { + // #9462: the budget is spent, so the marker the PREVIOUS attempt wrote must be cleared here. Returning + // without clearing only skips re-writing it -- it left `visualCaptureRetryPendingSha === headSha` standing + // forever (the sole other clear needs a successful capture, which by definition never came), which silently + // and permanently disabled the screenshotTableGate's close for that head. Best-effort, matching the mark + // write below: a failed clear only means the gate stays deferred until the head moves, never a crash. + if (args.pr.headSha) { + await clearPullRequestVisualCaptureRetryPending(env, args.repoFullName, args.pr.number, args.pr.headSha).catch((error) => { + console.log( + JSON.stringify({ + event: "visual_capture_retry_pending_clear_failed", + repoFullName: args.repoFullName, + pull: args.pr.number, + message: errorMessage(error).slice(0, 200), + }), + ); + }); + } + return; + } if (args.pr.headSha) { await markPullRequestVisualCaptureRetryPending(env, args.repoFullName, args.pr.number, args.pr.headSha).catch((error) => { console.log( diff --git a/src/selfhost/metrics.ts b/src/selfhost/metrics.ts index ce148888fd..4720f4d274 100644 --- a/src/selfhost/metrics.ts +++ b/src/selfhost/metrics.ts @@ -129,6 +129,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [ ["loopover_ai_review_non_cacheable_total", { help: "AI reviews skipped by cacheability rules.", type: "counter" }], ["loopover_ai_review_force_bypass_total", { help: "AI review cache force-bypass events.", type: "counter" }], ["loopover_ai_review_inconclusive_total", { help: "AI review inconclusive outcomes.", type: "counter" }], + ["loopover_ai_review_unpublishable_blocker_total", { help: "AI reviews where a reviewer named a real blocker whose title could not be published, so the verdict held instead of passing (#9460).", type: "counter" }], ["loopover_ai_review_onmerge_clamped_total", { help: "AI review on-merge mode clamp events.", type: "counter" }], ["loopover_ai_review_model_fallback_total", { help: "AI review model fallback attempts by primary and fallback model.", type: "counter" }], ["loopover_regate_ai_skipped_current_total", { help: "Regate requests skipped because AI state is current.", type: "counter" }], diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index eb7b393418..c435277d08 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -2576,6 +2576,37 @@ function synthesizeDefect( return { title, detail: title, confidence: source.confidence }; } +/** + * #9460: resolve "these reviewers named a blocker" into a defect — or, when that blocker cannot be published, + * into a HOLD rather than a silent pass. `synthesizeDefect` returns null for two situations that must NOT share + * an outcome: nobody named a real blocker (a genuine clean pass), and a real blocker whose title `toPublicSafe` + * refuses to publish because it carries ordinary review vocabulary (`score`, `ranking`, `reward`, `cohort`, … + * see src/queue-intelligence.ts). Collapsing both to `{defect: null, inconclusive: false}` let the second case + * auto-MERGE a change a reviewer had explicitly blocked — on this deployment's own live strategy, since + * AI_PROVIDER=claude-code,ollama resolves to `single` (resolveAiReviewerPlan, src/selfhost/ai.ts). + * + * Resolves to `inconclusive` rather than `split` deliberately: the situation genuinely IS "no usable public + * verdict for this head", which is `ai_review_inconclusive`'s own semantics and user-facing copy, and it routes + * to a neutral gate → human hold (src/queue/ai-review-orchestration.ts). `ai_review_split`'s copy instead + * asserts that one reviewer flagged a defect and another did not — a disagreement that never happened here, and + * which would be actively misleading in single-reviewer mode. `consensus` already fails closed for this case via + * its own `split` arm below; these two strategies did not. + */ +function defectOrHold(flagged: readonly ModelReview[]): { + defect: AiConsensusDefect | null; + split: boolean; + inconclusive: boolean; +} { + // A blockers array holding only blank strings is NOT a flag (realBlockersOf filters them) — that stays a + // clean pass, exactly as before. Only a genuinely-named-but-unpublishable blocker becomes a hold. + if (flagged.every((review) => realBlockersOf(review).length === 0)) + return { defect: null, split: false, inconclusive: false }; + const defect = synthesizeDefect(flagged); + if (defect) return { defect, split: false, inconclusive: false }; + incr("loopover_ai_review_unpublishable_blocker_total"); + return { defect: null, split: false, inconclusive: true }; +} + /** Combine the independent reviewer opinions into ONE gate decision per the configured strategy (#dual-ai-combiner). * `reviews` carries one slot per reviewer; a slot is `null` when that reviewer errored or returned unparseable * output. Returns the gate-relevant trio: a `defect` (→ blocker), `split` (reviewers disagree → HOLD), and @@ -2600,11 +2631,7 @@ export function combineReviews( // missing review can't certify the change → hold. const r = present[0]; if (!r) return { defect: null, split: false, inconclusive: true }; - return { - defect: r.blockers.length > 0 ? synthesizeDefect([r]) : null, - split: false, - inconclusive: false, - }; + return defectOrHold([r]); } if (opts.strategy === "synthesis") { @@ -2615,20 +2642,13 @@ export function combineReviews( if (missing > 0) return { defect: null, split: false, inconclusive: true }; const all = present.length > 0 && flagged.length === present.length; - return { - defect: all ? synthesizeDefect(present) : null, - split: false, - inconclusive: false, - }; + return all + ? defectOrHold(present) + : { defect: null, split: false, inconclusive: false }; } // `either`: any present reviewer's blocker blocks. With no present blocker but a missing opinion we cannot // certify the change is clean → hold (fail-closed). - if (flagged.length > 0) - return { - defect: synthesizeDefect(flagged), - split: false, - inconclusive: false, - }; + if (flagged.length > 0) return defectOrHold(flagged); return { defect: null, split: false, inconclusive: missing > 0 }; } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index aecf2875e9..839269ca91 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -466,6 +466,10 @@ export type AgentActionPlanInput = { // populates this field when the gate's configured `action` is `"close"` — an `"advisory"` violation (#4535) // never reaches the planner at all, by construction (see ScreenshotTableGateAction). screenshotTableMatch?: { matched: boolean; reason: string | null } | undefined; + /** #9462: the screenshot-table gate is violated in `close` mode but a bounded capture retry is still pending + * for this head, so the evidence is UNRESOLVED rather than absent. Holds the PR (no close, and critically no + * merge) until the retry settles. */ + screenshotTableEvidenceUnresolved?: boolean | undefined; pr: { mergeableState?: string | null | undefined; reviewDecision?: string | null | undefined; @@ -975,6 +979,17 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // already IS the full contract, so a separate enforcement label would be redundant noise on a PR that's // about to be closed anyway. const screenshotTableContributor = !input.authorIsOwner && !input.authorIsAdmin && !input.authorIsAutomationBot; + // #9462: the gate is violated in close mode but its evidence cannot be evaluated right now (the bot's own + // capture pipeline has a bounded retry pending for this head). Previously this left screenshotTableMatch + // undefined and the plan simply fell through to ordinary merit/CI evaluation -- so a green PR planned a + // MERGE, with no finding, hold or blocker representing "screenshot evidence unresolved". That is a + // disposition inversion: a PR the gate would have CLOSED could instead merge. Hold instead, mirroring + // pre-merge-checks' PRE_MERGE_CHECK_UNRESOLVED_CODE, whose doc is explicit that an unresolvable enforced + // check must never silently skip a hard requirement (an auto-merge bypass). Returning no actions is the hold: + // neither the close (the evidence may yet arrive) nor the merge (it may yet not). + if (input.screenshotTableEvidenceUnresolved === true && screenshotTableContributor) { + return actions; + } if (input.screenshotTableMatch?.matched === true && screenshotTableContributor) { if (acting("close")) { const reason = input.screenshotTableMatch.reason ?? "missing a before/after screenshot table"; diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 5131123299..aa5c8a3f97 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -2578,6 +2578,39 @@ describe("screenshot-table gate short-circuit (#2006)", () => { expect(classes(planAgentMaintenanceActions(missingTable({ screenshotTableMatch: { matched: false, reason: null } })))).not.toContain("close"); }); + // #9462 regression: while the bot's own capture pipeline has a bounded retry pending for this head, the gate's + // evidence is UNRESOLVED, not absent. Deferring only the close left the plan falling through to ordinary + // merit/CI evaluation, so a green PR planned a MERGE and the gate was silently bypassed — a disposition + // inversion (the PR the gate would have CLOSED merged instead). It must hold: no close AND no merge. + const evidenceUnresolved = (extra: Partial = {}) => + missingTable({ screenshotTableMatch: undefined, screenshotTableEvidenceUnresolved: true, ...extra }); + + it("#9462 holds — plans NEITHER a merge NOR a close while capture evidence is unresolved", () => { + const plan = planAgentMaintenanceActions(evidenceUnresolved()); + expect(plan).toEqual([]); + expect(classes(plan)).not.toContain("merge"); + expect(classes(plan)).not.toContain("close"); + }); + + it("#9462 holds even with a fully green gate and merge autonomy (the exact auto-merge bypass)", () => { + const plan = planAgentMaintenanceActions( + evidenceUnresolved({ conclusion: "success", ciState: "passed", autonomy: { close: "auto", approve: "auto", merge: "auto" } }), + ); + expect(classes(plan)).not.toContain("merge"); + }); + + it("#9462 does NOT hold for the owner, an admin, or an automation bot (same standing rule as the close)", () => { + // The unresolved-evidence hold is a CONTRIBUTOR protection; these authors fall through to normal disposition. + expect(planAgentMaintenanceActions(evidenceUnresolved({ authorIsOwner: true }))).not.toEqual([]); + expect(planAgentMaintenanceActions(evidenceUnresolved({ authorIsAdmin: true }))).not.toEqual([]); + expect(planAgentMaintenanceActions(evidenceUnresolved({ authorIsAutomationBot: true }))).not.toEqual([]); + }); + + it("#9462 an explicit false (or absent) unresolved flag leaves the close path untouched", () => { + expect(classes(planAgentMaintenanceActions(missingTable({ screenshotTableEvidenceUnresolved: false })))).toEqual(["close"]); + expect(classes(planAgentMaintenanceActions(missingTable()))).toEqual(["close"]); + }); + it("plans nothing when `close` autonomy is not acting", () => { expect(planAgentMaintenanceActions(missingTable({ autonomy: {} }))).toEqual([]); expect(classes(planAgentMaintenanceActions(missingTable({ autonomy: { close: "auto" } })))).toEqual(["close"]); diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 1f65e1a121..e3cb98da7f 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -2461,18 +2461,94 @@ describe("pure helpers", () => { ).toEqual({ defect: null, split: false, inconclusive: false }); }); - it("synthesized defect drops a blocker whose only finding is blank or unsafe (fail-safe, same discipline as consensus)", () => { + it("a blank-only blocker is not a flag at all — still a clean pass (no hold)", () => { + // whitespace-only → realBlockersOf filters it → nobody actually flagged anything. expect(combineReviews([r([" "])], { strategy: "single" })).toEqual({ defect: null, split: false, inconclusive: false, - }); // whitespace-only → no primary + }); + expect( + combineReviews([r([" "]), clean], { + strategy: "synthesis", + onMerge: "either", + }), + ).toEqual({ defect: null, split: false, inconclusive: false }); + }); + + // #9460 regression: a REAL blocker whose title toPublicSafe refuses to publish (ordinary review vocabulary — + // reward/payout/score/ranking/cohort) used to collapse to {defect: null, inconclusive: false} — indistinguishable + // from "the reviewer found nothing" — so the PR auto-MERGED with the defect unreported. It must HOLD instead. + // `single` is this deployment's live strategy (AI_PROVIDER=claude-code,ollama → resolveAiReviewerPlan). + it("#9460 single: an unpublishable blocker HOLDS instead of silently passing", () => { + expect( + combineReviews([r(["Boost your reward payout"])], { + strategy: "single", + }), + ).toEqual({ defect: null, split: false, inconclusive: true }); + }); + + it("#9460 single: a publishable blocker still yields a defect (no regression)", () => { + const out = combineReviews([blocked], { strategy: "single" }); + expect(out.defect).not.toBeNull(); + expect(out.inconclusive).toBe(false); + }); + + // Ordinary review vocabulary that the public-safe filter rejects. `ranking`/`cohort`/`reward` are plain + // substring matches (FORBIDDEN_PUBLIC_COMMENT_WORDS); a bare `score` is matched by BARE_SCORE_TERM_PATTERN's + // word boundary. Note synthesizeDefect calls toPublicSafe with NO options, so the gate-bearing title is + // filtered even on a repo in LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS (unlike composeAdvisoryNotes, which + // passes allowBareScoreTerm) — i.e. the allowlist does not rescue this path. + it.each([ + ["score is NaN when the input list is empty"], + ["the ranking comparator drops the tie-break"], + ["cohort assignment leaks across tenants"], + ["reward calculation overflows on a large diff"], + ])( + "#9460 single: %s is a real blocker that must hold, not pass", + (blockerTitle) => { + expect( + combineReviews([r([blockerTitle])], { strategy: "single" }), + ).toEqual({ defect: null, split: false, inconclusive: true }); + }, + ); + + // The word-boundary shape means a camelCase identifier is NOT filtered — "computeScore" has no boundary + // before "Score". Pinned so a future widening of the pattern is a deliberate, visible decision. + it("#9460 single: a camelCase identifier containing 'Score' is publishable and still yields a defect", () => { + const out = combineReviews( + [r(["computeScore divides by zero when items is empty"])], + { strategy: "single" }, + ); + expect(out.defect).not.toBeNull(); + expect(out.inconclusive).toBe(false); + }); + + it("#9460 synthesis/either: an unpublishable blocker HOLDS instead of silently passing", () => { expect( combineReviews([r(["Boost your reward payout"]), clean], { strategy: "synthesis", onMerge: "either", }), - ).toEqual({ defect: null, split: false, inconclusive: false }); // unsafe title dropped + ).toEqual({ defect: null, split: false, inconclusive: true }); + }); + + it("#9460 synthesis/both: an unpublishable blocker flagged by EVERY reviewer HOLDS", () => { + expect( + combineReviews( + [r(["Boost your reward payout"]), r(["Reward farming risk here"])], + { strategy: "synthesis", onMerge: "both" }, + ), + ).toEqual({ defect: null, split: false, inconclusive: true }); + }); + + it("#9460 synthesis/both: a partial flag still passes without a hold (unchanged)", () => { + expect( + combineReviews([r(["Boost your reward payout"]), clean], { + strategy: "synthesis", + onMerge: "both", + }), + ).toEqual({ defect: null, split: false, inconclusive: false }); }); it("a consensus defect carries the MIN of the two reviewers' confidences (#8)", () => { diff --git a/test/unit/queue-3.test.ts b/test/unit/queue-3.test.ts index 8cd0a031f6..b17e54f0da 100644 --- a/test/unit/queue-3.test.ts +++ b/test/unit/queue-3.test.ts @@ -2271,6 +2271,74 @@ describe("queue processors", () => { expect(sentJobs.some((job) => job.type === "recapture-preview")).toBe(false); }); + // #9462 regression: the sibling test above starts from a PR that never had a marker written, so it exercises + // the budget-exhausted early return but NOT the defect. The failing sequence is marker-SET-then-EXHAUSTED: + // the early return only skipped writing the marker AGAIN, so the previous attempt's marker stood forever -- + // the sole other clear (markPullRequestVisualCaptureSatisfied) needs a SUCCESSFUL capture, which by + // definition never arrives when the pipeline keeps failing. A permanently marked head silently disabled the + // screenshot gate's close for that head, turning a deliberately temporary deferral into a permanent bypass. + it("screenshot-table gate (#9462): a marker written by an earlier attempt is CLEARED once the budget is exhausted", async () => { + const buildCaptureSpy = vi.spyOn(visualCaptureModule, "buildCapture").mockRejectedValue(new Error("browserless connection refused")); + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + LOOPOVER_REVIEW_SCREENSHOTS: "true", + }); + const sentJobs: Array> = []; + env.JOBS = { async send(message: Record) { sentJobs.push(message); } } 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"); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 62, + title: "Update the app index route", + state: "open", + user: { login: "visual-contributor" }, + head: { sha: "vis62" }, + labels: [{ name: "visual" }], + body: "Changed the route layout, no table here.", + }); + 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/62/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/62/reviews")) return Response.json([]); + if (url.includes("/pulls/62/commits")) return Response.json([]); + if (url.endsWith("/pulls/62") && method === "PATCH") return Response.json({ number: 62, state: "closed" }); + if (url.endsWith("/pulls/62")) return Response.json({ number: 62, state: "open", user: { login: "visual-contributor" }, head: { sha: "vis62" }, mergeable_state: "clean" }); + if (url.includes("/commits/vis62/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/commits/vis62/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/vis62/check-suites")) return Response.json({ check_suites: [] }); + if (url.includes("/issues/62/labels") && method === "GET") return Response.json([]); + if (url.includes("/issues/62/comments")) return Response.json([]); + if (url.endsWith("/labels") && method === "POST") return Response.json([]); + if (url.endsWith("/check-runs") && method === "POST") return Response.json({ id: 902 }, { status: 201 }); + if (url.includes("/check-runs/902") && method === "PATCH") return Response.json({ id: 902 }); + return new Response("not found", { status: 404 }); + }); + + try { + // Attempt 0 errors -> the marker IS written for this head and a retry is scheduled. + await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-marker-clear-first", repoFullName: "JSONbored/gittensory", prNumber: 62, installationId: 123, attempt: 0 }); + expect((await getPullRequest(env, "JSONbored/gittensory", 62))?.visualCaptureRetryPendingSha).toBe("vis62"); + + // The final attempt exhausts the budget while the pipeline is still failing. + await processJob(env, { type: "recapture-preview", deliveryId: "screenshot-marker-clear-exhausted", repoFullName: "JSONbored/gittensory", prNumber: 62, installationId: 123, attempt: MAX_PREVIEW_POLL_ATTEMPTS }); + } finally { + buildCaptureSpy.mockRestore(); + } + + // The stale marker must not outlive the retries that justified it, or the gate stays disabled for this head. + expect((await getPullRequest(env, "JSONbored/gittensory", 62))?.visualCaptureRetryPendingSha).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