diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 2e7fe1ec4a..0d277c2350 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -1975,6 +1975,35 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back } } +// Bounded-age backstop (#2537 second gate pass): the reviewsInvalidatedAt comparison below is EXACT when the +// invalidation write actually happens, but a silently DROPPED markPullRequestReviewsInvalidated write leaves +// reviewsInvalidatedAt null forever -- there is then no marker at all to compare against, so the exact +// comparison alone would read "up to date" indefinitely no matter how long ago reviewsSyncedAt was. This is the +// only backstop for a signal that was never recorded in the first place; deliberately long so a +// normally-behaving PR (invalidation writes succeeding) never hits it in practice. +const REVIEWS_CACHE_MAX_AGE_MS = 48 * 60 * 60 * 1000; + +// #2537 follow-up (gate-flagged): a small, pure predicate mirroring fetchAndStorePullRequestDetails's own +// reviewsUpToDate check below, exported so the periodic re-gate sweep (queue/processors.ts) can independently +// decide whether a stale reviews cache is, on its own, a reason to force a refresh -- otherwise this row's +// invalidation state only gets EVALUATED when something ELSE already calls refreshPullRequestDetails, which a +// "quiet" PR (no new pushes, slop evidence + manifest gate both off, no pre-merge check paths) may never do. A +// SINGLE authoritative definition (this function) rather than two independently-maintained copies that could +// drift -- both the exact invalidation-marker comparison AND the bounded-age fallback live here, so a caller +// that only checks THIS predicate (e.g. the sweep, before deciding whether to even call refreshPullRequestDetails) +// agrees with fetchAndStorePullRequestDetails's own internal check once that call actually happens. +export function isReviewsCacheUpToDate( + syncState: Pick | null | undefined, +): boolean { + const reviewsSyncedAt = syncState?.reviewsSyncedAt; + if (!reviewsSyncedAt) return false; + const invalidationCleared = !syncState?.reviewsInvalidatedAt || reviewsSyncedAt > syncState.reviewsInvalidatedAt; + if (!invalidationCleared) return false; + const reviewsSyncedAtMs = Date.parse(reviewsSyncedAt); + if (!Number.isFinite(reviewsSyncedAtMs)) return false; + return Date.now() - reviewsSyncedAtMs < REVIEWS_CACHE_MAX_AGE_MS; +} + async function fetchAndStorePullRequestDetails( env: Env, repoFullName: string, @@ -2007,9 +2036,7 @@ async function fetchAndStorePullRequestDetails( // millisecond, and sub-millisecond ordering is unknowable from the stored strings — a tie must fail toward // "still needs a refetch," never toward silently trusting a possibly-stale cache. const reviewsSyncedAtBefore = existingState?.reviewsSyncedAt; - const reviewsUpToDate = - Boolean(reviewsSyncedAtBefore) && - (!existingState?.reviewsInvalidatedAt || (reviewsSyncedAtBefore ?? "") > existingState.reviewsInvalidatedAt); + const reviewsUpToDate = isReviewsCacheUpToDate(existingState); // Gate review finding (TOCTOU race): `existingState` above is a snapshot read at the TOP of this call. If a // `pull_request_review` webhook races in AFTER that read but BEFORE this function returns, an unconditional // "stamp reviewsSyncedAt to now" on the CALLER's side (the old design) would advance the timestamp PAST that diff --git a/src/queue/processors.ts b/src/queue/processors.ts index e3005cae00..6d7642568d 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -6,6 +6,7 @@ import { getLatestRepoGithubTotalsSnapshot, getFreshOfficialMinerDetection, getPullRequest, + getPullRequestDetailSyncState, getRepoAuthorPullRequestHistory, getRepository, getDecryptedRepositoryAiKey, @@ -93,6 +94,7 @@ import { fetchOpenPullRequestNumbersForCommit, fetchRequiredStatusContexts, invalidatePrStateCache, + isReviewsCacheUpToDate, primeDurablePrStateCache, refreshContributorActivity, refreshInstallationHealth, @@ -2320,11 +2322,19 @@ async function reReviewStoredPullRequest( linkedIssueAuthorLogins, }); await persistAdvisory(env, advisory); - if ( + // #2537 follow-up (gate-flagged): the durable review cache's only invalidation path is markPullRequestReviewsInvalidated + // on a webhook (processors.ts). A "quiet" PR (no new pushes, slop evidence + manifest gate both off, no + // pre-merge check paths) never hits any of the three reasons below, so a DROPPED invalidation write could sit + // stale indefinitely even though this per-PR sweep unit visits every open PR on a bounded cadence. + // Short-circuit the extra read when another reason already forces the refresh. + const otherRefreshReasons = shouldCollectSlopEvidence(settings) || settings.manifestPolicyGateMode !== "off" || - (await shouldRefreshFilesForPreMergeChecks(env, repoFullName)) - ) { + (await shouldRefreshFilesForPreMergeChecks(env, repoFullName)); + const reviewsCacheStale = + !otherRefreshReasons && + !isReviewsCacheUpToDate(await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null)); + if (otherRefreshReasons || reviewsCacheStale) { await refreshPullRequestDetails(env, repoFullName, prNumber).catch( () => undefined, ); diff --git a/test/unit/backfill-reviews-cache-scoping.test.ts b/test/unit/backfill-reviews-cache-scoping.test.ts index 067ec195e1..053639dfab 100644 --- a/test/unit/backfill-reviews-cache-scoping.test.ts +++ b/test/unit/backfill-reviews-cache-scoping.test.ts @@ -25,6 +25,7 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { clearGitHubResponseCacheForTest(); resetMetrics(); vi.unstubAllGlobals(); + vi.useRealTimers(); }); async function seedRegisteredRepo(env: Env) { @@ -76,6 +77,10 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { it("does not re-fetch reviews when reviewsSyncedAt is set and no invalidation has been recorded (cache hit), and leaves stored rows untouched", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + // Pin "now" shortly after the seeded reviewsSyncedAt -- within the new bounded-age backstop's + // 48h window, so this test's cache-hit assertion isn't defeated by real wall-clock time. + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-05-20T01:00:00.000Z")); await seedRegisteredRepo(env); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 61, @@ -115,6 +120,10 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { it("does not re-fetch reviews when reviewsInvalidatedAt predates reviewsSyncedAt (stale invalidation, still a cache hit)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + // Pin "now" shortly after the seeded reviewsSyncedAt -- within the new bounded-age backstop's + // 48h window, so this test's cache-hit assertion isn't defeated by real wall-clock time. + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-05-20T01:00:00.000Z")); await seedRegisteredRepo(env); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 62, @@ -141,6 +150,37 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { expect(urls.some((url) => url.includes("/pulls/62/reviews"))).toBe(false); }); + it("REGRESSION (bounded-age backstop): an unparseable reviewsSyncedAt is treated as stale (NaN branch, miss) rather than throwing", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 68, + title: "Open PR, unparseable review marker", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-68" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 68, + status: "complete", + headSha: "head-68", + reviewsSyncedAt: "not-a-date", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/68/reviews") + ? Response.json([{ id: 9, user: { login: "reviewer9" }, state: "APPROVED", submitted_at: "2026-05-19T00:00:00.000Z" }]) + : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 68); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/68/reviews"))).toBe(true); + }); + it("re-fetches reviews on the next sync after markPullRequestReviewsInvalidated bumps reviewsInvalidatedAt past reviewsSyncedAt (cache invalidation)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await seedRegisteredRepo(env); @@ -283,6 +323,10 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { it("does not treat a FILES-only failure as a reason to re-fetch reviews (only a review-specific failure forces a retry)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + // Pin "now" shortly after the seeded reviewsSyncedAt -- within the new bounded-age backstop's + // 48h window, so this test's cache-hit assertion isn't defeated by real wall-clock time. + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-05-20T01:00:00.000Z")); await seedRegisteredRepo(env); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 66, @@ -323,6 +367,10 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { it("REGRESSION: a head SHA change alone does not invalidate cached reviews (reviews are independent of the head, unlike files)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + // Pin "now" shortly after the seeded reviewsSyncedAt -- within the new bounded-age backstop's + // 48h window, so this test's cache-hit assertion isn't defeated by real wall-clock time. + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-05-20T01:00:00.000Z")); await seedRegisteredRepo(env); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 64, @@ -373,6 +421,10 @@ describe("GitHub PR reviews cache scoping (#2537)", () => { it("REGRESSION (gate finding): a manual force-files refresh does not also force an unrelated reviews refetch", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + // Pin "now" shortly after the seeded reviewsSyncedAt -- within the new bounded-age backstop's + // 48h window, so this test's cache-hit assertion isn't defeated by real wall-clock time. + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-05-20T01:00:00.000Z")); await seedRegisteredRepo(env); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 67, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index aaf2139318..5a59c90de7 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1003,6 +1003,9 @@ describe("queue processors", () => { 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" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, base: { ref: "main" }, labels: [], body: "Closes #1" }); + // Seed an UP-TO-DATE reviews-cache marker so this dedup-focused call-count test stays isolated from the + // reviews-staleness self-heal (#2537 follow-up) — that behavior has its own dedicated coverage below. + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/agent-repo", pullNumber: 7, status: "complete", reviewsSyncedAt: new Date().toISOString() }); let barePullGets = 0; let branchProtectionGets = 0; let liveCheckRunsGets = 0; @@ -1074,6 +1077,131 @@ describe("queue processors", () => { expect(mergeAttempts).toBe(0); }); + it("REGRESSION (#2537 follow-up): the per-PR sweep unit force-refreshes a STALE reviews cache even when no OTHER reason (slop evidence, manifest gate, pre-merge check paths) would have triggered a refresh", 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" }, 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" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, base: { ref: "main" }, labels: [], body: "Closes #1" }); + // A sync that predates an invalidation (STALE) and no other refresh trigger in play (slop evidence off, + // manifest gate off, no pre-merge check paths configured) — proves the sweep's own visit, not some unrelated + // setting, is what converges the stale reviews cache. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/agent-repo", + pullNumber: 7, + status: "complete", + reviewsSyncedAt: "2026-05-01T00:00:00.000Z", + reviewsInvalidatedAt: "2026-05-02T00:00:00.000Z", + }); + let reviewsGets = 0; + 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: "fake-installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") { + return Response.json({ number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + } + if (url.includes("/pulls/7/files")) return Response.json([]); + if (url.includes("/pulls/7/reviews")) { + reviewsGets += 1; + return Response.json([]); + } + if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + 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({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "reviews-stale-selfheal", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }); + + expect(reviewsGets).toBeGreaterThan(0); + }); + + it("REGRESSION (#2537 second pass): a SILENTLY DROPPED invalidation write (reviewsInvalidatedAt stays null forever) still self-heals via the bounded-age backstop", async () => { + // The invalidation-marker comparison alone (isReviewsCacheUpToDate) reads "up to date" forever when + // markPullRequestReviewsInvalidated's write is dropped -- there is no marker to compare a sync timestamp + // against. Only a bounded-age fallback, independent of the marker, can catch this: an old enough + // reviewsSyncedAt with NO invalidation recorded at all must still be treated as stale. + 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" }, 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" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 8, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a8" }, base: { ref: "main" }, labels: [], body: "Closes #1" }); + // No reviewsInvalidatedAt at all -- the marker comparison alone would read this as permanently up to date. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/agent-repo", + pullNumber: 8, + status: "complete", + reviewsSyncedAt: "2026-05-01T00:00:00.000Z", + }); + let reviewsGets = 0; + 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: "fake-installation-token" }); + if (/\/pulls\/8(?:\?|$)/.test(url) && method === "GET") { + return Response.json({ number: 8, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a8" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + } + if (url.includes("/pulls/8/files")) return Response.json([]); + if (url.includes("/pulls/8/reviews")) { + reviewsGets += 1; + return Response.json([]); + } + if (url.includes("/commits/a8/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a8/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({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + // Far past the 48h bounded-age backstop, well past the 2026-05-01 sync stamp. + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "reviews-dropped-invalidation-selfheal", repoFullName: "owner/agent-repo", prNumber: 8, installationId: 9001 }); + + expect(reviewsGets).toBeGreaterThan(0); + }); + + it("REGRESSION (#2537 follow-up): a failed read of the reviews-cache sync state fails OPEN — the sweep completes without crashing rather than propagating the D1 error", 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" }, 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" }, autoMaintain: { requireApprovals: 0, mergeMethod: "squash" }, aiReviewMode: "off", gatePack: "oss-anti-slop", gateCheckMode: "enabled", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, base: { ref: "main" }, labels: [], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/agent-repo", pullNumber: 7, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + // mockRejectedValue (not -Once): an earlier getPullRequestDetailSyncState read inside the resync/readiness + // path runs before this function's own read, so a single -Once rejection could be consumed there instead. + const syncStateSpy = vi.spyOn(repositoriesModule, "getPullRequestDetailSyncState").mockRejectedValue(new Error("D1 read failed")); + let reviewsGets = 0; + 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: "fake-installation-token" }); + if (/\/pulls\/7(?:\?|$)/.test(url) && method === "GET") { + return Response.json({ number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, mergeable_state: "clean", labels: [], body: "Closes #1" }); + } + if (url.includes("/pulls/7/files")) return Response.json([]); + if (url.includes("/pulls/7/reviews")) { + reviewsGets += 1; + return Response.json([]); + } + if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + 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({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + vi.setSystemTime(new Date("2026-05-28T02:00:00.000Z")); + + await expect( + processJob(env, { type: "agent-regate-pr", deliveryId: "reviews-syncstate-readfail", repoFullName: "owner/agent-repo", prNumber: 7, installationId: 9001 }), + ).resolves.toBeUndefined(); + + expect(syncStateSpy).toHaveBeenCalled(); + syncStateSpy.mockRestore(); + }); + it("#audit-rate-headroom: auto-maintain falls back to the public token when a post-gate mint fails", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITHUB_PUBLIC_TOKEN: "public-token" }); 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: [] } });