fix(gate): lift the bot's own manual-review hold, and stop drafts wedging the merge train

migrations/0208_manual_review_label_provenance.sql

@@ -0,0 +1,19 @@
+-- #9939: who applied the manual-review label, so the bot can lift what the bot applied.
+--
+-- The label is OVERLOADED: it is both the planner's own "held for a human" disposition and a maintainer's
+-- manual safety freeze. agent-actions.ts therefore refuses to auto-remove it while it clears every sibling
+-- disposition label, because it has no way to tell the two apart -- and auto-removing a human's deliberate
+-- freeze would be far worse than leaving a stale one. That caution is correct and stays.
+--
+-- The cost of having no provenance is a one-way latch. Observed on #9935: applied when a finding fired
+-- against a stale head, then never lifted after a rebase removed the cause and a fresh escalated review
+-- returned zero findings. The PR was mergeable, green and clean, and still refused to merge until a human
+-- removed the label by hand -- in the mode that is meant to have no human in it.
+--
+-- Recording the head SHA the PLANNER applied it at (and the reason it applied it FOR) makes the distinction
+-- decidable: bot-applied labels become liftable once that specific reason clears, while a label with no
+-- provenance row -- a human's, or one applied before this column existed -- keeps exactly today's behaviour
+-- and is never touched automatically. Nullable with no backfill for precisely that reason: absence means
+-- "not ours to lift", which is the safe reading for every pre-existing label.
+ALTER TABLE pull_requests ADD COLUMN manual_review_label_applied_sha TEXT;
+ALTER TABLE pull_requests ADD COLUMN manual_review_label_applied_reason TEXT;

src/db/repositories.ts

@@ -4731,6 +4731,36 @@ export async function markPullRequestVisualCaptureUnobtainable(env: Env, fullNam
     .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
 }
 
