fix(autonomy): retry generic merge 403s before holding (#2861)

Author: JSONbored

src/services/agent-action-executor.ts

@@ -57,6 +57,11 @@ function shouldRefreshInstallationHealthAfterPrWriteFailure(installationId: numb
   return true;
 }
 
+/** Test-only: clear the module-level installation health refresh cooldown so each test starts fresh. */
+export function clearInstallationHealthRefreshCooldownForTest(): void {
+  installationHealthRefreshAttempts.clear();
+}
+
 // A known-denied PR-write action (missing pull_requests:write) must not re-run the freshness + live-CI GitHub
 // calls and re-write an identical audit record on every sweep (#selfhost-runtime-drift) -- that burns queue/API
 // cycles on an outcome that cannot change until the maintainer re-consents (which itself only refreshes on the
@@ -334,9 +339,9 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
       }
     } catch (error) {
       await audit("error", errorMessage(error));
-      // RC3 terminal-fail merges: a merge that fails on perms (403/405) / required-check-absent (409) / a real
-      // conflict can NEVER complete for this commit — mark it terminally merge-blocked so the planner stops
-      // re-planning it every sweep. A possibly-transient failure is retried up to MERGE_RETRY_CAP then held.
+      // RC3 terminal-fail merges: immediate terminal failures (401/405/409/conflict) are marked once; generic
+      // GitHub 403s are retryable first because branch-protection/check/conversation state can converge shortly
+      // 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);
       }
@@ -557,11 +562,9 @@ export async function executeIssueMaintenanceActions(env: Env, ctx: IssueActionE
   return outcomes;
 }
 
-// RC3: persist the outcome of a FAILED merge so it is never retried blindly forever. A non-transient failure
-// (403/405 perms, 409 required-check-absent, merge conflict) is terminal immediately; an otherwise-unclassified
-// failure (e.g. base moved during the merge — a benign TOCTOU race) is retried up to MERGE_RETRY_CAP and then
-// escalated to the same terminal hold. Either way the planner suppresses the merge for this head SHA and the PR
-// is held for a human (never auto-closed).
+// RC3: persist only TERMINAL failed-merge outcomes. Auth/policy/conflict failures are terminal immediately; a
+// generic GitHub 403 is not, because it also covers branch-protection/check/conversation convergence after the
+// bot publishes its own review/check. Retry those up to MERGE_RETRY_CAP before holding the PR for a human.
 async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, error: unknown): Promise<void> {
   const headSha = ctx.headSha;
   /* v8 ignore next -- guarded at the call site; defensive. */

src/services/merge-failure.ts

@@ -9,8 +9,10 @@ import { errorMessage } from "../utils/json";
 //     401 inside the merge call itself, so a 401 reaching HERE means that retry also failed — a genuinely,
 //     persistently unauthorized installation, not a one-off stale-token race. Burning the full MERGE_RETRY_CAP
 //     against the same known-bad credential wastes calls for nothing; fail fast instead (#2264).
-//   • 403 Resource not accessible by integration → the App lacks pull_requests:write / the branch is
-//     protected against the App. A human must re-consent or merge.
+//   • 403 Resource not accessible by integration → GitHub returned a generic branch-protection / ruleset /
+//     installation-visibility rejection. The executor already checked the concrete App permissions before the
+//     merge call, so this is retryable first: required checks, conversation resolution, and permission snapshots
+//     can converge shortly after the review/check publication boundary.
 //   • 405 Method Not Allowed → merge not allowed (e.g. required reviews/checks policy forbids an App merge).
 //   • 409 Conflict → a required status check is absent / head moved into a non-mergeable state.
 //   • merge-conflict text → the branch genuinely conflicts with base; only the contributor can resolve it.
@@ -20,8 +22,6 @@ import { errorMessage } from "../utils/json";
 // MERGE_RETRY_CAP before escalating to the same terminal hold.
 export const MERGE_RETRY_CAP = 5;
 
-const TERMINAL_MERGE_STATUSES = new Set([403, 405, 409]);
-
 /** True when the merge error TEXT describes a real content conflict (vs a behind-but-clean branch). */
 function isMergeConflictMessage(message: string): boolean {
   return /merge conflict|not mergeable|cannot be merged|has conflicts|conflicts? with the base/i.test(message);
@@ -33,6 +33,10 @@ function isBaseBranchMovedMessage(message: string): boolean {
   return /base branch was modified/i.test(message);
 }
 
+function isConvergenceForbiddenMessage(message: string): boolean {
+  return /resource not accessible by integration|secondary rate limit|api rate limit|abuse detection/i.test(message);
+}
+
 /** Read the HTTP status off an Octokit RequestError (it sets `.status`); undefined for non-HTTP errors. */
 function httpStatus(error: unknown): number | undefined {
   const status = (error as { status?: unknown } | null | undefined)?.status;
@@ -46,13 +50,13 @@ export function classifyMergeFailure(error: unknown): { terminal: boolean; reaso
   const message = errorMessage(error);
   const status = httpStatus(error);
   if (status === 401) return { terminal: true, reason: `installation token rejected: App suspended or key rotated (401): ${message}` };
-  if (status === 403) return { terminal: true, reason: `merge forbidden (403 — pull_requests:write or branch protection): ${message}` };
+  if (status === 403 && isConvergenceForbiddenMessage(message)) return { terminal: false, reason: `merge forbidden for now (403 — branch protection or GitHub permission visibility may still be converging): ${message}` };
+  if (status === 403) return { terminal: true, reason: `merge forbidden (403): ${message}` };
   // A 405 "Base branch was modified" is a benign TOCTOU race, not a policy rejection — retry against the new base
   // (the executor caps retries at MERGE_RETRY_CAP before escalating to the same terminal hold).
   if (status === 405 && isBaseBranchMovedMessage(message)) return { terminal: false, reason: `base branch moved during merge — retrying: ${message}` };
   if (status === 405) return { terminal: true, reason: `merge not allowed (405 — repo merge policy forbids an automated merge): ${message}` };
   if (status === 409) return { terminal: true, reason: `merge conflict / required check absent (409): ${message}` };
-  if (status !== undefined && TERMINAL_MERGE_STATUSES.has(status)) return { terminal: true, reason: `merge rejected (${status}): ${message}` };
   if (isMergeConflictMessage(message)) return { terminal: true, reason: `branch conflicts with base — contributor must rebase: ${message}` };
   return { terminal: false, reason: message };
 }

test/unit/agent-action-executor.test.ts

@@ -43,6 +43,7 @@ import { createInstallationToken } from "../../src/github/app";
 import { fetchLiveCiAggregate, refreshInstallationHealthForInstallation } from "../../src/github/backfill";
 import {
   actionParams,
+  clearInstallationHealthRefreshCooldownForTest,
   clearWritePermissionDenialCooldownForTest,
   executeAgentMaintenanceActions,
   executeIssueMaintenanceActions,
@@ -93,6 +94,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
       liveHeadSha: args.expectedHeadSha ?? null,
       liveState: "open",
     }));
+    clearInstallationHealthRefreshCooldownForTest();
     clearWritePermissionDenialCooldownForTest();
     resetMetrics();
   });
