Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ import type {
RepositorySettings,
} from "../types";
import { errorMessage, nowIso, repoParts, strippedErrorMessage } from "../utils/json";
import { createInstallationToken, getAppInstallation } from "./app";
import { createInstallationToken, getAppInstallation, withInstallationTokenRetry } from "./app";
import {
GITTENSORY_LEGACY_CONTEXT_CHECK_NAME,
GITTENSORY_LEGACY_GATE_CHECK_NAME,
Expand Down Expand Up @@ -3456,8 +3456,17 @@ export async function fetchLiveIssueState(
issueNumber: number,
token: string | undefined,
admissionKey?: GitHubRateLimitAdmissionKey,
// When supplied, the read self-heals a stale cached installation token by re-minting once on a 401, matching
// the write-path convention (#6191) — the token argument is then owned by withInstallationTokenRetry. Omit it
// (public-token / no-installation reads) to fetch with the passed token exactly as before.
installationId?: number,
): Promise<string | undefined> {
const result = await githubJsonWithHeaders<{ state?: string | null }>(env, repoFullName, `/issues/${issueNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined);
const run = (accessToken: string | undefined) =>
githubJsonWithHeaders<{ state?: string | null }>(env, repoFullName, `/issues/${issueNumber}`, accessToken, githubRateLimitOptions(admissionKey));
const result = await (installationId === undefined
? run(token)
: withInstallationTokenRetry(env, installationId, run)
).catch(() => undefined);
return result?.data.state ?? undefined;
}

Expand All @@ -3471,8 +3480,15 @@ export async function fetchLivePullRequestHeadSha(
prNumber: number,
token: string | undefined,
admissionKey?: GitHubRateLimitAdmissionKey,
// See fetchLiveIssueState: when supplied, a stale cached installation token self-heals via one re-mint (#6191).
installationId?: number,
): Promise<string | undefined> {
const result = await githubJsonWithHeaders<{ head?: { sha?: string | null } | null }>(env, repoFullName, `/pulls/${prNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined);
const run = (accessToken: string | undefined) =>
githubJsonWithHeaders<{ head?: { sha?: string | null } | null }>(env, repoFullName, `/pulls/${prNumber}`, accessToken, githubRateLimitOptions(admissionKey));
const result = await (installationId === undefined
? run(token)
: withInstallationTokenRetry(env, installationId, run)
).catch(() => undefined);
return result?.data.head?.sha ?? undefined;
}

Expand All @@ -3486,9 +3502,16 @@ export async function fetchLivePullRequestResult(
prNumber: number,
token: string | undefined,
admissionKey?: GitHubRateLimitAdmissionKey,
// See fetchLiveIssueState: when supplied, a stale cached installation token self-heals via one re-mint (#6191),
// so a transient 401 is retried rather than surfaced as an "error" the freshness check fails closed on.
installationId?: number,
): Promise<LivePullRequestFetchResult> {
const run = (accessToken: string | undefined) =>
githubJsonWithHeaders<GitHubPullRequestPayload>(env, repoFullName, `/pulls/${prNumber}`, accessToken, githubRateLimitOptions(admissionKey));
try {
const result = await githubJsonWithHeaders<GitHubPullRequestPayload>(env, repoFullName, `/pulls/${prNumber}`, token, githubRateLimitOptions(admissionKey));
const result = installationId === undefined
? await run(token)
: await withInstallationTokenRetry(env, installationId, run);
return { status: "ok", data: result.data };
} catch (error) {
return { status: "error", error: strippedErrorMessage(error, "GitHub live PR fetch failed").slice(0, 240) };
Expand Down
22 changes: 16 additions & 6 deletions src/github/pr-freshness.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,11 @@ export async function fetchPullRequestFreshness(
const options: PullRequestFreshnessOptions =
args.requireDraft !== undefined ? { requireDraft: args.requireDraft } : {};
let tokenError: unknown;
const token =
(await createInstallationToken(env, args.installationId).catch((error) => {
tokenError = error;
return undefined;
})) ?? env.GITHUB_PUBLIC_TOKEN;
const installationToken = await createInstallationToken(env, args.installationId).catch((error) => {
tokenError = error;
return undefined;
});
const token = installationToken ?? env.GITHUB_PUBLIC_TOKEN;
if (!token) {
return classifyPullRequestFreshness(undefined, args.expectedHeadSha, {
...options,
Expand All @@ -121,7 +121,17 @@ export async function fetchPullRequestFreshness(
});
}
const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, args.installationId);
const live = await fetchLivePullRequestResult(env, args.repoFullName, args.pullNumber, token, admissionKey);
// Route through the read helper's self-heal ONLY when we hold an installation token: a stale cached one then
// re-mints once on a 401 (#6191) instead of failing the freshness check closed. A public-token fallback has no
// installation to re-mint, so it fetches with the token as before.
const live = await fetchLivePullRequestResult(
env,
args.repoFullName,
args.pullNumber,
token,
admissionKey,
installationToken !== undefined ? args.installationId : undefined,
);
if (live.status === "error") {
return classifyPullRequestFreshness(undefined, args.expectedHeadSha, {
...options,
Expand Down
89 changes: 89 additions & 0 deletions test/unit/live-read-token-self-heal.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import {
fetchLiveIssueState,
fetchLivePullRequestHeadSha,
fetchLivePullRequestResult,
} from "../../src/github/backfill";
import {
clearInstallationTokenCacheForTest,
setInstallationTokenStore,
} from "../../src/github/app";
import { createTestEnv } from "../helpers/d1";

// These read helpers historically fed a (possibly-stale) cached installation token straight into a raw GitHub
// GET with no retry, so a single 401 was surfaced as unavailable/undefined -- failing the reopen-guard and
// gate-override re-checks closed on a transient token issue. Passing an installationId now routes the read
// through withInstallationTokenRetry (#6191): the stale token is evicted, a fresh one is minted once, and the
// read succeeds -- matching the write-path self-heal convention.

function authToken(init: RequestInit | undefined): string {
return (new Headers(init?.headers).get("authorization") ?? "").replace(/^Bearer\s+/i, "");
}

// A cache store whose get() hands out `stale-token` until the first rejection, then `fresh-token`; the fetch
// stub rejects the stale token with a 401 exactly once and serves `body` to the freshly-minted token. Returns
// the ordered list of tokens the network actually saw so a test can prove the retry re-minted.
function seedSelfHealingToken(body: unknown): { seenTokens: string[] } {
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 = authToken(init);
seenTokens.push(token);
if (token === "stale-token") {
rejected = true;
return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 });
}
return Response.json(body);
});
return { seenTokens };
}

describe("live read helpers self-heal a stale cached installation token (#6191)", () => {
afterEach(() => {
vi.unstubAllGlobals();
setInstallationTokenStore(null);
clearInstallationTokenCacheForTest();
});

it("fetchLivePullRequestResult retries a 401 with a freshly-minted token", async () => {
const env = createTestEnv();
const { seenTokens } = seedSelfHealingToken({ state: "open", head: { sha: "sha7" } });
const result = await fetchLivePullRequestResult(env, "owner/repo", 7, "stale-token", undefined, 123);
expect(result).toEqual({ status: "ok", data: { state: "open", head: { sha: "sha7" } } });
expect(seenTokens).toEqual(["stale-token", "fresh-token"]);
});

it("fetchLiveIssueState retries a 401 with a freshly-minted token", async () => {
const env = createTestEnv();
const { seenTokens } = seedSelfHealingToken({ state: "open" });
expect(await fetchLiveIssueState(env, "owner/repo", 42, "stale-token", undefined, 123)).toBe("open");
expect(seenTokens).toEqual(["stale-token", "fresh-token"]);
});

it("fetchLivePullRequestHeadSha retries a 401 with a freshly-minted token", async () => {
const env = createTestEnv();
const { seenTokens } = seedSelfHealingToken({ head: { sha: "live-sha" } });
expect(await fetchLivePullRequestHeadSha(env, "owner/repo", 90, "stale-token", undefined, 123)).toBe("live-sha");
expect(seenTokens).toEqual(["stale-token", "fresh-token"]);
});

// Without an installationId the helpers keep their prior behavior exactly: the passed token is used as-is and a
// 401 is NOT retried (surfaced as error/undefined), since a public-token read has no installation to re-mint.
it("does not retry when no installationId is supplied (public-token read path unchanged)", async () => {
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
const seenTokens: string[] = [];
vi.stubGlobal("fetch", async (_input: RequestInfo | URL, init?: RequestInit) => {
seenTokens.push(authToken(init));
return new Response(JSON.stringify({ message: "Bad credentials" }), { status: 401 });
});
const result = await fetchLivePullRequestResult(env, "owner/repo", 7, "public-token");
expect(result.status).toBe("error");
expect(await fetchLiveIssueState(env, "owner/repo", 42, "public-token")).toBeUndefined();
expect(await fetchLivePullRequestHeadSha(env, "owner/repo", 90, "public-token")).toBeUndefined();
expect(seenTokens).toEqual(["public-token", "public-token", "public-token"]);
});
});
37 changes: 37 additions & 0 deletions test/unit/pr-freshness.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,17 @@ import {
pullRequestFreshnessDetail,
reviewedPullRequestHeadSha,
} from "../../src/github/pr-freshness";
import {
clearInstallationTokenCacheForTest,
setInstallationTokenStore,
} from "../../src/github/app";
import { createTestEnv } from "../helpers/d1";

describe("PR freshness guards", () => {
afterEach(() => {
vi.unstubAllGlobals();
setInstallationTokenStore(null);
clearInstallationTokenCacheForTest();
});

it("classifies a matching open head as current", () => {
Expand Down Expand Up @@ -220,6 +226,37 @@ describe("PR freshness guards", () => {
});
});

it("self-heals a stale cached installation token on the live PR-freshness read (401 -> re-mint -> current)", async () => {
// Regression for the read-path fail-closed bug: fetchPullRequestFreshness fed a stale cached installation
// token straight into the live PR fetch with no retry, so a single 401 was classified `status: "stale",
// reason: "unavailable"` -- failing the reopen-guard/gate-override re-check closed for what the write path
// would have transparently retried. It now routes through withInstallationTokenRetry (#6191).
const env = createTestEnv();
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({ state: "open", head: { sha: "sha7" } });
});
const result = await fetchPullRequestFreshness(env, {
installationId: 123,
repoFullName: "owner/repo",
pullNumber: 7,
expectedHeadSha: "sha7",
});
expect(result).toMatchObject({ status: "current", liveHeadSha: "sha7", liveState: "open" });
expect(seenTokens).toEqual(["stale-token", "fresh-token"]);
});

it("fails closed when no token can verify live PR state", async () => {
const env = createTestEnv();
const result = await fetchPullRequestFreshness(env, {
Expand Down