From 2face987cbd0b04c33605a745c755299f7afaa7d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:21:44 -0700 Subject: [PATCH] fix(actuation): classify a 401 merge failure as terminal, not transient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (App suspended or key rotated), not a one-off stale-token race. Previously this fell through to the generic terminal:false branch, burning the full MERGE_RETRY_CAP of 5 sweep-cycle retries against the same known-bad credential before finally holding. Add an explicit 401 branch returning terminal:true with a distinct "installation token rejected" reason, positioned before the 403 branch. --- src/services/merge-failure.ts | 6 ++++++ test/unit/merge-failure.test.ts | 10 ++++++++++ 2 files changed, 16 insertions(+) diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts index 40aa352407..9ee5f4a459 100644 --- a/src/services/merge-failure.ts +++ b/src/services/merge-failure.ts @@ -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). @@ -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). diff --git a/test/unit/merge-failure.test.ts b/test/unit/merge-failure.test.ts index fd630102f2..0b4a8ff149 100644 --- a/test/unit/merge-failure.test.ts +++ b/test/unit/merge-failure.test.ts @@ -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);