diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index c63648eb0a..322f7b9242 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -129,15 +129,22 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE await audit("denied", `${pullRequestFreshnessDetail(freshness)} — action not executed`); continue; } - // 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")) { + // 6) 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 + // 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 red-CI 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. Non-CI closes + // (gate verdict, duplicate/slop, conflict, linked-issue hard-rule, blacklist) are exempt — their adverse + // signal does not depend on CI still being red. + // A heuristic close staged BEFORE #2478 has no closeRequiresCiState at all -- that field didn't exist yet + // -- so `undefined` here is genuinely ambiguous (a legacy CI-driven close and a legacy non-CI close are + // byte-identical in storage). The planner now ALWAYS sets the field going forward (never omits it), so + // `undefined` can only mean a legacy row; treat it with the old, broader pre-#2478 guard (require CI still + // failed) rather than skipping the recheck, which would let a stale CI-driven close silently execute + // after CI recovers (flagged by the gate's own review of #2478). + const isAmbiguousLegacyHeuristicClose = action.actionClass === "close" && action.closeKind === "heuristic" && action.closeRequiresCiState === undefined; + if (action.actionClass === "merge" || (action.actionClass === "close" && action.closeRequiresCiState === "failed") || isAmbiguousLegacyHeuristicClose) { 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); @@ -151,7 +158,9 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE ? liveCi.ciState !== "passed" ? `live CI is no longer passing (now: ${liveCi.ciState})` : null - : liveCi.ciState !== "failed" + // isAmbiguousLegacyHeuristicClose falls back to "failed" (the old unconditional requirement); an + // explicitly-tagged fresh close compares against its own recorded requirement. + : liveCi.ciState !== (action.closeRequiresCiState ?? "failed") ? `CI state changed since planning (now: ${liveCi.ciState})` : null; if (staleReason) { @@ -317,10 +326,11 @@ 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), 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. + // staged close, silently defeating the breaker for the entire approval-queue accept path (#2127). ...(action.closeKind !== undefined ? { closeKind: action.closeKind } : {}), + // Round-trip the CI dependency separately from closeKind: closeKind is intentionally broad (gate-verdict / + // duplicate / slop / CI) for the close-precision breaker, but only red-CI closes need the live-CI guard. + ...(action.closeRequiresCiState !== undefined ? { closeRequiresCiState: action.closeRequiresCiState } : {}), }; } diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 1c8b800147..01cefff08d 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -62,6 +62,11 @@ export type PlannedAgentAction = { // (silently holding a close whose comment already promised closure would be incoherent). Absent on non-close // actions; treated as a heuristic close only when explicitly tagged "heuristic". closeKind?: "linked-issue-hard-rule" | "blacklist" | "heuristic"; + // For a CI-driven heuristic close, the CI state that must still hold at actuation time. Other heuristic + // closes (gate verdict, duplicate/slop, conflict) do not depend on red CI and must not be blocked by green CI. + // ALWAYS set for a heuristic close (never omitted) -- see the field's doc comment on AgentPendingActionParams + // in types.ts for why the tri-state (rather than an optional "failed") matters (#2478). + closeRequiresCiState?: "failed" | "not_required"; expectedHeadSha?: string; // For an `approve` action: retract the bot's own prior approval instead of posting a new one — a later commit // no longer qualifies for approval, but the PR isn't merging or closing this pass, so the stale APPROVE @@ -559,6 +564,9 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // Pin like merge/approve (#2452): lets the accept-time supersede check detect a force-push after staging; // the executor's own step-6 live-CI re-check (#2128) separately covers the CI-driven reason above. ...(input.pr.headSha ? { expectedHeadSha: input.pr.headSha } : {}), + // Always explicit (never omitted) -- see the field's doc comment (#2478): an omitted value on a REPLAYED + // staged action must unambiguously mean "legacy row, predates this field", not "not CI-driven". + closeRequiresCiState: ciFailed ? "failed" : "not_required", }); } // else: guarded → manual (needs-human/changes label above); not-good OWNER/automation → held diff --git a/src/types.ts b/src/types.ts index 045f8340e4..62c5f4685f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -693,10 +693,13 @@ export type AgentPendingActionParams = { 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. + // (#2127). closeKind?: "linked-issue-hard-rule" | "blacklist" | "heuristic"; + // For a CI-driven heuristic close, persist the CI state that must still hold when the staged action replays + // (#2364). This is separate from closeKind because heuristic closes also cover non-CI adverse signals. + // ALWAYS set (to "failed" or "not_required") for a freshly planned heuristic close (#2478) -- never omitted -- + // so `undefined` unambiguously means a LEGACY row staged before this field existed, not "not CI-driven". + closeRequiresCiState?: "failed" | "not_required"; 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 diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index a987d0a7bb..6d72b7844d 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -197,7 +197,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { 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" }; + const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [heuristicClose]); expect(outcomes[0]?.outcome).toBe("denied"); @@ -207,7 +207,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { 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" }; + const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [heuristicClose]); expect(outcomes[0]?.outcome).toBe("completed"); @@ -216,13 +216,14 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { 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" }; + const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; // 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. + // rebuilds it via pendingActionToPlanned(). Persist both the broad close kind and the narrower CI + // dependency so queued red-CI closes still get the live-CI re-check without applying it to every heuristic close. const persisted = actionParams(heuristicClose); const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: heuristicClose.reason }); expect(replayed.closeKind).toBe("heuristic"); + expect(replayed.closeRequiresCiState).toBe("failed"); vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [replayed]); expect(outcomes[0]?.outcome).toBe("denied"); @@ -230,6 +231,39 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(closePullRequest).not.toHaveBeenCalled(); }); + it("LIVE non-CI heuristic close proceeds when live CI is passing because the close reason is independent of CI", async () => { + const env = createTestEnv({}); + // "not_required", not omitted: the planner always tags a fresh heuristic close explicitly (#2478). + const gateClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "policy gate blocker", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required" }; + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [gateClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#2478, flagged by the gate's own review of #2478): a LEGACY heuristic close staged before closeRequiresCiState existed (closeKind heuristic, field entirely absent) still re-checks live CI and is DENIED once CI has turned green", async () => { + // Simulates a pending_agent_actions row persisted by code that predates #2478 -- closeKind: "heuristic" with + // no closeRequiresCiState key at all, since the field didn't exist yet. The fix must NOT silently skip the + // live-CI recheck for this row just because the new "not_required" tag is absent, or a stale CI-driven close + // could execute after CI recovers. + const env = createTestEnv({}); + const legacyHeuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" }; + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [legacyHeuristicClose]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("CI state changed since planning (now: passed)"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + + it("a LEGACY heuristic close (closeRequiresCiState absent) still proceeds when live CI is genuinely still failing, matching the old pre-#2478 behavior", async () => { + const env = createTestEnv({}); + const legacyHeuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic" }; + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [legacyHeuristicClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + 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" }; diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index d56d787e92..c63128ef52 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -357,6 +357,9 @@ describe("planAgentMaintenanceActions (#778)", () => { const cls = classes(plan); expect(cls).not.toContain("merge"); expect(cls).toContain("close"); + // "not_required", not undefined -- the planner always tags a heuristic close explicitly (#2478) so a + // REPLAYED staged action can tell "not CI-driven" apart from "legacy row, field didn't exist yet". + expect(plan.find((a) => a.actionClass === "close")?.closeRequiresCiState).toBe("not_required"); expect(plan.find((a) => a.actionClass === "label")?.label).toBe(AGENT_LABEL_CHANGES); }); @@ -485,6 +488,7 @@ describe("planAgentMaintenanceActions (#778)", () => { expect(close).toBeTruthy(); expect(close?.reason).toContain("CI is failing"); expect(close?.reason).toContain("codecov/patch"); + expect(close?.closeRequiresCiState).toBe("failed"); }); it("NEVER closes the owner's red-CI PR — held via the changes-requested LABEL only (no blocking request_changes), left open", () => { diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index 4277bdf7e0..77ec0cf6af 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -886,7 +886,7 @@ describe("agent approval queue (#779)", () => { 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" }); + expect(actionParams({ actionClass: "close", requiresApproval: false, reason: "x", closeComment: "C", closeKind: "heuristic", closeRequiresCiState: "failed" })).toEqual({ closeComment: "C", closeKind: "heuristic", closeRequiresCiState: "failed" }); }); it("lists all pending actions unfiltered and stores a null reason when omitted", async () => {