diff --git a/src/github/pr-freshness.ts b/src/github/pr-freshness.ts index 0aa903236d..fc4a2f9ed4 100644 --- a/src/github/pr-freshness.ts +++ b/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 | null | undefined, + live: Pick | 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( diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 0d7b39b68e..c9a14b2c48 100644 --- a/src/queue/processors.ts +++ b/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, ); diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 62a5d97e67..0292862177 100644 --- a/src/services/agent-action-executor.ts +++ b/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 | 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.` 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 diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 250c8cf67e..dbef88b1f4 100644 --- a/src/services/agent-approval-queue.ts +++ b/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, ); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index e9eae409d9..f95352c26c 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/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 diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 86dc882ccb..be27ff48af 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/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[], })), }; }); diff --git a/test/unit/mcp-automation-state.test.ts b/test/unit/mcp-automation-state.test.ts index 6f7265b870..ce5906e91a 100644 --- a/test/unit/mcp-automation-state.test.ts +++ b/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[], })), }; }); diff --git a/test/unit/pr-freshness.test.ts b/test/unit/pr-freshness.test.ts index c333ad70b1..5e85309a10 100644 --- a/test/unit/pr-freshness.test.ts +++ b/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" }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index bbc2ded361..4467adb372 100644 --- a/test/unit/queue.test.ts +++ b/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")); diff --git a/test/unit/routes-agent-approval.test.ts b/test/unit/routes-agent-approval.test.ts index 26223b780b..73e5d8931a 100644 --- a/test/unit/routes-agent-approval.test.ts +++ b/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[], })), }; });