fix(queue): reject stale gate overrides

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<string | null | undefined> {
+export async function resolveOverrideHeadSha(env: Env, installationId: number, repoFullName: string, pr: PullRequestRecord): Promise<OverrideHeadResolution> {
   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, {

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 () => {

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 });
   });
 });