+/** #9939: record that the PLANNER applied the manual-review label at `headSha`, for `reason`.
+ *
+ *  This is the provenance that makes the label liftable. The same label string is ALSO how a maintainer
+ *  freezes a PR by hand, and agent-actions.ts rightly refuses to auto-remove it without knowing which it is
+ *  looking at -- silently undoing a human's freeze is far worse than leaving a stale hold. Written only on
+ *  the planner's own add, so a human-applied label never acquires provenance and is never auto-removed.
+ *
+ *  Scoped to headSha like its visual-capture siblings: a new commit is a new decision, and a hold recorded
+ *  against an older head should not license removing a label on a head nobody has re-evaluated. */
+export async function markPullRequestManualReviewLabelApplied(env: Env, fullName: string, number: number, headSha: string, reason: string): Promise<void> {
+  const db = getDb(env.DB);
+  await db
+    .update(pullRequests)
+    .set({ manualReviewLabelAppliedSha: headSha, manualReviewLabelAppliedReason: boundedString(reason, 300), updatedAt: nowIso() })
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha)));
+}
+
+/** #9939: clear the provenance once the label is gone (removed by the planner, or by a maintainer).
+ *
+ *  Leaving it behind would let a LATER human-applied label inherit the bot's provenance and become
+ *  auto-removable -- exactly the override this whole mechanism exists to prevent. Not scoped to headSha: the
+ *  label is gone regardless of which head it was applied at. */
+export async function clearPullRequestManualReviewLabelProvenance(env: Env, fullName: string, number: number): Promise<void> {
+  const db = getDb(env.DB);
+  await db
+    .update(pullRequests)
+    .set({ manualReviewLabelAppliedSha: null, manualReviewLabelAppliedReason: null, updatedAt: nowIso() })
+    .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number)));
+}
+
 /** Release the retry latch: the retry chain that justified it has ended without a successful capture, so the
  *  screenshotTableGate must stop deferring and evaluate the evidence actually present.
  *
@@ -7441,6 +7471,8 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull
     visualCaptureSatisfiedSha: row.visualCaptureSatisfiedSha,
     visualCaptureRetryPendingSha: row.visualCaptureRetryPendingSha,
     visualCaptureRetryPendingAt: row.visualCaptureRetryPendingAt,
+    manualReviewLabelAppliedSha: row.manualReviewLabelAppliedSha,
+    manualReviewLabelAppliedReason: row.manualReviewLabelAppliedReason,
     screenshotTablePresenceSatisfied: parseJson<{ headSha: string; evidenceFingerprint: string } | null>(row.screenshotTablePresenceSatisfiedJson, null),
   };
 }

src/db/schema.ts

@@ -494,6 +494,12 @@ export const pullRequests = sqliteTable(
     // a sha with no timestamp reads as EXPIRED, which is both honest (the row predates the column) and what
     // releases the PRs already stuck behind one. loopover-computed, written with the sha, cleared with it.
     visualCaptureRetryPendingAt: text("visual_capture_retry_pending_at"),
+    // #9939: provenance for the manual-review label -- the head SHA the PLANNER applied it at, and the hold
+    // reason it applied it FOR. The label is overloaded (bot disposition AND maintainer freeze), so without
+    // this the planner cannot lift its own stale hold without risking overriding a human's. NULL means "not
+    // ours" -- a human-applied label, or one predating this column -- and is never auto-removed.
+    manualReviewLabelAppliedSha: text("manual_review_label_applied_sha"),
+    manualReviewLabelAppliedReason: text("manual_review_label_applied_reason"),
     // #9881: the head SHA at which the bot PROVED visual capture is structurally unobtainable for this repo --
     // the deployments read succeeded, found none at all, and the poll budget is spent. The screenshot-table
     // gate degrades its CLOSE to advisory for that head rather than closing a PR over evidence no

src/queue/processors.ts

@@ -2877,6 +2877,9 @@ function buildAgentMaintenancePlanInput(args: {
     guardrailEscalationModel: settings.guardrailEscalationModel ?? null,
     guardrailEscalationProvider: settings.guardrailEscalationProvider ?? null,
     manualReviewLabel: settings.manualReviewLabel,
+    // #9939: provenance for the label above -- non-null only when the PLANNER applied it, which is what
+    // lets a later pass lift its own stale hold without ever touching a maintainer's manual freeze.
+    manualReviewLabelAppliedSha: pr.manualReviewLabelAppliedSha,
     readyToMergeLabel: settings.readyToMergeLabel,
     changesRequestedLabel: settings.changesRequestedLabel,
     migrationCollisionLabel: settings.migrationCollisionLabel,

src/review/merge-train.ts

@@ -49,6 +49,18 @@ export type MergeTrainSibling = {
    *  processors.ts) rather than inventing a second, possibly-drifting definition here. Absent/undefined ⇒ not
    *  held (unchanged from before this field existed). */
   heldForManualReview?: boolean | undefined;
