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
6 changes: 6 additions & 0 deletions src/services/merge-failure.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ import { errorMessage } from "../utils/json";
// current commit, so retrying it every sweep is pointless and noisy — classify it once and let the executor
// mark the PR terminally merge-blocked (held for a human) instead of looping forever.
//
// • 401 Bad credentials → the installation token was rejected: the App was suspended or its private key was
// rotated mid-flight. withInstallationTokenRetry (src/github/app.ts) already evicts-and-retries ONCE on a
// 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.
// • 405 Method Not Allowed → merge not allowed (e.g. required reviews/checks policy forbids an App merge).
Expand Down Expand Up @@ -40,6 +45,7 @@ function httpStatus(error: unknown): number | undefined {
export function classifyMergeFailure(error: unknown): { terminal: boolean; reason: string } {
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}` };
// 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).
Expand Down
10 changes: 10 additions & 0 deletions test/unit/merge-failure.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,16 @@ describe("classifyMergeFailure", () => {
expect(result.reason).toMatch(/405/);
});

it("treats a 401 (installation token rejected) as terminal, distinct from a generic rejection (#2264)", () => {
// withInstallationTokenRetry already evicts-and-retries once on a 401 inside the merge call itself, so a 401
// reaching classifyMergeFailure means that retry also failed — a persistently unauthorized installation, not
// a one-off stale-token race. Must fail fast (terminal) rather than burn the full MERGE_RETRY_CAP.
const result = classifyMergeFailure(httpError(401, "Bad credentials"));
expect(result.terminal).toBe(true);
expect(result.reason).toMatch(/installation token rejected/i);
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);
expect(classifyMergeFailure(httpError(409, "Required status check is expected.")).terminal).toBe(true);
Expand Down
Loading