From 640ffea5763efd37dc2647dfdd56bd791c5c8ffc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 19:25:10 -0700 Subject: [PATCH 1/2] fix(review): re-check live mergeable state before executing a conflict close MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #3863. A conflict-justified heuristic close (`closeRequiresMergeableState: true`) was executed against whatever mergeable state was current when the review pass planned the action, not what's current when it actually executes. If the base branch changes between planning and actuation (Gittensory's own review latency creates exactly this window), a PR that has since become mergeable again gets closed anyway for a conflict that no longer exists. The approval-queue's staged accept-flow (agent-approval-queue.ts) already re-checks live mergeable state before honoring a conflict-justified close, but the immediate, no-approval-required execution path in agent-action-executor.ts did not — its own doc comment explicitly listed "conflict" as exempt from live re-verification. Extend the existing live-CI recheck block to also fetch live mergeable state for this one case, and deny (not merely skip) the action if the conflict has since cleared. --- src/services/agent-action-executor.ts | 42 ++++++++++++++----- test/unit/agent-action-executor.test.ts | 55 ++++++++++++++++++++++++- test/unit/agent-approval-queue.test.ts | 14 ++++++- 3 files changed, 98 insertions(+), 13 deletions(-) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 0292862177..d46dcc8af0 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"; @@ -363,8 +363,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 +372,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; diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index f95352c26c..cf0f979001 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, @@ -512,6 +515,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 }); 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, From 0d9fd9a7702416e2b7b0b2d8bedad55ed574579e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 19:38:54 -0700 Subject: [PATCH 2/2] feat(review): capture terminal agent-action execution failures to Sentry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent-action executor (executeAgentMaintenanceActions, executeIssueMaintenanceActions) recorded every mutation failure to audit_events only — real GitHub-mutation failures (merge/close/approve/ label/etc.) had zero Sentry visibility, unlike the equivalent "a real failure the maintainer must see" convention already used for exhausted AI-review-pass failures (captureReviewFailure in queue/processors.ts). Capture at each already-terminal point, not on every attempt: a merge held for a human (handleMergeFailure's terminal branch, after retries are exhausted or an immediately-terminal classification), and any non-merge action class, which has no retry loop so a single failure is already this pass's terminal outcome. A retryable, non-terminal merge failure stays silent, matching how a review pass that succeeds on fallback never fires captureReviewFailure either. Found while auditing the #3863 mergeable-state-recheck code path for instrumentation gaps. --- src/services/agent-action-executor.ts | 17 +++++++++++++++++ test/unit/agent-action-executor.test.ts | 19 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index d46dcc8af0..3301aa3388 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -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). @@ -455,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 @@ -695,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 }); } } @@ -723,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 cf0f979001..570a7b58a2 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -68,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"; @@ -1125,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]); @@ -1149,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 () => { @@ -1583,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