diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index c8af2f6c7d..b33abdd18b 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -1,6 +1,9 @@ import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, insertNotificationDeliveryIfAbsent, isGlobalAgentFrozen, markPullRequestApproved, markPullRequestMergeBlocked, recordAuditEvent } from "../db/repositories"; import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; +import { createInstallationToken } from "../github/app"; +import { fetchLiveCiAggregate } from "../github/backfill"; +import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; import { closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions"; import { fetchPullRequestFreshness, pullRequestFreshnessDetail } from "../github/pr-freshness"; @@ -114,12 +117,42 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE await audit("denied", `${pullRequestFreshnessDetail(freshness)} — action not executed`); continue; } - // 6) Write-permission readiness: a PR-write action needs `pull_requests: write` granted. + // 6) Live CI re-verification for a merge or a 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 guard + // above only re-checks head SHA/state, not CI. GitHub's own merge endpoint enforces branch-protection + // REQUIRED checks server-side, but only as a backstop when a repo actually configures them; a heuristic + // close has no server-side check at all. Re-read live CI right before the mutation so a check that + // flipped in this narrow window is never acted on from stale information. Deterministic closes + // (linked-issue hard-rule, blacklist) are exempt — they are zero-hallucination facts that do not depend + // on CI, and the linked-issue rule already has its own flag-then-verify pass. + if (action.actionClass === "merge" || (action.actionClass === "close" && action.closeKind === "heuristic")) { + const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined); + const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId); + const liveCi = await fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, undefined, admissionKey); + // The planner itself only ever stages a merge when ciState === "passed" exactly (reviewGood in + // agent-actions.ts; "pending" short-circuits to no actions at all upstream) -- the live re-check must + // require the SAME exact state, not just "not failed". Otherwise a check that regressed to pending or + // became unreadable (unverified) between planning and actuation would still merge, on the assumption + // that only an explicit failure invalidates the plan. + const staleReason = + action.actionClass === "merge" + ? liveCi.ciState !== "passed" + ? `live CI is no longer passing (now: ${liveCi.ciState})` + : null + : liveCi.ciState !== "failed" + ? `CI state changed since planning (now: ${liveCi.ciState})` + : null; + if (staleReason) { + await audit("denied", `${staleReason} — action not executed`); + continue; + } + } + // 7) Write-permission readiness: a PR-write action needs `pull_requests: write` granted. if (PR_WRITE_CLASSES.has(action.actionClass) && resolveAgentPermissionReadiness({ autonomy: ctx.autonomy, installationPermissions: ctx.installationPermissions }) !== "ready") { await audit("denied", "pull_requests: write not granted — maintainer must re-consent"); continue; } - // 7) live — perform the real mutation, recording success or the error. + // 8) live — perform the real mutation, recording success or the error. try { await performAction(env, ctx, action); await audit("completed", action.reason); @@ -241,7 +274,9 @@ export function actionParams(action: PlannedAgentAction): AgentPendingActionPara ...(action.dismissStaleApproval !== undefined ? { dismissStaleApproval: action.dismissStaleApproval } : {}), // 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). + // staged close, silently defeating the breaker for the entire approval-queue accept path (#2127), and the + // actuation-time live-CI re-check above (#2364) — which only applies to a heuristic close — would be + // silently skipped for a lost discriminator. ...(action.closeKind !== undefined ? { closeKind: action.closeKind } : {}), }; } diff --git a/src/types.ts b/src/types.ts index 5ad07488c1..890d950607 100644 --- a/src/types.ts +++ b/src/types.ts @@ -682,14 +682,17 @@ export type AgentPendingActionParams = { reviewBody?: string; mergeMethod?: AutoMergeMethod; closeComment?: string; + // Which kind of close this is (see PlannedAgentAction.closeKind), persisted so it round-trips through staging: + // the close-precision circuit-breaker still scopes itself correctly when a staged close is later accepted + // (#2127), and the actuation-time live-CI re-check (#2364) — which only applies to a heuristic close — still + // fires correctly once the row is replayed through pendingActionToPlanned, rather than silently skipping for + // a lost discriminator. + closeKind?: "linked-issue-hard-rule" | "blacklist" | "heuristic"; expectedHeadSha?: string; // For an `approve` action: retract the bot's own stale approval instead of posting a new one (see // PlannedAgentAction.dismissStaleApproval). Must round-trip through staging like every other action-specific // field. (#2254) dismissStaleApproval?: boolean; - // 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-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 375dfcb527..dfc2feb4ce 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -23,11 +23,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 actuation-time live CI re-check (#2128) defaults to "still passing" so the existing merge tests stay +// deterministic; individual tests below override this to exercise the staleness-denial path. +vi.mock("../../src/github/backfill", async (importOriginal) => ({ + ...(await importOriginal()), + fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] })), +})); import { closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../../src/github/pr-actions"; import { ensurePullRequestLabel, removePullRequestLabel } from "../../src/github/labels"; import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; -import { actionParams, executeAgentMaintenanceActions, pendingClosureLabelApplied, type AgentActionExecutionContext, type AgentActionOutcome } from "../../src/services/agent-action-executor"; +import { createInstallationToken } from "../../src/github/app"; +import { fetchLiveCiAggregate } from "../../src/github/backfill"; +import { actionParams, executeAgentMaintenanceActions, pendingActionToPlanned, pendingClosureLabelApplied, type AgentActionExecutionContext, type AgentActionOutcome } from "../../src/services/agent-action-executor"; import type { PlannedAgentAction } from "../../src/settings/agent-actions"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; import { isGlobalAgentFrozen, setGlobalAgentFrozen, upsertPullRequestFromGitHub } from "../../src/db/repositories"; @@ -157,6 +169,84 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(fetchPullRequestFreshness).toHaveBeenCalledWith(env, expect.objectContaining({ expectedHeadSha: "reviewed-sha" })); }); + 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" }; + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [heuristicClose]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("CI state changed since planning (now: passed)"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + + it("LIVE heuristic close proceeds when live CI is still failing (#2128)", async () => { + const env = createTestEnv({}); + const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" }; + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [heuristicClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("REGRESSION (#2364): a queued heuristic close still re-checks live CI after the approval-queue replay round trip", async () => { + const env = createTestEnv({}); + const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" }; + // Simulate the persist/replay path: stageForApproval calls actionParams() to persist the row, and accept + // rebuilds it via pendingActionToPlanned(). Without persisting closeKind, the rebuilt action would lose the + // discriminator the live-CI re-check keys on, silently skipping it for every accepted queued heuristic close. + const persisted = actionParams(heuristicClose); + const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: heuristicClose.reason }); + expect(replayed.closeKind).toBe("heuristic"); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [replayed]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("CI state changed since planning (now: passed)"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + + it("LIVE non-heuristic close (linked-issue hard-rule) skips the live CI re-check entirely (#2128)", async () => { + const env = createTestEnv({}); + const hardRuleClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "unlinked issue", closeComment: "closing", closeKind: "linked-issue-hard-rule" }; + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [hardRuleClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); + }); + + it("LIVE merge is denied when live CI has since turned failing (#2128)", async () => { + const env = createTestEnv({}); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: failed)"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#2364): LIVE merge is denied when live CI has since become pending, not just failed", async () => { + const env = createTestEnv({}); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "pending", hasPending: true, hasVisiblePending: true, failingDetails: [], nonRequiredFailingDetails: [] }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: pending)"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#2364): LIVE merge is denied when live CI has since become unverified (unreadable), not just failed", async () => { + const env = createTestEnv({}); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "unverified", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: unverified)"); + expect(mergePullRequest).not.toHaveBeenCalled(); + }); + + it("the live CI re-check fails open on a token-mint error — it is defense-in-depth, not the primary gate (#2128)", async () => { + const env = createTestEnv({}); + vi.mocked(createInstallationToken).mockRejectedValueOnce(new Error("mint failed")); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); + }); + it("LIVE label with labelOp=add + comment: adds the label AND posts the comment", async () => { const env = createTestEnv({}); const flag: PlannedAgentAction = { actionClass: "label", requiresApproval: false, reason: "flag", label: "gittensory:pending-closure", labelOp: "add", comment: "⚠️ flagged" }; diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 183c4045bf..929f157401 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -24,8 +24,9 @@ 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. +// The accept-time live re-check (#2126) AND the actuation-time live CI re-check (#2128) both default to +// "everything still looks fine" so the existing accept tests stay deterministic; individual tests below +// override these to exercise the staleness-supersede / staleness-denial paths. vi.mock("../../src/github/backfill", async (importOriginal) => ({ ...(await importOriginal()), fetchLiveCiAggregate: vi.fn(async () => ({ ciState: "passed" as const, hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [] })), diff --git a/test/unit/routes-agent-approval.test.ts b/test/unit/routes-agent-approval.test.ts index dbe5a125b3..30337daab2 100644 --- a/test/unit/routes-agent-approval.test.ts +++ b/test/unit/routes-agent-approval.test.ts @@ -23,7 +23,9 @@ 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. +// not a fail-open case. #2364's live CI re-check (in executeAgentMaintenanceActions) runs for every +// merge/heuristic-close accept too. Default to "passed" so the existing "accept executes the staged merge" +// happy path executes live instead of being (correctly) denied for CI neither test was simulating. vi.mock("../../src/github/backfill", async (importOriginal) => { const actual = await importOriginal(); return {