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..d6cce6b7fa 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,68 @@ 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); + // 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), + ]); + // 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 !== undefined && ciState !== "passed" + ? `live CI is no longer passing (now: ${ciState})` + : 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: ciState ?? null, 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 +134,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..183c4045bf 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,211 @@ 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 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 () => { + 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 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 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" } }); + 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" } }); + 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 +454,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 b2f76c9c5a..87f9d5d51d 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); 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";