From dab4ace56f95f26c45cf98451ee13cd3f5398416 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sat, 4 Jul 2026 02:40:59 -0700 Subject: [PATCH] fix(review): key the durable CI-state cache on resolved required contexts cachedFetchLiveCiAggregate (#2984) keyed its durable pull_request_detail_sync_state row from the raw, unresolved settings.expectedCiContexts config instead of the actually-resolved required-contexts set that cachedRequiredStatusContexts returns (branch protection merged with config, via mergeRequiredCiContexts). If branch protection changes server-side while expectedCiContexts config and head_sha stay the same, the durable key was unchanged, so the readiness path could keep serving a stale aggregate computed against the old required-context set for up to the cache's TTL. Derive the key from the resolved set instead. Also hardens deserializeCachedCiAggregate: JSON.parse succeeding does not guarantee the parsed value is an array, so a corrupted/malformed row's failingDetails/nonRequiredFailingDetails now fail open to a cache miss via an explicit Array.isArray check rather than handing callers a wrong shape. --- src/github/backfill.ts | 10 +++- src/queue/processors.ts | 22 +++++++-- test/unit/pr-detail-durable-cache.test.ts | 30 ++++++++++++ test/unit/queue.test.ts | 58 +++++++++++++++++++++++ 4 files changed, 113 insertions(+), 7 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index d30e5bdf4b..73f50746a5 100644 --- a/src/github/backfill.ts +++ b/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 { diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 443846d78d..bc2cc0610a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -534,6 +534,17 @@ function expectedCiContextsKeyPart(expectedCiContexts: ReadonlyArray | 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 | 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( * 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), ); } diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts index 1ee0a501b9..43585d3510 100644 --- a/test/unit/pr-detail-durable-cache.test.ts +++ b/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({ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index bb6c067f23..bd3c8cdf70 100644 --- a/test/unit/queue.test.ts +++ b/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 = []; + 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: [] } });