fix(approval-queue,lifecycle): reopen expired staged actions, restore accept-path protections, and stop honest iteration escalating (#9481, #9482, #9483)

migrations/0197_verdict_flip_fingerprint.sql

@@ -0,0 +1,14 @@
+-- #9483 (renumbered 0196 -> 0197: the retention index migration took 0196 first): the verdict-flip guard (#9016) counted a flip whenever the fresh verdict's direction changed, keyed
+-- per (repo, pull) with no content dimension. The exploit it defends against is re-rolling a non-deterministic
+-- reviewer against the SAME content until a lucky clean roll lands -- but an honest contributor iterating on
+-- real feedback produces exactly the same signal: defect(head A) -> fix -> clean(head B) is flip 1, a new real
+-- issue on head C is flip 2, fixed on head D is flip 3 -> escalate. Two honest fix cycles reached the
+-- threshold, and because flipCount never decreased the PR then escalated on EVERY later fresh verdict --
+-- an absorbing state whose own user-facing advice ("push a substantive fix so the next review reflects real
+-- content change") was structurally unable to clear it.
+--
+-- Recording the content fingerprint each verdict was produced against lets the guard distinguish the two: a
+-- re-roll at an UNCHANGED fingerprint is the abuse pattern and still counts; a verdict against genuinely
+-- different content resets the count. Nullable so pre-existing rows keep working (a null prior fingerprint
+-- simply cannot match, so the first verdict after this migration resets rather than escalating).
+ALTER TABLE ai_review_verdict_flips ADD COLUMN last_fingerprint TEXT;

src/db/repositories.ts

@@ -6554,6 +6554,42 @@ export async function createPendingAgentActionIfAbsent(
     .limit(1);
   /* v8 ignore next -- onConflictDoNothing only no-ops when a conflicting row exists, so the lookup always finds it. */
   if (!existing) throw new Error(`pending action conflict had no row: ${repoFullName}#${input.pullNumber} ${input.actionClass}`);
+  // #9481: an EXPIRED row must not block re-staging forever. The unique index is on
+  // (repo_full_name, pull_number, action_class) with no status predicate, so once #9032's sweep expired a row
+  // this insert conflicted against it permanently -- `created: false`, so stageForApproval returned before
+  // notifying and decidePendingAgentAction reported `already_decided`. Nothing anywhere deletes or reopens an
+  // expired row, so an auto_with_approval merge/close whose maintainer was away for the expiry window became
+  // PERMANENTLY unexecutable via the queue for that PR + action class -- strictly worse than the pre-#9032
+  // behaviour, where rows waited indefinitely but stayed acceptable. The sweep's own doc promises the
+  // opposite: "a later pass that re-plans the same action stages a fresh row with a fresh notification".
+  //
+  // Reopen it in place instead, guarded on status='expired' so a concurrent accept/reject can never be
+  // clobbered, and report `created: true` so the caller notifies exactly as it would for a brand-new row.
+  // Only `expired` reopens: `pending` is legitimately sticky (a decision is outstanding) and accepted/rejected
+  // are terminal by design.
+  if (existing.status === "expired") {
+    const [reopened] = await getDb(env.DB)
+      .update(agentPendingActions)
+      .set({
+        installationId: input.installationId,
+        autonomyLevel: input.autonomyLevel,
+        paramsJson: jsonString(input.params),
+        reason: input.reason ?? null,
+        status: "pending",
+        decidedBy: null,
+        decidedAt: null,
+        createdAt: nowIso(),
+      })
+      .where(and(eq(agentPendingActions.id, existing.id), eq(agentPendingActions.status, "expired")))
+      .returning();
+    // Losing the CAS means a concurrent decision already moved the row off `expired`. Reporting the row we
+    // read with created:false is the correct answer either way -- the caller must not notify -- so there is no
+    // need to re-read it just to say the same thing.
+    /* v8 ignore next -- the falsy arm is a genuine CAS loss (a concurrent accept/reject moved the row off
+       `expired` between the SELECT and this UPDATE), which no deterministic test can stage; it falls through
+       to the same created:false answer below. */
+    if (reopened) return { action: toAgentPendingActionRecord(reopened), created: true };
+  }
   return { action: toAgentPendingActionRecord(existing), created: false };
 }
 

src/queue/processors.ts

@@ -11491,7 +11491,7 @@ async function maybePublishPrPublicSurface(
             // gates on the AI verdict, so there is nothing to shop for there. Best-effort/fail-open by
             // construction (recordVerdictFlip never throws); a persistable placeholder never counts as a roll.
             if (aiReview && aiReview.persistable !== false && settings.aiReviewMode === "block") {
-              const verdictFlip = await recordVerdictFlip(env, repoFullName, pr.number, aiReview.findings ?? []);
+              const verdictFlip = await recordVerdictFlip(env, repoFullName, pr.number, aiReview.findings ?? [], inputFingerprint);
               if (verdictFlip.escalate) {
                 advisory.findings.push({
                   code: "ai_review_inconclusive",

src/review/verdict-flip-guard.ts

@@ -13,7 +13,14 @@ import { AI_JUDGMENT_BLOCKER_CODES } from "../rules/advisory";
  *  churn (one legitimate re-review after a real fix is not abuse). */
 export const VERDICT_FLIP_ESCALATION_THRESHOLD = 3;
 
-export type VerdictFlipState = { lastHadDefect: boolean; flipCount: number };
+export type VerdictFlipState = {
+  lastHadDefect: boolean;
+  flipCount: number;
+  /** #9483: the content fingerprint the prior fresh verdict was produced against. Null for rows written
+   *  before this was tracked (migration 0196) — a null can never match, so the first verdict after the
+   *  migration resets rather than escalating, which is the fail-open direction. */
+  lastFingerprint?: string | null | undefined;
+};
 export type VerdictFlipResult = VerdictFlipState & { escalate: boolean };
 
 /**
@@ -24,10 +31,31 @@ export type VerdictFlipResult = VerdictFlipState & { escalate: boolean };
  * consistently clean, across many honest re-reviews is not the abuse pattern this guards against, only
  * genuine oscillation is.
  */
-export function nextVerdictFlipState(prior: VerdictFlipState | null, hadDefect: boolean): VerdictFlipResult {
-  if (prior === null) return { lastHadDefect: hadDefect, flipCount: 0, escalate: false };
+export function nextVerdictFlipState(
+  prior: VerdictFlipState | null,
+  hadDefect: boolean,
+  fingerprint?: string | null | undefined,
+): VerdictFlipResult {
+  if (prior === null) return { lastHadDefect: hadDefect, flipCount: 0, lastFingerprint: fingerprint ?? null, escalate: false };
+  // #9483: only a re-roll against UNCHANGED content is the abuse pattern this guards against. The exploit is
+  // re-rolling a non-deterministic reviewer over the same diff until a lucky clean roll lands; an honest
+  // contributor iterating on real feedback produced an identical signal, so defect -> fix -> clean -> new
+  // issue -> fixed (two honest cycles) hit the threshold. Worse, flipCount never decreased, so the PR then
+  // escalated on every later fresh verdict forever -- an absorbing state whose own advice to the contributor
+  // ("push a substantive fix so the next review reflects real content change") could not clear it, because a
+  // substantive fix produced a fresh verdict that re-escalated. A changed fingerprint resets the count.
+  // Written as an early return rather than a `sameContent` boolean so TypeScript narrows `fingerprint` to a
+  // string below -- otherwise the counting path needs a `?? null` fallback whose null arm is unreachable.
+  if (fingerprint == null || prior.lastFingerprint == null || prior.lastFingerprint !== fingerprint) {
+    return { lastHadDefect: hadDefect, flipCount: 0, lastFingerprint: fingerprint ?? null, escalate: false };
+  }
   const flipCount = prior.lastHadDefect === hadDefect ? prior.flipCount : prior.flipCount + 1;
-  return { lastHadDefect: hadDefect, flipCount, escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD };
+  return {
+    lastHadDefect: hadDefect,
+    flipCount,
+    lastFingerprint: fingerprint,
+    escalate: flipCount >= VERDICT_FLIP_ESCALATION_THRESHOLD,
+  };
 }
 
 /** Whether a fresh review's findings carry a blocking AI-judgment defect (ai_consensus_defect /

src/review/verdict-flip-store.ts

@@ -7,12 +7,12 @@ import { errorMessage, nowIso } from "../utils/json";
 export async function readVerdictFlipState(env: Env, repoFullName: string, pullNumber: number): Promise<VerdictFlipState | null> {
   try {
     const row = await env.DB.prepare(
-      "SELECT last_had_defect AS lastHadDefect, flip_count AS flipCount FROM ai_review_verdict_flips WHERE repo_full_name = ? AND pull_number = ?",
+      "SELECT last_had_defect AS lastHadDefect, flip_count AS flipCount, last_fingerprint AS lastFingerprint FROM ai_review_verdict_flips WHERE repo_full_name = ? AND pull_number = ?",
     )
       .bind(repoFullName, pullNumber)
-      .first<{ lastHadDefect: number; flipCount: number }>();
+      .first<{ lastHadDefect: number; flipCount: number; lastFingerprint: string | null }>();
     if (!row) return null;
-    return { lastHadDefect: row.lastHadDefect === 1, flipCount: row.flipCount };
+    return { lastHadDefect: row.lastHadDefect === 1, flipCount: row.flipCount, lastFingerprint: row.lastFingerprint };
   } catch (error) {
     console.warn(JSON.stringify({ event: "verdict_flip_read_error", repoFullName, pullNumber, message: errorMessage(error).slice(0, 120) }));
     return null;
@@ -22,10 +22,10 @@ export async function readVerdictFlipState(env: Env, repoFullName: string, pullN
 async function writeVerdictFlipState(env: Env, repoFullName: string, pullNumber: number, next: VerdictFlipState): Promise<void> {
   try {
     await env.DB.prepare(
-      `INSERT INTO ai_review_verdict_flips (repo_full_name, pull_number, last_had_defect, flip_count, updated_at) VALUES (?, ?, ?, ?, ?)
-       ON CONFLICT(repo_full_name, pull_number) DO UPDATE SET last_had_defect = excluded.last_had_defect, flip_count = excluded.flip_count, updated_at = excluded.updated_at`,
+      `INSERT INTO ai_review_verdict_flips (repo_full_name, pull_number, last_had_defect, flip_count, last_fingerprint, updated_at) VALUES (?, ?, ?, ?, ?, ?)
+       ON CONFLICT(repo_full_name, pull_number) DO UPDATE SET last_had_defect = excluded.last_had_defect, flip_count = excluded.flip_count, last_fingerprint = excluded.last_fingerprint, updated_at = excluded.updated_at`,
     )
-      .bind(repoFullName, pullNumber, next.lastHadDefect ? 1 : 0, next.flipCount, nowIso())
+      .bind(repoFullName, pullNumber, next.lastHadDefect ? 1 : 0, next.flipCount, next.lastFingerprint ?? null, nowIso())
       .run();
   } catch (error) {
     console.warn(JSON.stringify({ event: "verdict_flip_write_error", repoFullName, pullNumber, message: errorMessage(error).slice(0, 120) }));
@@ -42,10 +42,13 @@ export async function recordVerdictFlip(
   repoFullName: string,
   pullNumber: number,
   findings: ReadonlyArray<{ code: string }>,
+  /** #9483: the review's content fingerprint. A flip only counts when it MATCHES the prior verdict's, so
+   *  honest iteration on genuinely changed content resets rather than accumulating toward a permanent hold. */
+  fingerprint?: string | null | undefined,
 ): Promise<VerdictFlipResult> {
   const hadDefect = findingsHadAiDefect(findings);
   const prior = await readVerdictFlipState(env, repoFullName, pullNumber);
-  const next = nextVerdictFlipState(prior, hadDefect);
+  const next = nextVerdictFlipState(prior, hadDefect, fingerprint);
   await writeVerdictFlipState(env, repoFullName, pullNumber, next);
   return next;
 }

src/services/agent-action-executor.ts

@@ -437,8 +437,11 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
     // 4) auto_with_approval stages the action in the approval queue (#779) for a one-tap maintainer decision
     //    instead of executing it now. Staging is not a GitHub mutation; execution/replay runs this guard later.
     if (action.requiresApproval) {
-      await stageForApproval(env, ctx, action, autonomyLevel);
-      await audit("queued", `awaiting maintainer approval — ${action.reason}`);
+      // #9481: the audit is conditional on staging having actually happened. It used to fire unconditionally,
+      // so when staging no-op'd against an existing row every later sweep still recorded "awaiting maintainer
+      // approval" -- an audit trail asserting a notification that was never sent.
+      const staged = await stageForApproval(env, ctx, action, autonomyLevel);
+      if (staged) await audit("queued", `awaiting maintainer approval — ${action.reason}`);
       continue;
     }
     // 5) Write-permission readiness: a PR-visible action needs its exact GitHub App write permission granted.
@@ -1350,7 +1353,9 @@ export function pendingActionToPlanned(input: { actionClass: AgentActionClass; p
 }
 
 // Persist the staged action + notify the maintainer ONCE (on first staging, not on every re-evaluation).
-async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction, autonomyLevel: AutonomyLevel): Promise<void> {
+/** Returns whether a row was actually staged (a fresh one, or an expired one reopened per #9481) -- the
+ *  caller only audits "awaiting maintainer approval" when a notification genuinely went out. */
+async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction, autonomyLevel: AutonomyLevel): Promise<boolean> {
   const { created } = await createPendingAgentActionIfAbsent(env, {
     repoFullName: ctx.repoFullName,
     pullNumber: ctx.pullNumber,
@@ -1360,7 +1365,7 @@ async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, acti
     params: actionParams(action),
     reason: action.reason,
   });
-  if (!created) return;
+  if (!created) return false;
   /* v8 ignore next -- a repo full name always has an owner segment; the empty fallback is purely defensive. */
   const recipientLogin = ctx.repoFullName.split("/")[0] ?? "";
   await insertNotificationDeliveryIfAbsent(env, {
@@ -1375,4 +1380,5 @@ async function stageForApproval(env: Env, ctx: AgentActionExecutionContext, acti
     deeplink: `https://github.com/${ctx.repoFullName}/pull/${ctx.pullNumber}`,
     actorLogin: AGENT_ACTOR,
   });
+  return true;
 }

src/services/agent-approval-queue.ts

@@ -499,6 +499,31 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
       agentDryRun: settings.agentDryRun,
       installationPermissions: installation ? installation.permissions : null,
       mergeTrainMode: settings.mergeTrainMode,
+      // #9482: three protections were silently VOID on this replay path because the ctx omitted the fields
+      // they key on, while the live path (src/queue/processors.ts) supplies all three.
+      //
+      // authorLogin: step 8c claims `claimContributorCapLock(env, repo, ctx.authorLogin ?? "")`, so an absent
+      // author produced the key `contributor-cap-lock:<repo>:` -- the EMPTY-author key. #9159's whole point is
+      // that an accept-merge and a sibling's cap-close for the same author serialize on one mutex; keyed on
+      // "" it provided zero mutual exclusion against the live path's `...:<author>` key, leaving the #7284
+      // TOCTOU wide open, while also making every accept-path merge in a repo contend on one shared key.
+      // maybeEscalateModeration also early-returns on a falsy authorLogin, so a staged-then-accepted
+      // blacklist / contributor-cap / review-nag close recorded NO moderation violation at all -- a
+      // contributor whose enforcement closes always route through approval accumulated zero standing
+      // violations toward the warning/ban ladder.
+      authorLogin: pr?.authorLogin,
+      moderationSettings: {
+        moderationGateMode: settings.moderationGateMode,
+        moderationRules: settings.moderationRules,
+        moderationWarningLabel: settings.moderationWarningLabel,
+        moderationBannedLabel: settings.moderationBannedLabel,
+      },
+      // expectedBaseRef: fetchPullRequestFreshness only checks the base when this is set (pr-freshness.ts), so
+      // #9055's base-retarget guard never fired on a replay. NOTE this is the base as it stands NOW, which
+      // closes the common case (a retarget between staging and accept is visible here) but not the full one:
+      // catching a retarget that the webhook already wrote back requires the STAGING-TIME base persisted into
+      // AgentPendingActionParams, which has no field for it. Tracked as the remaining half of #9482.
+      expectedBaseRef: pr?.baseRef,
       pullRequestCreatedAt: pr?.createdAt,
       pullRequestLinkedIssues: pr?.linkedIssues,
       pullRequestChangedFiles: pr?.changedFiles,