From 626f2e36ac1201f3a1cea6b5772932460a663989 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 04:12:55 -0700 Subject: [PATCH 1/4] fix(agent-actions): re-verify a staged merge's live state and breaker at accept time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit decidePendingAgentAction's only freshness check before replaying a staged approval-queue action was head-SHA equality. auto_with_approval rows have no expiry, so between staging and a maintainer's accept, CI could flip red, the base could go dirty, a reviewer could request changes, or the merge-precision circuit-breaker could engage — none of which move the head SHA, so none of them were caught. - Re-fetch live CI state, mergeable_state, and reviewDecision for a staged merge at accept time; supersede (deny, audit, leave the row untouched) instead of executing on stale justification. Best-effort: a failed live read fails open on that specific check, since the mutation call independently needs a valid token/state and fails cleanly on its own. - Re-apply the same merge/close precision circuit-breakers the live webhook path already applies, so a breaker engaged after staging still holds the row (downgrades to a needs-human-review label) instead of executing unmodified. - Re-sync the merge method to the repo's current config instead of the staging-time snapshot. While wiring the close breaker, found `closeKind` never survived staging at all — `actionParams()` dropped it, so `downgradeCloseToHold`'s heuristic-close match could never fire for any staged close regardless of this fix. Threaded it through `AgentPendingActionParams` and `actionParams()` so it round-trips. Advances #1936. Closes #2126, #2127, #2131. Advances #2132 (the CI/mergeable/ review portion lands here; the linked-issue-hard-rule re-check does not — see that issue for the remaining scope). --- src/services/agent-action-executor.ts | 4 + src/services/agent-approval-queue.ts | 61 ++++++++++- src/types.ts | 3 + test/unit/agent-approval-queue.test.ts | 137 +++++++++++++++++++++++++ test/unit/mcp-automation-state.test.ts | 9 ++ 5 files changed, 212 insertions(+), 2 deletions(-) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 2e2bde549a..fd9fef1581 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -234,6 +234,10 @@ export function actionParams(action: PlannedAgentAction): AgentPendingActionPara ...(action.mergeMethod !== undefined ? { mergeMethod: action.mergeMethod } : {}), ...(action.closeComment !== undefined ? { closeComment: action.closeComment } : {}), ...(action.expectedHeadSha !== undefined ? { expectedHeadSha: action.expectedHeadSha } : {}), + // Round-trip closeKind so a staged close's kind survives to accept-time — without it, the close-precision + // breaker's isHeuristicClose check (which matches on closeKind === "heuristic") could never fire for any + // staged close, silently defeating the breaker for the entire approval-queue accept path (#2127). + ...(action.closeKind !== undefined ? { closeKind: action.closeKind } : {}), }; } diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index a391b5c106..71d61f61ca 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -1,6 +1,11 @@ import { getInstallation, getPullRequest, getRepositorySettings, getPendingAgentAction, recordAuditEvent, setPendingAgentActionStatus } from "../db/repositories"; import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent-action-executor"; -import type { AgentPendingActionRecord } from "../types"; +import { downgradeCloseToHold, downgradeMergeToHold, type PlannedAgentAction } from "../settings/agent-actions"; +import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; +import { createInstallationToken } from "../github/app"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../github/backfill"; +import { githubRateLimitAdmissionKeyForToken } from "../github/client"; +import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; export type ApprovalDecision = "accept" | "reject"; @@ -55,6 +60,58 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "head_moved" }; } + // Re-derive live justification for a staged MERGE at accept time. auto_with_approval rows have no expiry, so + // CI can flip red, the base can go dirty, or a reviewer can request changes while the row just sits waiting for + // a maintainer — none of which move the head SHA, so the check above alone would not catch it. Best-effort: a + // failed live read fails OPEN on that specific check (the executor's own mutation call independently needs a + // valid token/state and will fail cleanly if something is actually wrong). (#2126) + let liveParams: AgentPendingActionParams = pending.params; + if (pending.actionClass === "merge" && pr?.headSha) { + const token = await createInstallationToken(env, pending.installationId).catch(() => undefined); + const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, pending.installationId); + const [ciAggregate, mergeableState, reviewDecision] = await Promise.all([ + fetchLiveCiAggregate(env, pending.repoFullName, pr.headSha, token, undefined, admissionKey), + fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey), + fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey), + ]); + const staleReason = + ciAggregate.ciState === "failed" + ? "live CI is now failing" + : mergeableState === "dirty" + ? "the base branch now conflicts (mergeable_state: dirty)" + : reviewDecision === "CHANGES_REQUESTED" + ? "a reviewer has since requested changes" + : null; + if (staleReason) { + await setPendingAgentActionStatus(env, pending.id, { status: "rejected", decidedBy: input.decidedBy }); + await recordAuditEvent(env, { + eventType: "agent.pending_action.superseded", + actor: input.decidedBy, + targetKey, + outcome: "denied", + detail: `superseded ${pending.actionClass}: ${staleReason} since staging`, + metadata: { ...baseMetadata, ciState: ciAggregate.ciState, mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null }, + }); + return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "stale_disposition" }; + } + // Re-sync the merge method to the CURRENT repo config, not the staging-time snapshot — the head-SHA pin + // above should stay frozen (that's the reviewed commit), but the merge method is a live preference with no + // reason to be frozen. (#2131) + /* v8 ignore next -- getRepositorySettings always resolves autoMaintain via its own default policy; this + * guard exists only because RepositorySettings' type allows autoMaintain to be undefined. */ + if (settings.autoMaintain?.mergeMethod) { + liveParams = { ...pending.params, mergeMethod: settings.autoMaintain.mergeMethod }; + } + } + + // Re-apply the SAME merge/close precision circuit-breakers the live webhook path applies before executing, so + // a breaker engaged AFTER staging (an operator halting a runaway auto-merge, or the auto-tuner tripping on a + // precision drop) still holds this sticky pending row instead of executing it unmodified. (#2127) + const [holdOnly, closeHoldOnly] = await Promise.all([isHoldOnly(env, pending.repoFullName), isCloseHoldOnly(env, pending.repoFullName)]); + let plan: PlannedAgentAction[] = [pendingActionToPlanned({ actionClass: pending.actionClass, params: liveParams, reason: pending.reason })]; + if (holdOnly) plan = downgradeMergeToHold(plan, true); + if (closeHoldOnly) plan = downgradeCloseToHold(plan, true); + const outcomes = await executeAgentMaintenanceActions( env, { @@ -67,7 +124,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de agentDryRun: settings.agentDryRun, installationPermissions: installation ? installation.permissions : null, }, - [pendingActionToPlanned({ actionClass: pending.actionClass, params: pending.params, reason: pending.reason })], + plan, ); /* v8 ignore next -- the executor returns one outcome per planned action, so the fallback is defensive. */ const execOutcome = outcomes[0]?.outcome ?? "no_outcome"; diff --git a/src/types.ts b/src/types.ts index fee2d4e0b8..d1719a72f0 100644 --- a/src/types.ts +++ b/src/types.ts @@ -683,6 +683,9 @@ export type AgentPendingActionParams = { mergeMethod?: AutoMergeMethod; closeComment?: string; expectedHeadSha?: string; + // WHICH kind of close this is (see PlannedAgentAction.closeKind) — must round-trip through staging so the + // close-precision circuit-breaker can still scope itself correctly when a staged close is later accepted (#2127). + closeKind?: "linked-issue-hard-rule" | "blacklist" | "heuristic"; }; export type AgentPendingActionStatus = "pending" | "accepted" | "rejected"; diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 49965109fb..1002771a6c 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -20,9 +20,23 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => { })), }; }); +vi.mock("../../src/github/app", async (importOriginal) => ({ + ...(await importOriginal()), + createInstallationToken: vi.fn(async () => "test-installation-token"), +})); +// The accept-time live re-check (#2126) defaults to "everything still looks fine" so the existing accept tests +// stay deterministic; individual tests below override these to exercise the staleness-supersede path. +vi.mock("../../src/github/backfill", async (importOriginal) => ({ + ...(await importOriginal()), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] })), + fetchLivePullRequestMergeState: vi.fn(async () => "clean"), + fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), +})); import { mergePullRequest } from "../../src/github/pr-actions"; import { ensurePullRequestLabel } from "../../src/github/labels"; +import { createInstallationToken } from "../../src/github/app"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../../src/github/backfill"; import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, type AgentActionExecutionContext } from "../../src/services/agent-action-executor"; import { decidePendingAgentAction } from "../../src/services/agent-approval-queue"; import { @@ -156,6 +170,126 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); }); + it("accept supersedes a staged merge when live CI has since turned failed (no head move) (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + // Also exercise a best-effort-failed mergeable/review read (undefined) alongside the CI failure — the + // audit metadata's nullish fallback must not throw, and ciState alone is still sufficient to deny. + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce(undefined); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("live CI is now failing"); + }); + + it("accept supersedes a staged merge when the base now conflicts (mergeable_state: dirty) (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty"); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("accept supersedes a staged merge when a reviewer has since requested changes (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchLivePullRequestReviewDecision).mockResolvedValueOnce("CHANGES_REQUESTED"); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("accept re-syncs the merge method to the CURRENT repo config, not the staging-time snapshot (#2131)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + // Staged while the default was "squash"; the maintainer has since changed the repo's default to "merge". + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" }, autoMaintain: { mergeMethod: "merge", requireApprovals: 0 } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "merge", sha: "h7" }); + }); + + it("accept downgrades a staged merge to a needs-human-review label when the precision breaker engaged after staging (#2127)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval", label: "auto" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + // The merge-precision breaker engages fleet-wide AFTER this merge was staged. + await env.DB.prepare("INSERT INTO system_flags (key, value) VALUES (?, ?)").bind("holdonly:owner/repo", "true").run(); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(mergePullRequest).not.toHaveBeenCalled(); + expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 5, "owner/repo", 7, "gittensory:needs-human-review", { createMissingLabel: true }); + }); + + it("accept executes a staged merge normally when the precision breaker is off", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + + it("accept still executes when the live re-check's token mint fails — fails OPEN on that specific check (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(createInstallationToken).mockRejectedValueOnce(new Error("installation suspended")); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + // The live-recheck's own token mint failing does not block the accept — the executor mints its own token + // for the actual mutation independently, so a transient failure here fails open on THIS check specifically. + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + + it("accept downgrades a staged heuristic close to a needs-human-review label when the close breaker engaged (#2127)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval", label: "auto" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 8, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h8" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 8, installationId: 5, actionClass: "close", autonomyLevel: "auto_with_approval", params: { closeComment: "noise", closeKind: "heuristic" }, reason: "ci-failed" }); + await env.DB.prepare("INSERT INTO system_flags (key, value) VALUES (?, ?)").bind("closehold:owner/repo", "true").run(); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + const { closePullRequest } = await import("../../src/github/pr-actions"); + expect(closePullRequest).not.toHaveBeenCalled(); + expect(ensurePullRequestLabel).toHaveBeenCalledWith(env, 5, "owner/repo", 8, "gittensory:needs-human-review", { createMissingLabel: true }); + }); + it("accept does not supersede when the PR record is absent (no live head to compare) — proceeds to the executor", async () => { const env = createTestEnv({}); // No PR seeded → getPullRequest returns null → pr?.headSha is undefined, so the staleness guard is skipped @@ -235,6 +369,9 @@ describe("agent approval queue (#779)", () => { expect(actionParams({ actionClass: "request_changes", requiresApproval: false, reason: "x", reviewBody: "B" })).toEqual({ reviewBody: "B" }); expect(actionParams({ actionClass: "merge", requiresApproval: false, reason: "x", mergeMethod: "rebase" })).toEqual({ mergeMethod: "rebase" }); expect(actionParams({ actionClass: "close", requiresApproval: false, reason: "x", closeComment: "C" })).toEqual({ closeComment: "C" }); + // closeKind must round-trip through staging — without it the close-precision breaker could never match a + // staged close as heuristic on accept (#2127). + expect(actionParams({ actionClass: "close", requiresApproval: false, reason: "x", closeComment: "C", closeKind: "heuristic" })).toEqual({ closeComment: "C", closeKind: "heuristic" }); }); it("lists all pending actions unfiltered and stores a null reason when omitted", async () => { diff --git a/test/unit/mcp-automation-state.test.ts b/test/unit/mcp-automation-state.test.ts index 9d1fdd6457..de26912dcf 100644 --- a/test/unit/mcp-automation-state.test.ts +++ b/test/unit/mcp-automation-state.test.ts @@ -10,6 +10,15 @@ import { createTestEnv } from "../helpers/d1"; vi.mock("../../src/github/app", async (importOriginal) => ({ ...(await importOriginal()), getRepositoryCollaboratorPermission: vi.fn(), + createInstallationToken: vi.fn(async () => "test-installation-token"), +})); +// decidePendingAgentAction's accept-time live re-check (#2126) needs these off-network, deterministic here — the +// dedicated staleness-supersede test coverage lives in agent-approval-queue.test.ts, not this MCP-surface file. +vi.mock("../../src/github/backfill", async (importOriginal) => ({ + ...(await importOriginal()), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] })), + fetchLivePullRequestMergeState: vi.fn(async () => "clean"), + fetchLivePullRequestReviewDecision: vi.fn(async () => undefined), })); const mockedPermission = vi.mocked(getRepositoryCollaboratorPermission); From 112391df5b3400188e6f47ca8bd52502a689ba44 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 06:45:16 -0700 Subject: [PATCH 2/4] fix(agent-actions): fail open per-check on a live re-check rejection The three live re-checks (CI aggregate, mergeable state, review decision) were awaited via a bare Promise.all, so a transient rejection from any one of them threw out of decidePendingAgentAction instead of failing open on that specific check -- exactly the design this code's own comment describes, but Promise.all does not provide that isolation even though each function already catches its own fetch errors internally today (a future edit removing one of those internal catches would silently reintroduce a crash with no test to catch it). Switch to Promise.allSettled and treat a rejected settle as "nothing concerning found" for that check, matching each function's own already-established fail-open return value. --- src/services/agent-approval-queue.ts | 13 +++++-- test/unit/agent-approval-queue.test.ts | 48 ++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 71d61f61ca..799fc24856 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -69,13 +69,20 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de if (pending.actionClass === "merge" && pr?.headSha) { const token = await createInstallationToken(env, pending.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, pending.installationId); - const [ciAggregate, mergeableState, reviewDecision] = await Promise.all([ + // Promise.allSettled, not Promise.all: each live re-check is independently best-effort (per the comment + // above), so ONE transient rejection must fail open on that specific check, not throw the whole accept + // out of decidePendingAgentAction. A settled-rejected check is treated the same as "nothing concerning + // found" -- exactly what each function's own internal fail-safe catch already resolves to on success. + const [ciResult, mergeableResult, reviewResult] = await Promise.allSettled([ fetchLiveCiAggregate(env, pending.repoFullName, pr.headSha, token, undefined, admissionKey), fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey), fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey), ]); + const ciState = ciResult.status === "fulfilled" ? ciResult.value.ciState : "unverified"; + const mergeableState = mergeableResult.status === "fulfilled" ? mergeableResult.value : undefined; + const reviewDecision = reviewResult.status === "fulfilled" ? reviewResult.value : undefined; const staleReason = - ciAggregate.ciState === "failed" + ciState === "failed" ? "live CI is now failing" : mergeableState === "dirty" ? "the base branch now conflicts (mergeable_state: dirty)" @@ -90,7 +97,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de targetKey, outcome: "denied", detail: `superseded ${pending.actionClass}: ${staleReason} since staging`, - metadata: { ...baseMetadata, ciState: ciAggregate.ciState, mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null }, + metadata: { ...baseMetadata, ciState, mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null }, }); return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "stale_disposition" }; } diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 1002771a6c..62d5ea61dc 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -275,6 +275,54 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); }); + it("accept still executes when a live re-check ITSELF rejects — fails OPEN on that specific check, not the whole accept (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + // A bare Promise.all over the three live re-checks would throw the whole accept out on this single + // rejection; Promise.allSettled must isolate it to just the CI check. + vi.mocked(fetchLiveCiAggregate).mockRejectedValueOnce(new Error("GitHub API transient 502")); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("accepted"); + expect(result.executionOutcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + }); + + it("accept still supersedes on a genuine mergeable-state hit when a SIBLING live re-check rejects (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchLivePullRequestReviewDecision).mockRejectedValueOnce(new Error("GitHub API transient 502")); + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty"); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + const audit = await env.DB.prepare("select detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ detail: string }>(); + expect(audit?.detail).toContain("mergeable_state: dirty"); + }); + + it("accept still supersedes on a genuine CI-failed hit when the mergeable-state live re-check rejects (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchLivePullRequestMergeState).mockRejectedValueOnce(new Error("GitHub API transient 502")); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + it("accept downgrades a staged heuristic close to a needs-human-review label when the close breaker engaged (#2127)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "auto_with_approval", label: "auto" } }); From 5171e322bdb5bff44c732f60e398656583609703 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 11:43:55 -0700 Subject: [PATCH 3/4] fix(agent-actions): treat any non-passing live CI state as stale, not just failed An accept-time live CI recheck that fulfills with "pending" or "unverified" (rather than rejecting) previously fell through to the same non-blocking path as "passed", letting a staged merge execute on live CI that had moved off green without ever going red. Distinguish a genuinely non-passing FULFILLED read from a REJECTED one (fail-open, unchanged) instead of collapsing both into a single sentinel string. --- src/services/agent-approval-queue.ts | 11 +++++++---- test/unit/agent-approval-queue.test.ts | 21 ++++++++++++++++++++- test/unit/routes-agent-approval.test.ts | 11 +++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 799fc24856..d6cce6b7fa 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -78,12 +78,15 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de fetchLivePullRequestMergeState(env, pending.repoFullName, pending.pullNumber, token, admissionKey), fetchLivePullRequestReviewDecision(env, pending.repoFullName, pending.pullNumber, token, admissionKey), ]); - const ciState = ciResult.status === "fulfilled" ? ciResult.value.ciState : "unverified"; + // A REJECTED promise stays undefined (fail-open — the read itself failed, not a genuine CI signal); a + // FULFILLED promise reporting anything other than "passed" (failed, pending, or unverified) is a real, + // non-stale-tolerant signal that the staged merge's justification no longer holds (#2126). + const ciState = ciResult.status === "fulfilled" ? ciResult.value.ciState : undefined; const mergeableState = mergeableResult.status === "fulfilled" ? mergeableResult.value : undefined; const reviewDecision = reviewResult.status === "fulfilled" ? reviewResult.value : undefined; const staleReason = - ciState === "failed" - ? "live CI is now failing" + ciState !== undefined && ciState !== "passed" + ? `live CI is no longer passing (now: ${ciState})` : mergeableState === "dirty" ? "the base branch now conflicts (mergeable_state: dirty)" : reviewDecision === "CHANGES_REQUESTED" @@ -97,7 +100,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de targetKey, outcome: "denied", detail: `superseded ${pending.actionClass}: ${staleReason} since staging`, - metadata: { ...baseMetadata, ciState, mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null }, + metadata: { ...baseMetadata, ciState: ciState ?? null, mergeableState: mergeableState ?? null, reviewDecision: reviewDecision ?? null }, }); return { status: "rejected", action: { ...pending, status: "rejected", decidedBy: input.decidedBy }, executionOutcome: "stale_disposition" }; } diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 62d5ea61dc..06d57754ce 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -187,7 +187,26 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).not.toHaveBeenCalled(); const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); expect(audit?.outcome).toBe("denied"); - expect(audit?.detail).toContain("live CI is now failing"); + expect(audit?.detail).toContain("live CI is no longer passing (now: failed)"); + }); + + it("accept supersedes a staged merge when live CI has since turned pending, not just failed (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + // A FULFILLED "pending" read is a genuine non-passing signal — distinct from a REJECTED read (fail-open, + // covered by the "ITSELF rejects" test below), which must NOT supersede. + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "pending", hasPending: true, hasVisiblePending: true, failingDetails: [], nonRequiredFailingDetails: [] }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + const audit = await env.DB.prepare("select outcome, detail from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ outcome: string; detail: string }>(); + expect(audit?.outcome).toBe("denied"); + expect(audit?.detail).toContain("live CI is no longer passing (now: pending)"); }); it("accept supersedes a staged merge when the base now conflicts (mergeable_state: dirty) (#2126)", async () => { diff --git a/test/unit/routes-agent-approval.test.ts b/test/unit/routes-agent-approval.test.ts index b28f4df600..dbe5a125b3 100644 --- a/test/unit/routes-agent-approval.test.ts +++ b/test/unit/routes-agent-approval.test.ts @@ -20,6 +20,17 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => { })), }; }); +// Without this mock, an unconfigured GITHUB_APP_PRIVATE_KEY leaves the accept-time live-recheck token mint +// undefined; fetchLiveCiAggregate then FULFILLS with ciState "unverified" (not a rejection), which the +// accept-time staleness check (agent-approval-queue.ts) now treats as non-"passed" — a genuine stale signal, +// not a fail-open case. Default to "passed" so a staged-merge happy path isn't accidentally exercising that. +vi.mock("../../src/github/backfill", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] })), + }; +}); import { mergePullRequest } from "../../src/github/pr-actions"; import { createSessionForGitHubUser } from "../../src/auth/security"; From 5c6125d481e11fd29f51186ccbf72db16289bd12 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 11:48:02 -0700 Subject: [PATCH 4/4] test(agent-actions): cover the null-ciState metadata branch and the routes happy path routes-agent-approval.test.ts's accept happy path never mocked fetchLiveCiAggregate, so an unconfigured GITHUB_APP_PRIVATE_KEY left the token undefined and the live read fulfilled with ciState "unverified" - now a genuine stale signal instead of an accidentally-tolerated one. --- test/unit/agent-approval-queue.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 06d57754ce..183c4045bf 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -327,6 +327,24 @@ describe("agent approval queue (#779)", () => { expect(audit?.detail).toContain("mergeable_state: dirty"); }); + it("accept still supersedes on a genuine mergeable-state hit when the CI live re-check ITSELF rejects — audits a null ciState, not the rejection's own value (#2126)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } }); + await seedInstallation(env); + await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "h7" }, labels: [], body: "x" }); + const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, installationId: 5, actionClass: "merge", autonomyLevel: "auto_with_approval", params: { mergeMethod: "squash", expectedHeadSha: "h7" }, reason: "clean" }); + vi.mocked(fetchLiveCiAggregate).mockRejectedValueOnce(new Error("GitHub API transient 502")); + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty"); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + expect(result.status).toBe("rejected"); + expect(result.executionOutcome).toBe("stale_disposition"); + expect(mergePullRequest).not.toHaveBeenCalled(); + const audit = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?").bind("agent.pending_action.superseded").first<{ detail: string; metadata_json: string }>(); + expect(audit?.detail).toContain("mergeable_state: dirty"); + expect(JSON.parse(audit?.metadata_json ?? "{}")).toMatchObject({ ciState: null }); + }); + it("accept still supersedes on a genuine CI-failed hit when the mergeable-state live re-check rejects (#2126)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { merge: "auto_with_approval" } });