fix(agent): three absorbing states get an escape — merge blocks, approval rows, and confidence parking

migrations/0185_merge_block_expiry.sql

@@ -0,0 +1,8 @@
+-- #9012: an infra-scoped terminal merge failure (401 installation-token rejection, exhausted secondary
+-- rate-limit window) is a property of the installation, not of the commit -- it fails every in-flight merge in
+-- the fleet at once and heals for all of them at once. Before this column, every terminal class wrote a
+-- head-scoped block whose ONLY escape was the contributor pushing a new commit, so one token rotation
+-- permanently stranded every green, approved PR it caught, invisibly. An infra block now carries an expiry and
+-- is re-probed once the window passes; a commit-scoped block (real conflict, repo merge policy) leaves this
+-- NULL and keeps the original until-a-new-commit semantics.
+ALTER TABLE pull_requests ADD COLUMN merge_blocked_until TEXT;

migrations/0186_low_confidence_hold_counter.sql

@@ -0,0 +1,9 @@
+-- #9034: confidence-parking used to be an unbounded absorbing state. A blocker BELOW the close-confidence floor
+-- still blocks, but under the default `hold_for_review` disposition it converts a one-shot close into an OPEN
+-- hold -- with no cap on how many times the same PR may re-enter that hold. A PR shaped to keep drawing
+-- low-confidence blockers therefore survives indefinitely, consuming the manual queue on every roll, and (with
+-- the re-roll surface) can be walked toward a clean merge from there. These columns count the holds so the
+-- Nth one closes instead. The head SHA makes the count per-ROLL rather than per-pass: a re-gate of the same
+-- commit is the same hold, while each new commit that draws a fresh low-confidence blocker is a new one.
+ALTER TABLE pull_requests ADD COLUMN low_confidence_hold_count INTEGER NOT NULL DEFAULT 0;
+ALTER TABLE pull_requests ADD COLUMN low_confidence_hold_head_sha TEXT;

src/db/repositories.ts

@@ -525,6 +525,14 @@ export async function upsertPullRequestFromGitHub(
         lastSeenOpenAt,
         payloadJson: jsonString(payload),
         githubUpdatedAt: resolvedGithubUpdatedAt,
+        // #9012: a new commit starts the failed-merge budget fresh. This is what mergeAttemptCount's own schema
+        // and function docs have always PROMISED ("a new commit's attempts start fresh once the row's head
+        // advances") -- bumpPullRequestMergeAttempt scopes the *increment* to the head SHA, which stops a stale
+        // head from bumping the counter but never resets the counter itself, so the value survived every push.
+        // The consequence was cumulative: once one head exhausted MERGE_RETRY_CAP, every later head was
+        // one-strike-terminal on the first transient failure it met. Only reset on a REAL head change, so an
+        // ordinary resync (same head) cannot hand a genuinely failing merge an unlimited retry budget.
+        ...(headShaChanged ? { mergeAttemptCount: 0 } : {}),
         updatedAt: syncedAt,
       },
     });
