fix(review): key the durable CI-state cache on resolved required contexts

src/github/backfill.ts

@@ -3321,13 +3321,19 @@ export function deserializeCachedCiAggregate(
 ): LiveCiAggregate | null {
   if (!cached.ciState) return null;
   try {
+    const failingDetails = JSON.parse(cached.ciFailingDetailsJson ?? "[]");
+    const nonRequiredFailingDetails = JSON.parse(cached.ciNonRequiredFailingDetailsJson ?? "[]");
+    // A corrupted/malformed row (e.g. hand-edited D1 row, or a future schema change that leaves an old JSON
+    // shape behind) must fail OPEN as a cache miss, not hand callers a wrong shape -- never trust the parse
+    // result's type just because JSON.parse didn't throw.
+    if (!Array.isArray(failingDetails) || !Array.isArray(nonRequiredFailingDetails)) return null;
     return {
       ciState: cached.ciState,
       hasPending: cached.ciHasPending ?? false,
       hasVisiblePending: cached.ciHasVisiblePending ?? false,
       hasMissingRequiredContext: cached.ciHasMissingRequiredContext ?? false,
-      failingDetails: JSON.parse(cached.ciFailingDetailsJson ?? "[]"),
-      nonRequiredFailingDetails: JSON.parse(cached.ciNonRequiredFailingDetailsJson ?? "[]"),
+      failingDetails,
+      nonRequiredFailingDetails,
       ciCompletenessWarning: cached.ciCompletenessWarning ?? null,
     };
   } catch {

src/queue/processors.ts

@@ -534,6 +534,17 @@ function expectedCiContextsKeyPart(expectedCiContexts: ReadonlyArray<string> | n
   return [...expectedCiContexts].sort().join("");
 }
 
+// Stable, order-independent cache-key fragment for the RESOLVED required-contexts set (#selfhost-ci-verification):
+// unlike expectedCiContextsKeyPart above (the raw, unresolved settings.expectedCiContexts config), this reflects
+// mergeRequiredCiContexts' actual output -- live branch-protection required contexts unioned with config. The
+// durable cross-job CI-state cache (cachedFetchLiveCiAggregate) MUST key on this, not on the raw config: branch
+// protection can change server-side while expectedCiContexts config stays put, and a stale durable row keyed only
+// on the unchanged config would keep serving an aggregate computed against the old required-context set.
+function resolvedRequiredContextsKeyPart(requiredContexts: ReadonlySet<string> | null | undefined): string {
+  if (!requiredContexts || requiredContexts.size === 0) return "";
+  return [...requiredContexts].sort().join(" ");
+}
+
 // RC2 + #selfhost-ci-verification: the EFFECTIVE required-status-check contexts for this repo/baseRef, merging
 // live branch-protection required contexts with the maintainer-configured settings.expectedCiContexts fallback
 // (mergeRequiredCiContexts — branch protection stays authoritative when readable; expectedCiContexts is the
@@ -582,10 +593,11 @@ function evictLiveFactOnReject<T>(
  * degrades to a live fetch, never blocks it.
  *
  * `forceRefresh` (set by refreshLiveCiAggregate below, mirroring refreshLiveMergeState's OWN "never durable-
- * cached" contract for merge-state): skips the cache READ entirely, always fetching live -- a "refresh" caller
- * needs a genuinely fresh read even within the SAME job pass (e.g. re-checking CI right after this pass's own
- * gate/check-run publication, which can flip a status GitHub hasn't sent a webhook for yet). The WRITE-through
- * still happens on a forced refresh, so a LATER pass/job still benefits from this read.
+ * cached" contract for merge-state): skips the freshness CHECK entirely (the existing row is still fetched, to
+ * carry its `status` field into the write-through's previousState, but is never treated as a hit), so this always
+ * fetches live -- a "refresh" caller needs a genuinely fresh read even within the SAME job pass (e.g. re-checking
+ * CI right after this pass's own gate/check-run publication, which can flip a status GitHub hasn't sent a webhook
+ * for yet). The WRITE-through still happens on a forced refresh, so a LATER pass/job still benefits from this read.
  *
  * Deliberately implemented HERE, not in backfill.ts (where writeThroughCiStateCache/isCiStateCacheFresh/
  * deserializeCachedCiAggregate live) -- a same-module call from backfill.ts to its own
@@ -650,7 +662,7 @@ function fetchLiveCiAggregateWithRequiredContexts(
     .then((requiredContexts) => ({ requiredContexts, resolved: true }))
     .catch(() => ({ requiredContexts: null, resolved: false }))
     .then(({ requiredContexts, resolved }) =>
-      cachedFetchLiveCiAggregate(env, repoFullName, prNumber, headSha, token, requiredContexts, expectedCiContextsKeyPart(expectedCiContexts), forceRefresh, resolved, admissionKey),
+      cachedFetchLiveCiAggregate(env, repoFullName, prNumber, headSha, token, requiredContexts, resolvedRequiredContextsKeyPart(requiredContexts), forceRefresh, resolved, admissionKey),
     );
 }
 

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

@@ -646,6 +646,36 @@ describe("durable CI-state cache (#selfhost-ci-verification)", () => {
       ).toBeNull();
     });
 
+    it("fails open to null when ciFailingDetailsJson parses to valid JSON that is NOT an array (corrupted row shape)", () => {
+      // JSON.parse succeeds here (it's valid JSON), so this exercises the Array.isArray guard specifically,
+      // not the catch block above -- a corrupted/malformed row must never hand callers a wrong shape.
+      expect(
+        deserializeCachedCiAggregate({
+          ciState: "passed",
+          ciHasPending: false,
+          ciHasVisiblePending: false,
+          ciHasMissingRequiredContext: false,
+          ciFailingDetailsJson: '{"not":"an array"}',
+          ciNonRequiredFailingDetailsJson: "[]",
+          ciCompletenessWarning: null,
+        }),
+      ).toBeNull();
+    });
+
+    it("fails open to null when ciNonRequiredFailingDetailsJson parses to valid JSON that is NOT an array (corrupted row shape)", () => {
+      expect(
+        deserializeCachedCiAggregate({
+          ciState: "passed",
+          ciHasPending: false,
+          ciHasVisiblePending: false,
+          ciHasMissingRequiredContext: false,
+          ciFailingDetailsJson: "[]",
+          ciNonRequiredFailingDetailsJson: '"just a string"',
+          ciCompletenessWarning: null,
+        }),
+      ).toBeNull();
+    });
+
     it("defaults hasPending/hasVisiblePending/hasMissingRequiredContext to false and the JSON arrays to [] when null", () => {
       expect(
         deserializeCachedCiAggregate({

test/unit/queue.test.ts

@@ -2170,6 +2170,64 @@ describe("queue processors", () => {
     expect(deferred?.n).toBe(0);
   });
 
+  // REGRESSION (#selfhost-ci-verification): the DURABLE cross-job CI-state cache row must be keyed on the actual
+  // RESOLVED required-contexts set (mergeRequiredCiContexts' output: live branch-protection contexts unioned with
+  // settings.expectedCiContexts), not on the raw, unresolved expectedCiContexts config alone. Branch protection can
+  // change server-side (a maintainer adds a required check in GitHub's UI) while expectedCiContexts config stays
+  // put and the head_sha is unchanged -- if the durable row were keyed only on the config, the readiness path would
+  // keep serving a stale aggregate computed against the OLD required-context set for up to the 60s TTL, producing a
+  // wrong merge/close verdict. Two processJob passes at the SAME head_sha, same unchanged expectedCiContexts config,
+  // but DIFFERENT branch-protection required contexts between them, must each independently reach a live CI read
+  // (both misses) and each persist their OWN ciRequiredContextsKey -- proving the key tracks the resolved set.
+  it("REGRESSION (#selfhost-ci-verification): the durable CI-state cache keys on the RESOLVED required-contexts set, not the raw expectedCiContexts config", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
+    await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: { pull_requests: "write", checks: "write" }, events: [] } });
+    await upsertRepositoryFromGitHub(env, { name: "agent-repo", full_name: "owner/agent-repo", private: false, owner: { login: "owner" } }, 9001);
+    await upsertRepositorySettings(env, { repoFullName: "owner/agent-repo", autonomy: { merge: "auto", update_branch: "auto" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" });
+    await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Branch protection drift", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, base: { ref: "main" }, labels: [], body: "Closes #1" });
+    // expectedCiContexts is configured ONCE and never changes across the two calls below -- only branch protection
+    // (the OTHER input to mergeRequiredCiContexts) drifts between them.
+    await upsertRepoFocusManifest(env, "owner/agent-repo", { gate: { expectedCiContexts: ["lint"] } });
+    let requiredContextsFromBranchProtection: Array<string | null> = [];
+    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 (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") return Response.json({ number: 7, title: "Branch protection drift", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" });
+      if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]);
+      if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "lint", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] });
+      if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] });
+      if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } });
+      if (url.includes("/branches/")) return Response.json({ contexts: requiredContextsFromBranchProtection });
+      return Response.json({});
+    });
+
+    // Pass 1: branch protection requires nothing extra beyond expectedCiContexts's own "lint" -- the resolved set
+    // is exactly {"lint"}, satisfied by the check-run above, so CI resolves and the durable row's key reflects it.
+    requiredContextsFromBranchProtection = [];
+    await processJob(env, { type: "agent-regate-pr", deliveryId: "branch-protection-before", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 });
+    const rowAfterPass1 = await getPullRequestDetailSyncState(env, "owner/agent-repo", 7);
+    expect(rowAfterPass1?.ciState).toBe("passed");
+    const keyAfterPass1 = rowAfterPass1?.ciRequiredContextsKey ?? null;
+
+    // A maintainer now adds "required-build" as a branch-protection required check via GitHub's UI -- the SAME
+    // head_sha, the SAME (unchanged) expectedCiContexts config, but the RESOLVED required-contexts set just grew.
+    requiredContextsFromBranchProtection = ["required-build"];
+    await processJob(env, { type: "agent-regate-pr", deliveryId: "branch-protection-after", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 });
+    const rowAfterPass2 = await getPullRequestDetailSyncState(env, "owner/agent-repo", 7);
+    const keyAfterPass2 = rowAfterPass2?.ciRequiredContextsKey ?? null;
+
+    // The durable row's key must differ once the RESOLVED set changed -- if it were still derived from the raw,
+    // unchanged expectedCiContexts config (the bug), keyAfterPass2 would equal keyAfterPass1 even though the
+    // resolved required-contexts set is now materially different (missing "required-build" entirely).
+    expect(keyAfterPass2).not.toBe(keyAfterPass1);
+    // "required-build" never appears in any check-run/status ⇒ once it is folded into the resolved required set,
+    // reduceLiveCiAggregate can no longer treat it as satisfied ⇒ the aggregate correctly flips to pending,
+    // proving pass 2 actually re-derived against the NEW resolved set rather than serving pass 1's stale "passed"
+    // row from a durable cache keyed on the unchanged config.
+    expect(rowAfterPass2?.ciState).toBe("pending");
+  });
+
   it("#sweep-resync: a failing resync upsert is swallowed (fail-open) — the sweep never throws", async () => {
     const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() });
     await upsertInstallation(env, { action: "created", installation: { id: 9001, account: { login: "owner", id: 1, type: "Organization" }, target_type: "Organization", repository_selection: "selected", permissions: {}, events: [] } });