From ea2c0ea191437871936d2a0d54ae0916b20739b5 Mon Sep 17 00:00:00 2001 From: bohdansolovie Date: Wed, 15 Jul 2026 23:28:34 +0200 Subject: [PATCH] fix(github): only swallow 404 in removePullRequestLabel (#6192) Match ensurePullRequestLabel's narrow-swallow pattern so a missing label (404) stays best-effort, while 403/5xx/rate-limit failures propagate instead of being silently discarded. Co-authored-by: Cursor --- src/github/labels.ts | 21 +++++++++---- test/unit/github-labels.test.ts | 53 +++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/src/github/labels.ts b/src/github/labels.ts index 147cc1cf8..6908c5a5f 100644 --- a/src/github/labels.ts +++ b/src/github/labels.ts @@ -69,8 +69,10 @@ export async function ensurePullRequestLabel( return { applied: true, created }; } -/** Remove a single label from a PR if present. Best-effort — a 404 (label not on the PR) is ignored. Used to - * keep the mutually-exclusive managed TYPE labels (gittensor:bug/feature/priority) down to exactly one. */ +/** Remove a single label from a PR if present. Best-effort — a 404 (label not on the PR) is ignored; any other + * GitHub API failure (403/5xx/rate-limit) is re-thrown, matching {@link ensurePullRequestLabel}'s narrow-swallow + * convention. Used to keep the mutually-exclusive managed TYPE labels (gittensor:bug/feature/priority) down to + * exactly one. */ export async function removePullRequestLabel(env: Env, installationId: number, repoFullName: string, pullNumber: number, labelName: string, mode: AgentActionMode = "live"): Promise { let owner: string; let repo: string; @@ -81,7 +83,16 @@ export async function removePullRequestLabel(env: Env, installationId: number, r } const token = await createInstallationToken(env, installationId); const octokit = makeInstallationOctokit(env, token, mode, githubRateLimitAdmissionKeyForInstallation(installationId)); - await octokit - .request("DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}", { owner, repo, issue_number: pullNumber, name: labelName }) - .catch(() => undefined); + 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; + } } diff --git a/test/unit/github-labels.test.ts b/test/unit/github-labels.test.ts index 9bf6154a3..09402269c 100644 --- a/test/unit/github-labels.test.ts +++ b/test/unit/github-labels.test.ts @@ -58,6 +58,59 @@ describe("GitHub PR labels", () => { expect(called).toBe(false); }); + it("removePullRequestLabel: swallows a 404 when the label is already absent (#6192)", async () => { + const calls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + calls.push(`${method} ${url}`); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/4/labels/") && method === "DELETE") { + return Response.json({ message: "Label does not exist" }, { status: 404 }); + } + return new Response("unexpected", { status: 500 }); + }); + + await expect( + removePullRequestLabel(createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), 123, "JSONbored/gittensory", 4, "gittensor"), + ).resolves.toBeUndefined(); + expect(calls.some((call) => call.startsWith("DELETE ") && call.includes("/labels/"))).toBe(true); + }); + + it("removePullRequestLabel: re-throws non-404 failures (403/5xx) instead of swallowing them (#6192)", async () => { + for (const status of [403, 500]) { + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/4/labels/") && method === "DELETE") { + return Response.json({ message: status === 403 ? "Resource not accessible by integration" : "Internal Server Error" }, { status }); + } + return new Response("unexpected", { status: 599 }); + }); + + await expect( + removePullRequestLabel(createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), 123, "JSONbored/gittensory", 4, "gittensor"), + ).rejects.toMatchObject({ status }); + } + }); + + it("removePullRequestLabel: succeeds when DELETE returns 200/204", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/4/labels/") && method === "DELETE") { + return new Response(null, { status: 204 }); + } + return new Response("unexpected", { status: 500 }); + }); + + await expect( + removePullRequestLabel(createTestEnv({ GITHUB_APP_PRIVATE_KEY: generateRsaPrivateKeyPem() }), 123, "JSONbored/gittensory", 4, "gittensor"), + ).resolves.toBeUndefined(); + }); + it("does nothing when the label is already applied", async () => { const calls: string[] = []; vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {