diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 0292862177..3301aa3388 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -16,7 +16,7 @@ import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; -import { fetchLiveCiAggregate, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, mergeRequiredCiContexts, refreshInstallationHealthForInstallation } from "../github/backfill"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { ensurePullRequestAssignee } from "../github/assignees"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; @@ -35,6 +35,7 @@ import { type ModerationRuleType, } from "../settings/moderation-rules"; import { incr } from "../selfhost/metrics"; +import { captureError } from "../selfhost/sentry"; // The agent actor name on every audit record — the App acts on the maintainer's behalf per their configured // autonomy (the config IS the authorization; there is no human commenter to authorize, unlike #824). @@ -363,8 +364,8 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // 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. + // (gate verdict, duplicate/slop, 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 @@ -372,28 +373,48 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // 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 requiresLiveCiRecheck = action.actionClass === "merge" || (action.actionClass === "close" && action.closeRequiresCiState === "failed") || isAmbiguousLegacyHeuristicClose; + // #3863: a base-conflict-justified heuristic close (closeRequiresMergeableState === true) is read from the + // SAME planning-pass snapshot as the CI check above -- an unrelated PR merging into the base branch during + // a slow review pass (AI review, gate evaluation) can clear the conflict before this mutation runs, and + // nothing re-verified it right before acting. The approval-queue's accept-time path already does this SAME + // live re-check for a STAGED close (agent-approval-queue.ts); this is the immediate, same-pass execution + // path, which had no equivalent. + const requiresLiveMergeableRecheck = action.actionClass === "close" && action.closeKind === "heuristic" && action.closeRequiresMergeableState === true; + if (requiresLiveCiRecheck || requiresLiveMergeableRecheck) { const ciToken = await createInstallationToken(env, ctx.installationId).catch(() => undefined); const admissionKey = githubRateLimitAdmissionKeyForToken(env, ciToken, ctx.installationId); // mergeRequiredCiContexts(null, ...) -- no live branch-protection re-fetch here, just the maintainer's own // configured expectedCiContexts (or null/fold-all when unset), matching the "no branch protection" arm of // the planning pass's own merge (mergeRequiredCiContexts is pure and already exported for that call site). - const liveCi = await fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, mergeRequiredCiContexts(null, ctx.expectedCiContexts), admissionKey); + const [liveCi, liveMergeableState] = await Promise.all([ + requiresLiveCiRecheck + ? fetchLiveCiAggregate(env, ctx.repoFullName, expectedHeadSha, ciToken, mergeRequiredCiContexts(null, ctx.expectedCiContexts), admissionKey) + : Promise.resolve(undefined), + requiresLiveMergeableRecheck ? fetchLivePullRequestMergeState(env, ctx.repoFullName, ctx.pullNumber, ciToken, admissionKey) : Promise.resolve(undefined), + ]); // 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})` + const ciStaleReason = !requiresLiveCiRecheck + ? null + : action.actionClass === "merge" + ? liveCi!.ciState !== "passed" + ? `live CI is no longer passing (now: ${liveCi!.ciState})` : null // 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})` + : liveCi!.ciState !== (action.closeRequiresCiState ?? "failed") + ? `CI state changed since planning (now: ${liveCi!.ciState})` : null; + // Only a CONFIRMED "clean" clears a conflict-justified close -- an ambiguous/unresolvable live read + // (unknown, unstable, blocked, or a failed fetch, which resolves to undefined) is not proof the conflict + // resolved, matching the approval-queue's own fail-safe-toward-keeping-the-close precedent (#3863). + const mergeableStaleReason = + requiresLiveMergeableRecheck && liveMergeableState === "clean" ? "the base-branch conflict that justified this close has since cleared" : null; + const staleReason = ciStaleReason ?? mergeableStaleReason; if (staleReason) { await audit("denied", `${staleReason} — action not executed`); continue; @@ -435,6 +456,14 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE // after the gate publishes. A possibly-transient failure is retried up to MERGE_RETRY_CAP, then held. if (action.actionClass === "merge" && ctx.headSha) { await handleMergeFailure(env, ctx, error); + } else { + // Non-merge action classes have no retry loop -- a single failure here is already this pass's terminal + // outcome (the planner may re-attempt on the next sweep if the underlying condition clears itself), so + // it is captured immediately rather than only on eventual exhaustion. Mirrors handleMergeFailure's own + // terminal-hold capture below and the "a real failure the maintainer must see" convention already used + // for review-pass failures (selfhost/sentry.ts's captureReviewFailure, queue/processors.ts). Previously + // this class of failure was audit-log-only, invisible without a manual audit_events query. + captureError(error, { kind: "agent_action_execution_failed", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, actionClass: action.actionClass }); } // #2265: a permission-looking 403 on a PR-write mutation can mean the LOCAL installations.permissions // snapshot is stale after a maintainer-initiated downgrade (GitHub sends no downgrade webhook). Rate-limit @@ -675,6 +704,9 @@ export async function executeIssueMaintenanceActions(env: Env, ctx: IssueActionE await audit("completed", action.reason); } catch (error) { await audit("error", errorMessage(error)); + // Mirrors executeAgentMaintenanceActions's non-merge capture below -- issue-side label/close has no retry + // loop either, so a single failure here is already this pass's terminal outcome. + captureError(error, { kind: "agent_issue_action_execution_failed", repo: ctx.repoFullName, issue: ctx.issueNumber, installationId: ctx.installationId, actionClass: action.actionClass }); } } @@ -703,6 +735,11 @@ async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, er } if (!terminal) return; await markPullRequestMergeBlocked(env, ctx.repoFullName, ctx.pullNumber, headSha, reason); + // A merge held for a human is the terminal outcome of this whole retry sequence -- exactly the "a real + // failure the maintainer must see" case captureReviewFailure already covers for an exhausted AI review pass. + // Fires once per hold (not per retry attempt), so a transient failure that resolves within MERGE_RETRY_CAP + // never reaches Sentry at all. + captureError(error, { kind: "agent_merge_blocked", repo: ctx.repoFullName, pr: ctx.pullNumber, installationId: ctx.installationId, reason: reason.slice(0, 280) }); await recordAuditEvent(env, { eventType: "agent.action.merge_blocked", actor: AGENT_ACTOR, diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index f95352c26c..570a7b58a2 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -34,9 +34,12 @@ vi.mock("../../src/github/app", async (importOriginal) => ({ })); // 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. +// The actuation-time live mergeable-state re-check (#3863) defaults to "dirty" (conflict still present) so no +// existing test needs to override it; the tests below explicitly set it 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, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null })), + fetchLivePullRequestMergeState: vi.fn(async () => "dirty" as const), refreshInstallationHealthForInstallation: vi.fn(async () => null), })); @@ -45,7 +48,7 @@ import { ensurePullRequestLabel, removePullRequestLabel } from "../../src/github import { ensurePullRequestAssignee } from "../../src/github/assignees"; import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; import { createInstallationToken } from "../../src/github/app"; -import { fetchLiveCiAggregate, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; +import { fetchLiveCiAggregate, fetchLivePullRequestMergeState, refreshInstallationHealthForInstallation } from "../../src/github/backfill"; import { actionParams, applyModerationEscalationForRule, @@ -65,6 +68,7 @@ import { STRUCTURED_CLOSE_REASONS_MAX_COUNT } from "../../src/settings/agent-exe import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; import { clearProcessLocalGlobalAgentFrozenCacheForTest, getGlobalContributorBlacklist, isGlobalAgentFrozen, setGlobalAgentFrozen, upsertGlobalModerationConfig, upsertPullRequestFromGitHub } from "../../src/db/repositories"; import * as repositoriesModule from "../../src/db/repositories"; +import * as sentryModule from "../../src/selfhost/sentry"; import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; import { createTestEnv } from "../helpers/d1"; import { MODERATION_VIOLATION_EVENT_TYPE } from "../../src/settings/moderation-rules"; @@ -512,6 +516,56 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(fetchLiveCiAggregate).not.toHaveBeenCalled(); }); + it("REGRESSION (#3863): a base-conflict-justified heuristic close is DENIED when the live mergeable_state has since cleared", async () => { + const env = createTestEnv({}); + const conflictClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "conflicts with the base branch", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true }; + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("clean"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [conflictClose]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(outcomes[0]?.detail).toContain("the base-branch conflict that justified this close has since cleared"); + expect(closePullRequest).not.toHaveBeenCalled(); + }); + + it("a base-conflict-justified heuristic close proceeds when the live mergeable_state is still dirty (#3863)", async () => { + const env = createTestEnv({}); + const conflictClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "conflicts with the base branch", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true }; + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [conflictClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("a base-conflict-justified heuristic close fails open (still proceeds) when the live mergeable_state read is ambiguous/unresolved (#3863)", async () => { + const env = createTestEnv({}); + const conflictClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "conflicts with the base branch", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true }; + // "unknown" (still computing) and a failed fetch (undefined) are both NOT a confirmed "clean" -- neither is + // proof the conflict resolved, so the close must not be silently blocked by an inconclusive live read. + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("unknown"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [conflictClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(closePullRequest).toHaveBeenCalledWith(env, 123, "owner/repo", 7); + }); + + it("a non-conflict heuristic close (closeRequiresMergeableState omitted/false) skips the live mergeable-state re-check entirely (#3863)", async () => { + const env = createTestEnv({}); + const gateClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "policy gate blocker", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: false }; + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [gateClose]); + expect(outcomes[0]?.outcome).toBe("completed"); + expect(fetchLivePullRequestMergeState).not.toHaveBeenCalled(); + }); + + it("REGRESSION (#3863): closeRequiresMergeableState round-trips through the persist/replay round trip so a staged conflict close still re-checks live mergeable-state", async () => { + const env = createTestEnv({}); + const conflictClose: PlannedAgentAction = { actionClass: "close", requiresApproval: false, reason: "conflicts with the base branch", closeComment: "closing", closeKind: "heuristic", closeRequiresCiState: "not_required", closeRequiresMergeableState: true }; + const persisted = actionParams(conflictClose); + const replayed = pendingActionToPlanned({ actionClass: "close", params: persisted, reason: conflictClose.reason }); + expect(replayed.closeRequiresMergeableState).toBe(true); + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("clean"); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [replayed]); + expect(outcomes[0]?.outcome).toBe("denied"); + expect(closePullRequest).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, hasMissingRequiredContext: false, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null }); @@ -1072,16 +1126,22 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { it("records a failed mutation as error rather than swallowing it", async () => { const env = createTestEnv({}); vi.mocked(mergePullRequest).mockRejectedValueOnce(new Error("Pull Request is not mergeable")); + const captureSpy = vi.spyOn(sentryModule, "captureError"); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); expect(outcomes[0]?.outcome).toBe("error"); expect(outcomes[0]?.detail).toMatch(/not mergeable/i); expect((await auditFor(env, "merge"))?.outcome).toBe("error"); + // "not mergeable" is immediately terminal (classifyMergeFailure), so this held-for-human outcome must be + // Sentry-visible, not just an audit_events row a maintainer has to go looking for (#3862/#3863 gap sweep). + expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_merge_blocked", repo: "owner/repo", pr: 7 })); + captureSpy.mockRestore(); }); it("REGRESSION: a generic GitHub 403 merge rejection does not immediately pin merge_blocked_sha", async () => { const env = createTestEnv({}); await upsertPullRequestFromGitHub(env, "owner/repo", { number: 7, title: "PR", state: "open", user: { login: "c" }, head: { sha: "sha7" }, labels: [], body: "" }); vi.mocked(mergePullRequest).mockRejectedValueOnce(Object.assign(new Error("Resource not accessible by integration"), { status: 403 })); + const captureSpy = vi.spyOn(sentryModule, "captureError"); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]); @@ -1096,15 +1156,24 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { .bind("agent.action.merge_blocked") .first<{ count: number }>(); expect(blocked?.count).toBe(0); + // A retryable (non-terminal) failure must stay silent in Sentry -- only the eventual terminal hold (above) + // or MERGE_RETRY_CAP exhaustion should ever page anyone, or every transient 403 would be alert noise. + expect(captureSpy).not.toHaveBeenCalled(); + captureSpy.mockRestore(); }); it("opportunistically refreshes installation health when a PR-write mutation fails with a 403 (#2265)", async () => { const env = createTestEnv({}); vi.mocked(closePullRequest).mockRejectedValueOnce(Object.assign(new Error("Resource not accessible by integration"), { status: 403 })); + const captureSpy = vi.spyOn(sentryModule, "captureError"); const outcomes = await executeAgentMaintenanceActions(env, ctx(), [close]); expect(outcomes[0]?.outcome).toBe("error"); expect(refreshInstallationHealthForInstallation).toHaveBeenCalledTimes(1); expect(refreshInstallationHealthForInstallation).toHaveBeenCalledWith(env, 123); + // Non-merge action classes have no retry loop, so a single failure is already this pass's terminal outcome + // and must be Sentry-visible immediately (#3862/#3863 gap sweep). + expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_action_execution_failed", actionClass: "close" })); + captureSpy.mockRestore(); }); it("does not refresh installation health for a non-403 mutation failure (#2265)", async () => { @@ -1530,9 +1599,12 @@ describe("executeIssueMaintenanceActions (#2270 issue-side actuation)", () => { it("records a failed mutation as error rather than swallowing it", async () => { const env = createTestEnv({}); vi.mocked(closeIssue).mockRejectedValueOnce(new Error("github 500")); + const captureSpy = vi.spyOn(sentryModule, "captureError"); const outcomes = await executeIssueMaintenanceActions(env, issueCtx(), [issueClose]); expect(outcomes[0]?.outcome).toBe("error"); expect((await auditFor(env, "close"))?.outcome).toBe("error"); + expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_issue_action_execution_failed", actionClass: "close" })); + captureSpy.mockRestore(); }); // #terminal-outcome-audit: the issue-actions executor has its own `audit` closure (a separate function scope diff --git a/test/unit/agent-approval-queue.test.ts b/test/unit/agent-approval-queue.test.ts index be27ff48af..ac794c4831 100644 --- a/test/unit/agent-approval-queue.test.ts +++ b/test/unit/agent-approval-queue.test.ts @@ -754,7 +754,11 @@ describe("agent approval queue (#779)", () => { await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "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" }); - vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty"); + // Queues exactly 2 responses (Once, not a persistent mockResolvedValue): the accept-time recheck AND the + // executor's own actuation-time recheck (#3863) each consume one call and must see the SAME still-conflicting + // state for this test's premise to hold; an unconsumed persistent override would otherwise leak into later + // tests since this file's beforeEach only clearAllMocks (not resetAllMocks). + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty").mockResolvedValueOnce("dirty"); const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7, @@ -778,6 +782,10 @@ describe("agent approval queue (#779)", () => { await upsertRepositorySettings(env, { repoFullName: "owner/repo", autonomy: { close: "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" }); + // The conflict itself is still live (dirty) -- only the review-decision differs from the "cleared" test + // above. Queues exactly 2 responses (see the comment on the test above) so both the accept-time recheck and + // the executor's own actuation-time recheck (#3863) see a consistent "still conflicting" state. + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty").mockResolvedValueOnce("dirty"); vi.mocked(fetchLivePullRequestReviewDecision).mockResolvedValueOnce("CHANGES_REQUESTED"); const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", @@ -929,6 +937,10 @@ describe("agent approval queue (#779)", () => { // The live review-decision read itself FAILS (transient API error) -- this must fail open (not stale), // not be silently treated as "confirmed no changes requested" merely because the resolved value is undefined. vi.mocked(fetchLivePullRequestReviewDecision).mockRejectedValueOnce(new Error("GitHub API transient 502")); + // The conflict itself is still live (dirty) -- this test's premise (the close proceeds despite the + // review-decision read failing) needs it to hold. Queues exactly 2 responses (see the comment on the + // "when the live conflict signal remains" test above) for the accept-time and actuation-time (#3863) rechecks. + vi.mocked(fetchLivePullRequestMergeState).mockResolvedValueOnce("dirty").mockResolvedValueOnce("dirty"); const { action } = await createPendingAgentActionIfAbsent(env, { repoFullName: "owner/repo", pullNumber: 7,