From ec76e5a5ba388295a1e39590b0487cef08e1e317 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 14:22:31 -0700 Subject: [PATCH 1/3] fix(gate): stop two subsystem failures from becoming wrong auto-verdicts (#9460, #9462) An unpublishable AI blocker and a stuck visual-capture marker each let a subsystem failure resolve as a verdict rather than as "cannot determine". pre-merge-checks already models the correct shape (an unresolvable enforced check -> NEUTRAL -> hold, never a silent auto-merge bypass); neither of these followed it. combineReviews (#9460): synthesizeDefect returns null both when nobody flagged anything and when a real blocker's title is unpublishable, and single/synthesis collapsed both to a clean pass -- so a blocker phrased with ordinary review vocabulary (reward/ranking/cohort, or a bare score term) auto-MERGED the change it blocked. single is this deployment's live strategy. Both strategies now hold; consensus already failed closed and is untouched. 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 -- the allowlist never rescued this path. Visual capture (#9462): scheduleVisualCaptureRetry's budget-exhausted early return only skipped re-writing visualCaptureRetryPendingSha, leaving the previous attempt's marker standing forever (the sole other clear needs a successful capture, which by definition never comes). A permanently-marked head silently disabled the screenshot gate's close -- a permanent bypass out of what was designed as a temporary deferral. The marker is now cleared on exhaustion, and the unresolved state is threaded into the planner so the deferral holds the PR rather than falling through to a merge. --- src/db/repositories.ts | 16 +++++++ src/queue/processors.ts | 32 ++++++++++++- src/services/ai-review.ts | 52 ++++++++++++++------- src/settings/agent-actions.ts | 15 ++++++ test/unit/agent-actions.test.ts | 33 +++++++++++++ test/unit/ai-review.test.ts | 82 +++++++++++++++++++++++++++++++-- 6 files changed, 209 insertions(+), 21 deletions(-) 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/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)", () => { From ff1bb153f1e6aa59c129ead3f5ed6998c09f3df9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 16:58:59 -0700 Subject: [PATCH 2/3] fix(metrics): register the unpublishable-blocker counter in DEFAULT_METRIC_META (#9460) The drift guard in test/unit/selfhost-metrics.test.ts requires every literal metric name emitted from src/ to carry a registered meta entry; the counter added alongside the combineReviews hold had none, failing the backend coverage suite. --- src/selfhost/metrics.ts | 1 + 1 file changed, 1 insertion(+) 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" }], From 88486aae6b0f4f68b8d863f45cb4ca8e364dc5b0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 17:19:36 -0700 Subject: [PATCH 3/3] test(gate): cover the capture-retry marker clear on budget exhaustion (#9462) The existing #9030 exhaustion test starts from a PR that never had a marker written, so it exercises the early return but not the defect. This drives the real failing sequence -- attempt 0 errors and writes the marker, then the final attempt exhausts the budget with the pipeline still failing -- and asserts the marker does not outlive the retries that justified it. Verified to fail against the unfixed code (expected 'vis62' to be null), so it pins the fix rather than merely passing alongside it. Closes the codecov/patch gap flagged on the PR. --- test/unit/queue-3.test.ts | 68 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) 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