From b4692451587742ae561393435fa72a29199fd44c Mon Sep 17 00:00:00 2001 From: Andriy Polanski Date: Mon, 27 Jul 2026 14:59:22 +0000 Subject: [PATCH 1/2] test: align retention and salvageability suites with #9083 and #9085 Update the Postgres retention mock for PK-ordered deletes and flip the confidence-less salvageability assertion to match CONFIDENCE_WHEN_UNSTATED. Co-authored-by: Cursor --- test/unit/salvageability.test.ts | 10 +++++++--- test/unit/selfhost-pg-retention.test.ts | 21 ++++++++++++++------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/test/unit/salvageability.test.ts b/test/unit/salvageability.test.ts index 2f4c8c843f..2a22834f3d 100644 --- a/test/unit/salvageability.test.ts +++ b/test/unit/salvageability.test.ts @@ -81,11 +81,15 @@ describe("resolveAiReviewSalvageableHold", () => { expect(hold!.comment).toContain("HELD with guidance"); }); - it("defaults: no configured floor uses the gate default, and a confidence-less blocker counts as certainty (at/above floor)", () => { + it("defaults: no configured floor uses the gate default; a confidence-less blocker is sub-floor (#9085), so salvageability stands down", () => { + // With an explicit high confidence and no configured floor, the default close floor (0.93) applies and + // the salvageability hold fires. + expect(resolveAiReviewSalvageableHold(aiEval(0.99), { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeDefined(); + // #9085: an ABSENT confidence degrades to CONFIDENCE_WHEN_UNSTATED (0.5), below the default floor — the + // low-confidence hold owns that case; salvageability must not treat silence as certainty. const noConfidence = aiEval(0.99); delete (noConfidence.blockers[0] as { confidence?: number }).confidence; - const hold = resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv); - expect(hold).toBeDefined(); + expect(resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeUndefined(); }); it("knob unset (the default) or no score: never changes a disposition", () => { diff --git a/test/unit/selfhost-pg-retention.test.ts b/test/unit/selfhost-pg-retention.test.ts index 96c9742d70..28264ee39e 100644 --- a/test/unit/selfhost-pg-retention.test.ts +++ b/test/unit/selfhost-pg-retention.test.ts @@ -1,9 +1,10 @@ // Unit tests for data-retention pruning (src/db/retention.ts) against the Postgres backend (#977). Mocks // pg.Pool so no real DB is needed — real-Postgres integration coverage lives in test/integration/selfhost-pg.test.ts. -// retention.ts itself is unchanged: it still emits SQLite-dialect SQL (rowid, `?1`-style numbered -// placeholders); src/selfhost/pg-dialect.ts's translateSql() is what makes it Postgres-safe, same as every -// other query path on this backend. These tests exercise that translation end-to-end through the real -// pruneExpiredRecords()/processJob() call path, not just the dialect translator in isolation. +// retention.ts emits SQLite-dialect SQL (`?1`-style numbered placeholders; `rowid` only as a fallback for +// tables without a mapped single-column PK — #9083 prefers a real indexable PK like `id`/`delivery_id`); +// src/selfhost/pg-dialect.ts's translateSql() is what makes it Postgres-safe (including rowid→ctid), same +// as every other query path on this backend. These tests exercise that translation end-to-end through the +// real pruneExpiredRecords()/processJob() call path, not just the dialect translator in isolation. import { describe, expect, it, vi } from "vitest"; import type { Pool } from "pg"; import { createPgAdapter } from "../../src/selfhost/pg-adapter"; @@ -18,7 +19,9 @@ interface MockPgPool { /** A minimal fake Postgres that also acts as a regression guard: if untranslated SQLite SQL (the `rowid` * pseudo-column) ever reaches it again, it throws the exact error a real Postgres raised in the live - * self-host incident (dead-lettered job `_selfhost_jobs.id = 61132`, `prune-retention`, 5 attempts). */ + * self-host incident (dead-lettered job `_selfhost_jobs.id = 61132`, `prune-retention`, 5 attempts). + * #9083: production deletes by a real PK (`id` / `delivery_id`) when mapped; the mock must accept that + * shape (and the rowid→ctid fallback for unmapped tables), not only the pre-#9083 `ctid`-only form. */ function makeRetentionPgPool(remaining: Record = {}): MockPgPool { const calls: string[] = []; const fn = vi.fn().mockImplementation(async (sql: unknown) => { @@ -32,10 +35,14 @@ function makeRetentionPgPool(remaining: Record = {}): MockPgPool return { rows: [{ n: remaining[table] ?? 0 }], rowCount: 1 }; } - const deleteMatch = /^DELETE FROM (\w+) WHERE ctid IN \(SELECT ctid FROM \1 WHERE .*? LIMIT (\d+)\)$/i.exec(q); + // #9083 PK-ordered delete: `DELETE FROM t WHERE IN (SELECT FROM t WHERE … LIMIT n)` + // (pk is usually `id`/`delivery_id`; unmapped tables fall back to rowid→ctid after dialect translate). + const deleteMatch = + /^DELETE FROM (\w+) WHERE (\w+) IN \(SELECT \2 FROM \1 WHERE .*? LIMIT (\d+)\)$/i.exec(q) ?? + /^DELETE FROM (\w+) WHERE ctid IN \(SELECT ctid FROM \1 WHERE .*? LIMIT (\d+)\)$/i.exec(q); if (deleteMatch) { const table = deleteMatch[1] as string; - const limit = Number(deleteMatch[2]); + const limit = Number(deleteMatch[deleteMatch.length - 1]); const have = remaining[table] ?? 0; const changes = Math.min(have, limit); remaining[table] = have - changes; From 1c649a149058610973567240342b9cdd2b736256 Mon Sep 17 00:00:00 2001 From: Andriy Polanski Date: Mon, 27 Jul 2026 15:26:03 +0000 Subject: [PATCH 2/2] add #9315 --- src/github/app.ts | 94 +++++++++++++++++++++--------------- test/unit/github-app.test.ts | 73 ++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 38 deletions(-) diff --git a/src/github/app.ts b/src/github/app.ts index 3aea62ecc4..53420b44ee 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -483,33 +483,41 @@ export async function getRepositoryCollaboratorPermission( const parsed = parseRepoFullNameStrict(repoFullName); if (!parsed || !login) return null; const { owner, repo: name } = parsed; - const token = await createInstallationToken(env, installationId); - const response = await timeoutFetch( - `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/collaborators/${encodeURIComponent(login)}/permission`, - { - headers: githubHeaders({ token, json: true }), - githubRateLimitAdmission: true, - githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(installationId), - }, - ); - if (response.status === 404) return null; - if (!response.ok) { - const body = await response.text(); - throw new Error( - `Failed to fetch GitHub collaborator permission (${response.status}): ${body.slice(0, 200)}`, + // Route through withInstallationTokenRetry so a stale cached installation token self-heals once + // (#9315) — same convention every sibling GitHub helper in this directory already uses (#6191 / #8892). + // Fail-closed callers (requireRepoWriteAccess / isPerTenantAdmin) must not deny a legitimate + // collaborator on a transient bad-credentials 401. + return await withInstallationTokenRetry(env, installationId, async (token) => { + const response = await timeoutFetch( + `https://api.github.com/repos/${encodeURIComponent(owner)}/${encodeURIComponent(name)}/collaborators/${encodeURIComponent(login)}/permission`, + { + headers: githubHeaders({ token, json: true }), + githubRateLimitAdmission: true, + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(installationId), + }, ); - } - const payload = (await response.json()) as { - permission?: GitHubRepositoryCollaboratorPermission; - role_name?: string; - }; - // #9152: `permission` is GitHub's coarse legacy field — only ever admin/write/read/none, so Maintain collapses - // into "write" and Triage into "read" — making `permission === "maintain"` dead at both call sites - // (isPerTenantAdmin, resolveRealRepoPermissionAssociation). `role_name` carries the actual granular tier - // (including "maintain"/"triage", or a custom org role) and is always present on this endpoint's real response; - // prefer it, falling back to `permission` only for a response that omits it (defensive — keeps the pre-#9152 - // behavior for any caller/fixture that only ever set `permission`). - return payload.role_name ?? payload.permission ?? null; + if (response.status === 404) return null; + if (!response.ok) { + const body = await response.text(); + // Attach `status` so withInstallationTokenRetry's isGitHubBadCredentialsError / permission-scope + // detectors can recognize a 401/403 even when the body text is empty or non-matching. + throw Object.assign( + new Error(`Failed to fetch GitHub collaborator permission (${response.status}): ${body.slice(0, 200)}`), + { status: response.status }, + ); + } + const payload = (await response.json()) as { + permission?: GitHubRepositoryCollaboratorPermission; + role_name?: string; + }; + // #9152: `permission` is GitHub's coarse legacy field — only ever admin/write/read/none, so Maintain collapses + // into "write" and Triage into "read" — making `permission === "maintain"` dead at both call sites + // (isPerTenantAdmin, resolveRealRepoPermissionAssociation). `role_name` carries the actual granular tier + // (including "maintain"/"triage", or a custom org role) and is always present on this endpoint's real response; + // prefer it, falling back to `permission` only for a response that omits it (defensive — keeps the pre-#9152 + // behavior for any caller/fixture that only ever set `permission`). + return payload.role_name ?? payload.permission ?? null; + }); } /** Account-age throttle (#2561, anti-abuse): `GET /users/{login}` for `created_at`, so a repo can flag a @@ -524,18 +532,28 @@ export async function getGithubUserCreatedAt( ): Promise { if (!login) return null; try { - const token = await createInstallationToken(env, installationId); - const response = await timeoutFetch( - `https://api.github.com/users/${encodeURIComponent(login)}`, - { - headers: githubHeaders({ token, json: true }), - githubRateLimitAdmission: true, - githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(installationId), - }, - ); - if (!response.ok) return null; - const payload = (await response.json()) as { created_at?: unknown }; - return typeof payload.created_at === "string" ? payload.created_at : null; + // withInstallationTokenRetry (#9315): a stale-token 401 must re-mint once rather than fail-open to null + // and silently disable the account-age check. Non-token failures still fail open below. + return await withInstallationTokenRetry(env, installationId, async (token) => { + const response = await timeoutFetch( + `https://api.github.com/users/${encodeURIComponent(login)}`, + { + headers: githubHeaders({ token, json: true }), + githubRateLimitAdmission: true, + githubRateLimitAdmissionKey: githubRateLimitAdmissionKeyForInstallation(installationId), + }, + ); + if (response.status === 401 || response.status === 403) { + const body = await response.text(); + throw Object.assign( + new Error(`Failed to fetch GitHub user created_at (${response.status}): ${body.slice(0, 200)}`), + { status: response.status }, + ); + } + if (!response.ok) return null; + const payload = (await response.json()) as { created_at?: unknown }; + return typeof payload.created_at === "string" ? payload.created_at : null; + }); } catch { return null; } diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index ce0472e50e..7bd5df90b3 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -838,6 +838,32 @@ describe("GitHub check runs", () => { ).rejects.toThrow(/Failed to fetch GitHub collaborator permission/); }); + it("#9315: getRepositoryCollaboratorPermission self-heals a stale cached installation token (401 -> re-mint -> permission)", async () => { + // Same bug class as #6191/#8892: this read fed a stale cached token straight into the collaborator + // permission fetch with no retry, so a transient 401 failed closed for requireRepoWriteAccess / + // isPerTenantAdmin. It now routes through withInstallationTokenRetry. + let rejected = false; + setInstallationTokenStore({ + get: async () => ({ token: rejected ? "fresh-token" : "stale-token", expiresAtMs: Date.now() + 60 * 60_000 }), + set: async () => {}, + }); + const seenTokens: string[] = []; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + const token = (new Headers(init?.headers).get("authorization") ?? "").replace(/^Bearer\s+/i, ""); + seenTokens.push(token); + if (token === "stale-token") { + rejected = true; + return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 }); + } + return Response.json({ permission: "write", role_name: "maintain" }); + }); + + await expect( + getRepositoryCollaboratorPermission(createTestEnv(), 123, "JSONbored/gittensory", "maintainer"), + ).resolves.toBe("maintain"); + expect(seenTokens).toEqual(["stale-token", "fresh-token"]); + }); + it("getGithubUserCreatedAt fetches the account creation date, and fails OPEN (null) on any error (#2561)", async () => { const privateKey = await generatePrivateKeyPem(); await expect(getGithubUserCreatedAt(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "")).resolves.toBeNull(); @@ -871,6 +897,53 @@ describe("GitHub check runs", () => { ).resolves.toBeNull(); }); + it("#9315: getGithubUserCreatedAt self-heals a stale cached installation token (401 -> re-mint -> created_at)", async () => { + // Same bug class as #6191/#8892: a stale-token 401 previously fail-opened to null and silently disabled + // the account-age throttle. It now routes through withInstallationTokenRetry; non-token failures still + // fail open (covered by the existing #2561 test above). + let rejected = false; + setInstallationTokenStore({ + get: async () => ({ token: rejected ? "fresh-token" : "stale-token", expiresAtMs: Date.now() + 60 * 60_000 }), + set: async () => {}, + }); + const seenTokens: string[] = []; + vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => { + const token = (new Headers(init?.headers).get("authorization") ?? "").replace(/^Bearer\s+/i, ""); + seenTokens.push(token); + if (token === "stale-token") { + rejected = true; + return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 }); + } + return Response.json({ login: "newbie", created_at: "2026-06-01T00:00:00Z" }); + }); + + await expect(getGithubUserCreatedAt(createTestEnv(), 123, "newbie")).resolves.toBe("2026-06-01T00:00:00Z"); + expect(seenTokens).toEqual(["stale-token", "fresh-token"]); + }); + + it("#9315: getGithubUserCreatedAt still fail-opens when a 401 persists after the retry", async () => { + setInstallationTokenStore({ + get: async () => ({ token: "always-stale", expiresAtMs: Date.now() + 60 * 60_000 }), + set: async () => {}, + }); + vi.stubGlobal("fetch", async () => new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 })); + await expect(getGithubUserCreatedAt(createTestEnv(), 123, "newbie")).resolves.toBeNull(); + }); + + it("#9315: getGithubUserCreatedAt still fail-opens on a non-retryable 403", async () => { + // 403 without the installation permission-scope message is not self-healable — throw propagates out of + // withInstallationTokenRetry and the outer catch returns null (same fail-open contract as before). + const privateKey = await generatePrivateKeyPem(); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + return new Response(JSON.stringify({ message: "forbidden" }), { status: 403 }); + }); + await expect( + getGithubUserCreatedAt(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "newbie"), + ).resolves.toBeNull(); + }); + it("cancelInFlightWorkflowRunsForHeadSha lists in_progress + queued runs at a head SHA and cancels each (#2462)", async () => { const privateKey = await generatePrivateKeyPem(); const cancelledIds: number[] = [];