fix(agent): deny approve/merge when a live manual-review hold exists (#3472) (#3483)

Two independent plan-and-execute passes can evaluate the same PR head
(a webhook re-review racing a regate-repair/sweep pass): the loser of
the AI-review lock holds the PR for manual review (label + assign),
while the winner's own slower AI review later resolves cleanly and
stages approve/merge from a plan-time snapshot that predates the
hold. Neither the per-PR actuation lock nor the executor's freshness
guard checks the PR's live labels, so the hold was silently bypassed.

Add a live guard immediately before approve/merge that re-checks the
same freshness fetch for the configured manual-review label and
denies the action if it's present -- self-scoping to the same head
(a new commit already fails freshness first) with no new locking or
persistence required.

Closes #3482

Author: JSONbored

src/github/pr-freshness.ts

@@ -17,6 +17,10 @@ export type PullRequestFreshness =
       status: "current";
       liveHeadSha: string | null;
       liveState: string | null;
+      // Live label names off the SAME fetch that proved this head is current — lets a caller re-check a
+      // disposition label (e.g. a manual-review hold) against ground truth immediately before a mutation,
+      // without a second GitHub call (#3472 split-brain).
+      liveLabels: string[];
     }
   | {
       status: "stale";
@@ -41,7 +45,7 @@ export function reviewedPullRequestHeadSha(
 }
 
 export function classifyPullRequestFreshness(
-  live: Pick<GitHubPullRequestPayload, "state" | "head" | "draft"> | null | undefined,
+  live: Pick<GitHubPullRequestPayload, "state" | "head" | "draft" | "labels"> | null | undefined,
   expectedHeadSha: string | null | undefined,
   options?: PullRequestFreshnessOptions,
 ): PullRequestFreshness {
@@ -85,7 +89,8 @@ export function classifyPullRequestFreshness(
   if (options?.requireDraft && live.draft !== true) {
     return { status: "stale", reason: "no_longer_draft", expectedHeadSha: expected, liveHeadSha, liveState };
   }
-  return { status: "current", liveHeadSha, liveState };
+  const liveLabels = (live.labels ?? []).map((label) => label.name).filter((name): name is string => Boolean(name));
+  return { status: "current", liveHeadSha, liveState, liveLabels };
 }
 
 export async function fetchPullRequestFreshness(

src/queue/processors.ts

@@ -2849,6 +2849,9 @@ async function runAgentMaintenancePlanAndExecute(
       // merge or a CI-driven close) must honor the same configured expectedCiContexts this plan was evaluated
       // against, or the two can disagree on ciState.
       expectedCiContexts: settings.expectedCiContexts,
+      // #3472 split-brain: the executor's own live manual-review hold guard (immediately before approve/merge)
+      // must check the SAME configured label the planner itself resolves labels.manualReview from.
+      manualReviewLabel: settings.manualReviewLabel,
     },
     breakerOnPlan,
   );

src/services/agent-action-executor.ts

@@ -24,7 +24,7 @@ import { closeIssue, closePullRequest, createIssueComment, createPullRequestRevi
 import { fetchPullRequestFreshness, pullRequestFreshnessDetail } from "../github/pr-freshness";
 import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy";
 import { boundStructuredCloseReasonsForPersistence, buildAgentActionAudit, formatAgentPermissionDenial, isGlobalAgentPause, resolveAgentActionMode, resolveAgentPermissionReadiness, type AgentActionMode } from "../settings/agent-execution";
-import type { PlannedAgentAction } from "../settings/agent-actions";
+import { AGENT_LABEL_NEEDS_REVIEW, type PlannedAgentAction } from "../settings/agent-actions";
 import type { AgentActionClass, AgentPendingActionParams, AutonomyLevel, AutonomyPolicy } from "../types";
 import { errorMessage } from "../utils/json";
 import {
@@ -175,6 +175,12 @@ export type AgentActionExecutionContext = {
   // required-only view was already clean). Absent/undefined ⇒ fold-all mode, unchanged from before this field
   // existed.
   expectedCiContexts?: ReadonlyArray<string> | null | undefined;
+  // settings.manualReviewLabel (#3472 split-brain), resolved by the CALLER (same "the executor has no settings
+  // access" shape as expectedCiContexts above): the approve/merge live label guard (step 7b below) needs the
+  // SAME configured label name the planner itself resolves labels.manualReview from (agent-actions.ts), so a
+  // custom label name is honored instead of only ever checking the literal default. `null` explicitly disables
+  // the manual-review label (and this guard with it); absent/undefined uses the default AGENT_LABEL_NEEDS_REVIEW.
+  manualReviewLabel?: string | null | undefined;
 };
 
 export type ModerationContextSettings = {
@@ -215,7 +221,8 @@ function coupledCloseOutcome(planned: PlannedAgentAction[], outcomes: AgentActio
  * through the SAME deny-toward-safety gate stack:
  *   pause (#776 kill-switch) → current autonomy → dry_run → approval (auto_with_approval → #779 queue) →
  *   write-permission (#775, checked BEFORE any GitHub call so a known-denied write never spends freshness/live-CI
- *   API budget) → label/close correlation → freshness → live-CI re-verification → the real mutation.
+ *   API budget) → label/close correlation → freshness → manual-review hold (approve/merge only, #3472) →
+ *   live-CI re-verification → the real mutation.
  * Only `live` mode performs a real mutation; `dry_run` records what it WOULD do. Every path writes one
  * `agent.action.<class>` audit record (#776) EXCEPT a write-permission denial repeated within
  * PR_WRITE_DENIAL_COOLDOWN_MS of the last one for the same installation/repo/PR/action-class, which is counted but
@@ -332,6 +339,23 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
         await audit("denied", `${pullRequestFreshnessDetail(freshness)} — action not executed`);
         continue;
       }
+      // 7b) Manual-review hold guard (#3472 split-brain): approve/merge is planned from a snapshot (the DB's
+      // cached pr.labels, or a plan staged earlier for approval) that can predate a SIBLING pass for this exact
+      // PR/head publishing a manual-review hold (label + assign) while THIS pass's own — possibly much slower —
+      // AI review or gate evaluation was still in flight. The per-PR actuation lock (#2129) only serializes each
+      // pass's plan-and-execute critical section; it does not make one pass aware of another's disposition, and
+      // the stored PR row can itself lag the live label write by a full webhook round-trip. Re-check the SAME
+      // live fetch that just proved this head is current (no extra GitHub call) for the configured manual-review
+      // label: if present, a hold is standing for this exact head and must not be silently overridden by a
+      // merit verdict computed before that hold existed. Only a maintainer removing the label, or a new commit
+      // (which the freshness check above already denies as stale), lifts it.
+      if (action.actionClass === "approve" || action.actionClass === "merge") {
+        const manualReviewLabel = ctx.manualReviewLabel === null ? null : (ctx.manualReviewLabel ?? AGENT_LABEL_NEEDS_REVIEW);
+        if (manualReviewLabel !== null && freshness.liveLabels.some((label) => label.toLowerCase() === manualReviewLabel.toLowerCase())) {
+          await audit("denied", `manual-review label "${manualReviewLabel}" is present on the live PR — ${action.actionClass} not executed`);
+          continue;
+        }
+      }
     }
     // 8) Live CI re-verification for a merge or a CI-driven heuristic close (#2128): the CI aggregate that drove
     //    either decision was read seconds-to-tens-of-seconds earlier, in the planning pass, and the freshness

src/services/agent-approval-queue.ts

@@ -356,6 +356,10 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
       // executeAgentMaintenanceActions) needs the same configured expectedCiContexts this accept-time
       // re-check (above) and the original plan were both evaluated against.
       expectedCiContexts: settings.expectedCiContexts,
+      // #3472 split-brain: a staged approve/merge can sit queued long enough for a SIBLING pass to publish a
+      // manual-review hold on this same PR/head before the maintainer accepts — the executor's own live guard
+      // (step 7b of executeAgentMaintenanceActions) needs the configured label to check for.
+      manualReviewLabel: settings.manualReviewLabel,
     },
     plan,
   );

test/unit/agent-action-executor.test.ts

@@ -24,6 +24,7 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => {
       status: "current" as const,
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [] as string[],
     })),
   };
 });
