diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 73b65d3bb3..f86d778c95 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -15,7 +15,7 @@ import { upsertGlobalContributorBlacklist, } from "../db/repositories"; import { isAuthorBlacklisted } from "../settings/contributor-blacklist"; -import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; +import { classifyMergeFailure, isMergeConflictMessage, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; import { resolveDispositionReason } from "../review/outcomes-wire"; import { cancelInFlightWorkflowRunsForHeadSha, createInstallationToken, githubErrorStatus, isGitHubRateLimitedError } from "../github/app"; @@ -618,6 +618,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 if (action.actionClass === "update_branch" && isMergeConflictMessage(errorMessage(error))) { + // LOOPOVER-24: update_branch performs a real merge internally, so it fails with the same "merge + // conflict" shape a MERGE action does -- but unlike a merge's terminal hold (a PR permanently blocked + // until a human intervenes), this is NOT a stuck state: forceUpdateBranch's caller (prReadyForReview) + // already falls through to reviewing the PR on its current, non-rebased head when this returns false + // (see forceUpdateBranch's own doc comment), exactly like every other "couldn't rebase, review anyway" + // path. The branch owner, not the bot, needs to resolve the conflict -- paging on every naturally- + // diverged PR this happens to hit isn't warranted. Still recorded by the audit() call above. } 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 diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts index 2c29b56f22..8000ce699e 100644 --- a/src/services/merge-failure.ts +++ b/src/services/merge-failure.ts @@ -22,8 +22,11 @@ import { errorMessage } from "../utils/json"; // MERGE_RETRY_CAP before escalating to the same terminal hold. export const MERGE_RETRY_CAP = 5; -/** True when the merge error TEXT describes a real content conflict (vs a behind-but-clean branch). */ -function isMergeConflictMessage(message: string): boolean { +/** True when the merge error TEXT describes a real content conflict (vs a behind-but-clean branch). Exported + * for reuse by the update_branch action class (LOOPOVER-24): update-branch performs a real merge internally, + * so it fails with this SAME message shape, and the classification is identical -- the branch owner, not the + * bot, must resolve it. */ +export function isMergeConflictMessage(message: string): boolean { return /merge conflict|not mergeable|cannot be merged|has conflicts|conflicts? with the base/i.test(message); } diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 87bf15494c..22e6c5f6b8 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -1456,6 +1456,33 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(refreshInstallationHealthForInstallation).not.toHaveBeenCalled(); }); + it("REGRESSION (LOOPOVER-24): a merge-conflict update_branch failure does not page Sentry", async () => { + const env = createTestEnv({}); + vi.mocked(updatePullRequestBranch).mockRejectedValueOnce(new Error("merge conflict between base and head")); + const captureSpy = vi.spyOn(sentryModule, "captureError"); + + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [updateBranch]); + + expect(outcomes[0]).toMatchObject({ actionClass: "update_branch", outcome: "error" }); + expect((await auditFor(env, "update_branch"))?.outcome).toBe("error"); + // The caller (prReadyForReview/forceUpdateBranch) already falls through to reviewing the PR on its + // current head when update_branch fails -- this is not a stuck state the way a merge's terminal hold + // is, so it must stay out of Sentry entirely rather than paging on every naturally-diverged PR. + expect(captureSpy).not.toHaveBeenCalled(); + captureSpy.mockRestore(); + }); + + it("a non-conflict update_branch failure still pages Sentry (#agent_action_execution_failed unchanged)", async () => { + const env = createTestEnv({}); + vi.mocked(updatePullRequestBranch).mockRejectedValueOnce(new Error("network timeout")); + const captureSpy = vi.spyOn(sentryModule, "captureError"); + + await executeAgentMaintenanceActions(env, ctx(), [updateBranch]); + + expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "agent_action_execution_failed", actionClass: "update_branch" }), "agent_action_execution_failed"); + captureSpy.mockRestore(); + }); + it("debounces permission-looking installation health refreshes per installation (#2265)", async () => { const env = createTestEnv({}); vi.useFakeTimers();