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

Two independent reasons an autonomous queue stalls with no human in it, both
found on live PRs.

1) The manual-review label was a ONE-WAY LATCH.

The sibling-label cleanup deliberately refuses to touch it, because the same
label string is also a maintainer's manual freeze and the planner had no way to
tell them apart -- and silently undoing a human's freeze is far worse than
leaving a stale hold. That caution was right; the missing piece was provenance.

Seen on #9935: the label went on when a finding fired against a stale head, a
rebase removed the cause, a fresh escalated review returned ZERO findings, and
the PR was mergeable, green and clean -- and still would not merge:

  agent.action.merge | denied | manual-review label "manual-review" is present
                               on the live PR -- merge not executed

It took a human removing the label by hand, in the mode that is supposed to
have no human in it.

The planner now records the head SHA and reason when IT applies the label, and
may lift it once manualHoldReason is null (no reason wants a hold this pass, so
a label applied for reason A can never be lifted by reason B clearing). A label
with NO provenance -- a human's, or one predating this -- is never touched.
That asymmetry is the whole safety property.

2) A draft sibling wedged the merge train.

The train already evicts conflicted and manual-review-held siblings on the
principle that waiting for a PR which is not trying to reach merge buys nothing
the 24h cap does not already bound far worse. A DRAFT is the strongest form of
that signal: GitHub itself refuses to merge one, so it is not one merge away
from landing, it is one author action away from being eligible at all.

The production shape: a maintainer PR with red CI cannot be auto-closed
(maintainers are exempt on purpose, so they can iterate), so it stays open and
overlapping for as long as the fix takes -- holding every newer overlapping PR
behind it, green and approved, for up to the full 24 hours. Marking it draft is
the author saying "skip me", and the train now understands that.

Eviction removes ONE PR from the queue; it does not disable the gate. A real
older sibling behind the draft still holds the line.

Author: JSONbored

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