diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 4e3aee347..6e0ecc063 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3163,17 +3163,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 }; } /** @@ -3241,13 +3244,14 @@ 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; + } + // 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, { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 454f7ca21..0bd9d29d8 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -6372,7 +6372,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, { @@ -6435,16 +6435,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..51ff06f51 100644 --- a/test/unit/resolve-override-head-sha.test.ts +++ b/test/unit/resolve-override-head-sha.test.ts @@ -31,32 +31,40 @@ 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("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")); 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 }); }); });