fix(gate): stop two subsystem failures from becoming wrong auto-verdicts (#9460, #9462)

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<void> {
+  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).

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<string> | null;
   blacklistEntry: ReturnType<typeof findBlacklistEntry>;
   screenshotTableMatch: AgentActionPlanInput["screenshotTableMatch"];
+  screenshotTableEvidenceUnresolved: AgentActionPlanInput["screenshotTableEvidenceUnresolved"];
   contributorCapMatch: AgentActionPlanInput["contributorCapMatch"];
   linkedIssueHardRule: AgentActionPlanInput["linkedIssueHardRule"];
   linkedIssueRulesConfig: Awaited<ReturnType<typeof loadLinkedIssueHardRules>>;
@@ -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<void> {
-  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(

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" }],

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 };
   }
 

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";

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<AgentActionPlanInput> = {}) =>
+    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"]);