fix(github): write through all three PR-state cache fields together

The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.

All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.

Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.

Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.

Author: JSONbored

src/github/backfill.ts

@@ -2792,6 +2792,42 @@ async function writeThroughPrStateCache(
   }).catch(() => undefined);
 }
 
+/**
+ * Shared live-fetch for the three cached PR-state readers below (#2537 review fix). A SINGLE `GET /pulls/{n}`
+ * already returns `mergeable_state`, `state`, AND `head.sha` together, so a cache miss on any ONE field now
+ * fetches and write-throughs ALL THREE at once under the one shared `prStateFetchedAt` stamp they share --
+ * instead of writing only the field the caller happened to ask for. Without this, a fresh write for field A
+ * would make an UN-fetched field B look "fresh" to the NEXT reader (they share one timestamp), so that reader
+ * would silently return `undefined` for a field that was simply never populated, mistaking it for a
+ * confirmed-empty GitHub value. Reusing the full-payload fetch costs nothing extra: all three narrow fetchers
+ * (`fetchLivePullRequestMergeState` / `fetchLivePullRequestState` / `fetchLivePullRequestHeadSha`) already hit
+ * this exact same endpoint, just extracting one field each -- this only changes what the CACHED wrappers fetch
+ * internally; those narrow fetchers stay untouched for their other, uncached, act-boundary callers.
+ * Returns the full payload, or `undefined` on a failed fetch -- in which case the cache is left untouched
+ * entirely (a failed live read must not poison it with a false "confirmed fresh" stamp).
+ */
+async function fetchAndCachePrStateFields(
+  env: Env,
+  repoFullName: string,
+  prNumber: number,
+  token: string | undefined,
+  admissionKey: GitHubRateLimitAdmissionKey | undefined,
+  previousStatus: PullRequestDetailSyncStateRecord["status"] | undefined,
+): Promise<GitHubPullRequestPayload | undefined> {
+  const live = await fetchLivePullRequest(env, repoFullName, prNumber, token, admissionKey);
+  if (!live) return undefined;
+  const liveHeadSha = live.head?.sha;
+  await writeThroughPrStateCache(env, repoFullName, prNumber, previousStatus, {
+    prMergeableState: live.mergeable_state ?? null,
+    prState: live.state ?? null,
+    // Omit (not null) when the live payload carries no head SHA -- mirrors primeDurablePrStateCache's own
+    // PARTIAL-UPDATE CONTRACT guard below: a PR-state write must never CLEAR the headSha the files cache
+    // (#audit-rate-headroom) relies on.
+    ...(liveHeadSha ? { headSha: liveHeadSha } : {}),
+  });
+  return live;
+}
+
 /** Prime the durable PR-state cache (#2537) from an ALREADY-FETCHED live payload (e.g. the sweep-resync's
  *  `fetchLivePullRequest` read), so OTHER readers (readiness, dup-winner, gate-override) benefit from this
  *  already-paid-for fetch instead of re-fetching moments later. Best-effort, mirrors writeThroughPrStateCache's
@@ -2816,7 +2852,8 @@ export async function primeDurablePrStateCache(
 
 /** Cached read of the PR's live mergeable_state, backed by pull_request_detail_sync_state (#2537). A fresh cache
  *  row (webhook-invalidated, capped at PR_STATE_CACHE_MAX_AGE_MS) is served without a GitHub call; otherwise
- *  fetches live via fetchLivePullRequestMergeState and (best-effort) writes the result back for the next reader.
+ *  fetches live via fetchAndCachePrStateFields (which write-throughs ALL THREE cached fields together, not just
+ *  this one, since they share one fetchedAt stamp) and returns this field from that shared response.
  *  Fail-open throughout: any cache read/write hiccup falls back to / degrades to a live fetch, never blocks it. */
 export async function cachedFetchLivePullRequestMergeState(
   env: Env,
@@ -2831,13 +2868,12 @@ export async function cachedFetchLivePullRequestMergeState(
     return cached.prMergeableState ?? undefined;
   }
   incr(PR_STATE_CACHE_METRIC, { field: "mergeable_state", result: "miss" });
-  const live = await fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey);
-  await writeThroughPrStateCache(env, repoFullName, prNumber, cached?.status, { prMergeableState: live ?? null });
-  return live;
+  const live = await fetchAndCachePrStateFields(env, repoFullName, prNumber, token, admissionKey, cached?.status);
+  return live?.mergeable_state ?? undefined;
 }
 
 /** Cached read of the PR's live state (open/closed), backed by pull_request_detail_sync_state (#2537). Same
- *  freshness/fail-open contract as cachedFetchLivePullRequestMergeState. */
+ *  freshness/fail-open/shared-fetch contract as cachedFetchLivePullRequestMergeState. */
 export async function cachedFetchLivePullRequestState(
   env: Env,
   repoFullName: string,
@@ -2851,9 +2887,8 @@ export async function cachedFetchLivePullRequestState(
     return cached.prState ?? undefined;
   }
   incr(PR_STATE_CACHE_METRIC, { field: "state", result: "miss" });
-  const live = await fetchLivePullRequestState(env, repoFullName, prNumber, token, admissionKey);
-  await writeThroughPrStateCache(env, repoFullName, prNumber, cached?.status, { prState: live ?? null });
-  return live;
+  const live = await fetchAndCachePrStateFields(env, repoFullName, prNumber, token, admissionKey, cached?.status);
+  return live?.state ?? undefined;
 }
 
 /** Cached read of the PR's live head SHA, backed by pull_request_detail_sync_state (#2537). Reuses the EXISTING
@@ -2873,9 +2908,8 @@ export async function cachedFetchLivePullRequestHeadSha(
     return cached.headSha;
   }
   incr(PR_STATE_CACHE_METRIC, { field: "head_sha", result: "miss" });
-  const live = await fetchLivePullRequestHeadSha(env, repoFullName, prNumber, token, admissionKey);
-  if (live) await writeThroughPrStateCache(env, repoFullName, prNumber, cached?.status, { headSha: live });
-  return live;
+  const live = await fetchAndCachePrStateFields(env, repoFullName, prNumber, token, admissionKey, cached?.status);
+  return live?.head?.sha ?? undefined;
 }
 
 /** Invalidate the durable PR-state cache fields (#2537) — called on synchronize/closed/reopened. Explicit null

test/unit/pr-detail-durable-cache.test.ts

@@ -53,6 +53,37 @@ describe("durable PR-state cache (#2537)", () => {
     };
   }
 
+  // REGRESSION (#2595 review defect): the three cached readers below share ONE prStateFetchedAt column as their
+  // freshness stamp. Before this fix, each reader wrote through ONLY the one field it cared about, so a write
+  // from reader A would make reader B's UN-fetched field look "fresh" to a subsequent call -- silently returning
+  // undefined for a field that was simply never populated, not confirmed empty on GitHub. The fix fetches the
+  // full PR payload (all three narrow fetchers already hit the exact same endpoint) and writes all three fields
+  // through together on every live fetch, so this cross-field false-freshness can no longer happen.
+  it("REGRESSION (#2595): a live fetch from ONE cached reader also warms the OTHER TWO, since they share one fetchedAt stamp", async () => {
+    const env = createTestEnv();
+    let fetchCount = 0;
+    stubFetchTracking((url) => {
+      if (url.includes("/pulls/40")) {
+        fetchCount += 1;
+        return Response.json({ number: 40, mergeable_state: "clean", state: "open", head: { sha: "shared-sha" } });
+      }
+      return new Response("not found", { status: 404 });
+    });
+
+    // Only the mergeable_state reader is called...
+    const mergeableState = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 40, "tok");
+    expect(mergeableState).toBe("clean");
+    expect(fetchCount).toBe(1);
+
+    // ...yet the OTHER two fields are now cache HITS too, without a second GitHub call, and return the REAL
+    // fetched values -- not a false "confirmed fresh, but never actually fetched" undefined.
+    const state = await cachedFetchLivePullRequestState(env, "owner/repo", 40, "tok");
+    const headSha = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 40, "tok");
+    expect(state).toBe("open");
+    expect(headSha).toBe("shared-sha");
+    expect(fetchCount).toBe(1); // no additional GitHub calls were needed
+  });
+
   describe("cachedFetchLivePullRequestMergeState", () => {
     it("cache miss on first read — fetches live and writes the row through", async () => {
       const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
@@ -196,14 +227,44 @@ describe("durable PR-state cache (#2537)", () => {
       expect(fetchCount).toBe(1);
     });
 
-    it("does not write through when the live head SHA is undefined (never clears a prior headSha)", async () => {
+    // REGRESSION (#2595 review defect): the three cached readers share ONE prStateFetchedAt stamp, so a live
+    // fetch triggered by ANY of them must write through ALL THREE fields together -- otherwise a field this
+    // reader doesn't care about (mergeable_state/state) would look "fresh" to a later, different reader despite
+    // never having been fetched. A fresh full-payload fetch that carries no head.sha still writes the OTHER two
+    // fields through (and still never CLEARS a prior headSha -- the PARTIAL-UPDATE CONTRACT is preserved).
+    it("writes mergeable_state/state through even when the live head SHA is undefined, and never clears a prior headSha", async () => {
       const env = createTestEnv();
-      stubFetchTracking(() => Response.json({ number: 31 })); // no head.sha
+      await upsertPullRequestDetailSyncState(env, {
+        repoFullName: "owner/repo",
+        pullNumber: 31,
+        status: "never_synced",
+        headSha: "prior-sha",
+        prStateFetchedAt: "2020-01-01T00:00:00.000Z", // stale -- forces a live re-fetch
+      });
+      stubFetchTracking(() => Response.json({ number: 31, mergeable_state: "clean", state: "open" })); // no head.sha
 
       const result = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 31, "tok");
 
+      expect(result).toBeUndefined(); // no head.sha in the live payload
+      const row = await getPullRequestDetailSyncState(env, "owner/repo", 31);
+      expect(row?.headSha).toBe("prior-sha"); // NOT cleared -- omitted, not written as null
+      expect(row?.prMergeableState).toBe("clean"); // written through together with this call's own fetch
+      expect(row?.prState).toBe("open");
+      expect(row?.prStateFetchedAt).not.toBe("2020-01-01T00:00:00.000Z"); // the shared stamp advanced
+    });
+
+    it("still creates a row (with the other fields null) on a fresh PR whose live payload carries no head.sha at all", async () => {
+      const env = createTestEnv();
+      stubFetchTracking(() => Response.json({ number: 32 })); // no head.sha, no mergeable_state, no state
+
+      const result = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 32, "tok");
+
       expect(result).toBeUndefined();
-      expect(await getPullRequestDetailSyncState(env, "owner/repo", 31)).toBeNull();
+      const row = await getPullRequestDetailSyncState(env, "owner/repo", 32);
+      expect(row?.headSha).toBeNull(); // never had one to preserve
+      expect(row?.prMergeableState).toBeNull();
+      expect(row?.prState).toBeNull();
+      expect(row?.prStateFetchedAt).not.toBeNull(); // the fetch DID succeed (confirmed-empty, not "never fetched")
     });
   });
 

test/unit/queue.test.ts

@@ -13301,7 +13301,7 @@ describe("queue processors", () => {
         await seedWarmPrStateCache(env, "JSONbored/gittensory", 200);
         vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
           const url = input.toString();
-          if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
+          if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" });
           return Response.json({});
         });
 
@@ -13333,7 +13333,7 @@ describe("queue processors", () => {
       await seedWarmPrStateCache(env, "JSONbored/gittensory", 201);
       vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
         const url = input.toString();
-        if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
+        if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" });
         return Response.json({});
       });
 
@@ -13370,7 +13370,7 @@ describe("queue processors", () => {
         });
         vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
           const url = input.toString();
-          if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
+          if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" });
           if (url.includes("/pulls/202/files")) return Response.json([]);
           if (url.includes("/pulls/202/reviews")) return Response.json([]);
           return Response.json({});
@@ -13411,7 +13411,7 @@ describe("queue processors", () => {
       });
       vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
         const url = input.toString();
-        if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
+        if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" });
         return Response.json({});
       });
 
@@ -13472,7 +13472,7 @@ describe("queue processors", () => {
       vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
         const url = input.toString();
         const method = (init?.method ?? "GET").toUpperCase();
-        if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
+        if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" });
         if (url.endsWith("/pulls/8") && method === "PATCH") {
           closedViaPatch = JSON.parse(String(init?.body ?? "{}")).state === "closed";
           return Response.json({ number: 8, state: "closed" });
@@ -13555,7 +13555,7 @@ describe("queue processors", () => {
       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", expires_at: "2026-05-28T00:04:00.000Z" });
+        if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token", expires_at: "2026-05-28T00:04:00.000Z" });
         if (url.includes("/pulls/9/files")) return Response.json([{ filename: "src/cache.ts", additions: 5, deletions: 1, status: "modified" }]);
         // ALWAYS live-dirty — the seeded durable cache above claims "clean".
         if (/\/pulls\/9(?:\?|$)/.test(url)) return Response.json({ number: 9, mergeable_state: "dirty" });