diff --git a/src/github/backfill.ts b/src/github/backfill.ts index a830339e04..ed258dd90f 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2150,6 +2150,15 @@ export async function fetchLivePullRequestState(env: Env, repoFullName: string, return result?.data.state ?? undefined; } +/** The PR's LIVE head commit SHA via REST `GET /pulls/{n}`. The stored `pr.headSha` lags GitHub when a commit + * lands between a webhook and its processing; the gate-override command (#16 / audit) re-fetches the live head + * so the neutral check-run targets the commit a maintainer is actually looking at, not a phantom old SHA. + * Best-effort: returns undefined on any error so the caller fails open to the stored head. */ +export async function fetchLivePullRequestHeadSha(env: Env, repoFullName: string, prNumber: number, token: string | undefined): Promise { + const result = await githubJsonWithHeaders<{ head?: { sha?: string | null } | null }>(env, repoFullName, `/pulls/${prNumber}`, token).catch(() => undefined); + return result?.data.head?.sha ?? undefined; +} + /** Resolve the OPEN PRs associated with a commit SHA via the REST `GET /repos/{owner}/{repo}/commits/{sha}/pulls` * endpoint. This is the only PR↔commit resolution that works for FORK (cross-repo) PRs, whose CI-completion * webhooks (`check_suite`/`check_run`) carry an EMPTY `pull_requests[]`. Returns the de-duplicated open PR numbers. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8f2ecb10ea..505b0d829d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -72,6 +72,7 @@ import { fetchAndStorePullRequestFilesForReview, fetchLinkedIssueFacts, fetchLiveCiAggregate, + fetchLivePullRequestHeadSha, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, fetchLivePullRequestState, @@ -3106,6 +3107,19 @@ async function recordGithubProductUsage( }).catch(() => undefined); } +/** + * Resolve the head SHA a `gate-override` should neutralize (#16 / audit). The stored `pr.headSha` lags GitHub + * when a commit lands between the override comment and its processing, so re-fetch the LIVE head and override + * THAT commit (the neutral check-run is per-commit by design). FAIL-OPEN: an unreadable live fetch returns the + * cached head, so a transient GitHub hiccup never strands the override — it just targets the stored SHA as before. + * Mirrors the rebase path's live re-fetch (prReadyForReview) and the dup-winner live reconcile. + */ +export async function resolveOverrideHeadSha(env: Env, installationId: number, repoFullName: string, pr: PullRequestRecord): Promise { + const token = (await createInstallationToken(env, installationId).catch(() => undefined)) ?? env.GITHUB_PUBLIC_TOKEN; + const liveHeadSha = await fetchLivePullRequestHeadSha(env, repoFullName, pr.number, token); + return liveHeadSha ?? pr.headSha; +} + /** * Handle `@gittensory gate-override ` on a PR thread. SECURITY-SENSITIVE: this finalizes the Gate * check to neutral for the current commit, so authorization MUST come from real repo permission @@ -3171,7 +3185,13 @@ async function maybeProcessGateOverrideCommand(env: Env, deliveryId: string, pay return true; } - const { advisory } = await buildAuthorizedPrActionAdvisory(env, repoFullName, pr, settings); + // #16 (audit): the cached pr.headSha can be stale if a commit landed between the comment and this processing. + // The override is a per-commit neutral check-run, so posting it on the cached SHA is a silent no-op on the LIVE + // head (whose Gate check stays blocking). Re-fetch the live head and override THAT commit (fail-open to the + // cached head), then thread it through the advisory so the check-run + audit target the right SHA. + const headForOverride = await resolveOverrideHeadSha(env, installationId, repoFullName, pr); + const prAtLiveHead = headForOverride === pr.headSha ? pr : { ...pr, headSha: headForOverride }; + const { advisory } = await buildAuthorizedPrActionAdvisory(env, repoFullName, prAtLiveHead, settings); const safeReason = sanitizePublicComment((command.reason ?? "").trim() || "No reason provided."); await createOrUpdateOverriddenGateCheckRun(env, installationId, repoFullName, advisory, { actor, reason: safeReason }); await recordAuditEvent(env, { @@ -3180,7 +3200,7 @@ async function maybeProcessGateOverrideCommand(env: Env, deliveryId: string, pay targetKey: `${repoFullName}#${pr.number}`, outcome: "completed", detail: safeReason, - metadata: { deliveryId, repoFullName, headSha: advisory.headSha ?? null }, + metadata: { deliveryId, repoFullName, headSha: advisory.headSha ?? null, cachedHeadSha: pr.headSha ?? null }, }); const confirmation = sanitizePublicComment( [ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 6514b32be2..3fdd01e66f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6214,6 +6214,139 @@ describe("queue processors", () => { expect(overrideAdvisory ?? null).toBeNull(); }); + it("overrides the LIVE head, not the stale cached SHA, when a commit landed after the command (#16)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + }); + // The stored row still carries the OLD head; a new commit ("live-sha") landed between the comment and now. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 90, + title: "Override me", + state: "open", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: { sha: "stale-sha" }, + labels: [], + body: "Validation: npm test", + }); + const seen = { staleCheckGets: 0, liveCheckGets: 0 }; + const patchBodies: Array<{ conclusion?: string }> = []; + 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("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); + // The LIVE head re-fetch — the row says stale-sha but GitHub's head is now live-sha. + if (url.includes("/pulls/90") && method === "GET") return Response.json({ number: 90, state: "open", head: { sha: "live-sha" } }); + if (url.includes("/commits/stale-sha/check-runs") && method === "GET") { + seen.staleCheckGets += 1; + return Response.json({ total_count: 0, check_runs: [] }); + } + if (url.includes("/commits/live-sha/check-runs") && method === "GET") { + seen.liveCheckGets += 1; + return Response.json({ total_count: 1, check_runs: [{ id: 556, name: "Gittensory Gate" }] }); + } + if (url.includes("/check-runs/556") && method === "PATCH") { + patchBodies.push(JSON.parse(String(init?.body ?? "{}")) as { conclusion?: string }); + return Response.json({ id: 556 }); + } + if (url.includes("/issues/90/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/90/comments") && method === "POST") return Response.json({ id: 9101 }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "gate-override-live-head", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 90, title: "Override me", state: "open", user: { login: "contributor" }, pull_request: {} }, + comment: { id: 803, body: "@gittensory gate-override flaky", author_association: "NONE", user: { login: "maintainer", type: "User" } }, + sender: { login: "maintainer", type: "User" }, + }, + }); + + // The neutral PATCH targeted the LIVE head's Gate run (id 556), and the stale SHA was never touched. + expect(seen.liveCheckGets).toBe(1); + expect(seen.staleCheckGets).toBe(0); + expect(patchBodies[0]?.conclusion).toBe("neutral"); + const audit = await env.DB.prepare("select metadata_json from audit_events where event_type = ?") + .bind("github_app.gate_overridden") + .first<{ metadata_json: string }>(); + const metadata = JSON.parse(audit?.metadata_json ?? "{}") as { headSha?: string; cachedHeadSha?: string }; + expect(metadata.headSha).toBe("live-sha"); + expect(metadata.cachedHeadSha).toBe("stale-sha"); + }); + + it("records null head SHAs in the override audit when the PR head is unresolved (#16 fail-safe)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "off", + publicSurface: "off", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + linkedIssueGateMode: "off", + }); + // A cached row with no head SHA (never detail-synced); the live fetch also yields no head. + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 90, + title: "Override me", + state: "open", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: {}, + labels: [], + body: "Validation: npm test", + }); + 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("/collaborators/maintainer/permission")) return Response.json({ permission: "admin" }); + if (url.includes("/pulls/90") && method === "GET") return Response.json({ number: 90, state: "open", head: {} }); + if (url.includes("/issues/90/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/90/comments") && method === "POST") return Response.json({ id: 9102 }); + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "gate-override-null-head", + eventName: "issue_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + issue: { number: 90, title: "Override me", state: "open", user: { login: "contributor" }, pull_request: {} }, + // No reason after the command — exercises the "No reason provided." fallback too. + comment: { id: 804, body: "@gittensory gate-override", author_association: "NONE", user: { login: "maintainer", type: "User" } }, + sender: { login: "maintainer", type: "User" }, + }, + }); + + const audit = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") + .bind("github_app.gate_overridden") + .first<{ detail: string; metadata_json: string }>(); + expect(audit?.detail).toBe("No reason provided."); + const metadata = JSON.parse(audit?.metadata_json ?? "{}") as { headSha?: string | null; cachedHeadSha?: string | null }; + expect(metadata.headSha).toBeNull(); + expect(metadata.cachedHeadSha).toBeNull(); + }); + it("ignores gate-override commands on edited comments", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); diff --git a/test/unit/resolve-override-head-sha.test.ts b/test/unit/resolve-override-head-sha.test.ts new file mode 100644 index 0000000000..6ed575aecc --- /dev/null +++ b/test/unit/resolve-override-head-sha.test.ts @@ -0,0 +1,62 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { resolveOverrideHeadSha } from "../../src/queue/processors"; +import { createInstallationToken } from "../../src/github/app"; +import type { PullRequestRecord } from "../../src/types"; +import { createTestEnv } from "../helpers/d1"; + +// resolveOverrideHeadSha re-fetches the live head via createInstallationToken + REST /pulls/{n}; mock the token +// mint (the test env holds no App key) and stub fetch per case. +vi.mock("../../src/github/app", async (importOriginal) => ({ + ...(await importOriginal()), + createInstallationToken: vi.fn(), +})); +const mockedToken = vi.mocked(createInstallationToken); + +function makePr(headSha: string | null): PullRequestRecord { + return { repoFullName: "owner/repo", number: 90, title: "Override me", state: "open", labels: [], linkedIssues: [], headSha }; +} + +function stubLiveHead(sha: string | null): void { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (String(input).includes("/pulls/90")) return new Response(JSON.stringify(sha === null ? {} : { head: { sha } }), { status: 200 }); + return new Response("not found", { status: 404 }); + }); +} + +describe("resolveOverrideHeadSha (#16 / gate-override stale head)", () => { + beforeEach(() => { + mockedToken.mockReset(); + }); + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("returns the LIVE head when the token mint succeeds (overrides the current commit, not the cached one)", async () => { + const env = createTestEnv(); + mockedToken.mockResolvedValue("inst-tok"); + stubLiveHead("live-sha"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("live-sha"); + expect(mockedToken).toHaveBeenCalledWith(env, 123); + }); + + it("falls back to the public token when the mint throws, and still resolves the live head", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-tok" }); + mockedToken.mockRejectedValue(new Error("no app key")); + stubLiveHead("live-sha"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("live-sha"); + }); + + it("fails OPEN to the cached head when the live fetch is unreadable", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-tok" }); + mockedToken.mockResolvedValue("inst-tok"); + vi.stubGlobal("fetch", async () => new Response("nope", { status: 500 })); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("stale-sha"); + }); + + it("returns the cached head when the live payload omits head.sha (fail-open)", async () => { + const env = createTestEnv(); + mockedToken.mockResolvedValue("inst-tok"); + stubLiveHead(null); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("stale-sha"); + }); +});