+  /** True when this sibling is a DRAFT (#9939). The strongest possible "not trying to merge" signal there is:
+   *  GitHub itself refuses to merge a draft, so a draft sibling is not one merge away from landing, it is one
+   *  AUTHOR ACTION away from even being eligible. Same reasoning that evicted the manual-review hold above,
+   *  only more so -- a manual-review hold at least describes a PR someone might unblock, while a draft is the
+   *  author's own declaration that it is not ready.
+   *
+   *  This is the head-of-line case that hurt in production: a maintainer PR with red CI cannot be auto-closed
+   *  (maintainers are exempt, deliberately, so they can iterate), so it stays open and overlapping for as long
+   *  as the fix takes -- holding every newer overlapping PR behind it for up to the full 24h cap even when
+   *  those are green and approved. Marking it draft now says "skip me" in a way the train understands.
+   *  Absent/undefined ⇒ not a draft (unchanged from before this field existed). */
+  isDraft?: boolean | undefined;
 };
 
 /** How long an older, OVERLAPPING sibling can hold up a newer one before it's excluded from blocking (24
@@ -126,6 +138,10 @@ export function shouldWaitForOlderSiblings(input: ShouldWaitForOlderSiblingsInpu
     .filter((sibling) => sibling.number !== thisPrNumber)
     .filter((sibling) => sibling.mergeableState !== "dirty")
     .filter((sibling) => !sibling.heldForManualReview)
+    // #9939: a draft is not trying to reach merge -- GitHub will not merge one at all. Evicted outright
+    // rather than merely capped, exactly like the manual-review hold above: waiting on a PR that cannot
+    // merge buys nothing, and the 24h cap is far too long to be the only bound on it.
+    .filter((sibling) => !sibling.isDraft && sibling.mergeableState !== "draft")
     .filter((sibling) => isOlder(sibling))
     .filter((sibling) => overlaps(thisPrLinkedIssues, thisPrChangedFiles, sibling))
     .filter((sibling) => {

src/services/agent-action-executor.ts

@@ -32,6 +32,7 @@ import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy";
 import { boundStructuredCloseReasonsForPersistence, buildAgentActionAudit, formatAgentPermissionDenial, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness, type AgentActionMode } from "../settings/agent-execution";
 import { AGENT_LABEL_NEEDS_REVIEW, type PlannedAgentAction } from "../settings/agent-actions";
 import type { AgentActionClass, AgentPendingActionParams, AutonomyLevel, AutonomyPolicy } from "../types";
+import { clearPullRequestManualReviewLabelProvenance, markPullRequestManualReviewLabelApplied } from "../db/repositories";
 import { errorMessage } from "../utils/json";
 import {
   MODERATION_VIOLATION_EVENT_TYPE,
@@ -697,6 +698,10 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
           linkedIssues: sibling.linkedIssues,
           changedFiles: pathsByPullNumber.get(sibling.number),
           heldForManualReview: manualReviewLabel !== null && sibling.labels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase()),
+          // #9939: a draft sibling never blocks -- GitHub will not merge it, so it is not "about to land".
+          // `?? false` rather than passing the nullable straight through: the field is optional on the record
+          // and the gate treats absent as "not a draft", so normalising here keeps the two readings identical.
+          isDraft: sibling.isDraft ?? false,
         })),
         nowMs: Date.now(),
       });
@@ -1287,8 +1292,21 @@ async function performAction(env: Env, ctx: AgentActionExecutionContext, action:
       // optional comment (the Pass-1 flag warning, or the resolved note) posted alongside the label mutation.
       if (action.labelOp === "remove") {
         await removePullRequestLabel(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.label ?? "");
+        // #9939: the label is gone, so its provenance must go with it. Leaving a stale marker behind would let
+        // a LATER human-applied label inherit the bot's provenance and become auto-removable -- exactly the
+        // override the provenance exists to prevent. Best-effort: a failed clear only means the next pass
+        // re-evaluates with a marker for a label that is not there, which the `hasLabel` guard already ignores.
+        if (ctx.manualReviewLabel && action.label === ctx.manualReviewLabel) {
+          await clearPullRequestManualReviewLabelProvenance(env, ctx.repoFullName, ctx.pullNumber).catch(() => {});
+        }
       } else {
         await ensurePullRequestLabel(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.label ?? "", { createMissingLabel: true });
+        // #9939: record that the PLANNER (not a maintainer) applied this hold, so a later pass may lift it
+        // once nothing wants it. Written only here, on the bot's own add, which is what keeps a
+        // human-applied label provenance-free and therefore untouchable.
+        if (ctx.manualReviewLabel && action.label === ctx.manualReviewLabel && ctx.headSha) {
+          await markPullRequestManualReviewLabelApplied(env, ctx.repoFullName, ctx.pullNumber, ctx.headSha, action.reason).catch(() => {});
+        }
       }
       if (action.comment) await createIssueComment(env, ctx.installationId, ctx.repoFullName, ctx.pullNumber, action.comment);
       return;

src/settings/agent-actions.ts

@@ -286,6 +286,11 @@ export type AgentActionPlanInput = {
   // Configured manual-review hold label. Undefined uses the default "manual-review"; null disables only the
   // label, not the guardrail hold. Separate from review_state_label so operators can avoid ready/changes labels.
   manualReviewLabel?: string | null | undefined;
+  /** #9939: the head SHA at which the PLANNER previously applied `manualReviewLabel`, or null/absent when the
+   *  bot did not apply it. This is the provenance that makes the label liftable: the same label string is
+   *  also how a maintainer freezes a PR by hand, so without it the planner cannot remove its own stale hold
+   *  without risking silently undoing a human's. Absent ⇒ never auto-removed. */
+  manualReviewLabelAppliedSha?: string | null | undefined;
   // Optional disposition label overrides. Undefined uses generic defaults; null disables that specific label.
   readyToMergeLabel?: string | null | undefined;
   changesRequestedLabel?: string | null | undefined;