@@ -309,7 +311,7 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
   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, failingDetails: [], nonRequiredFailingDetails: [], ciCompletenessWarning: null });
-    const outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]);
+    const outcomes = await executeAgentMaintenanceActions(env, ctx({ installationId: 127 }), [merge]);
     expect(outcomes[0]?.outcome).toBe("denied");
     expect(outcomes[0]?.detail).toContain("live CI is no longer passing (now: failed)");
     expect(mergePullRequest).not.toHaveBeenCalled();
@@ -730,6 +732,26 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => {
     expect((await auditFor(env, "merge"))?.outcome).toBe("error");
   });
 
+  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 outcomes = await executeAgentMaintenanceActions(env, ctx(), [merge]);
+
+    expect(outcomes[0]).toMatchObject({ actionClass: "merge", outcome: "error" });
+    const row = await env.DB.prepare(
+      "select merge_attempt_count as mergeAttemptCount, merge_blocked_sha as mergeBlockedSha, merge_blocked_reason as mergeBlockedReason from pull_requests where repo_full_name = ? and number = ?",
+    )
+      .bind("owner/repo", 7)
+      .first<{ mergeAttemptCount: number; mergeBlockedSha: string | null; mergeBlockedReason: string | null }>();
+    expect(row).toEqual({ mergeAttemptCount: 1, mergeBlockedSha: null, mergeBlockedReason: null });
+    const blocked = await env.DB.prepare("select count(*) as count from audit_events where event_type = ?")
+      .bind("agent.action.merge_blocked")
+      .first<{ count: number }>();
+    expect(blocked?.count).toBe(0);
+  });
+
   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 }));

test/unit/merge-failure.test.ts

@@ -29,8 +29,16 @@ describe("classifyMergeFailure", () => {
     expect(result.reason).toMatch(/suspended or key rotated/i);
   });
 
-  it("treats 403, 409, and real merge-conflict text as terminal", () => {
-    expect(classifyMergeFailure(httpError(403, "Resource not accessible by integration")).terminal).toBe(true);
+  it("retries GitHub's generic 403 merge rejection because branch protection can still converge", () => {
+    for (const message of ["Resource not accessible by integration", "secondary rate limit", "API rate limit exceeded", "abuse detection mechanism triggered"]) {
+      const result = classifyMergeFailure(httpError(403, message));
+      expect(result.terminal).toBe(false);
+      expect(result.reason).toMatch(/converging/i);
+    }
+  });
+
+  it("treats non-convergence 403s, 409, and real merge-conflict text as terminal", () => {
+    expect(classifyMergeFailure(httpError(403, "Repository does not allow squash merges")).terminal).toBe(true);
     expect(classifyMergeFailure(httpError(409, "Required status check is expected.")).terminal).toBe(true);
     expect(classifyMergeFailure(new Error("The branch has conflicts that must be resolved")).terminal).toBe(true);
   });