Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/services/merge-failure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@ function isConvergenceForbiddenMessage(message: string): boolean {
return /resource not accessible by integration|secondary rate limit|api rate limit|abuse detection/i.test(message);
}

/** A GitHub rate-limit signal — the message GitHub attaches to a secondary/abuse or primary rate-limit
* response. A merge failing on this is an infra-scoped, self-healing window, whether GitHub spelled it 403 or
* 429 (src/github/client.ts treats both the same). Exported so the 403 and 429 branches share one definition
* (#9693). */
export function isRateLimitMessage(message: string): boolean {
return /secondary rate limit|abuse|api rate limit exceeded/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;
Expand Down Expand Up @@ -119,6 +127,13 @@ export function classifyMergeFailure(error: unknown): { terminal: boolean; reaso
const message = errorMessage(error);
const status = httpStatus(error);
if (status === 401) return { terminal: true, scope: "infra", reason: `installation token rejected: App suspended or key rotated (401): ${message}` };
// A secondary rate-limit window surfaces as 403 OR 429 (src/github/client.ts:422) and is fleet-wide + self-
// healing — infra-scoped so it lapses on the TTL re-probe instead of stranding every in-flight merge on a
// head-scoped block until an unrelated commit lands (#9693). The 429 arm sits with the other status branches;
// the rate-limited-403 arm must precede the terminal `403` below so it is not swallowed by it.
if (status === 429) return { terminal: false, scope: "infra", reason: `merge rate-limited (429 — secondary rate limit, self-healing window): ${message}` };
if (status === 403 && isRateLimitMessage(message))
return { terminal: false, scope: "infra", reason: `merge rate-limited (403 — secondary rate limit, self-healing window): ${message}` };
if (status === 403 && isConvergenceForbiddenMessage(message))
return { terminal: false, scope: "infra", reason: `merge forbidden for now (403 — branch protection or GitHub permission visibility may still be converging): ${message}` };
if (status === 403) return { terminal: true, scope: "commit", reason: `merge forbidden (403): ${message}` };
Expand Down
43 changes: 42 additions & 1 deletion test/unit/merge-block-recovery.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, it } from "vitest";
import { getPullRequest, markPullRequestMergeBlocked, bumpPullRequestMergeAttempt, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, upsertInstallation } from "../../src/db/repositories";
import { activeMergeBlockedSha, classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeBlockInEffect } from "../../src/services/merge-failure";
import { activeMergeBlockedSha, classifyMergeFailure, INFRA_MERGE_BLOCK_TTL_MS, isMergeBlockInEffect, MERGE_RETRY_CAP } from "../../src/services/merge-failure";
import { AGENT_LABEL_NEEDS_REVIEW, AGENT_LABEL_READY, planAgentMaintenanceActions } from "../../src/settings/agent-actions";
import { createTestEnv } from "../helpers/d1";

Expand Down Expand Up @@ -115,6 +115,47 @@ describe("markPullRequestMergeBlocked persists the scope (#9012)", () => {
});
});

