From 195c79a972188f1710c82616fd9fa2a7a96465ee Mon Sep 17 00:00:00 2001 From: bohdansolovie Date: Thu, 16 Jul 2026 01:19:17 +0200 Subject: [PATCH] fix(github): retry label and assignee writes on stale installation tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensurePullRequestLabel, removePullRequestLabel, and ensurePullRequestAssignee now use withInstallationTokenRetry so a 401 (or scope 403) invalidates the cached token and retries once — matching the other GitHub-write helpers. Closes #6191 Co-authored-by: Cursor --- src/github/assignees.ts | 75 ++++++++++---------- src/github/labels.ts | 106 +++++++++++++++-------------- test/unit/github-assignees.test.ts | 34 +++++++++ test/unit/github-labels.test.ts | 61 +++++++++++++++++ 4 files changed, 190 insertions(+), 86 deletions(-) diff --git a/src/github/assignees.ts b/src/github/assignees.ts index 70b8de53f3..7600c986be 100644 --- a/src/github/assignees.ts +++ b/src/github/assignees.ts @@ -1,4 +1,4 @@ -import { createInstallationToken } from "./app"; +import { withInstallationTokenRetry } from "./app"; import { githubRateLimitAdmissionKeyForInstallation, makeInstallationOctokit } from "./client"; import type { AgentActionMode } from "../settings/agent-execution"; @@ -35,44 +35,47 @@ export async function ensurePullRequestAssignee( ): Promise<{ applied: boolean }> { const { owner, repo } = parseRepoFullName(repoFullName); - const token = await createInstallationToken(env, installationId); - // Non-live mode suppresses the assign write; the GET dedup probe below still runs. - const octokit = makeInstallationOctokit(env, token, options.mode ?? "live", githubRateLimitAdmissionKeyForInstallation(installationId)); - const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}", { - owner, - repo, - issue_number: pullNumber, - }); - const existingAssignees = (existing.data.assignees ?? []) as GitHubUser[]; - if (existingAssignees.some((assignee) => assignee.login?.toLowerCase() === login.toLowerCase())) { - return { applied: true }; - } - - let result; - try { - result = await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", { + // Wrap the whole write path in withInstallationTokenRetry so a stale cached installation token self-heals + // exactly once — matching comments.ts / pr-actions.ts (#6191). + return withInstallationTokenRetry(env, installationId, async (token) => { + // Non-live mode suppresses the assign write; the GET dedup probe below still runs. + const octokit = makeInstallationOctokit(env, token, options.mode ?? "live", githubRateLimitAdmissionKeyForInstallation(installationId)); + const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}", { owner, repo, issue_number: pullNumber, - assignees: [login], }); - } catch (error: unknown) { - // GitHub blocks assigning bot/agent logins via App installation tokens (HTTP 403). This is a GitHub - // platform restriction with no workaround using installation-token auth. Return applied:false so the - // caller can fall back to a by:{login} label instead of propagating an unactionable error. - if ( - typeof error === "object" && - error !== null && - "status" in error && - (error as { status: number }).status === 403 && - "message" in error && - typeof (error as { message: unknown }).message === "string" && - (error as { message: string }).message.includes("Assigning agents is not supported") - ) { - return { applied: false }; + const existingAssignees = (existing.data.assignees ?? []) as GitHubUser[]; + if (existingAssignees.some((assignee) => assignee.login?.toLowerCase() === login.toLowerCase())) { + return { applied: true }; } - throw error; - } - const resultAssignees = (result.data.assignees ?? []) as GitHubUser[]; - return { applied: resultAssignees.some((assignee) => assignee.login?.toLowerCase() === login.toLowerCase()) }; + + let result; + try { + result = await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/assignees", { + owner, + repo, + issue_number: pullNumber, + assignees: [login], + }); + } catch (error: unknown) { + // GitHub blocks assigning bot/agent logins via App installation tokens (HTTP 403). This is a GitHub + // platform restriction with no workaround using installation-token auth. Return applied:false so the + // caller can fall back to a by:{login} label instead of propagating an unactionable error. + if ( + typeof error === "object" && + error !== null && + "status" in error && + (error as { status: number }).status === 403 && + "message" in error && + typeof (error as { message: unknown }).message === "string" && + (error as { message: string }).message.includes("Assigning agents is not supported") + ) { + return { applied: false }; + } + throw error; + } + const resultAssignees = (result.data.assignees ?? []) as GitHubUser[]; + return { applied: resultAssignees.some((assignee) => assignee.login?.toLowerCase() === login.toLowerCase()) }; + }); } diff --git a/src/github/labels.ts b/src/github/labels.ts index 6908c5a5f1..8622dfecff 100644 --- a/src/github/labels.ts +++ b/src/github/labels.ts @@ -1,4 +1,4 @@ -import { createInstallationToken } from "./app"; +import { withInstallationTokenRetry } from "./app"; import { githubRateLimitAdmissionKeyForInstallation, makeInstallationOctokit } from "./client"; import type { AgentActionMode } from "../settings/agent-execution"; @@ -28,45 +28,48 @@ export async function ensurePullRequestLabel( ): Promise<{ applied: boolean; created: boolean }> { const { owner, repo } = parseRepoFullName(repoFullName); - const token = await createInstallationToken(env, installationId); - // Non-live mode suppresses the label create + apply writes; the GET dedup probe below still runs. - const octokit = makeInstallationOctokit(env, token, options.mode ?? "live", githubRateLimitAdmissionKeyForInstallation(installationId)); - const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/labels", { - owner, - repo, - issue_number: pullNumber, - per_page: 100, - }); - const labels = existing.data as GitHubLabel[]; - if (labels.some((label) => label.name?.toLowerCase() === labelName.toLowerCase())) { - return { applied: false, created: false }; - } + // Wrap the whole write path in withInstallationTokenRetry so a stale cached installation token self-heals + // exactly once — matching comments.ts / pr-actions.ts (#6191). + return withInstallationTokenRetry(env, installationId, async (token) => { + // Non-live mode suppresses the label create + apply writes; the GET dedup probe below still runs. + const octokit = makeInstallationOctokit(env, token, options.mode ?? "live", githubRateLimitAdmissionKeyForInstallation(installationId)); + const existing = await octokit.request("GET /repos/{owner}/{repo}/issues/{issue_number}/labels", { + owner, + repo, + issue_number: pullNumber, + per_page: 100, + }); + const labels = existing.data as GitHubLabel[]; + if (labels.some((label) => label.name?.toLowerCase() === labelName.toLowerCase())) { + return { applied: false, created: false }; + } - let created = false; - if (options.createMissingLabel) { - try { - await octokit.request("POST /repos/{owner}/{repo}/labels", { - owner, - repo, - name: labelName, - color: "7ee787", - description: "Gittensor contributor context", - }); - created = true; - } catch (error) { - const e = error as { status?: number; message?: string }; - // Only swallow the specific "already_exists" duplicate; other 422s (e.g. invalid name) must propagate. - if (e.status !== 422 || !e.message?.includes("already_exists")) throw error; + let created = false; + if (options.createMissingLabel) { + try { + await octokit.request("POST /repos/{owner}/{repo}/labels", { + owner, + repo, + name: labelName, + color: "7ee787", + description: "Gittensor contributor context", + }); + created = true; + } catch (error) { + const e = error as { status?: number; message?: string }; + // Only swallow the specific "already_exists" duplicate; other 422s (e.g. invalid name) must propagate. + if (e.status !== 422 || !e.message?.includes("already_exists")) throw error; + } } - } - await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/labels", { - owner, - repo, - issue_number: pullNumber, - labels: [labelName], + await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/labels", { + owner, + repo, + issue_number: pullNumber, + labels: [labelName], + }); + return { applied: true, created }; }); - return { applied: true, created }; } /** Remove a single label from a PR if present. Best-effort — a 404 (label not on the PR) is ignored; any other @@ -81,18 +84,21 @@ export async function removePullRequestLabel(env: Env, installationId: number, r } catch { return; } - const token = await createInstallationToken(env, installationId); - const octokit = makeInstallationOctokit(env, token, mode, githubRateLimitAdmissionKeyForInstallation(installationId)); - try { - await octokit.request("DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}", { - owner, - repo, - issue_number: pullNumber, - name: labelName, - }); - } catch (error) { - const e = error as { status?: number }; - // Only swallow the documented "label not on the PR" 404; other failures must propagate (#6192). - if (e.status !== 404) throw error; - } + // Self-heal a stale cached installation token once before giving up (#6191). + await withInstallationTokenRetry(env, installationId, async (token) => { + const octokit = makeInstallationOctokit(env, token, mode, githubRateLimitAdmissionKeyForInstallation(installationId)); + try { + await octokit.request("DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}", { + owner, + repo, + issue_number: pullNumber, + name: labelName, + }); + } catch (error) { + const e = error as { status?: number }; + // Only swallow the documented "label not on the PR" 404; other failures must propagate (#6192). + // Note: a 401 bad_credentials here is NOT swallowed — withInstallationTokenRetry sees it and retries once. + if (e.status !== 404) throw error; + } + }); } diff --git a/test/unit/github-assignees.test.ts b/test/unit/github-assignees.test.ts index be126b0337..3de31cf6bc 100644 --- a/test/unit/github-assignees.test.ts +++ b/test/unit/github-assignees.test.ts @@ -1,11 +1,13 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; import { ensurePullRequestAssignee } from "../../src/github/assignees"; import { createTestEnv } from "../helpers/d1"; describe("GitHub PR assignees (#3182)", () => { afterEach(() => { vi.unstubAllGlobals(); + clearInstallationTokenCacheForTest(); }); it("rejects invalid repository names before making GitHub calls", async () => { @@ -132,6 +134,38 @@ describe("GitHub PR assignees (#3182)", () => { expect(result).toEqual({ applied: false }); }); + + it("ensurePullRequestAssignee: evicts a stale installation token and retries once on 401 (#6191)", async () => { + clearInstallationTokenCacheForTest(); + let tokenMints = 0; + let issueGetAttempts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) { + tokenMints += 1; + return Response.json({ token: `token-${tokenMints}` }); + } + if (url.includes("/issues/4") && !url.includes("/assignees") && method === "GET") { + issueGetAttempts += 1; + if (issueGetAttempts === 1) return Response.json({ message: "Bad credentials" }, { status: 401 }); + return Response.json({ assignees: [{ login: "alice" }] }); + } + return new Response("unexpected", { status: 500 }); + }); + + const result = await ensurePullRequestAssignee( + createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), + 998877, + "JSONbored/gittensory", + 4, + "alice", + ); + + expect(result).toEqual({ applied: true }); + expect(issueGetAttempts).toBe(2); + expect(tokenMints).toBe(2); + }); }); function generateRsaPrivateKeyPem(): string { diff --git a/test/unit/github-labels.test.ts b/test/unit/github-labels.test.ts index 09402269c4..4a0a0b5fd3 100644 --- a/test/unit/github-labels.test.ts +++ b/test/unit/github-labels.test.ts @@ -1,11 +1,13 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; import { ensurePullRequestLabel, removePullRequestLabel } from "../../src/github/labels"; import { createTestEnv } from "../helpers/d1"; describe("GitHub PR labels", () => { afterEach(() => { vi.unstubAllGlobals(); + clearInstallationTokenCacheForTest(); }); it("rejects invalid repository names before making GitHub calls", async () => { @@ -111,6 +113,65 @@ describe("GitHub PR labels", () => { ).resolves.toBeUndefined(); }); + it("ensurePullRequestLabel: evicts a stale installation token and retries once on 401 (#6191)", async () => { + clearInstallationTokenCacheForTest(); + let tokenMints = 0; + let labelGetAttempts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) { + tokenMints += 1; + return Response.json({ token: `token-${tokenMints}` }); + } + if (url.includes("/issues/4/labels") && method === "GET") { + labelGetAttempts += 1; + if (labelGetAttempts === 1) return Response.json({ message: "Bad credentials" }, { status: 401 }); + return Response.json([{ name: "gittensor" }]); + } + return new Response("unexpected", { status: 500 }); + }); + + const result = await ensurePullRequestLabel( + createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), + 998877, + "JSONbored/gittensory", + 4, + "gittensor", + { createMissingLabel: true }, + ); + + expect(result).toEqual({ applied: false, created: false }); + expect(labelGetAttempts).toBe(2); + expect(tokenMints).toBe(2); + }); + + it("removePullRequestLabel: evicts a stale installation token and retries once on 401 (#6191)", async () => { + clearInstallationTokenCacheForTest(); + let tokenMints = 0; + let deleteAttempts = 0; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) { + tokenMints += 1; + return Response.json({ token: `token-${tokenMints}` }); + } + if (url.includes("/issues/4/labels/") && method === "DELETE") { + deleteAttempts += 1; + if (deleteAttempts === 1) return Response.json({ message: "Bad credentials" }, { status: 401 }); + return new Response(null, { status: 204 }); + } + return new Response("unexpected", { status: 500 }); + }); + + await expect( + removePullRequestLabel(createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), 998877, "JSONbored/gittensory", 4, "gittensor"), + ).resolves.toBeUndefined(); + expect(deleteAttempts).toBe(2); + expect(tokenMints).toBe(2); + }); + it("does nothing when the label is already applied", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {