Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/github/pr-freshness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down
3 changes: 3 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
28 changes: 26 additions & 2 deletions src/services/agent-action-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions src/services/agent-approval-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down
60 changes: 59 additions & 1 deletion test/unit/agent-action-executor.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
})),
};
});
Expand Down Expand Up @@ -100,6 +101,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
status: "current",
liveHeadSha: args.expectedHeadSha ?? null,
liveState: "open",
liveLabels: [],
}));
clearInstallationHealthRefreshCooldownForTest();
clearWritePermissionDenialCooldownForTest();
Expand Down Expand Up @@ -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" };
Expand Down Expand Up @@ -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"]);
Expand Down Expand Up @@ -969,6 +1025,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
status: "current",
liveHeadSha: "sha7",
liveState: "open",
liveLabels: [],
})
.mockResolvedValueOnce({
status: "stale",
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions test/unit/agent-approval-queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
})),
};
});
Expand Down
1 change: 1 addition & 0 deletions test/unit/mcp-automation-state.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
})),
};
});
Expand Down
27 changes: 22 additions & 5 deletions test/unit/pr-freshness.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});

Expand All @@ -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" });
Expand Down Expand Up @@ -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", () => {
Expand All @@ -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)", () => {
Expand Down Expand Up @@ -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" });
Expand Down
2 changes: 2 additions & 0 deletions test/unit/queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
})),
};
});
Expand Down Expand Up @@ -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"));
Expand Down
1 change: 1 addition & 0 deletions test/unit/routes-agent-approval.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[],
})),
};
});
Expand Down
Loading