// #9693: a 429 secondary-rate-limit window that burns MERGE_RETRY_CAP must persist an INFRA-scoped (expiring)
// block, not a commit-scoped one that strands the PR until an unrelated commit lands. handleMergeFailure derives
// the expiry from classifyMergeFailure(...).scope, so we drive that composition exactly as the executor does.
describe("a rate-limit-exhausted merge persists a self-healing infra block (#9693)", () => {
const NOW = Date.parse("2026-07-26T12:00:00.000Z");
const blockFor = (error: unknown) => {
// Mirror handleMergeFailure's retry-cap path: on exhaustion the classified scope decides the expiry.
const { scope, terminal } = classifyMergeFailure(error);
// A 429/rate-limited failure is non-terminal, so it reaches the cap path and escalates there.
expect(terminal).toBe(false);
return scope === "infra" ? new Date(NOW + INFRA_MERGE_BLOCK_TTL_MS).toISOString() : undefined;
};

it("persists a non-null, lapsing mergeBlockedUntil after MERGE_RETRY_CAP 429 failures", async () => {
const env = createTestEnv();
await seedPr(env, "sha-1");
// Exhaust the retry budget on the same head, exactly as repeated 429 attempts would.
for (let i = 0; i < MERGE_RETRY_CAP; i += 1) await bumpPullRequestMergeAttempt(env, "alice/repo", 5, "sha-1");
const expiresAt = blockFor(httpError(429, "You have exceeded a secondary rate limit"));
expect(expiresAt).not.toBeUndefined();
await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "merge could not complete after 5 attempt(s)", expiresAt);

const stored = await getPullRequest(env, "alice/repo", 5);
expect(stored?.mergeBlockedUntil).not.toBeNull();
// The block genuinely lapses on the TTL — no new commit required.
expect(isMergeBlockInEffect(stored!, "sha-1", NOW + INFRA_MERGE_BLOCK_TTL_MS + 1)).toBe(false);
});

it("keeps a 409 merge-conflict block commit-scoped (mergeBlockedUntil null), unchanged", async () => {
const env = createTestEnv();
await seedPr(env, "sha-1");
// A 409 is terminal on the first failure — commit-scoped, no expiry.
const { scope, terminal } = classifyMergeFailure(httpError(409, "Required status check is expected."));
expect(terminal).toBe(true);
const expiresAt = scope === "infra" ? new Date(NOW + INFRA_MERGE_BLOCK_TTL_MS).toISOString() : undefined;
await markPullRequestMergeBlocked(env, "alice/repo", 5, "sha-1", "merge conflict (409)", expiresAt);

expect((await getPullRequest(env, "alice/repo", 5))?.mergeBlockedUntil ?? null).toBeNull();
});
});

// #9012 compounding bug: mergeAttemptCount's own schema and function docs promised "a new commit's attempts
// start fresh once the row's head advances", but nothing reset it — bumpPullRequestMergeAttempt only scoped the
// INCREMENT to the head. So once one head exhausted MERGE_RETRY_CAP, every later head was one-strike-terminal.
Expand Down
28 changes: 24 additions & 4 deletions test/unit/merge-failure.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { describe, expect, it } from "vitest";
import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, isWorkflowScopeRefusalMessage, MERGE_RETRY_CAP } from "../../src/services/merge-failure";
import { classifyMergeFailure, isMergeConflictMessage, isNoNewBaseCommitsMessage, isRateLimitMessage, isWorkflowScopeRefusalMessage, MERGE_RETRY_CAP } from "../../src/services/merge-failure";

/** Build an Octokit-style RequestError: an Error carrying an HTTP `.status`. */
function httpError(status: number, message: string): Error {
Expand Down Expand Up @@ -36,13 +36,33 @@ describe("classifyMergeFailure", () => {
});

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"]) {
// A non-rate-limit convergence 403 (permission visibility still settling) — the converging arm.
const result = classifyMergeFailure(httpError(403, "Resource not accessible by integration"));
expect(result.terminal).toBe(false);
expect(result.scope).toBe("infra");
expect(result.reason).toMatch(/converging/i);
});

it("classifies a 429 and a rate-limited 403 as an infra-scoped, self-healing window (#9693)", () => {
// 429: no branch matched this before, so it fell through to a commit-scoped block that stranded the PR.
const rateLimited429 = classifyMergeFailure(httpError(429, "You have exceeded a secondary rate limit"));
expect(rateLimited429).toMatchObject({ terminal: false, scope: "infra" });
expect(rateLimited429.reason).toMatch(/rate-limited/i);
// 403 spellings of the same window are treated identically, before the terminal 403 branch.
for (const message of ["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);
expect(result).toMatchObject({ terminal: false, scope: "infra" });
expect(result.reason).toMatch(/rate-limited/i);
}
});

it("exposes isRateLimitMessage matching only rate-limit text (#9693)", () => {
expect(isRateLimitMessage("You have exceeded a secondary rate limit")).toBe(true);
expect(isRateLimitMessage("abuse detection mechanism triggered")).toBe(true);
expect(isRateLimitMessage("API rate limit exceeded")).toBe(true);
expect(isRateLimitMessage("Repository does not allow squash merges")).toBe(false);
});

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);
Expand Down