@@ -4246,15 +4254,67 @@ export async function bumpPullRequestDraftConversionCount(env: Env, fullName: st
 
 /** Mark a PR terminally merge-blocked for its current head SHA: the planner skips the `merge` disposition while
  *  merge_blocked_sha == headSha. Scoped to headSha so a later commit (a pushed fix) auto-clears the block (the
- *  guard compares it to the live head). Records the human-readable terminal reason. */
-export async function markPullRequestMergeBlocked(env: Env, fullName: string, number: number, headSha: string, reason: string): Promise<void> {
+ *  guard compares it to the live head). Records the human-readable terminal reason.
+ *
+ *  `expiresAt` (#9012) additionally lapses the block at an instant, for an INFRA-scoped cause — a rejected
+ *  installation token or an exhausted secondary-rate-limit window, which is a property of the installation and
+ *  not of the code, and which therefore cannot be cleared by the only escape a commit-scoped block offers. It
+ *  also zeroes merge_attempt_count so the post-expiry re-probe starts from a full retry budget rather than
+ *  being one-strike-terminal on the next hiccup. Omitted (undefined) = commit-scoped: unchanged behavior. */
+export async function markPullRequestMergeBlocked(
+  env: Env,
+  fullName: string,
+  number: number,
+  headSha: string,
+  reason: string,
+  expiresAt?: string | undefined,
+): Promise<void> {
   const db = getDb(env.DB);
   await db
     .update(pullRequests)
-    .set({ mergeBlockedSha: headSha, mergeBlockedReason: reason.slice(0, 280), updatedAt: nowIso() })
+    .set({
+      mergeBlockedSha: headSha,
+      mergeBlockedReason: reason.slice(0, 280),
+      mergeBlockedUntil: expiresAt ?? null,
+      ...(expiresAt !== undefined ? { mergeAttemptCount: 0 } : {}),
+      updatedAt: nowIso(),
+    })
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
 }
 
+/** #9034: count this PR into the AI-review low-confidence hold tally and return the new total.
+ *
+ *  Idempotent per HEAD, which is what makes the number mean "rolls", not "passes": the re-gate sweep, a CI
+ *  event, and a label webhook can all re-evaluate the same commit within minutes, and every one of them would
+ *  otherwise bump the counter and burn the cap against a single genuine hold. Only a head the counter has not
+ *  already seen advances it.
+ *
+ *  Deliberately NOT reset when the head changes (contrast bumpPullRequestMergeAttempt, whose whole point is
+ *  that a new commit earns a fresh budget): a PR that keeps drawing low-confidence blockers across successive
+ *  pushes is exactly the shape being capped, so a push must not buy another life. Mirrors
+ *  bumpPullRequestDraftConversionCount's same reasoning for the same reason.
+ *
+ *  Returns the pre-existing total unchanged when the head was already counted, so callers can compare against
+ *  the cap on every pass without needing to know whether this particular pass advanced anything. */
+export async function bumpPullRequestLowConfidenceHold(env: Env, fullName: string, number: number, headSha: string | null | undefined): Promise<number> {
+  const db = getDb(env.DB);
+  const where = and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number));
+  const [existing] = await db
+    .select({ count: pullRequests.lowConfidenceHoldCount, countedHead: pullRequests.lowConfidenceHoldHeadSha })
+    .from(pullRequests)
+    .where(where)
+    .limit(1);
+  if (!existing) return 0;
+  // `low_confidence_hold_count` is NOT NULL DEFAULT 0, so the row always carries a number here.
+  const current = Number(existing.count);
+  // An absent head SHA cannot be deduped against, so it must not count -- otherwise a stretch of sparse
+  // payloads would silently exhaust the cap and close a PR that was only ever held once.
+  if (headSha == null || existing.countedHead === headSha) return current;
+  const next = current + 1;
+  await db.update(pullRequests).set({ lowConfidenceHoldCount: next, lowConfidenceHoldHeadSha: headSha, updatedAt: nowIso() }).where(where);
+  return next;
+}
+
 // Linked-issue hard-rule violation memory (#linked-issue-hard-rule-persistence).
 
 /** Record the FIRST confirmed linked-issue hard-rule violation for a PR. Deliberately NOT scoped to headSha
@@ -6903,6 +6963,9 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull
     mergeAttemptCount: row.mergeAttemptCount,
     mergeBlockedSha: row.mergeBlockedSha,
     mergeBlockedReason: row.mergeBlockedReason,
+    mergeBlockedUntil: row.mergeBlockedUntil,
+    lowConfidenceHoldCount: row.lowConfidenceHoldCount,
+    lowConfidenceHoldHeadSha: row.lowConfidenceHoldHeadSha,
     approvedHeadSha: row.approvedHeadSha,
     // Read straight from the row, NEVER the GitHub payload — this is a loopover-internal sweep marker.
     lastRegatedAt: row.lastRegatedAt,

src/db/schema.ts

@@ -377,10 +377,24 @@ export const pullRequests = sqliteTable(
     copycatMatchedPullNumber: integer("copycat_matched_pull_number"),
     // RC3 terminal-fail merges: failed-merge attempt count + the head SHA at which the merge is terminally
     // blocked (perms/required-check/conflict) so the planner stops planning a merge. Keyed to head SHA → a new
-    // commit auto-clears it. loopover-computed (executor-written), omitted from the GitHub-sync SET clause.
+    // commit auto-clears it. loopover-computed (executor-written), omitted from the GitHub-sync SET clause --
+    // except merge_attempt_count, which the sync clause DOES reset when the head advances (#9012), because
+    // "a new commit's attempts start fresh" was documented here from the start but never actually implemented,
+    // leaving every head after the first exhaustion one-strike-terminal on any transient failure.
     mergeAttemptCount: integer("merge_attempt_count").notNull().default(0),
     mergeBlockedSha: text("merge_blocked_sha"),
     mergeBlockedReason: text("merge_blocked_reason"),
+    // #9012: expiry for an INFRA-scoped block (rejected installation token, exhausted rate-limit window) --
+    // causes that belong to the installation rather than to the commit, so waiting for a commit that will never
+    // come is the wrong recovery. NULL = commit-scoped: blocked until the head advances, as before.
+    mergeBlockedUntil: text("merge_blocked_until"),
+    // #9034: how many distinct heads of this PR have been parked in the AI-review low-confidence hold, plus the
+    // head the last one was counted for (so a re-gate of the SAME commit is the same hold, not a new one).
+    // Deliberately NOT reset by a new commit -- unlike merge_attempt_count above, repeated low-confidence holds
+    // are the pattern being capped, so letting a push zero the counter would hand back exactly the unbounded
+    // survival this exists to end. loopover-computed, omitted from the GitHub-sync SET clause.
+    lowConfidenceHoldCount: integer("low_confidence_hold_count").notNull().default(0),
+    lowConfidenceHoldHeadSha: text("low_confidence_hold_head_sha"),
     // Review-evasion: repeated ready<->draft cycling (#gaming-tactic-draft-cycle). Counts every converted_to_draft
     // webhook ever processed for this PR NUMBER -- deliberately NOT scoped to head SHA like mergeAttemptCount,
     // since cycling back to draft after a fresh push is exactly the same evasion shape a new commit must not

src/queue/job-dispatch.ts

@@ -50,6 +50,7 @@ import { generateSignalSnapshots } from "./signal-snapshot";
 import { isDecisionAuditEnabled, runDecisionAuditSample } from "../review/decision-audit";
 import { isRiskControlEnabled, runRiskControlRecalibration } from "../review/risk-control-wire";
 import { runRetentionPrune } from "./retention";
+import { sweepStaleApprovalQueue } from "../services/agent-approval-queue";
 // The 15 handlers below have no reason to move -- each is only reachable via this dispatcher (or, for
 // mapWithConcurrency, ALSO used by other still-in-processors.ts code), so they stay put and are exported
 // there purely for this one-directional import-back (processors.ts itself never calls processJob).
@@ -272,6 +273,13 @@ export async function processJob(env: Env, message: JobMessage): Promise<void> {
     }
     case "agent-regate-sweep":
       if (!message.repoFullName && message.requestedBy !== "test") {
+        // #9032: piggyback the approval-queue staleness pass on the sweep's own fan-out tick rather than adding
+        // a job type and a cron entry for a bounded DB scan. Best-effort and deliberately BEFORE the fan-out:
+        // a failure here must not cost the tick its re-gate work, which is the sweep's actual job.
+        const staleness = await sweepStaleApprovalQueue(env).catch(() => null);
+        if (staleness && (staleness.reminded > 0 || staleness.expired > 0)) {
+          console.log(JSON.stringify({ event: "approval_queue_staleness_swept", ...staleness }));
+        }
         await fanOutAgentRegateSweepJobs(env, message.requestedBy);
         return;
       }

src/queue/processors.ts

@@ -299,6 +299,8 @@ import {
   executeIssueMaintenanceActions,
   pendingClosureLabelApplied,
 } from "../services/agent-action-executor";
+import { activeMergeBlockedSha } from "../services/merge-failure";
+import { applyLowConfidenceHoldCap } from "../review/low-confidence-hold-cap";
 import { loadIssueQualityReportMap } from "../services/issue-quality";
 import { generateAndSendReviewRecap } from "../services/review-recap";
 import {
@@ -2660,7 +2662,12 @@ function buildAgentMaintenancePlanInput(args: {
         pr.createdAt,
       ),
       headSha: pr.headSha,
-      mergeBlockedSha: pr.mergeBlockedSha,
+      // #9012: pass through only a block that is STILL IN EFFECT. An infra-scoped block (rejected installation
+      // token, exhausted rate-limit window) carries an expiry; once it lapses the planner must see no block at
+      // all and re-probe the merge, so a fleet-wide token blip stops stranding green, approved PRs forever.
+      // Resolved here rather than in the planner so the planner stays a pure function of its inputs, clock-free.
+      mergeBlockedSha: activeMergeBlockedSha(pr, pr.headSha, Date.now()),
+      mergeBlockedReason: pr.mergeBlockedReason,
       approvedHeadSha: pr.approvedHeadSha,
       authorLogin: pr.authorLogin,
       linkedIssues: pr.linkedIssues,
@@ -3248,7 +3255,14 @@ async function runAgentMaintenancePlanAndExecute(
   // (no extra network/DB call, unlike migrationCollisionHold/unlinkedIssueMatchHold above) -- undefined unless the
   // gate failed SOLELY on a sub-aiReviewCloseConfidence-floor ai_consensus_defect/ai_review_split finding under
   // the (default) hold_for_review disposition. See resolveAiReviewLowConfidenceHold's own doc comment.
-  const aiReviewLowConfidenceHold = resolveAiReviewLowConfidenceHold(gate, settings);
+  const aiReviewLowConfidenceHoldCandidate = resolveAiReviewLowConfidenceHold(gate, settings);
+  // #9034: the hold is bounded. Confidence-parking a close is the right call while the verdict is genuinely
+  // uncertain, but repeated across independent rolls of the SAME PR it becomes an indefinite open hold that
+  // never escalates -- the PR survives, each roll costs a maintainer, and nothing counts. Past the cap the
+  // sub-floor finding has been reproduced enough times that the close is no longer the uncertain call the hold
+  // protects against, so it fires. The counter only advances on a head it has not already seen, so this budget
+  // is spent by real rolls rather than by the several re-gate passes a single commit attracts.
+  const aiReviewLowConfidenceHold = await applyLowConfidenceHoldCap(env, { repoFullName, pullNumber: pr.number, headSha: pr.headSha }, aiReviewLowConfidenceHoldCandidate);
   // #8962 salvageability hold — the OTHER side of the floor: an at/above-floor AI-judgment close routed to
   // hold-with-guidance when the deterministic salvageability score clears gate.aiReview.salvageabilityMinScore.
   // Knob unset (the default) short-circuits before any IO; the low-confidence hold keeps precedence.

src/review/low-confidence-hold-cap.ts

@@ -0,0 +1,72 @@
+import { bumpPullRequestLowConfidenceHold, recordAuditEvent } from "../db/repositories";
+
+/**
+ * #9034 — the bound on AI-review confidence parking.
+ *
+ * `resolveAiReviewLowConfidenceHold` (src/rules/advisory.ts) converts a would-be one-shot close into an OPEN
+ * hold when the blocking AI finding sits below the repo's close-confidence floor. That is the right call while
+ * the verdict is genuinely uncertain: an uncertain close is the expensive kind of mistake, and a human should
+ * see it. What was missing is any notion of "again": nothing counted how many times the SAME PR re-entered the
+ * hold, so a PR shaped to keep drawing sub-floor blockers survived indefinitely, cost a maintainer on every
+ * roll, and could be walked toward a clean merge from there. It was an absorbing state with no escape, the same
+ * shape as a permanently merge-blocked PR (#9012) or an unattended approval row (#9032).
+ *
+ * Deliberately its own module rather than a constant in advisory.ts: advisory.ts is one half of the
+ * hand-maintained gate-decision twin pair enforced by scripts/check-engine-parity.ts, and this cap has no engine
+ * counterpart to mirror — the engine's gate-advisory.ts carries no low-confidence hold resolver at all. Putting
+ * it here keeps the twin untouched instead of forcing a no-op engine release to satisfy the parity guard, and
+ * matches how MERGE_RETRY_CAP already lives beside its own policy (src/services/merge-failure.ts) rather than in
+ * the shared advisory module.
+ */
+
+/**
+ * How many times one PR may be parked in the low-confidence hold before the hold stops applying and the close it
+ * was suppressing fires.
+ *
+ * Past the cap the sub-floor finding has been reproduced by several independent passes, which is itself the
+ * corroboration a single pass's confidence number lacked — so the close is no longer the uncertain call the hold
+ * exists to protect against. Three is deliberately generous next to MERGE_RETRY_CAP: this budget is spent by
+ * human-visible holds a maintainer could resolve at any point, not by silent retries.
+ */
+export const AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP = 3;
+
+/** Whether a PR has exhausted its low-confidence hold budget. `holds` is the running per-PR count from
+ *  bumpPullRequestLowConfidenceHold, which advances once per distinct head — so this counts ROLLS, not the
+ *  several re-gate passes a single commit attracts. Pure. */
+export function isLowConfidenceHoldCapped(holds: number): boolean {
+  return holds > AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP;
+}
+
+/**
+ * Apply the cap to a low-confidence hold the gate just resolved (#9034). Returns the hold unchanged while the
+ * PR still has budget, or `undefined` once it does not — which lets the close the hold was suppressing fire.
+ *
+ * The counting lives here rather than at the re-gate call site so the "how many rolls has this PR spent"
+ * question has exactly one answer in the codebase, and so the capped path is directly testable: reaching that
+ * point through the pipeline needs a live gate evaluation, settings, GitHub state and a planner run — far too
+ * much machinery to stand up just to observe one boolean.
+ *
+ * Generic in the hold's shape because it neither reads nor changes it beyond quoting the reason into the audit
+ * trail — the hold is advisory.ts's to define.
+ */
+export async function applyLowConfidenceHoldCap<T extends { reason: string }>(
+  env: Env,
+  target: { repoFullName: string; pullNumber: number; headSha: string | null | undefined },
+  hold: T | undefined,
+): Promise<T | undefined> {
+  if (hold === undefined) return undefined;
+  const holds = await bumpPullRequestLowConfidenceHold(env, target.repoFullName, target.pullNumber, target.headSha);
+  if (!isLowConfidenceHoldCapped(holds)) return hold;
+  await recordAuditEvent(env, {
+    eventType: "agent.low_confidence_hold.capped",
+    actor: "loopover",
+    targetKey: `${target.repoFullName}#${target.pullNumber}`,
+    outcome: "denied",
+    detail: `low-confidence hold cap reached (${holds} > ${AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP}) — the suppressed close now proceeds`,
+    metadata: { repoFullName: target.repoFullName, pullNumber: target.pullNumber, holds, cap: AI_REVIEW_LOW_CONFIDENCE_HOLD_CAP, reason: hold.reason },
+  }).catch(
+    /* v8 ignore next -- best-effort: losing the audit row must never resurrect the hold the cap just lifted. */
+    () => undefined,
+  );
+  return undefined;
+}