From fed1787730af5442373d46269cb124714922e9fe Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 05:14:54 -0700 Subject: [PATCH 1/2] fix(github): close the remaining CI-status re-fetch gap outside the shared live-facts cache Closes #2539. The staged-merge approval-queue acceptance path (decidePendingAgentAction's own #2126 re-check) and the auto-maintain action executor's pre-mutation re-check (#2128) both call fetchLiveCiAggregate independently, moments apart in the SAME synchronous accept pass, using the identical unfiltered fetchLiveCiAggregate(..., requiredContexts: undefined, ...) shape -- a genuine, safe-to-coalesce duplicate fetch for the exact same question about the exact same commit. Thread the accept-time read forward as an optional prefetchedLiveCi on AgentActionExecutionContext, keyed by headSha: the executor's own re-check reuses it only when the headSha matches exactly, otherwise it fetches fresh -- identical to today's behavior for every other caller. Scope note: processors.ts's readiness/planner/post-publish call sites already share a DIFFERENT, required-context-FILTERED live-CI cache (#1941). That cache is deliberately NOT threaded into this fix -- reusing a filtered aggregate for this unfiltered fold-all re-check would silently loosen the merge-safety gate for repos with required-status-checks configured. The two call sites this PR wires together are the only pair that share both timing (same synchronous pass) and semantics (unfiltered). --- src/services/agent-action-executor.ts | 25 ++++++++-- src/services/agent-approval-queue.ts | 11 ++++- test/unit/agent-action-executor.test.ts | 61 +++++++++++++++++++++++++ test/unit/agent-approval-queue.test.ts | 51 +++++++++++++++++++++ 4 files changed, 143 insertions(+), 5 deletions(-) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 50dc84f2a5..176687ed2d 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -2,7 +2,7 @@ import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, insertNo import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; -import { fetchLiveCiAggregate, refreshInstallationHealthForInstallation } from "../github/backfill"; +import { fetchLiveCiAggregate, refreshInstallationHealthForInstallation, type LiveCiAggregate } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions"; @@ -47,6 +47,15 @@ export type AgentActionExecutionContext = { installationPermissions: Record | null | undefined; // PR author login — surfaced as the "Submitter" in the per-repo Discord action notification. authorLogin?: string | null | undefined; + // #2539: an UNFILTERED live CI aggregate (same fetchLiveCiAggregate(..., requiredContexts: undefined, ...) + // shape this executor's own pre-mutation re-check below uses) the CALLER already fetched moments earlier in + // the SAME synchronous accept/execute pass — currently only decidePendingAgentAction's own merge re-check. + // Reused ONLY when its headSha exactly matches the action being executed; this is a same-pass coalescing + // shortcut, never a staleness-tolerant cache — a mismatched or absent headSha always falls back to a fresh + // fetch. Do NOT populate this from a required-context-FILTERED aggregate (e.g. processors.ts's + // refreshLiveCiAggregate/cachedLiveCiAggregate) — that would silently loosen this re-check's fold-all + // guarantee for repos with required-status-checks configured. + prefetchedLiveCi?: { headSha: string; aggregate: LiveCiAggregate } | undefined; }; export type AgentActionOutcome = { @@ -145,9 +154,17 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // 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); + // #2539: reuse the caller's already-fetched (moments earlier, SAME pass, matching unfiltered semantics) + // aggregate when its headSha still matches this exact action — a genuine same-instant coalesce, not a + // staleness shortcut (a mismatched headSha, e.g. a different action in this same plan, always re-fetches). + const liveCi = + ctx.prefetchedLiveCi && ctx.prefetchedLiveCi.headSha === expectedHeadSha + ? ctx.prefetchedLiveCi.aggregate + : await (async () => { + const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined); + const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId); + return 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 diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 6a05efdb98..8f2dffd3f3 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -5,7 +5,7 @@ import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent- import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, type LiveCiAggregate } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -174,6 +174,13 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // 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; + // #2539: this accept-time re-check and the executor's own pre-mutation re-check (agent-action-executor.ts) ask + // the IDENTICAL question -- "is live CI still passed for this exact headSha" -- moments apart in this same + // synchronous accept pass, using the SAME unfiltered fetchLiveCiAggregate(..., undefined, ...) shape. Capture a + // fulfilled read here so the executor call below can reuse it instead of re-fetching. Left undefined on a + // rejected/skipped read (non-merge action, or this block never ran) — the executor then just fetches fresh, + // identical to today's behavior. + let prefetchedLiveCi: { headSha: string; aggregate: LiveCiAggregate } | undefined; if (pending.actionClass === "merge" && pr?.headSha) { const token = await createInstallationToken(env, pending.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, pending.installationId); @@ -186,6 +193,7 @@ 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), ]); + if (ciResult.status === "fulfilled") prefetchedLiveCi = { headSha: pr.headSha, aggregate: ciResult.value }; // 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). @@ -293,6 +301,7 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions: installation ? installation.permissions : null, + prefetchedLiveCi, }, plan, ); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index ecefb3d9a7..ac03dabba0 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -317,6 +317,67 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); }); + describe("prefetchedLiveCi coalescing (#2539)", () => { + it("reuses a matching-headSha prefetch for a merge instead of calling fetchLiveCiAggregate again", async () => { + const env = createTestEnv({}); + const outcomes = await executeAgentMaintenanceActions( + env, + ctx({ prefetchedLiveCi: { headSha: "sha7", aggregate: { ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), + [merge], + ); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); + expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); + }); + + it("a stale-failing prefetch still denies the merge — the coalescing shortcut carries the SAME staleness signal a fresh fetch would", async () => { + const env = createTestEnv({}); + const outcomes = await executeAgentMaintenanceActions( + env, + ctx({ prefetchedLiveCi: { headSha: "sha7", aggregate: { ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), + [merge], + ); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: failed)"); + expect(mergePullRequest).not.toHaveBeenCalled(); + expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); + }); + + it("REGRESSION: a prefetch for a DIFFERENT headSha is ignored — always falls back to a fresh fetch, never silently acts on stale-SHA CI data", async () => { + const env = createTestEnv({}); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + const outcomes = await executeAgentMaintenanceActions( + env, + ctx({ prefetchedLiveCi: { headSha: "some-other-sha", aggregate: { ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), + [merge], + ); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); + expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(1); + }); + + it("reuses the prefetch for a CI-driven heuristic close too, not just merge", async () => { + const env = createTestEnv({}); + const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; + const outcomes = await executeAgentMaintenanceActions( + env, + ctx({ prefetchedLiveCi: { headSha: "sha7", aggregate: { ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), + [heuristicClose], + ); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); + }); + + it("no prefetch (default ctx()) behaves exactly as before — always fetches fresh", async () => { + const env = createTestEnv({}); + vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(1); + }); + }); + 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 77ec0cf6af..feb095a792 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -535,6 +535,57 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "merge", sha: "h7" }); }); + describe("prefetchedLiveCi coalescing (#2539)", () => { + it("a successful staged-merge accept fetches live CI exactly ONCE, not twice — the accept re-check's own read is reused by the executor's pre-mutation re-check", 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(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(1); + }); + + it("a rejected (fail-open) accept-time CI read leaves the executor to fetch its OWN fresh value, still exactly once total", 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" }); + // The accept-time read itself fails (fail-open, per #2126) -- prefetchedLiveCi stays undefined, so the + // executor's pre-mutation re-check must fall back to its OWN fresh fetch (the second mock resolution). + vi.mocked(fetchLiveCiAggregate) + .mockRejectedValueOnce(new Error("GitHub API transient 502")) + .mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); + + 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: "squash", sha: "h7" }); + expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(2); + }); + + it("a non-merge staged action (approve) never populates a prefetch — the executor's own CI re-check (if any) still runs independently", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); + await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "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: "approve", autonomyLevel: "auto_with_approval", params: { reviewBody: "lgtm", expectedHeadSha: "h7" }, reason: "gate passed" }); + + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); + + expect(result.status).toBe("accepted"); + expect(createPullRequestReview).toHaveBeenCalledWith(env, 5, "owner/repo", 7, "APPROVE", "lgtm", "h7"); + // approve is not a CI-gated action class in the executor's own re-check (#2128), so it never calls + // fetchLiveCiAggregate at all -- confirming the merge-only branch above never runs for other action classes. + expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); + }); + }); + 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" } }); From 722c7e2877cb58375e81030b5697daeead3ad138 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:18:21 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(github):=20revert=20the=20accept-time/p?= =?UTF-8?q?re-mutation=20CI=20coalescing=20=E2=80=94=20confirmed=20unsafe?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate's own AI review correctly flagged this: the "moments apart, same synchronous pass" assumption behind prefetchedLiveCi was wrong. Traced the actual call chain between decidePendingAgentAction's accept-time CI read and the executor's pre-mutation re-check and found real, non-trivial async work in between -- isHoldOnly/isCloseHoldOnly (DB reads), createInstallationToken + resolveLinkedIssueHardRule (a GitHub API call when hard rules are configured), and, inside the executor itself, fetchPullRequestFreshness (an unconditional live GitHub call) -- all running before the (previously coalesced) CI check. That is exactly the window #2128's pre-mutation re-check exists to catch a CI flip in; reusing the earlier read defeated the guard it was reusing. Reverts prefetchedLiveCi entirely: the field on AgentActionExecutionContext, the capture/thread-through in decidePendingAgentAction, and the reuse branch in executeAgentMaintenanceActions's pre-mutation check, which now always fetches fresh again, exactly as before #2539. Replaces the coalescing tests with one regression test asserting the correct (always-two-fetches) behavior, so this specific unsafe shortcut can't be silently reintroduced. #2539's other orphaned call site (the duplicate-sibling reconciliation / gate-override reads coalesced via cachedFetchLivePullRequestMergeState / cachedFetchLivePullRequestState, already shipped in #2537) remains correctly coalesced -- those are genuinely non-authoritative reads, not the merge/close actuation boundary this revert is about. --- src/services/agent-action-executor.ts | 25 ++-------- src/services/agent-approval-queue.ts | 11 +---- test/unit/agent-action-executor.test.ts | 61 ------------------------- test/unit/agent-approval-queue.test.ts | 57 ++++------------------- 4 files changed, 15 insertions(+), 139 deletions(-) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 176687ed2d..50dc84f2a5 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -2,7 +2,7 @@ import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, insertNo import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; -import { fetchLiveCiAggregate, refreshInstallationHealthForInstallation, type LiveCiAggregate } from "../github/backfill"; +import { fetchLiveCiAggregate, refreshInstallationHealthForInstallation } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; import { closeIssue, closePullRequest, createIssueComment, createPullRequestReview, dismissLatestBotApproval, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions"; @@ -47,15 +47,6 @@ export type AgentActionExecutionContext = { installationPermissions: Record | null | undefined; // PR author login — surfaced as the "Submitter" in the per-repo Discord action notification. authorLogin?: string | null | undefined; - // #2539: an UNFILTERED live CI aggregate (same fetchLiveCiAggregate(..., requiredContexts: undefined, ...) - // shape this executor's own pre-mutation re-check below uses) the CALLER already fetched moments earlier in - // the SAME synchronous accept/execute pass — currently only decidePendingAgentAction's own merge re-check. - // Reused ONLY when its headSha exactly matches the action being executed; this is a same-pass coalescing - // shortcut, never a staleness-tolerant cache — a mismatched or absent headSha always falls back to a fresh - // fetch. Do NOT populate this from a required-context-FILTERED aggregate (e.g. processors.ts's - // refreshLiveCiAggregate/cachedLiveCiAggregate) — that would silently loosen this re-check's fold-all - // guarantee for repos with required-status-checks configured. - prefetchedLiveCi?: { headSha: string; aggregate: LiveCiAggregate } | undefined; }; export type AgentActionOutcome = { @@ -154,17 +145,9 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // 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) { - // #2539: reuse the caller's already-fetched (moments earlier, SAME pass, matching unfiltered semantics) - // aggregate when its headSha still matches this exact action — a genuine same-instant coalesce, not a - // staleness shortcut (a mismatched headSha, e.g. a different action in this same plan, always re-fetches). - const liveCi = - ctx.prefetchedLiveCi && ctx.prefetchedLiveCi.headSha === expectedHeadSha - ? ctx.prefetchedLiveCi.aggregate - : await (async () => { - const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined); - const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId); - return fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, undefined, admissionKey); - })(); + 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 diff --git a/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 8f2dffd3f3..6a05efdb98 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -5,7 +5,7 @@ import { executeAgentMaintenanceActions, pendingActionToPlanned } from "./agent- import { downgradeCloseToHold, downgradeMergeToHold, isProtectedAutomationAuthor, type PlannedAgentAction } from "../settings/agent-actions"; import { findBlacklistEntry } from "../settings/contributor-blacklist"; import { isCloseHoldOnly, isHoldOnly } from "../review/outcomes-wire"; -import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, type LiveCiAggregate } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import type { AgentPendingActionParams, AgentPendingActionRecord } from "../types"; @@ -174,13 +174,6 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de // 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; - // #2539: this accept-time re-check and the executor's own pre-mutation re-check (agent-action-executor.ts) ask - // the IDENTICAL question -- "is live CI still passed for this exact headSha" -- moments apart in this same - // synchronous accept pass, using the SAME unfiltered fetchLiveCiAggregate(..., undefined, ...) shape. Capture a - // fulfilled read here so the executor call below can reuse it instead of re-fetching. Left undefined on a - // rejected/skipped read (non-merge action, or this block never ran) — the executor then just fetches fresh, - // identical to today's behavior. - let prefetchedLiveCi: { headSha: string; aggregate: LiveCiAggregate } | undefined; if (pending.actionClass === "merge" && pr?.headSha) { const token = await createInstallationToken(env, pending.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, pending.installationId); @@ -193,7 +186,6 @@ 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), ]); - if (ciResult.status === "fulfilled") prefetchedLiveCi = { headSha: pr.headSha, aggregate: ciResult.value }; // 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). @@ -301,7 +293,6 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions: installation ? installation.permissions : null, - prefetchedLiveCi, }, plan, ); diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index ac03dabba0..ecefb3d9a7 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -317,67 +317,6 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); }); - describe("prefetchedLiveCi coalescing (#2539)", () => { - it("reuses a matching-headSha prefetch for a merge instead of calling fetchLiveCiAggregate again", async () => { - const env = createTestEnv({}); - const outcomes = await executeAgentMaintenanceActions( - env, - ctx({ prefetchedLiveCi: { headSha: "sha7", aggregate: { ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), - [merge], - ); - expect(outcomes[0]?.outcome).toBe("completed"); - expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); - expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); - }); - - it("a stale-failing prefetch still denies the merge — the coalescing shortcut carries the SAME staleness signal a fresh fetch would", async () => { - const env = createTestEnv({}); - const outcomes = await executeAgentMaintenanceActions( - env, - ctx({ prefetchedLiveCi: { headSha: "sha7", aggregate: { ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), - [merge], - ); - expect(outcomes[0]?.outcome).toBe("denied"); - expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: failed)"); - expect(mergePullRequest).not.toHaveBeenCalled(); - expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); - }); - - it("REGRESSION: a prefetch for a DIFFERENT headSha is ignored — always falls back to a fresh fetch, never silently acts on stale-SHA CI data", async () => { - const env = createTestEnv({}); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); - const outcomes = await executeAgentMaintenanceActions( - env, - ctx({ prefetchedLiveCi: { headSha: "some-other-sha", aggregate: { ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), - [merge], - ); - expect(outcomes[0]?.outcome).toBe("completed"); - expect(mergePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7, { mergeMethod: "squash", sha: "sha7" }); - expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(1); - }); - - it("reuses the prefetch for a CI-driven heuristic close too, not just merge", async () => { - const env = createTestEnv({}); - const heuristicClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "CI failed", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "failed" }; - const outcomes = await executeAgentMaintenanceActions( - env, - ctx({ prefetchedLiveCi: { headSha: "sha7", aggregate: { ciState: "failed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null } } }), - [heuristicClose], - ); - expect(outcomes[0]?.outcome).toBe("completed"); - expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); - expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); - }); - - it("no prefetch (default ctx()) behaves exactly as before — always fetches fresh", async () => { - const env = createTestEnv({}); - vi.mocked(fetchLiveCiAggregate).mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); - const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); - expect(outcomes[0]?.outcome).toBe("completed"); - expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(1); - }); - }); - 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 feb095a792..cf55da40c8 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -535,55 +535,18 @@ describe("agent approval queue (#779)", () => { expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "merge", sha: "h7" }); }); - describe("prefetchedLiveCi coalescing (#2539)", () => { - it("a successful staged-merge accept fetches live CI exactly ONCE, not twice — the accept re-check's own read is reused by the executor's pre-mutation re-check", 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(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); - expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(1); - }); - - it("a rejected (fail-open) accept-time CI read leaves the executor to fetch its OWN fresh value, still exactly once total", 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" }); - // The accept-time read itself fails (fail-open, per #2126) -- prefetchedLiveCi stays undefined, so the - // executor's pre-mutation re-check must fall back to its OWN fresh fetch (the second mock resolution). - vi.mocked(fetchLiveCiAggregate) - .mockRejectedValueOnce(new Error("GitHub API transient 502")) - .mockResolvedValueOnce({ ciState: "passed", hasPending: false, hasVisiblePending: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); - - 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: "squash", sha: "h7" }); - expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(2); - }); - - it("a non-merge staged action (approve) never populates a prefetch — the executor's own CI re-check (if any) still runs independently", async () => { - const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: "x" }); - await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { approve: "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: "approve", autonomyLevel: "auto_with_approval", params: { reviewBody: "lgtm", expectedHeadSha: "h7" }, reason: "gate passed" }); + it("REGRESSION (#2539 evaluated, reverted): a successful staged-merge accept still fetches live CI TWICE — once for the #2126 accept-time re-check, once for the executor's own #2128 pre-mutation re-check. These must NOT be coalesced: real async work (isHoldOnly/isCloseHoldOnly DB reads, the linked-issue hard-rule resolution, and the executor's own fetchPullRequestFreshness call) runs between the two reads, so reusing the earlier one would let CI flip from passed to failed/pending in that window without the pre-mutation guard ever seeing it — exactly the staleness #2128 exists to catch.", 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" }); + const result = await decidePendingAgentAction(env, { id: action.id, decision: "accept", decidedBy: "owner" }); - expect(result.status).toBe("accepted"); - expect(createPullRequestReview).toHaveBeenCalledWith(env, 5, "owner/repo", 7, "APPROVE", "lgtm", "h7"); - // approve is not a CI-gated action class in the executor's own re-check (#2128), so it never calls - // fetchLiveCiAggregate at all -- confirming the merge-only branch above never runs for other action classes. - expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); - }); + expect(result.status).toBe("accepted"); + expect(mergePullRequest).toHaveBeenCalledWith(env, 5, "owner/repo", 7, { mergeMethod: "squash", sha: "h7" }); + expect(fetchLiveCiAggregate).toHaveBeenCalledTimes(2); }); it("accept downgrades a staged merge to a needs-human-review label when the precision breaker engaged after staging (#2127)", async () => {