@@ -1271,6 +1276,40 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne
     });
   }
 
+  // 1b) #9939: LIFT a manual-review hold the planner itself applied, once nothing wants it any more.
+  //
+  // The sibling-label cleanup further down deliberately refuses to touch this label, because the same string
+  // is also a maintainer's manual freeze and that cleanup has no way to tell them apart. Correct -- but it
+  // made the label a ONE-WAY LATCH: applied on a pass where a hold was real, and never lifted once the hold
+  // cleared. Observed on #9935, which was mergeable, green, and re-reviewed to zero findings, and still
+  // refused to merge until a human removed the label by hand.
+  //
+  // `manualReviewLabelAppliedSha` is the missing provenance. Non-null means the PLANNER applied it, so the
+  // planner may take it back; null means a human applied it (or it predates the column) and it is left
+  // strictly alone. Guarded on `manualHoldReason === null`, i.e. NO reason wants a hold this pass -- which is
+  // why this cannot lift a label applied for reason A just because reason B cleared. Deliberately not scoped
+  // to the recorded head: a rebase that resolves the cause is the single most common way a hold goes stale,
+  // and refusing to lift it there would leave the exact #9935 case unfixed.
+  if (
+    manualHoldReason === null &&
+    labels.manualReview !== null &&
+    input.manualReviewLabelAppliedSha != null &&
+    hasLabel(input.pr.labels, labels.manualReview)
+    // No "is an add already planned this pass?" guard: every site that ADDS this label requires a live hold
+    // (a guardrail hit, the migration-collision fallback, the owner/automation fallback), and all of those
+    // set manualHoldReason -- so the null check above already excludes every case where an add could be in
+    // flight. A guard here would be an arm no input can reach.
+  ) {
+    actions.push({
+      actionClass: "label",
+      autonomyClass: "merge",
+      requiresApproval: approval("merge"),
+      reason: `manual-review hold resolved — clearing the "${labels.manualReview}" label the bot applied`,
+      label: labels.manualReview,
+      labelOp: "remove",
+    });
+  }
+
   // 1c) migration-collision manual-review fallback (#manual-review-coverage) — the migration-collision LABEL
   // itself stays gated on review_state_label below (section 2, unchanged) so an operator's dedicated filter on
   // that specific label is undisturbed. But when review_state_label is OFF (a one-shot repo that only configures

src/types.ts

@@ -777,6 +777,13 @@ export type PullRequestRecord = {
    *  review/visual/visual-capture-retry-latch.ts for why a sha with no timestamp reads as expired rather than
    *  live. Publish-written alongside the sha; read straight from the row. */
   visualCaptureRetryPendingAt?: string | null | undefined;
+  /** #9939: the head SHA at which the PLANNER applied the manual-review label, and the hold reason it applied
+   *  it for. Together they are the provenance that lets the planner lift its OWN stale hold without ever
+   *  touching a maintainer's manual freeze -- the label is the same string for both. Absent ⇒ not the bot's
+   *  to remove (a human applied it, or it predates this), which is the safe default for every existing label.
+   *  Planner-written; read straight from the row. */
+  manualReviewLabelAppliedSha?: string | null | undefined;
+  manualReviewLabelAppliedReason?: string | null | undefined;
   /** Screenshot-table PRESENCE-mode staleness correlation (#stale-screenshot-table-fix): the (headSha,
    *  evidenceFingerprint) checkpoint the screenshotTableGate's presence-mode check last satisfied for this PR
    *  (see evaluateScreenshotTableGate's staleness comment). `null`/absent = presence mode has never satisfied