From d3ac2cc2f909c8b352402b77ff3cbee3a3bc345c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 12:30:23 -0700 Subject: [PATCH] fix(queue): opportunistically refresh installation health on a PR-write 403 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local installations.permissions snapshot is read once at the top of each sweep/webhook pass and self-heals only via a maintainer-consented permission-upgrade webhook (GitHub sends none for a downgrade) or the ~30-minute health-refresh cron. A maintainer-initiated downgrade of pull_requests write access can therefore go undetected by the readiness gate for up to 30 minutes, during which the executor keeps attempting merge/close/review/update_branch calls that GitHub correctly rejects with 403 — wasted attempts, not a security gap (GitHub's own server-side enforcement is already the real backstop). When a PR-write mutation fails with a 403, opportunistically trigger a per-installation health refresh so the DB row (and therefore every later sweep/webhook read of it, for this or any other PR on the installation) self-heals immediately instead of waiting for the next cron tick. Exports the existing githubErrorStatus helper from github/app.ts to detect the 403 without duplicating status-reading logic. --- src/github/app.ts | 2 +- src/services/agent-action-executor.ts | 12 +++++++++ test/unit/agent-action-executor.test.ts | 36 +++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/github/app.ts b/src/github/app.ts index f4c5584f02..781fbce1e3 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -142,7 +142,7 @@ export async function createInstallationToken( return mint; } -function githubErrorStatus(error: unknown): number | null { +export function githubErrorStatus(error: unknown): number | null { const err = error as { status?: number; response?: { status?: number } | null; diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index fd9fef1581..cf4d9170fa 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -1,6 +1,8 @@ import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, insertNotificationDeliveryIfAbsent, isGlobalAgentFrozen, markPullRequestApproved, markPullRequestMergeBlocked, recordAuditEvent } from "../db/repositories"; import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; import { notifyActionToDiscord, notifyActionToSlack, type NotifyOutcome } from "./notify-discord"; +import { githubErrorStatus } from "../github/app"; +import { refreshInstallationHealthForInstallation } from "../github/backfill"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; import { closePullRequest, createIssueComment, createPullRequestReview, mergePullRequest, updatePullRequestBranch } from "../github/pr-actions"; import { fetchPullRequestFreshness, pullRequestFreshnessDetail } from "../github/pr-freshness"; @@ -148,6 +150,16 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE if (action.actionClass === "merge" && ctx.headSha) { await handleMergeFailure(env, ctx, error); } + // #2265: a 403 on a PR-write mutation often means the LOCAL installations.permissions snapshot is stale — + // GitHub webhooks a consented permission UPGRADE but sends nothing for a maintainer-initiated downgrade, so + // the write-permission readiness gate (step 6 above) can keep reporting "ready" for up to the 30-minute + // health-refresh cron interval after a live downgrade. Opportunistically refresh now so the DB row (and + // therefore every later sweep/webhook read of it, for this or any other PR on the installation) self-heals + // immediately instead of waiting for the next cron tick. GitHub's own server-side enforcement (this very + // 403) is already the real backstop, so a failed refresh here is safe to swallow. + if (PR_WRITE_CLASSES.has(action.actionClass) && githubErrorStatus(error) === 403) { + await refreshInstallationHealthForInstallation(env, ctx.installationId).catch(() => undefined); + } } } diff --git a/test/unit/agent-action-executor.test.ts b/test/unit/agent-action-executor.test.ts index 071c589b6b..9174985c83 100644 --- a/test/unit/agent-action-executor.test.ts +++ b/test/unit/agent-action-executor.test.ts @@ -22,10 +22,14 @@ vi.mock("../../src/github/pr-freshness", async (importOriginal) => { })), }; }); +vi.mock("../../src/github/backfill", () => ({ + refreshInstallationHealthForInstallation: vi.fn(async () => null), +})); import { closePullRequest, createIssueComment, createPullRequestReview, mergePullRequest, updatePullRequestBranch } from "../../src/github/pr-actions"; import { ensurePullRequestLabel, removePullRequestLabel } from "../../src/github/labels"; import { fetchPullRequestFreshness } from "../../src/github/pr-freshness"; +import { refreshInstallationHealthForInstallation } from "../../src/github/backfill"; import { actionParams, executeAgentMaintenanceActions, pendingClosureLabelApplied, type AgentActionExecutionContext, type AgentActionOutcome } from "../../src/services/agent-action-executor"; import type { PlannedAgentAction } from "../../src/settings/agent-actions"; import { AGENT_LABEL_PENDING_CLOSURE } from "../../src/review/linked-issue-hard-rules"; @@ -320,6 +324,38 @@ describe("executeAgentMaintenanceActions (#778 gate stack)", () => { expect(outcomes[0]?.detail).toMatch(/not mergeable/i); expect((await auditFor(env, "merge"))?.outcome).toBe("error"); }); + + 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 })); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [close]); + expect(outcomes[0]?.outcome).toBe("error"); + expect(refreshInstallationHealthForInstallation).toHaveBeenCalledTimes(1); + expect(refreshInstallationHealthForInstallation).toHaveBeenCalledWith(env, 123); + }); + + it("does not refresh installation health for a non-403 mutation failure (#2265)", async () => { + const env = createTestEnv({}); + vi.mocked(closePullRequest).mockRejectedValueOnce(new Error("network timeout")); + await executeAgentMaintenanceActions(env, ctx(), [close]); + expect(refreshInstallationHealthForInstallation).not.toHaveBeenCalled(); + }); + + it("does not refresh installation health on a 403 from a non-PR-write action (label uses issues:write, not pull_requests) (#2265)", async () => { + const env = createTestEnv({}); + vi.mocked(ensurePullRequestLabel).mockRejectedValueOnce(Object.assign(new Error("Resource not accessible by integration"), { status: 403 })); + await executeAgentMaintenanceActions(env, ctx(), [label]); + expect(refreshInstallationHealthForInstallation).not.toHaveBeenCalled(); + }); + + it("swallows a failed installation-health refresh — best-effort, does not affect the recorded outcome (#2265)", async () => { + const env = createTestEnv({}); + vi.mocked(closePullRequest).mockRejectedValueOnce(Object.assign(new Error("Resource not accessible by integration"), { status: 403 })); + vi.mocked(refreshInstallationHealthForInstallation).mockRejectedValueOnce(new Error("refresh boom")); + const outcomes = await executeAgentMaintenanceActions(env, ctx(), [close]); + expect(outcomes[0]?.outcome).toBe("error"); + expect((await auditFor(env, "close"))?.outcome).toBe("error"); + }); }); describe("pendingClosureLabelApplied (#1136 Pass-2 trigger)", () => {