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

* 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.

* 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.

* 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.

Author: JSONbored

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