From d6b0225aefe0e43fbfded88c5bc9edf203b1c0c4 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Thu, 25 Jun 2026 23:51:24 -0700 Subject: [PATCH 1/3] fix(queue): reject stale gate overrides --- src/queue/processors.ts | 31 +++++++++++---------- test/unit/queue.test.ts | 20 ++++++------- test/unit/resolve-override-head-sha.test.ts | 14 +++++----- 3 files changed, 34 insertions(+), 31 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 547e378e7..0241a2e4b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3111,17 +3111,20 @@ async function recordGithubProductUsage( }).catch(() => undefined); } +export type OverrideHeadResolution = { headSha: string | null | undefined; stale: boolean; liveHeadSha?: string | 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. + * Resolve the head SHA a `gate-override` should neutralize (#16 / audit). The override is a maintainer approval + * for the cached PR head that existed when the command was queued; if GitHub now reports a different live head, + * treat the command as stale instead of neutralizing code the maintainer did not approve. FAIL-OPEN only when the + * live fetch is unreadable/omits head.sha, which preserves the old cached-SHA behavior without bypassing a newer + * live commit. */ -export async function resolveOverrideHeadSha(env: Env, installationId: number, repoFullName: string, pr: PullRequestRecord): Promise { +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; + if (liveHeadSha && pr.headSha && liveHeadSha !== pr.headSha) return { headSha: pr.headSha, stale: true, liveHeadSha }; + return { headSha: pr.headSha, stale: false, liveHeadSha }; } /** @@ -3189,13 +3192,13 @@ async function maybeProcessGateOverrideCommand(env: Env, deliveryId: string, pay return true; } - // #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 headResolution = await resolveOverrideHeadSha(env, installationId, repoFullName, pr); + if (headResolution.stale) { + await recordGateOverrideSkip(env, deliveryId, repoFullName, `${repoFullName}#${pr.number}`, actor, "stale_pr_head"); + return true; + } + const prAtResolvedHead = headResolution.headSha === pr.headSha ? pr : { ...pr, headSha: headResolution.headSha }; + const { advisory } = await buildAuthorizedPrActionAdvisory(env, repoFullName, prAtResolvedHead, settings); const safeReason = sanitizePublicComment((command.reason ?? "").trim() || "No reason provided."); await createOrUpdateOverriddenGateCheckRun(env, installationId, repoFullName, advisory, { actor, reason: safeReason }); await recordAuditEvent(env, { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 3fdd01e66..189021313 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6214,7 +6214,7 @@ 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 () => { + it("skips a stale gate-override 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, { @@ -6277,16 +6277,16 @@ describe("queue processors", () => { }, }); - // The neutral PATCH targeted the LIVE head's Gate run (id 556), and the stale SHA was never touched. - expect(seen.liveCheckGets).toBe(1); + // A maintainer's override is bound to the cached head they approved; a newer live head is not neutralized. + expect(seen.liveCheckGets).toBe(0); 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"); + expect(patchBodies).toEqual([]); + const overridden = await env.DB.prepare("select id from audit_events where event_type = ?").bind("github_app.gate_overridden").first<{ id: number }>(); + expect(overridden ?? null).toBeNull(); + const skipped = await env.DB.prepare("select detail, metadata_json from audit_events where event_type = ?") + .bind("github_app.gate_override_skipped") + .first<{ detail: string; metadata_json: string }>(); + expect(skipped?.detail).toBe("stale_pr_head"); }); it("records null head SHAs in the override audit when the PR head is unresolved (#16 fail-safe)", async () => { diff --git a/test/unit/resolve-override-head-sha.test.ts b/test/unit/resolve-override-head-sha.test.ts index 6ed575aec..2a5868fb9 100644 --- a/test/unit/resolve-override-head-sha.test.ts +++ b/test/unit/resolve-override-head-sha.test.ts @@ -31,32 +31,32 @@ describe("resolveOverrideHeadSha (#16 / gate-override stale head)", () => { vi.unstubAllGlobals(); }); - it("returns the LIVE head when the token mint succeeds (overrides the current commit, not the cached one)", async () => { + it("keeps the cached head when the live head matches", async () => { const env = createTestEnv(); mockedToken.mockResolvedValue("inst-tok"); - stubLiveHead("live-sha"); - expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("live-sha"); + stubLiveHead("cached-sha"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("cached-sha"))).toEqual({ headSha: "cached-sha", stale: false, liveHeadSha: "cached-sha" }); expect(mockedToken).toHaveBeenCalledWith(env, 123); }); - it("falls back to the public token when the mint throws, and still resolves the live head", async () => { + it("marks the command stale when the live head differs from the cached 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"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toEqual({ headSha: "stale-sha", stale: true, liveHeadSha: "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"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toEqual({ headSha: "stale-sha", stale: false, liveHeadSha: undefined }); }); 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"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toEqual({ headSha: "stale-sha", stale: false, liveHeadSha: undefined }); }); }); From 07606eaebcaf723f1e6e061bb782090f304be7b4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 26 Jun 2026 13:47:12 -0700 Subject: [PATCH 2/3] test(queue): cover the null cached-head arm of resolveOverrideHeadSha --- test/unit/resolve-override-head-sha.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/unit/resolve-override-head-sha.test.ts b/test/unit/resolve-override-head-sha.test.ts index 2a5868fb9..51ff06f51 100644 --- a/test/unit/resolve-override-head-sha.test.ts +++ b/test/unit/resolve-override-head-sha.test.ts @@ -39,6 +39,14 @@ describe("resolveOverrideHeadSha (#16 / gate-override stale head)", () => { expect(mockedToken).toHaveBeenCalledWith(env, 123); }); + it("does not mark stale when the cached head is absent (null), even if a live head exists", async () => { + const env = createTestEnv(); + mockedToken.mockResolvedValue("inst-tok"); + stubLiveHead("live-sha"); + // pr.headSha is null → the `pr.headSha` arm short-circuits; there is nothing the maintainer approved to neutralize. + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr(null))).toEqual({ headSha: null, stale: false, liveHeadSha: "live-sha" }); + }); + it("marks the command stale when the live head differs from the cached head", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-tok" }); mockedToken.mockRejectedValue(new Error("no app key")); From f2ad662259ad37e9a29aac6d825d7d3100ac7db8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 26 Jun 2026 14:00:02 -0700 Subject: [PATCH 3/3] refactor(queue): drop the unreachable head-substitution branch in gate-override resolution --- src/queue/processors.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 2f93c0c56..6e0ecc063 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3249,8 +3249,9 @@ async function maybeProcessGateOverrideCommand(env: Env, deliveryId: string, pay await recordGateOverrideSkip(env, deliveryId, repoFullName, `${repoFullName}#${pr.number}`, actor, "stale_pr_head"); return true; } - const prAtResolvedHead = headResolution.headSha === pr.headSha ? pr : { ...pr, headSha: headResolution.headSha }; - const { advisory } = await buildAuthorizedPrActionAdvisory(env, repoFullName, prAtResolvedHead, settings); + // Not stale (the stale case returned above), and resolveOverrideHeadSha returns the cached head (pr.headSha) + // in that case — so the override acts on `pr` directly; there is no other head to substitute. + const { advisory } = await buildAuthorizedPrActionAdvisory(env, repoFullName, pr, settings); const safeReason = sanitizePublicComment((command.reason ?? "").trim() || "No reason provided."); await createOrUpdateOverriddenGateCheckRun(env, installationId, repoFullName, advisory, { actor, reason: safeReason }); await recordAuditEvent(env, {