diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 31b8ea45ac..9478309761 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -129,6 +129,7 @@ import { getGithubUserCreatedAt, getInstallationId, getRepositoryCollaboratorPermission, + githubErrorStatus, GITTENSORY_GATE_CHECK_NAME, isGitHubRateLimitedError, isForeignAppInstallation, @@ -5716,8 +5717,21 @@ type PublicSurfaceOutput = "comment" | "label" | "check_run" | "gate_check_run"; type PublicSurfaceOutputFailure = { output: PublicSurfaceOutput; error: string; + // Captured AT CATCH TIME, not reconstructed later: errorMessage() already reduces `error` to a plain string by + // the time it lands here, discarding the `.status`/`.response` shape isGitHubTransientPublishError needs. A + // permission_missing check-run push (no live error object) is correctly "false" via the default below. + transient: boolean; }; +// A revoked/expired installation token mid-request, a GitHub 5xx, or a rate-limit blip are all momentary — the +// job should retry, not silently drop a computed review. A 4xx auth/permission/not-found error is not: retrying +// forever would never converge, so it keeps today's swallow-and-audit behavior. +function isGitHubTransientPublishError(error: unknown): boolean { + if (isGitHubRateLimitedError(error)) return true; + const status = githubErrorStatus(error); + return status !== null && status >= 500; +} + // Intentionally writes to check_summaries only, not audit_events (#2908): this fires on every successful gate- // check publish, which is a very high-frequency event (every review pass, potentially several times per PR as // it iterates) -- check_summaries is the purpose-built, already-queryable canonical record for "when was this @@ -6926,6 +6940,22 @@ export async function enrichOpenPullRequestsWithChangedFiles(env: Env, repoFullN }); } +// GITTENSORY-5: a transient publish failure (rate limit / GitHub 5xx / momentary token issue) used to be +// swallowed and only audited — the job still completed "successfully" from the queue's point of view, so a +// review that computed real output silently never reached the PR, with no retry. Extending RetryableJobError +// (same shape as RetryablePullRequestFreshnessUnavailableError / PrActuationLockContendedError above) makes the +// queue retry the whole job instead. Thrown only when NOTHING published at all (see finishPublicSurfacePublication) +// and at least one failure was transient — a permanent 4xx keeps today's swallow-and-audit behavior. +class RetryablePublicSurfacePublishFailedError extends RetryableJobError { + constructor(repoFullName: string, prNumber: number) { + super(`public-surface publish failed transiently for ${repoFullName}#${prNumber}; retrying`, { + retryAfterMs: 60_000, + retryKind: "public_surface_publish_transient", + }); + this.name = "RetryablePublicSurfacePublishFailedError"; + } +} + async function maybePublishPrPublicSurface( env: Env, installationId: number, @@ -7367,6 +7397,13 @@ async function maybePublishPrPublicSurface( head_sha: advisory.headSha, failedOutputs: failedOutputs.map((failure) => failure.output), }); + // At least one output failed for a reason that can plausibly clear on its own (rate limit / 5xx / momentary + // token issue) — retry the whole job instead of leaving the review permanently unposted. A mix of transient + // and permanent failures still retries: the permanent one re-fails identically next pass and re-audits, but + // the transient one gets the chance it needs, and nothing here is published twice (publishedOutputs is empty). + if (failedOutputs.some((failure) => failure.transient)) { + throw new RetryablePublicSurfacePublishFailedError(repoFullName, pr.number); + } } if (gateSurfaceIncomplete) { await recordAuditEvent(env, { @@ -8524,6 +8561,7 @@ async function maybePublishPrPublicSurface( failedOutputs.push({ output: "check_run", error: checkRunResult.warning, + transient: false, }); await recordAuditEvent(env, { eventType: "github_app.check_run_permission_missing", @@ -8539,7 +8577,7 @@ async function maybePublishPrPublicSurface( } } catch (error) { const message = errorMessage(error); - failedOutputs.push({ output: "check_run", error: message }); + failedOutputs.push({ output: "check_run", error: message, transient: isGitHubTransientPublishError(error) }); await recordPublicSurfaceOutputFailure( env, "check_run", @@ -8824,7 +8862,7 @@ async function maybePublishPrPublicSurface( incr("gittensory_reviews_published_total", { repo: repoFullName }); } catch (error) { const message = errorMessage(error); - failedOutputs.push({ output: "comment", error: message }); + failedOutputs.push({ output: "comment", error: message, transient: isGitHubTransientPublishError(error) }); await recordPublicSurfaceOutputFailure( env, "comment", @@ -8879,7 +8917,7 @@ async function maybePublishPrPublicSurface( publishedOutputs.push("label"); } catch (error) { const message = errorMessage(error); - failedOutputs.push({ output: "label", error: message }); + failedOutputs.push({ output: "label", error: message, transient: isGitHubTransientPublishError(error) }); await recordPublicSurfaceOutputFailure( env, "label", diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 63f3193ec2..c0f26b5412 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -14171,7 +14171,7 @@ describe("queue processors", () => { expect(audit?.detail).toMatch(/Checks: write permission is missing/i); }); - it("audits advisory context check publish failures without blocking webhook processing", async () => { + it("audits advisory context check publish failures AND retries the job (GitHub 5xx is transient, GITTENSORY-5)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( env, @@ -14212,7 +14212,7 @@ describe("queue processors", () => { pull_request: { number: 25, title: "Context check", state: "open", user: { login: "contributor" }, head: { sha: "context500" }, labels: [], body: "No issue needed." }, }, }), - ).resolves.toBeUndefined(); + ).rejects.toMatchObject({ retryKind: "public_surface_publish_transient" }); const outputFailure = await env.DB.prepare("select event_type, detail from audit_events where event_type = ?") .bind("github_app.pr_check_run_publish_failed") @@ -14224,7 +14224,9 @@ describe("queue processors", () => { .first<{ detail: string; metadata_json: string }>(); expect(aggregate).toMatchObject({ detail: "check_run" }); expect(aggregate?.metadata_json).toContain('"output":"check_run"'); - // The total publish failure (nothing reached the PR) escalates to Sentry at error level, not just the ledger. + expect(aggregate?.metadata_json).toContain('"transient":true'); + // The total publish failure (nothing reached the PR) escalates to Sentry at error level, not just the ledger — + // this still fires BEFORE the retryable throw, so the failure stays observable even though the job also retries. expect(captureSpy).toHaveBeenCalledWith(expect.any(Error), expect.objectContaining({ kind: "publish", repo: "JSONbored/gittensory" })); captureSpy.mockRestore(); }); @@ -14357,7 +14359,7 @@ describe("queue processors", () => { expect(published?.metadata_json).toContain('"output":"comment"'); }); - it("records an aggregate public-surface failure when no configured output publishes", async () => { + it("records an aggregate public-surface failure when no configured output publishes (permanent failure, no retry)", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await persistRegistrySnapshot( env, @@ -14384,29 +14386,150 @@ describe("queue processors", () => { if (url.includes("/users/oktofeesh1/repos")) return Response.json([]); if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); if (url.includes("/issues/31/comments") && method === "GET") return Response.json([]); - if (url.includes("/issues/31/comments") && method === "POST") return new Response("comment failed", { status: 503 }); + // A 403 with no rate-limit signal (permissions revoked, not a burst limit) is PERMANENT: retrying forever + // would never converge, so this must keep today's swallow-and-audit behavior, not throw a retryable error. + if (url.includes("/issues/31/comments") && method === "POST") return new Response(JSON.stringify({ message: "Resource not accessible by integration" }), { status: 403 }); return new Response("not found", { status: 404 }); }); - await processJob(env, { - type: "github-webhook", - deliveryId: "all-public-outputs-failed", - eventName: "pull_request", - payload: { - action: "opened", - installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, - repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, - pull_request: { number: 31, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, - }, - }); + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "all-public-outputs-failed", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request: { number: 31, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, + }, + }), + ).resolves.toBeUndefined(); const aggregate = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") .bind("github_app.pr_public_surface_failed") .first<{ detail: string; metadata_json: string }>(); expect(aggregate).toMatchObject({ detail: "comment" }); expect(aggregate?.metadata_json).toContain('"output":"comment"'); + expect(aggregate?.metadata_json).toContain('"transient":false'); + const published = await env.DB.prepare("select event_type from audit_events where event_type = ?").bind("github_app.pr_public_surface_published").all(); + expect(published.results).toEqual([]); + const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("all-public-outputs-failed").first<{ status: string }>(); + expect(webhookRow?.status).toBe("processed"); + }); + + it("retries the whole job when a transient GitHub 5xx drops every public-surface output (GITTENSORY-5)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicSurface: "comment_only", + checkRunMode: "off", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([{ githubUsername: "oktofeesh1", githubId: "123", totalPrs: 2, totalMergedPrs: 2, isEligible: true, credibility: 1 }]); + if (url === "https://api.gittensor.io/miners/123") return Response.json({ repositories: [] }); + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1" }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/32/comments") && method === "GET") return Response.json([]); + // GitHub 5xx during publish: momentary, not the caller's fault — the job must retry, not silently drop the + // review the same way JSONbored/awesome-claude#4251 did (Sentry GITTENSORY-5). + if (url.includes("/issues/32/comments") && method === "POST") return new Response("upstream unavailable", { status: 502 }); + return new Response("not found", { status: 404 }); + }); + + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "transient-publish-failure", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request: { number: 32, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, + }, + }), + ).rejects.toMatchObject({ retryKind: "public_surface_publish_transient" }); + + // The failure IS still audited (observability doesn't regress) — it just also throws so the queue retries. + const aggregate = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") + .bind("github_app.pr_public_surface_failed") + .first<{ detail: string; metadata_json: string }>(); + expect(aggregate).toMatchObject({ detail: "comment" }); + expect(aggregate?.metadata_json).toContain('"transient":true'); const published = await env.DB.prepare("select event_type from audit_events where event_type = ?").bind("github_app.pr_public_surface_published").all(); expect(published.results).toEqual([]); + // The webhook row is marked "error", not "processed" — a thrown job is exactly what lets the queue retry it. + const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("transient-publish-failure").first<{ status: string }>(); + expect(webhookRow?.status).toBe("error"); + }); + + it("leaves a fully successful public-surface publish unaffected by the transient-retry check", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicSurface: "comment_only", + checkRunMode: "off", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([{ githubUsername: "oktofeesh1", githubId: "123", totalPrs: 2, totalMergedPrs: 2, isEligible: true, credibility: 1 }]); + if (url === "https://api.gittensor.io/miners/123") return Response.json({ repositories: [] }); + if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]); + if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] }); + if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1" }); + if (url.includes("/users/oktofeesh1/repos")) return Response.json([]); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/issues/33/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/33/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + return new Response("not found", { status: 404 }); + }); + + await expect( + processJob(env, { + type: "github-webhook", + deliveryId: "public-surface-clean-publish", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: true, owner: { login: "JSONbored" } }, + pull_request: { number: 33, title: "Miner work", state: "open", user: { login: "oktofeesh1" }, labels: [], body: "Fixes #1" }, + }, + }), + ).resolves.toBeUndefined(); + + const webhookRow = await env.DB.prepare("select status from webhook_events where delivery_id = ?").bind("public-surface-clean-publish").first<{ status: string }>(); + expect(webhookRow?.status).toBe("processed"); + const failed = await env.DB.prepare("select event_type from audit_events where event_type = ?").bind("github_app.pr_public_surface_failed").all(); + expect(failed.results).toEqual([]); + const published = await env.DB.prepare("select metadata_json from audit_events where event_type = ?").bind("github_app.pr_public_surface_published").first<{ metadata_json: string }>(); + expect(published?.metadata_json).toContain('"publishedOutputs":["comment"]'); + expect(published?.metadata_json).toContain('"failedOutputs":[]'); }); it("keeps repository and PR webhook processing internal when installation context is absent", async () => { @@ -14561,7 +14684,9 @@ describe("queue processors", () => { if (url.includes("/labels") && method === "GET") return Response.json([]); if (url.includes("/labels") && method === "POST") { calls.labels += 1; - return new Response("label failed", { status: 503 }); + // A permanent failure (permissions gap, not a momentary blip) — this test is about duplicate-comment + // suppression on a label-only surface, not about retry classification, so it must stay non-transient. + return new Response(JSON.stringify({ message: "Resource not accessible by integration" }), { status: 403 }); } return new Response("not found", { status: 404 }); }); @@ -14580,13 +14705,14 @@ describe("queue processors", () => { }), ).resolves.toBeUndefined(); - // gittensor context-label apply (fails 503, recorded) + the best-effort type-label create attempt (also 503, + // gittensor context-label apply (fails 403, recorded) + the best-effort type-label create attempt (also 403, // swallowed). The context-label failure is still recorded below; the type label never drops the recording. expect(calls).toEqual({ comments: 0, labels: 2 }); const outputFailure = await env.DB.prepare("select event_type, detail from audit_events where event_type = ?") .bind("github_app.pr_label_publish_failed") .first<{ event_type: string; detail: string }>(); - expect(outputFailure).toMatchObject({ event_type: "github_app.pr_label_publish_failed", detail: "label failed" }); + expect(outputFailure?.event_type).toBe("github_app.pr_label_publish_failed"); + expect(outputFailure?.detail).toMatch(/Resource not accessible by integration/); const aggregate = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") .bind("github_app.pr_public_surface_failed") .first<{ detail: string; metadata_json: string }>();