@@ -100,6 +101,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
       status: "current",
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [],
     }));
     clearInstallationHealthRefreshCooldownForTest();
     clearWritePermissionDenialCooldownForTest();
@@ -365,6 +367,60 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
     expect(createPullRequestReview).toHaveBeenCalledWith(env, 123, "owner/repo", 7, "APPROVE", "", "reviewed-sha");
   });
 
+  it("REGRESSION (#3472 split-brain): LIVE merge is denied when the default manual-review label is present on the live PR, even though the plan itself computed a clean merge", async () => {
+    const env = createTestEnv({});
+    // Simulates a sibling pass (e.g. a webhook re-review) publishing a manual-review hold WHILE this pass's own
+    // gate evaluation was still in flight -- the plan below was computed before that hold existed, so only the
+    // executor's live re-check (not the plan) can catch it.
+    vi.mocked(fetchPullRequestFreshness).mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: ["manual-review"] });
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]);
+    expect(outcomes[0]?.outcome).toBe("denied");
+    expect(outcomes[0]?.detail).toContain('manual-review label "manual-review" is present on the live PR — merge not executed');
+    expect(mergePullRequest).not.toHaveBeenCalled();
+  });
+
+  it("REGRESSION (#3472 split-brain): LIVE approve is denied when the default manual-review label is present on the live PR", async () => {
+    const env = createTestEnv({});
+    vi.mocked(fetchPullRequestFreshness).mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: ["manual-review"] });
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [approve]);
+    expect(outcomes[0]?.outcome).toBe("denied");
+    expect(outcomes[0]?.detail).toContain('manual-review label "manual-review" is present on the live PR — approve not executed');
+    expect(createPullRequestReview).not.toHaveBeenCalled();
+  });
+
+  it("LIVE merge proceeds when the live PR carries other labels but not the manual-review hold", async () => {
+    const env = createTestEnv({});
+    vi.mocked(fetchPullRequestFreshness).mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: ["size/L", "gittensor:bug"] });
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]);
+    expect(outcomes[0]?.outcome).toBe("completed");
+    expect(mergePullRequest).toHaveBeenCalled();
+  });
+
+  it("honors a CUSTOM configured manualReviewLabel name (case-insensitive) instead of only the literal default", async () => {
+    const env = createTestEnv({});
+    vi.mocked(fetchPullRequestFreshness).mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: ["Needs-Human"] });
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ manualReviewLabel: "needs-human" }), [merge]);
+    expect(outcomes[0]?.outcome).toBe("denied");
+    expect(outcomes[0]?.detail).toContain('manual-review label "needs-human" is present on the live PR — merge not executed');
+  });
+
+  it("skips the manual-review hold guard entirely when manualReviewLabel is explicitly disabled (null)", async () => {
+    const env = createTestEnv({});
+    vi.mocked(fetchPullRequestFreshness).mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: ["manual-review"] });
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ manualReviewLabel: null }), [merge]);
+    expect(outcomes[0]?.outcome).toBe("completed");
+    expect(mergePullRequest).toHaveBeenCalled();
+  });
+
+  it("the manual-review hold guard is scoped to approve/merge only -- a label/close/assign action is unaffected by the live manual-review label", async () => {
+    const env = createTestEnv({});
+    vi.mocked(fetchPullRequestFreshness).mockResolvedValue({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: ["manual-review"] });
+    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [label, close]);
+    expect(outcomes.map((o) => o.outcome)).toEqual(["completed", "completed"]);
+    expect(ensurePullRequestLabel).toHaveBeenCalled();
+    expect(closePullRequest).toHaveBeenCalled();
+  });
+
   it("LIVE heuristic close is denied when live CI has since turned green (#2128)", async () => {
     const env = createTestEnv({});
     const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" };
@@ -855,7 +911,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
       // regression tripwire -- it must NEVER be consumed, because the label reuses the close's already-proven
       // outcome instead of re-checking freshness against the now-closed PR (which would read "stale"/closed and
       // wrongly deny the label). The toHaveBeenCalledTimes(1) assertion below is what actually proves this.
-      .mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open" })
+      .mockResolvedValueOnce({ status: "current", liveHeadSha: "sha7", liveState: "open", liveLabels: [] })
       .mockResolvedValueOnce({ status: "stale", reason: "closed", expectedHeadSha: "sha7", liveHeadSha: "sha7", liveState: "closed" });
     const outcomes = await executeAgentMaintenanceActions(env, ctx(), [coupledClose, coupledLabel]);
     expect(outcomes.map((o) => o.outcome)).toEqual(["completed", "completed"]);
@@ -969,6 +1025,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
         status: "current",
         liveHeadSha: "sha7",
         liveState: "open",
+        liveLabels: [],
       })
       .mockResolvedValueOnce({
         status: "stale",
@@ -1113,6 +1170,7 @@ describe("moderation-rules engine escalation (#selfhost-mod-engine)", () => {
       status: "current",
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [],
     }));
     // clearAllMocks() resets call history but does NOT drain a queued mockRejectedValueOnce/mockResolvedValueOnce
     // left over from an earlier test elsewhere in this file (e.g. the installation-health-refresh tests above

test/unit/agent-approval-queue.test.ts

@@ -18,6 +18,7 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => {
       status: "current" as const,
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [] as string[],
     })),
   };
 });

test/unit/mcp-automation-state.test.ts

@@ -34,6 +34,7 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => {
       status: "current" as const,
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [] as string[],
     })),
   };
 });

test/unit/pr-freshness.test.ts

@@ -14,7 +14,7 @@ describe("PR freshness guards", () => {
 
   it("classifies a matching open head as current", () => {
     const result = classifyPullRequestFreshness({ state: "open", head: { sha: "sha1" } }, "sha1");
-    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open" });
+    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open", liveLabels: [] });
     expect(pullRequestFreshnessDetail(result)).toBe("PR is current");
   });
 
@@ -23,9 +23,26 @@ describe("PR freshness guards", () => {
       status: "current",
       liveHeadSha: null,
       liveState: "open",
+      liveLabels: [],
     });
   });
 
+  it("carries live label names alongside a current status (#3472 split-brain)", () => {
+    const result = classifyPullRequestFreshness(
+      { state: "open", head: { sha: "sha1" }, labels: [{ name: "manual-review" }, { name: "size/L" }] },
+      "sha1",
+    );
+    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open", liveLabels: ["manual-review", "size/L"] });
+  });
+
+  it("drops nameless label entries when carrying live labels", () => {
+    const result = classifyPullRequestFreshness(
+      { state: "open", head: { sha: "sha1" }, labels: [{ name: "manual-review" }, {}] },
+      "sha1",
+    );
+    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open", liveLabels: ["manual-review"] });
+  });
+
   it("treats unavailable live state as stale for callers that require proof", () => {
     const result = classifyPullRequestFreshness(undefined, "sha1");
     expect(result).toMatchObject({ status: "stale", reason: "unavailable", expectedHeadSha: "sha1" });
@@ -115,7 +132,7 @@ describe("PR freshness guards", () => {
 
   it("does not require draft state by default, even when the PR is no longer a draft", () => {
     const result = classifyPullRequestFreshness({ state: "open", head: { sha: "sha1" }, draft: false }, "sha1");
-    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open" });
+    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open", liveLabels: [] });
   });
 
   it("REGRESSION (#2130 follow-up): treats a same-head PR converted back to ready_for_review as stale when the caller requires draft", () => {
@@ -126,7 +143,7 @@ describe("PR freshness guards", () => {
 
   it("treats a still-draft PR as current when the caller requires draft", () => {
     const result = classifyPullRequestFreshness({ state: "open", head: { sha: "sha1" }, draft: true }, "sha1", { requireDraft: true });
-    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open" });
+    expect(result).toEqual({ status: "current", liveHeadSha: "sha1", liveState: "open", liveLabels: [] });
   });
 
   it("treats a missing draft field as stale when the caller requires draft (fail-safe: only an explicit true counts)", () => {
@@ -158,10 +175,10 @@ describe("PR freshness guards", () => {
     expect(reviewedPullRequestHeadSha(" AbC123 ", "fallback")).toBe("abc123");
     expect(
       classifyPullRequestFreshness({ state: "open", head: { sha: "AbC123" } }, "abc123"),
-    ).toEqual({ status: "current", liveHeadSha: "abc123", liveState: "open" });
+    ).toEqual({ status: "current", liveHeadSha: "abc123", liveState: "open", liveLabels: [] });
     expect(
       classifyPullRequestFreshness({ state: "open", head: { sha: "abc123" } }, " ABC123 "),
-    ).toEqual({ status: "current", liveHeadSha: "abc123", liveState: "open" });
+    ).toEqual({ status: "current", liveHeadSha: "abc123", liveState: "open", liveLabels: [] });
     expect(
       classifyPullRequestFreshness({ state: "open", head: { sha: "NewSha" } }, "oldsha"),
     ).toMatchObject({ status: "stale", reason: "head_changed", expectedHeadSha: "oldsha", liveHeadSha: "newsha" });

test/unit/queue.test.ts

@@ -72,6 +72,7 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => {
       status: "current" as const,
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [] as string[],
     })),
   };
 });
@@ -102,6 +103,7 @@ describe("queue processors", () => {
       status: "current",
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [],
     }));
     vi.useFakeTimers({ toFake: ["Date"] });
     vi.setSystemTime(new Date("2026-05-28T00:00:00.000Z"));

test/unit/routes-agent-approval.test.ts

@@ -17,6 +17,7 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => {
       status: "current" as const,
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
+      liveLabels: [] as string[],
     })),
   };
 });