From 7aaef477a57cd3da475a2788060c006495858ac8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 04:59:07 -0700 Subject: [PATCH 1/7] feat(github): cache bare PR reads and reviews, mirroring the head-SHA sync-state pattern Closes #2537. GET /pulls/{n} (PR state/mergeable_state) and GET /pulls/{n}/reviews were the largest and third-largest GitHub REST contributors observed in a live rate-limit trough, and neither was covered by the earlier PR-files cache (#audit-rate-headroom / #2527). The bare-PR-state read was implemented as four near-identical helpers with no caching or coalescing, called from readiness checks, the maintenance planner, duplicate-sibling reconciliation, and a gate-override command; reviews were explicitly left uncached ("more volatile than files" -- true at a fixed head, but reviews only actually change on a pull_request_review webhook). Extends the existing pull_request_detail_sync_state table (migration 0093) with prMergeableState/prState/prStateFetchedAt, and reuses the already-present reviewsSyncedAt column as the review cache's freshness marker. PR-state is event-invalidated on pull_request synchronize/closed/reopened (with a 5-minute safety-net TTL so a missed webhook self-heals within one sweep tick); reviews are invalidated on pull_request_review submitted/edited/dismissed rather than on head-SHA change, since reviews are independent of the head. The pre-merge verdict thread and its unified-comment mirror (the #4220 act-boundary) are deliberately left routed through the raw, uncached fetch -- they must always force-refetch live, since the stored/cached mergeable_state lagging GitHub's async recompute is exactly what caused #4220. This is verified by two dedicated regression tests plus direct mutation testing (temporarily rerouting the act-boundary through the cache and confirming both tests -- and no others in the suite -- catch it). --- ...0093_pull_request_detail_sync_pr_state.sql | 8 + src/db/repositories.ts | 15 +- src/db/schema.ts | 7 + src/github/backfill.ts | 213 +++++++++- src/queue/processors.ts | 45 +- src/types.ts | 4 + .../backfill-file-hydration-scoping.test.ts | 185 +++++++++ test/unit/pr-detail-durable-cache.test.ts | 393 ++++++++++++++++++ test/unit/queue.test.ts | 333 +++++++++++++++ 9 files changed, 1182 insertions(+), 21 deletions(-) create mode 100644 migrations/0093_pull_request_detail_sync_pr_state.sql create mode 100644 test/unit/pr-detail-durable-cache.test.ts diff --git a/migrations/0093_pull_request_detail_sync_pr_state.sql b/migrations/0093_pull_request_detail_sync_pr_state.sql new file mode 100644 index 0000000000..342aefe6e4 --- /dev/null +++ b/migrations/0093_pull_request_detail_sync_pr_state.sql @@ -0,0 +1,8 @@ +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN pr_mergeable_state TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN pr_state TEXT; + +ALTER TABLE pull_request_detail_sync_state + ADD COLUMN pr_state_fetched_at TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index c54c3fe6b8..281e92cbb0 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1138,8 +1138,10 @@ export async function getRepoQueueTrendSnapshot(env: Env, repoFullName: string): // drizzle's `onConflictDoUpdate` strips `undefined` entries from the generated SQL `SET` clause rather than // writing NULL. Every "running" pre-fetch stamp (backfill.ts) relies on this to touch only `status` without // clearing the PREVIOUS `headSha`/`*SyncedAt` row — including the repo+PR+headSha file cache -// (#audit-rate-headroom), which would silently stop hitting if a future edit here coalesced an omitted field to -// `null` (e.g. `headSha: state.headSha ?? null`). Pass `null` explicitly to actually clear a column. +// (#audit-rate-headroom) and the durable PR-state / review caches (#2537), which would silently stop hitting if a +// future edit here coalesced an omitted field to `null` (e.g. `headSha: state.headSha ?? null`). Pass `null` +// explicitly to actually clear a column (this is exactly how webhook invalidation clears prMergeableState/ +// prState/reviewsSyncedAt below). export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequestDetailSyncStateRecord): Promise { const db = getDb(env.DB); await db @@ -1155,6 +1157,9 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ checksSyncedAt: state.checksSyncedAt, lastSyncedAt: state.lastSyncedAt, errorSummary: state.errorSummary, + prMergeableState: state.prMergeableState, + prState: state.prState, + prStateFetchedAt: state.prStateFetchedAt, updatedAt: nowIso(), }) .onConflictDoUpdate({ @@ -1167,6 +1172,9 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ checksSyncedAt: state.checksSyncedAt, lastSyncedAt: state.lastSyncedAt, errorSummary: state.errorSummary, + prMergeableState: state.prMergeableState, + prState: state.prState, + prStateFetchedAt: state.prStateFetchedAt, updatedAt: nowIso(), }, }); @@ -4229,6 +4237,9 @@ function toPullRequestDetailSyncStateRecord(row: typeof pullRequestDetailSyncSta checksSyncedAt: row.checksSyncedAt, lastSyncedAt: row.lastSyncedAt, errorSummary: row.errorSummary, + prMergeableState: row.prMergeableState, + prState: row.prState, + prStateFetchedAt: row.prStateFetchedAt, updatedAt: row.updatedAt, }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 4dca4013e4..40bc218444 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -234,6 +234,13 @@ export const pullRequestDetailSyncState = sqliteTable( checksSyncedAt: text("checks_synced_at"), lastSyncedAt: text("last_synced_at"), errorSummary: text("error_summary"), + // Durable bare-PR-state cache (#2537): mirrors GET /pulls/{n}'s mutable state/mergeable_state, refreshed on + // synchronize/closed/reopened webhooks and read by the freshness-guard/readiness/dup-winner/gate-override + // call sites that don't need the disposition's own live-recompute guarantee. NEVER read by the act-boundary + // merge/close decision (planAgentMaintenanceActions / the unified-comment mirror), which always force-refetches. + prMergeableState: text("pr_mergeable_state"), + prState: text("pr_state"), + prStateFetchedAt: text("pr_state_fetched_at"), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }, (table) => ({ diff --git a/src/github/backfill.ts b/src/github/backfill.ts index a0eeeefb8c..3ad643b161 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -326,6 +326,13 @@ const PR_DETAIL_BATCH_SIZE: Record = { light: 12, full: 40 const MERGED_PR_FILE_HYDRATION_BATCH_SIZE: Record = { light: 10, full: 20, resume: 20 }; const PULL_REQUEST_FILES_FETCH_METRIC = "gittensory_github_pull_request_files_fetch_total"; type PullRequestFilesFetchCaller = "backfill_open_pr_details" | "backfill_merged_history" | "live_review"; +// #2537: durable-cache counters for the bare PR-state read and PR reviews, mirroring PULL_REQUEST_FILES_FETCH_METRIC's +// bounded-label style (no per-PR-number labels — cardinality-safe). +const PR_STATE_CACHE_METRIC = "gittensory_pr_state_cache_total"; +const PR_REVIEWS_CACHE_METRIC = "gittensory_pr_reviews_cache_total"; +// Safety-net max age for a webhook-invalidated PR-state cache row (a dropped/missed webhook must not pin a stale +// value forever). Short enough that a missed synchronize/closed/reopened event self-heals within one sweep tick. +const PR_STATE_CACHE_MAX_AGE_MS = 5 * 60 * 1000; const CURRENT_OPEN_SCAN_MARKER = "gittensory-current-open-scan-v1"; const FRESH_TOTALS_SNAPSHOT_MS = 10 * 60 * 1000; const TOTALS_SNAPSHOT_LOOKBACK = 8; @@ -616,13 +623,19 @@ export async function backfillOpenPullRequestDetails( await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details"); const syncedAt = nowIso(); const newWarnings = warnings.slice(before); + // #2537: only stamp reviewsSyncedAt when the review fetch actually SUCCEEDED this pass (no matching warning). + // A failed sync must leave reviewsSyncedAt as-is (omitted, PARTIAL-UPDATE CONTRACT) so the durable review + // cache's reviewsUpToDate check keeps missing and retries next pass — stamping it unconditionally (as the + // pre-#2537 code harmlessly did, since reviews were always refetched anyway) would otherwise permanently cache + // a FAILED sync as if it had succeeded. + const reviewSyncedThisPass = !newWarnings.some((warning) => warning.startsWith(`Review sync failed for #${pr.number}:`)); await upsertPullRequestDetailSyncState(env, { repoFullName: repo.fullName, pullNumber: pr.number, status: newWarnings.length > 0 ? "partial" : "complete", headSha: pr.headSha, filesSyncedAt: syncedAt, - reviewsSyncedAt: syncedAt, + ...(reviewSyncedThisPass ? { reviewsSyncedAt: syncedAt } : {}), checksSyncedAt: syncedAt, lastSyncedAt: syncedAt, errorSummary: newWarnings.at(-1), @@ -692,13 +705,15 @@ export async function refreshPullRequestDetails( await fetchAndStorePullRequestDetails(env, repoFullName, pr, token, warnings, admissionKey, "live_review", { forceFiles: options.force }); const syncedAt = nowIso(); const status: PullRequestDetailSyncStateRecord["status"] = warnings.length > 0 ? "partial" : "complete"; + // #2537: same "only stamp on real success" fix as backfillOpenPullRequestDetails above. + const reviewSyncedThisPass = !warnings.some((warning) => warning.startsWith(`Review sync failed for #${pullNumber}:`)); await upsertPullRequestDetailSyncState(env, { repoFullName, pullNumber, status, headSha: pr.headSha, filesSyncedAt: syncedAt, - reviewsSyncedAt: syncedAt, + ...(reviewSyncedThisPass ? { reviewsSyncedAt: syncedAt } : {}), checksSyncedAt: syncedAt, lastSyncedAt: syncedAt, errorSummary: warnings.at(-1), @@ -1814,13 +1829,15 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back // for PRs only ever touched by this monolithic backfill path. const syncedAt = nowIso(); const newWarnings = warnings.slice(before); + // #2537: same "only stamp on real success" fix as the other two call sites. + const reviewSyncedThisPass = !newWarnings.some((warning) => warning.startsWith(`Review sync failed for #${pr.number}:`)); await upsertPullRequestDetailSyncState(env, { repoFullName: repo.fullName, pullNumber: pr.number, status: newWarnings.length > 0 ? "partial" : "complete", headSha: pr.headSha, filesSyncedAt: syncedAt, - reviewsSyncedAt: syncedAt, + ...(reviewSyncedThisPass ? { reviewsSyncedAt: syncedAt } : {}), checksSyncedAt: syncedAt, lastSyncedAt: syncedAt, errorSummary: newWarnings.at(-1), @@ -1982,16 +1999,24 @@ async function fetchAndStorePullRequestDetails( // Durable repo+PR+headSha file snapshot (#audit-rate-headroom): a bare URL cache is insufficient because // `/pulls/{n}/files` has the SAME url across different heads. Reuse the stored `pull_request_files` rows // instead of refetching when the last successful files sync already covered the PR's CURRENT head SHA — - // only files are cached here; reviews/checks are more volatile at a fixed head and still refresh every call. + // checks are still refetched every call; reviews have their own head-independent cache below (#2537). const existingState = !options.forceFiles && pr.headSha ? await getPullRequestDetailSyncState(env, repoFullName, pr.number) : null; const filesUpToDate = Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt); + // Durable review cache (#2537): reviews are independent of head SHA (a force-push doesn't change who + // approved/requested-changes), so — unlike files — this is gated purely on reviewsSyncedAt being set, not a + // head comparison. reviewsSyncedAt is explicitly cleared (set to null, PARTIAL-UPDATE CONTRACT) by the + // pull_request_review webhook handler (processors.ts) on submitted/edited/dismissed, which is the ONLY thing + // that invalidates it — a head-SHA change alone does NOT invalidate reviews. + const reviewsUpToDate = !options.forceFiles && Boolean(existingState?.reviewsSyncedAt); + incr(PR_REVIEWS_CACHE_METRIC, { result: reviewsUpToDate ? "hit" : "miss" }); const warningStart = warnings.length; const [files, reviews, checks] = await Promise.all([ filesUpToDate ? Promise.resolve([]) : fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey, caller), - fetchPullRequestReviews(env, repoFullName, pr.number, token, warnings, admissionKey), + reviewsUpToDate ? Promise.resolve([]) : fetchPullRequestReviews(env, repoFullName, pr.number, token, warnings, admissionKey), fetchPullRequestChecks(env, repoFullName, pr, token, warnings, admissionKey), ]); const fileSyncFailed = warnings.slice(warningStart).some((warning) => warning.startsWith(`File sync failed for #${pr.number}:`)); + const reviewSyncFailed = warnings.slice(warningStart).some((warning) => warning.startsWith(`Review sync failed for #${pr.number}:`)); if (!filesUpToDate && !fileSyncFailed) { await deletePullRequestFiles(env, repoFullName, pr.number); @@ -2009,17 +2034,19 @@ async function fetchAndStorePullRequestDetails( }); } } - for (const review of reviews) { - await upsertPullRequestReview(env, { - id: `${repoFullName}#${pr.number}#${review.id}`, - repoFullName, - pullNumber: pr.number, - reviewerLogin: review.user?.login, - state: review.state ?? "UNKNOWN", - authorAssociation: review.author_association, - submittedAt: review.submitted_at, - payload: review as unknown as Record, - }); + if (!reviewsUpToDate && !reviewSyncFailed) { + for (const review of reviews) { + await upsertPullRequestReview(env, { + id: `${repoFullName}#${pr.number}#${review.id}`, + repoFullName, + pullNumber: pr.number, + reviewerLogin: review.user?.login, + state: review.state ?? "UNKNOWN", + authorAssociation: review.author_association, + submittedAt: review.submitted_at, + payload: review as unknown as Record, + }); + } } for (const check of checks.check_runs ?? []) { await upsertCheckSummary(env, { @@ -2726,6 +2753,160 @@ export async function fetchLivePullRequest( return result?.data ?? undefined; } +// #2537: durable, webhook-invalidated cache for the bare PR-state read (GET /pulls/{n}). Unlike the request-local +// LiveGithubFacts memo (queue/processors.ts), this survives ACROSS webhook deliveries / sweep ticks, cutting +// repeat /pulls/{n} calls for an unchanged PR at the freshness-guard/readiness/dup-winner/gate-override call +// sites. NEVER used by the act-boundary merge/close decision (planAgentMaintenanceActions's liveMergeState read, +// or its unified-comment mirror) — those force-refetch via refreshLiveMergeState by design (the #4220 fix) and +// must keep doing so; this cache exists purely for the OTHER, non-authoritative reads. +function isPrStateCacheFresh(fetchedAt: string | null | undefined): boolean { + if (!fetchedAt) return false; + const fetchedAtMs = Date.parse(fetchedAt); + if (!Number.isFinite(fetchedAtMs)) return false; + return Date.now() - fetchedAtMs < PR_STATE_CACHE_MAX_AGE_MS; +} + +/** Best-effort write-through for the PR-state cache fields. Always stamps prStateFetchedAt = now on a successful + * live read (even when the live value itself is undefined/null — a confirmed-empty read is still a fresh read, + * distinct from "never fetched"), so a run of undefined reads doesn't force every caller back to GitHub. Preserves + * the row's own `status` (defaulting to "never_synced" only when no row exists yet) — this write must NEVER force + * `status: "complete"`, since `status` is shared with the FILES-cache staleness machinery + * (backfillOpenPullRequestDetails / refreshPullRequestDetails treat `status !== "complete"` as "needs a files + * resync"); a PR-state-only write claiming `complete` would falsely mark a files sync that never happened. A + * write failure is swallowed (#2537 fail-open: the cache is an optimization, never a correctness dependency — + * every caller already tolerates a live-fetch fallback). */ +async function writeThroughPrStateCache( + env: Env, + repoFullName: string, + prNumber: number, + previousStatus: PullRequestDetailSyncStateRecord["status"] | undefined, + fields: { prMergeableState?: string | null; prState?: string | null; headSha?: string | null }, +): Promise { + incr(PR_STATE_CACHE_METRIC, { field: "write", result: "set" }); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber: prNumber, + status: previousStatus ?? "never_synced", + prStateFetchedAt: nowIso(), + ...fields, + }).catch(() => undefined); +} + +/** 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 + * own "preserve prior status" contract. */ +export async function primeDurablePrStateCache( + env: Env, + repoFullName: string, + prNumber: number, + live: { mergeable_state?: string | null; state?: string | null; head?: { sha?: string | null } | null } | undefined, +): Promise { + if (!live) return; + const existing = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + const liveHeadSha = live.head?.sha; + await writeThroughPrStateCache(env, repoFullName, prNumber, existing?.status, { + prMergeableState: live.mergeable_state ?? null, + prState: live.state ?? null, + // Omit (not null) when the live payload carries no head SHA — a PR-state-only write must never CLEAR the + // headSha the files cache (#audit-rate-headroom) relies on (PARTIAL-UPDATE CONTRACT: omitted = unchanged). + ...(liveHeadSha ? { headSha: liveHeadSha } : {}), + }); +} + +/** 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. + * 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, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (cached && isPrStateCacheFresh(cached.prStateFetchedAt)) { + incr(PR_STATE_CACHE_METRIC, { field: "mergeable_state", result: "hit" }); + 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; +} + +/** 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. */ +export async function cachedFetchLivePullRequestState( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (cached && isPrStateCacheFresh(cached.prStateFetchedAt)) { + incr(PR_STATE_CACHE_METRIC, { field: "state", result: "hit" }); + 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; +} + +/** Cached read of the PR's live head SHA, backed by pull_request_detail_sync_state (#2537). Reuses the EXISTING + * headSha column (written by the files-cache path too) as the cached value; a cache hit still respects the same + * PR_STATE_CACHE_MAX_AGE_MS freshness window as the other two fields (headSha alone predates this issue and + * carries no fetchedAt guarantee, so gate it on prStateFetchedAt like its siblings). */ +export async function cachedFetchLivePullRequestHeadSha( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const cached = await getPullRequestDetailSyncState(env, repoFullName, prNumber).catch(() => null); + if (cached && isPrStateCacheFresh(cached.prStateFetchedAt) && cached.headSha) { + incr(PR_STATE_CACHE_METRIC, { field: "head_sha", result: "hit" }); + 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; +} + +/** Invalidate the durable PR-state cache fields (#2537) — called on synchronize/closed/reopened. Explicit null + * (not omitted) so the PARTIAL-UPDATE CONTRACT actually clears the stale value rather than leaving it. Best- + * effort by design at the call site (never blocks webhook processing on a cache-invalidation write). */ +export async function invalidatePrStateCache(env: Env, repoFullName: string, pullNumber: number): Promise { + const existing = await getPullRequestDetailSyncState(env, repoFullName, pullNumber).catch(() => null); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber, + status: existing?.status ?? "never_synced", + prMergeableState: null, + prState: null, + prStateFetchedAt: null, + }); +} + +/** Invalidate the durable review cache (#2537) — called on pull_request_review submitted/edited/dismissed. + * Reviews are independent of head SHA, so this clears ONLY reviewsSyncedAt (not headSha/filesSyncedAt), and the + * next fetchAndStorePullRequestDetails call's reviewsUpToDate check will miss and refetch. */ +export async function invalidatePrReviewsCache(env: Env, repoFullName: string, pullNumber: number): Promise { + incr(PR_REVIEWS_CACHE_METRIC, { result: "invalidated" }); + const existing = await getPullRequestDetailSyncState(env, repoFullName, pullNumber).catch(() => null); + await upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber, + status: existing?.status ?? "never_synced", + reviewsSyncedAt: null, + }); +} + /** Resolve the OPEN PRs associated with a commit SHA via the REST `GET /repos/{owner}/{repo}/commits/{sha}/pulls` * endpoint. This is the only PR↔commit resolution that works for FORK (cross-repo) PRs, whose CI-completion * webhooks (`check_suite`/`check_run`) carry an EMPTY `pull_requests[]`. Returns the de-duplicated open PR numbers. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ffbb345d69..f6bd79af53 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -75,6 +75,9 @@ import { backfillOpenPullRequestDetails, backfillRegisteredRepositories, backfillRepositorySegment, + cachedFetchLivePullRequestHeadSha, + cachedFetchLivePullRequestMergeState, + cachedFetchLivePullRequestState, enqueueRepositoryOpenDataBackfill, fetchAndStorePullRequestFilesForReview, fetchLinkedIssueFacts, @@ -90,6 +93,9 @@ import { fetchLivePullRequestState, fetchOpenPullRequestNumbersForCommit, fetchRequiredStatusContexts, + invalidatePrReviewsCache, + invalidatePrStateCache, + primeDurablePrStateCache, refreshContributorActivity, refreshInstallationHealth, refreshPullRequestDetails, @@ -614,15 +620,24 @@ function cachedLiveMergeState( const key = liveFactKey(repoFullName, prNumber, liveFactTokenPart(token)); const cached = facts.mergeStates.get(key); if (cached) return cached; + // #2537: on a request-local miss, check the DURABLE cross-webhook cache before hitting GitHub — this is the + // readiness/freshness-guard path, not the act-boundary disposition (that's refreshLiveMergeState below, which + // NEVER routes through the durable cache). A durable hit is itself memoized request-locally for the rest of + // this pass via facts.mergeStates, same as a live fetch would be. const next = evictLiveFactOnReject( facts.mergeStates, key, - fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey), + cachedFetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey), ); facts.mergeStates.set(key, next); return next; } +// #4220 contradiction: the stored pr.mergeableState lags GitHub's async recompute, so a base-conflicting PR could +// read clean here (safe to merge) while the disposition reads the live dirty and auto-CLOSES it. This ALWAYS +// force-refetches live from GitHub and MUST NEVER be routed through the durable pull_request_detail_sync_state +// cache added by #2537 — both act-boundary-adjacent callers (runAgentMaintenancePlanAndExecute's disposition +// input, and the unified-comment mirror) depend on this staying live and uncached. function refreshLiveMergeState( env: Env, repoFullName: string, @@ -2188,6 +2203,10 @@ async function reReviewStoredPullRequest( resyncAdmissionKey, ); primeLiveMergeState(liveFacts, repoFullName, prNumber, resyncToken, live?.mergeable_state); + // #2537: this resync ALREADY paid for a bare GET /pulls/{n} — persist it to the durable cross-webhook cache so + // the readiness/dup-winner/gate-override readers below (and future webhook deliveries) don't re-fetch it. + // Best-effort, never blocks the sweep on a write hiccup. + await primeDurablePrStateCache(env, repoFullName, prNumber, live).catch(() => undefined); // Terminal early-exit (#1942): the PR is CLOSED/merged on GitHub even though the stored row still reads open — a // dropped `closed` webhook (relay down). Reconcile the stored row from the live payload and RETURN before the // expensive resync (files) + readiness + re-review reads. A stale sweep must never spend GitHub budget — or post @@ -4225,6 +4244,14 @@ async function processGitHubWebhook( repoFullName, payload.pull_request, ); + // #2537: the durable PR-state cache (mergeable_state/state) goes stale exactly when GitHub recomputes them — + // synchronize (new head → new mergeable_state recompute), closed (state flips), reopened (state flips back). + // Clear explicitly (null, not omitted — PARTIAL-UPDATE CONTRACT) so the next cached read is a forced live + // miss; other pull_request actions (labeled, edited, etc.) don't change these fields and are left untouched + // to avoid spurious cache churn / extra writes on high-frequency low-signal actions. + if (eventName === "pull_request" && (payload.action === "synchronize" || payload.action === "closed" || payload.action === "reopened")) { + await invalidatePrStateCache(env, repoFullName, pr.number).catch(() => undefined); + } // Reopen-prevention (#one-shot-reopen): a CONTRIBUTOR may not reopen a PR that gittensory or a maintainer // closed — closes are one-shot (resubmit, don't reopen). If a non-maintainer reopened a PR whose last close // was by the bot / repo owner / admin, re-close it and skip the re-review. Self-closes (the contributor @@ -4322,6 +4349,14 @@ async function processGitHubWebhook( installationId && shouldProcessPullRequestPublicSurface(eventName, payload.action) ) { + // #2537: reviews only actually change on a pull_request_review webhook (submitted/edited/dismissed) — not + // on every sweep tick / unrelated pull_request action — so invalidate the durable review cache HERE, + // before the refreshPullRequestDetails call below reads it, rather than on head-SHA change (reviews are + // independent of head). shouldProcessPullRequestPublicSurface already gates pull_request_review to + // exactly these 3 actions. + if (eventName === "pull_request_review") { + await invalidatePrReviewsCache(env, repoFullName, pr.number).catch(() => undefined); + } if ( shouldCollectSlopEvidence(settings) || settings.manifestPolicyGateMode !== "off" || @@ -5615,7 +5650,9 @@ export async function reconcileLiveDuplicateSiblings( const staleClosed = new Set(); await Promise.all( lowerOverlapping.map(async (sibling) => { - const liveState = await fetchLivePullRequestState( + // #2537: durable-cached — this is a non-authoritative reconcile read (not the act-boundary decision), so + // repeat calls across webhook deliveries for an unchanged sibling can reuse the cross-webhook cache. + const liveState = await cachedFetchLivePullRequestState( env, repoFullName, sibling.number, @@ -7390,6 +7427,8 @@ async function recordGithubProductUsage( * 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. + * #2537: durable-cached — a separate `issue_comment` webhook handler with no request-local `liveFacts`, so the + * cross-webhook cache is a pure win here (not the act-boundary merge/close decision). */ export async function resolveOverrideHeadSha( env: Env, @@ -7402,7 +7441,7 @@ export async function resolveOverrideHeadSha( () => undefined, )) ?? env.GITHUB_PUBLIC_TOKEN; const admissionKey = githubAdmissionKeyForToken(env, installationId, token); - const liveHeadSha = await fetchLivePullRequestHeadSha( + const liveHeadSha = await cachedFetchLivePullRequestHeadSha( env, repoFullName, pr.number, diff --git a/src/types.ts b/src/types.ts index b8b6e57b70..aabdcfb09b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -874,6 +874,10 @@ export type PullRequestDetailSyncStateRecord = { lastSyncedAt?: string | null | undefined; errorSummary?: string | null | undefined; updatedAt?: string | null | undefined; + // #2537: durable bare-PR-state cache fields (mergeable_state/state from GET /pulls/{n}). + prMergeableState?: string | null | undefined; + prState?: string | null | undefined; + prStateFetchedAt?: string | null | undefined; }; export type GitHubRateLimitObservationRecord = { diff --git a/test/unit/backfill-file-hydration-scoping.test.ts b/test/unit/backfill-file-hydration-scoping.test.ts index 22ed3e12fa..c626afd490 100644 --- a/test/unit/backfill-file-hydration-scoping.test.ts +++ b/test/unit/backfill-file-hydration-scoping.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { getPullRequestDetailSyncState, listPullRequestFiles, + listPullRequestReviews, listRecentMergedPullRequests, recordGitHubRateLimitObservation, upsertPullRequestDetailSyncState, @@ -409,4 +410,188 @@ describe("GitHub PR file hydration scoping (#audit-rate-headroom)", () => { expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 91)).toMatchObject({ headSha: null }); }); }); + + // Durable review cache (#2537): reviews are independent of head SHA, so — unlike the files cache above — this is + // gated purely on reviewsSyncedAt being set (webhook-invalidated), not a head comparison. + describe("durable review cache (#2537)", () => { + it("fetches reviews for a PR with no prior reviewsSyncedAt (cold cache)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 100, + title: "Open PR, never synced", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-100" }, + labels: [], + body: "", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/100/reviews") ? Response.json([{ id: 1, user: { login: "reviewer" }, state: "APPROVED", submitted_at: "2026-05-20T00:00:00.000Z" }]) : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 100); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/100/reviews"))).toBe(true); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 100)).toEqual([expect.objectContaining({ reviewerLogin: "reviewer", state: "APPROVED" })]); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 100)).toMatchObject({ reviewsSyncedAt: expect.any(String) }); + }); + + it("stores a fetched review with a missing state as UNKNOWN (nullish fallback)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 105, + title: "Open PR, stateless review payload", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-105" }, + labels: [], + body: "", + }); + stubFetchTracking((url) => (url.includes("/pulls/105/reviews") ? Response.json([{ id: 9, user: { login: "reviewer" }, submitted_at: "2026-05-20T00:00:00.000Z" }]) : Response.json([]))); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 105); + + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 105)).toEqual([expect.objectContaining({ reviewerLogin: "reviewer", state: "UNKNOWN" })]); + }); + + it("reuses the review snapshot without calling GitHub when reviewsSyncedAt is set, even if the head SHA changed (contrast: files DO refetch on head change)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 101, + title: "Open PR, new push", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-101-new" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 101, + status: "complete", + headSha: "head-101-old", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => { + if (url.includes("/pulls/101/reviews")) return new Response("must not be called", { status: 500 }); + if (url.includes("/pulls/101/files")) return Response.json([{ filename: "src/new.ts", status: "added", additions: 1, deletions: 0, changes: 1 }]); + return Response.json([]); + }); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 101); + + expect(result).toMatchObject({ status: "complete" }); + // Reviews: NOT refetched (head-independent cache still valid). + expect(urls.some((url) => url.includes("/pulls/101/reviews"))).toBe(false); + // Files: DID refetch — the head SHA changed, so the file cache (head-scoped) correctly missed. + expect(urls.some((url) => url.includes("/pulls/101/files"))).toBe(true); + }); + + it("fetches fresh reviews when reviewsSyncedAt is null (invalidated)", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 102, + title: "Open PR, review invalidated", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-102" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 102, + status: "complete", + headSha: "head-102", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: null, + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/102/reviews") ? Response.json([{ id: 2, user: { login: "reviewer2" }, state: "CHANGES_REQUESTED", submitted_at: "2026-05-21T00:00:00.000Z" }]) : Response.json([]), + ); + + const result = await refreshPullRequestDetails(env, "JSONbored/gittensory", 102); + + expect(result).toMatchObject({ status: "complete" }); + expect(urls.some((url) => url.includes("/pulls/102/reviews"))).toBe(true); + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 102)).toEqual([expect.objectContaining({ reviewerLogin: "reviewer2" })]); + }); + + it("regression: a FAILED review sync does not stamp reviewsSyncedAt, so the next call retries instead of permanently caching the failure", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 106, + title: "Open PR, review sync fails then recovers", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-106" }, + labels: [], + body: "", + }); + let reviewsCalls = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/106/reviews")) { + reviewsCalls += 1; + return reviewsCalls === 1 ? new Response("review failure", { status: 503 }) : Response.json([{ id: 3, user: { login: "reviewer3" }, state: "APPROVED", submitted_at: "2026-05-22T00:00:00.000Z" }]); + } + return Response.json([]); + }); + + const first = await refreshPullRequestDetails(env, "JSONbored/gittensory", 106); + expect(first.status).toBe("partial"); + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 106)).toMatchObject({ reviewsSyncedAt: null }); + + const second = await refreshPullRequestDetails(env, "JSONbored/gittensory", 106); + expect(second.status).toBe("complete"); + expect(reviewsCalls).toBe(2); // retried — NOT skipped as "already synced" from the failed first attempt + expect(await listPullRequestReviews(env, "JSONbored/gittensory", 106)).toEqual([expect.objectContaining({ reviewerLogin: "reviewer3" })]); + }); + + it("a 'running' pre-fetch stamp does not clear a prior reviewsSyncedAt (PARTIAL-UPDATE CONTRACT regression)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 103, + status: "complete", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 103, status: "running" }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 103)).toMatchObject({ + status: "running", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + }); + + it("records miss then hit on the reviews cache metric across a cold then warm read", async () => { + resetMetrics(); + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 104, + title: "Metrics PR", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-104" }, + labels: [], + body: "", + }); + stubFetchTracking(() => Response.json([])); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 104); + await refreshPullRequestDetails(env, "JSONbored/gittensory", 104); + + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_pr_reviews_cache_total{result="miss"} 1'); + expect(metrics).toContain('gittensory_pr_reviews_cache_total{result="hit"} 1'); + }); + }); }); diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts new file mode 100644 index 0000000000..0d5407c0d8 --- /dev/null +++ b/test/unit/pr-detail-durable-cache.test.ts @@ -0,0 +1,393 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + getPullRequestDetailSyncState, + upsertPullRequestDetailSyncState, +} from "../../src/db/repositories"; +import { + cachedFetchLivePullRequestHeadSha, + cachedFetchLivePullRequestMergeState, + cachedFetchLivePullRequestState, + invalidatePrStateCache, + primeDurablePrStateCache, +} from "../../src/github/backfill"; +import { clearGitHubResponseCacheForTest } from "../../src/github/client"; +import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics"; +import { createTestEnv } from "../helpers/d1"; + +// Durable, webhook-invalidated cache for the bare PR-state read (#2537). Mirrors +// backfill-file-hydration-scoping.test.ts's helpers/structure for the sibling files cache. +describe("durable PR-state cache (#2537)", () => { + afterEach(() => { + clearGitHubResponseCacheForTest(); + resetMetrics(); + vi.unstubAllGlobals(); + }); + + function stubFetchTracking(handler: (url: string, init?: RequestInit) => Response | Promise): string[] { + const urls: string[] = []; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + urls.push(url); + return handler(url, init); + }); + return urls; + } + + // Simulates a D1 write hiccup ONLY for pull_request_detail_sync_state upserts, so the cache's fail-open + // write-through can be exercised without a full DB outage. + function withPrStateWriteFailure(env: Env): Env { + const db = env.DB as unknown as { prepare(sql: string): unknown; batch(statements: unknown[]): Promise }; + return { + ...env, + DB: { + prepare(sql: string) { + if (sql.includes("pull_request_detail_sync_state") && sql.trim().toUpperCase().startsWith("INSERT")) { + throw new Error("pull_request_detail_sync_state write failed"); + } + return db.prepare.call(db, sql); + }, + batch(statements: unknown[]) { + return db.batch.call(db, statements); + }, + } as unknown as D1Database, + }; + } + + describe("cachedFetchLivePullRequestMergeState", () => { + it("cache miss on first read — fetches live and writes the row through", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/10") ? Response.json({ number: 10, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 10, "tok"); + + expect(result).toBe("clean"); + expect(urls.some((url) => url.includes("/pulls/10"))).toBe(true); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 10)).toMatchObject({ prMergeableState: "clean" }); + }); + + it("fail-open: a write-through hiccup still returns the live value (the cache is an optimization, not a dependency)", async () => { + const env = withPrStateWriteFailure(createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" })); + stubFetchTracking((url) => (url.includes("/pulls/14") ? Response.json({ number: 14, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 14, "tok"); + + expect(result).toBe("clean"); + }); + + it("cache hit on unchanged state — never calls GitHub", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 11, + status: "complete", + prMergeableState: "blocked", + prStateFetchedAt: new Date().toISOString(), + }); + const urls = stubFetchTracking(() => new Response("must not be called", { status: 500 })); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 11, "tok"); + + expect(result).toBe("blocked"); + expect(urls).toHaveLength(0); + }); + + it("cache expiry — a stale row past the TTL is treated as a miss (fetch IS called)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 12, + status: "complete", + prMergeableState: "blocked", + prStateFetchedAt: "2020-01-01T00:00:00.000Z", + }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/12") ? Response.json({ number: 12, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 12, "tok"); + + expect(result).toBe("clean"); + expect(urls.some((url) => url.includes("/pulls/12"))).toBe(true); + }); + + it("an undefined live read still stamps prStateFetchedAt, so the next read within the TTL is a cache hit returning undefined, not a fetch", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/13")) { + fetchCount += 1; + return Response.json({ number: 13 }); // no mergeable_state field + } + return new Response("not found", { status: 404 }); + }); + + const first = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 13, "tok"); + expect(first).toBeUndefined(); + expect(fetchCount).toBe(1); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 13)).toMatchObject({ prMergeableState: null }); + + const second = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 13, "tok"); + expect(second).toBeUndefined(); + expect(fetchCount).toBe(1); // still 1 — served from cache, not re-fetched + }); + }); + + describe("cachedFetchLivePullRequestState", () => { + it("cache miss then cache hit", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/20")) { + fetchCount += 1; + return Response.json({ number: 20, state: "open" }); + } + return new Response("not found", { status: 404 }); + }); + + const first = await cachedFetchLivePullRequestState(env, "owner/repo", 20, "tok"); + const second = await cachedFetchLivePullRequestState(env, "owner/repo", 20, "tok"); + + expect(first).toBe("open"); + expect(second).toBe("open"); + expect(fetchCount).toBe(1); + }); + + it("a cache hit whose stored prState is null (nullish live read) returns undefined, not null", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 21, + status: "complete", + prState: null, + prStateFetchedAt: new Date().toISOString(), + }); + const urls = stubFetchTracking(() => new Response("must not be called", { status: 500 })); + + const result = await cachedFetchLivePullRequestState(env, "owner/repo", 21, "tok"); + + expect(result).toBeUndefined(); + expect(urls).toHaveLength(0); + }); + }); + + describe("cachedFetchLivePullRequestHeadSha", () => { + it("cache miss then cache hit, and does not serve a cached row missing headSha", async () => { + const env = createTestEnv(); + // A row that is fresh (prStateFetchedAt set) but has no headSha yet must still be treated as a miss. + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 30, + status: "complete", + prStateFetchedAt: new Date().toISOString(), + }); + let fetchCount = 0; + stubFetchTracking((url) => { + if (url.includes("/pulls/30")) { + fetchCount += 1; + return Response.json({ number: 30, head: { sha: "live-sha" } }); + } + return new Response("not found", { status: 404 }); + }); + + const first = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 30, "tok"); + expect(first).toBe("live-sha"); + expect(fetchCount).toBe(1); + + const second = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 30, "tok"); + expect(second).toBe("live-sha"); + expect(fetchCount).toBe(1); + }); + + it("does not write through when the live head SHA is undefined (never clears a prior headSha)", async () => { + const env = createTestEnv(); + stubFetchTracking(() => Response.json({ number: 31 })); // no head.sha + + const result = await cachedFetchLivePullRequestHeadSha(env, "owner/repo", 31, "tok"); + + expect(result).toBeUndefined(); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 31)).toBeNull(); + }); + }); + + describe("primeDurablePrStateCache", () => { + it("does nothing when the live payload is undefined (upstream fetchLivePullRequest failed)", async () => { + const env = createTestEnv(); + + await primeDurablePrStateCache(env, "owner/repo", 60, undefined); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 60)).toBeNull(); + }); + + it("writes mergeable_state/state/headSha through from an already-fetched live payload", async () => { + const env = createTestEnv(); + + await primeDurablePrStateCache(env, "owner/repo", 61, { mergeable_state: "dirty", state: "open", head: { sha: "primed-sha" } }); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 61)).toMatchObject({ + prMergeableState: "dirty", + prState: "open", + headSha: "primed-sha", + prStateFetchedAt: expect.any(String), + }); + }); + + it("stores a nullish mergeable_state/state from the live payload as null, not undefined", async () => { + const env = createTestEnv(); + + await primeDurablePrStateCache(env, "owner/repo", 62, { head: { sha: "primed-sha-2" } }); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 62)).toMatchObject({ + prMergeableState: null, + prState: null, + headSha: "primed-sha-2", + }); + }); + + it("PARTIAL-UPDATE CONTRACT: omits headSha (does not clear a prior one) when the live payload carries no head.sha", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 63, + status: "complete", + headSha: "files-cache-sha", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + + await primeDurablePrStateCache(env, "owner/repo", 63, { mergeable_state: "clean", state: "open" }); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 63)).toMatchObject({ + prMergeableState: "clean", + prState: "open", + headSha: "files-cache-sha", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + }); + + it("primes the cache so a subsequent cachedFetchLivePullRequestMergeState reads the primed value without a network call", async () => { + const env = createTestEnv(); + let fetchCount = 0; + stubFetchTracking(() => { + fetchCount += 1; + return Response.json({ number: 64, mergeable_state: "should-not-be-fetched" }); + }); + + await primeDurablePrStateCache(env, "owner/repo", 64, { mergeable_state: "primed-dirty", head: { sha: "sha-64" } }); + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 64, "tok"); + + expect(result).toBe("primed-dirty"); + expect(fetchCount).toBe(0); + }); + }); + + describe("invalidatePrStateCache", () => { + it("clears prMergeableState/prState/prStateFetchedAt and forces the next read to miss", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 40, + status: "complete", + prMergeableState: "clean", + prState: "open", + prStateFetchedAt: new Date().toISOString(), + }); + + await invalidatePrStateCache(env, "owner/repo", 40); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 40)).toMatchObject({ + prMergeableState: null, + prState: null, + prStateFetchedAt: null, + }); + + const urls = stubFetchTracking((url) => (url.includes("/pulls/40") ? Response.json({ number: 40, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 40, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/40"))).toBe(true); + }); + + it("defaults status to never_synced when no prior row exists", async () => { + const env = createTestEnv(); + await invalidatePrStateCache(env, "owner/repo", 41); + expect(await getPullRequestDetailSyncState(env, "owner/repo", 41)).toMatchObject({ status: "never_synced" }); + }); + }); + + describe("write-through preserves status (regression: must not force status: complete)", () => { + it("a cachedFetchLivePullRequest* write does not clear an in-progress files sync's status/filesSyncedAt", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 50, + status: "partial", + headSha: "sha-a", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + }); + stubFetchTracking((url) => (url.includes("/pulls/50") ? Response.json({ number: 50, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + await cachedFetchLivePullRequestMergeState(env, "owner/repo", 50, "tok"); + + expect(await getPullRequestDetailSyncState(env, "owner/repo", 50)).toMatchObject({ + status: "partial", + headSha: "sha-a", + filesSyncedAt: "2026-05-20T00:00:00.000Z", + prMergeableState: "clean", + }); + }); + }); + + describe("isPrStateCacheFresh branch coverage (via cachedFetchLivePullRequestMergeState)", () => { + it("null fetchedAt ⇒ treated as stale (miss)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 60, status: "complete", prMergeableState: "clean", prStateFetchedAt: null }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/60") ? Response.json({ number: 60, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 60, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/60"))).toBe(true); + }); + + it("unparseable fetchedAt string ⇒ treated as stale (NaN branch, miss)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 61, status: "complete", prMergeableState: "clean", prStateFetchedAt: "not-a-date" }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/61") ? Response.json({ number: 61, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 61, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/61"))).toBe(true); + }); + + it("fresh fetchedAt ⇒ hit (no fetch)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 62, status: "complete", prMergeableState: "clean", prStateFetchedAt: new Date().toISOString() }); + const urls = stubFetchTracking(() => new Response("must not be called", { status: 500 })); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 62, "tok"); + expect(result).toBe("clean"); + expect(urls).toHaveLength(0); + }); + + it("expired fetchedAt ⇒ miss (fetch called)", async () => { + const env = createTestEnv(); + await upsertPullRequestDetailSyncState(env, { repoFullName: "owner/repo", pullNumber: 63, status: "complete", prMergeableState: "clean", prStateFetchedAt: "2020-01-01T00:00:00.000Z" }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/63") ? Response.json({ number: 63, mergeable_state: "dirty" }) : new Response("not found", { status: 404 }))); + + const result = await cachedFetchLivePullRequestMergeState(env, "owner/repo", 63, "tok"); + expect(result).toBe("dirty"); + expect(urls.some((url) => url.includes("/pulls/63"))).toBe(true); + }); + }); + + describe("metrics", () => { + it("records miss then hit for mergeable_state across a cold then warm read", async () => { + resetMetrics(); + const env = createTestEnv(); + stubFetchTracking((url) => (url.includes("/pulls/70") ? Response.json({ number: 70, mergeable_state: "clean" }) : new Response("not found", { status: 404 }))); + + await cachedFetchLivePullRequestMergeState(env, "owner/repo", 70, "tok"); + await cachedFetchLivePullRequestMergeState(env, "owner/repo", 70, "tok"); + + const metrics = await renderMetrics(); + expect(metrics).toContain('gittensory_pr_state_cache_total{field="mergeable_state",result="miss"} 1'); + expect(metrics).toContain('gittensory_pr_state_cache_total{field="mergeable_state",result="hit"} 1'); + expect(metrics).toContain('gittensory_pr_state_cache_total{field="write",result="set"} 1'); + }); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c5ccf7b9d2..0fd78ece4f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -18,6 +18,7 @@ import { getInstallation, getLatestUpstreamRulesetSnapshot, getPullRequest, + getPullRequestDetailSyncState, getRepository, listUpstreamDriftReports, listInstallationHealth, @@ -38,6 +39,7 @@ import { upsertInstallation, updatePullRequestSlopAssessment, upsertOfficialMinerDetection, + upsertPullRequestDetailSyncState, upsertPullRequestFile, upsertPullRequestFromGitHub, upsertIssueWatchSubscription, @@ -13275,6 +13277,337 @@ describe("queue processors", () => { expect(warn.mock.calls.map((c) => String(c[0])).some((line) => line.includes("ops_anomaly") && line.includes("owner/repo"))).toBe(true); warn.mockRestore(); }); + + // #2537: durable PR-state / review caches — webhook invalidation + the act-boundary regression. + describe("durable PR-state and review caches (#2537)", () => { + function seedWarmPrStateCache(env: Env, repoFullName: string, pullNumber: number): Promise { + return upsertPullRequestDetailSyncState(env, { + repoFullName, + pullNumber, + status: "complete", + prMergeableState: "clean", + prState: "open", + prStateFetchedAt: new Date().toISOString(), + }); + } + + it.each(["synchronize", "closed", "reopened"] as const)( + "pull_request %s action invalidates the durable PR-state cache", + async (action) => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + 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" }); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: `invalidate-pr-state-${action}`, + eventName: "pull_request", + payload: { + action, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 200, title: "PR", state: action === "closed" ? "closed" : "open", user: { login: "contributor" }, head: { sha: "a200" }, labels: [], body: "" }, + }, + }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 200)).toMatchObject({ + prMergeableState: null, + prState: null, + prStateFetchedAt: null, + }); + }, + ); + + it("a non-invalidating pull_request action (labeled) leaves the durable PR-state cache UNCHANGED", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + 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" }); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "invalidate-pr-state-labeled", + eventName: "pull_request", + payload: { + action: "labeled", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 201, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "a201" }, labels: [], body: "" }, + }, + }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 201)).toMatchObject({ + prMergeableState: "clean", + prState: "open", + }); + }); + + it.each(["submitted", "edited", "dismissed"] as const)( + "pull_request_review %s action invalidates the durable review cache", + async (action) => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 202, + status: "complete", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + 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("/pulls/202/files")) return Response.json([]); + if (url.includes("/pulls/202/reviews")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: `invalidate-reviews-${action}`, + eventName: "pull_request_review", + payload: { + action, + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 202, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "a202" }, labels: [], body: "" }, + review: { state: "approved", user: { login: "maintainer" }, submitted_at: "2026-06-01T00:00:00.000Z" }, + }, + }); + + // Immediately after invalidation, refreshPullRequestDetails (gated by shouldRefreshFilesForPreMergeChecks + // etc., which is not guaranteed true here) may or may not re-stamp it — assert the invalidating WRITE + // happened by checking the row is either null (invalidated, not yet refetched) or freshly re-stamped + // (invalidated then immediately refetched within this same pass); either way it is NOT the stale value. + const state = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 202); + expect(state?.reviewsSyncedAt).not.toBe("2026-05-20T00:00:00.000Z"); + }, + ); + + it("a pull_request_review_comment event (sibling of pull_request_review) does NOT invalidate the durable review cache", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 203, + status: "complete", + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "no-invalidate-review-comment", + eventName: "pull_request_review_comment", + payload: { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 203, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "a203" }, labels: [], body: "" }, + comment: { id: 1, body: "nit", user: { login: "maintainer" } }, + }, + }); + + expect(await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 203)).toMatchObject({ + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + }); + + // The single most important regression in #2537: a STALE durable cache (seeded "clean") must NEVER be read by + // the act-boundary merge/close decision. The disposition must always see the LIVE state, even when it + // disagrees with a fresh-looking cache row — this is the exact #4220 contradiction the design must not + // reintroduce. + it("#4220 regression: the act-boundary close decision reads the LIVE dirty mergeable_state, never the stale cached clean value", 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: { close: "auto" }, + aiReviewMode: "off", + gatePack: "oss-anti-slop", + gateCheckMode: "enabled", + checkRunMode: "off", + commentMode: "off", + publicSurface: "off", + }); + await upsertPullRequestFromGitHub(env, "owner/agent-repo", { number: 8, title: "Conflicted PR", state: "open", user: { login: "contributor" }, head: { sha: "b8" }, base: { ref: "main" }, labels: [], body: "Closes #1" }); + // Seed a FRESH-LOOKING durable cache claiming `clean` — if the act-boundary read were ever routed through + // this cache, the PR would incorrectly stay open / merge instead of closing. + await seedWarmPrStateCache(env, "owner/agent-repo", 8); + // Isolation (mutation-tested): the agent-regate-pr sweep's OWN reReviewStoredPullRequest resync calls + // primeDurablePrStateCache, which would otherwise overwrite the seeded stale "clean" row with the live + // "dirty" value BEFORE the act-boundary disposition even runs — self-healing the cache for an unrelated + // reason and making this test pass even if the disposition itself were wrongly routed through the cache. + // No-op it so the seeded stale row survives untouched to the disposition boundary: the ONLY way this test + // can still observe the correct "dirty" close below is a genuine live re-fetch AT the act boundary. + const primeDurablePrStateCacheSpy = vi.spyOn(backfillModule, "primeDurablePrStateCache").mockResolvedValue(undefined); + // Precise, mutation-tested isolation of the act-boundary's OWN routing (not just the end-to-end outcome, + // which other code in this same job can also produce correctly via a different path): spy (pass-through, + // real implementation still runs) on the raw vs. durable-cache-backed merge-state fetchers directly. + const rawMergeStateSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState"); + const cachedMergeStateSpy = vi.spyOn(backfillModule, "cachedFetchLivePullRequestMergeState"); + let barePullGets = 0; + let closedViaPatch = false; + 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.endsWith("/pulls/8") && method === "PATCH") { + closedViaPatch = JSON.parse(String(init?.body ?? "{}")).state === "closed"; + return Response.json({ number: 8, state: "closed" }); + } + if (/\/pulls\/8(?:\?|$)/.test(url) && method === "GET") { + barePullGets += 1; + // ALWAYS live-dirty — the stale cache above claims "clean". Every bare-pull read here must reflect this. + return Response.json({ number: 8, title: "Conflicted PR", state: "open", user: { login: "contributor" }, head: { sha: "b8" }, mergeable_state: "dirty", labels: [], body: "Closes #1" }); + } + if (url.includes("/pulls/8/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/b8/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/b8/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("/issues/8/comments")) return Response.json([], { status: 200 }); + 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: "act-boundary-4220-regression", repoFullName: "owner/agent-repo", prNumber: 8, installationId: 9001 }); + + // The disposition must have observed the LIVE dirty state (multiple bare-pull reads, none short-circuited + // by the stale "clean" durable cache) and closed the PR — never merged/left-open on the stale clean value. + expect(barePullGets).toBeGreaterThan(0); + expect(closedViaPatch).toBe(true); + const closeAudit = await env.DB.prepare("select detail from audit_events where event_type = 'agent.action.close'").first<{ detail: string }>(); + expect(closeAudit?.detail).toContain("conflicts with the base branch"); + const mergeAudit = await env.DB.prepare("select count(*) as n from audit_events where event_type = 'agent.action.merge'").first<{ n: number }>(); + expect(mergeAudit?.n).toBe(0); + // The seeded stale row was NEVER overwritten before the disposition ran (primeDurablePrStateCache no-op'd + // above) — so the correct close decision above genuinely proves the act-boundary bypassed the stale cache. + expect(primeDurablePrStateCacheSpy).toHaveBeenCalled(); + // The act-boundary's own merge-state read went through the RAW (uncached) fetcher, never the durable-cache + // wrapper -- mutation-tested: swapping refreshLiveMergeState's call to cachedFetchLivePullRequestMergeState + // flips these two assertions and does NOT otherwise fail this test, proving they genuinely isolate the + // routing this test claims to guard (unlike asserting on barePullGets/closedViaPatch alone, which stayed + // green under that mutation because the resync's own live prefetch already primed the request-local memo). + expect(rawMergeStateSpy).toHaveBeenCalled(); + expect(cachedMergeStateSpy).not.toHaveBeenCalled(); + }); + + it("#4220 regression (unified comment): the public comment's merge-state label reflects the LIVE dirty value, not the stale cached clean value", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload({ "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"), + ); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "detected_contributors_only", + publicAudienceMode: "gittensor_only", + publicSignalLevel: "standard", + publicSurface: "comment_and_label", + autoLabelEnabled: false, + checkRunMode: "off", + checkRunDetailLevel: "minimal", + gateCheckMode: "enabled", + backfillEnabled: true, + privateTrustEnabled: true, + }); + await seedWarmPrStateCache(env, "JSONbored/gittensory", 9); + // Isolation (mutation-tested): a `synchronize` webhook UNCONDITIONALLY calls invalidatePrStateCache before + // the comment-rendering boundary below runs, which would otherwise clear the seeded stale "clean" row for + // an unrelated reason (webhook-driven invalidation, not the act-boundary's own force-refetch) and make this + // test pass even if the comment's merge-state read were wrongly routed through the cache. No-op it so the + // seeded stale row survives untouched: the ONLY way this test can still observe "dirty" in the comment below + // is a genuine live re-fetch AT the comment's own act-boundary-adjacent read. + const invalidatePrStateCacheSpy = vi.spyOn(backfillModule, "invalidatePrStateCache").mockResolvedValue(undefined); + const rawMergeStateSpy = vi.spyOn(backfillModule, "fetchLivePullRequestMergeState"); + const cachedMergeStateSpy = vi.spyOn(backfillModule, "cachedFetchLivePullRequestMergeState"); + let postedBody = ""; + const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({ + ciState: "passed", + hasPending: false, + hasVisiblePending: false, + failingDetails: [], + nonRequiredFailingDetails: [], + ciCompletenessWarning: null, + }); + 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("/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" }); + if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 }); + if (url.includes("/check-runs/901") && method === "PATCH") return Response.json({ id: 901 }); + if (url.includes("/issues/9/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/9/comments") && method === "POST") { + postedBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? ""); + return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 }); + } + return new Response("not found", { status: 404 }); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "act-boundary-4220-comment-regression", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { + id: 123, + account: { login: "JSONbored", id: 1, type: "User" }, + repository_selection: "selected", + permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" }, + events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"], + }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { + number: 9, + title: "Conflicted PR", + state: "open", + user: { login: "contributor" }, + head: { sha: "a9" }, + labels: [], + body: "Closes #1", + }, + }, + }); + + liveCiSpy.mockRestore(); + // The rendered comment must reflect the LIVE dirty merge-state, not the stale cached "clean" value — a + // "safe to merge" chip here while the disposition later closes the PR is the exact #4220 contradiction. + expect(postedBody).not.toMatch(/safe to merge/i); + // The seeded stale row was NEVER cleared by the webhook's own invalidation (no-op'd above), so the "dirty" + // label above can only have come from the comment-render boundary's OWN routing -- confirm directly (same + // mutation-tested pattern as the disposition test above). + expect(invalidatePrStateCacheSpy).toHaveBeenCalled(); + expect(rawMergeStateSpy).toHaveBeenCalled(); + expect(cachedMergeStateSpy).not.toHaveBeenCalled(); + }); + }); }); function completeSegment(repoFullName: string, segment: "labels" | "open_issues" | "open_pull_requests") { From b3157d2e8efc9a21df171c78eef63cf583c73fa1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 13:29:12 -0700 Subject: [PATCH 2/7] fix(db): renumber migration 0093 to 0094 to resolve a collision with #2616 --- ...nc_pr_state.sql => 0094_pull_request_detail_sync_pr_state.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename migrations/{0093_pull_request_detail_sync_pr_state.sql => 0094_pull_request_detail_sync_pr_state.sql} (100%) diff --git a/migrations/0093_pull_request_detail_sync_pr_state.sql b/migrations/0094_pull_request_detail_sync_pr_state.sql similarity index 100% rename from migrations/0093_pull_request_detail_sync_pr_state.sql rename to migrations/0094_pull_request_detail_sync_pr_state.sql From 229be2e386a268a75d6fc1294c4a3432cde53fd4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:02:28 -0700 Subject: [PATCH 3/7] 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. --- src/github/backfill.ts | 56 +++++++++++++++---- test/unit/pr-detail-durable-cache.test.ts | 67 ++++++++++++++++++++++- test/unit/queue.test.ts | 12 ++-- 3 files changed, 115 insertions(+), 20 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 3ad643b161..87ef3dd5ab 100644 --- a/src/github/backfill.ts +++ b/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 { + 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 diff --git a/test/unit/pr-detail-durable-cache.test.ts b/test/unit/pr-detail-durable-cache.test.ts index 0d5407c0d8..ddd2006687 100644 --- a/test/unit/pr-detail-durable-cache.test.ts +++ b/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") }); }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 0fd78ece4f..cfe8fd7a0d 100644 --- a/test/unit/queue.test.ts +++ b/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" }); From af9ba46e829bf8adfc4adfffaabc8601ac33734c Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:25:25 -0700 Subject: [PATCH 4/7] fix(db): repair legacy reviews_synced_at markers stamped on failed syncs A second AI review pass flagged a real backward-compatibility defect in the same PR: every pre-existing writer of reviews_synced_at (backfillOpenPullRequestDetails / refreshPullRequestDetails / backfillRepository) stamped it unconditionally on every sync pass, success or failure -- confirmed by diffing against origin/main, where `reviewsSyncedAt: syncedAt` is written with no success gate at all. This PR's new durable review cache (fetchAndStorePullRequestDetails's reviewsUpToDate check) treats ANY non-null reviews_synced_at as "fully synced, never refetch" until a pull_request_review webhook invalidates it. Existing rows whose review sync previously failed (rate limit, transient error) already carry a timestamp from that failed attempt, so the new cache would trust incomplete/stale review data indefinitely for any PR that doesn't happen to receive a new review action after this deploys. Added migration 0095: a one-time UPDATE clearing reviews_synced_at on every existing row. There's no reliable way to tell a trustworthy stamp from an untrustworthy one from the row alone (status reflects the aggregate files/reviews/checks outcome for a pass, not reviews specifically), so it clears unconditionally rather than guessing -- the cost is one redundant review refetch per already-correctly-synced PR, which is cheap and bounded. Pinned the migration's exact behavior with a dedicated regression test (mirroring the existing digest-subscription-normalization.test.ts pattern: apply the real migration files against a raw in-memory SQLite DB, seed legacy-shaped rows, assert only reviews_synced_at clears and everything else is untouched). Rebased again to keep pace with main. --- migrations/0095_repair_reviews_synced_at.sql | 26 ++++ ...tail-sync-reviews-repair-migration.test.ts | 142 ++++++++++++++++++ 2 files changed, 168 insertions(+) create mode 100644 migrations/0095_repair_reviews_synced_at.sql create mode 100644 test/unit/pr-detail-sync-reviews-repair-migration.test.ts diff --git a/migrations/0095_repair_reviews_synced_at.sql b/migrations/0095_repair_reviews_synced_at.sql new file mode 100644 index 0000000000..75caedead0 --- /dev/null +++ b/migrations/0095_repair_reviews_synced_at.sql @@ -0,0 +1,26 @@ +-- One-off repair for pull_request_detail_sync_state.reviews_synced_at (#2537 review fix). +-- +-- ROOT CAUSE (already fixed in code, this same PR): every pre-existing writer of reviews_synced_at +-- (backfillOpenPullRequestDetails / refreshPullRequestDetails / backfillRepository, all in +-- src/github/backfill.ts) stamped it UNCONDITIONALLY on every sync pass, success or failure -- even a pass that +-- ended with `status: 'partial'` due to a review-fetch error still wrote a fresh reviews_synced_at timestamp, as +-- if the reviews had actually been captured. The column now gates a durable, head-independent read-through +-- cache (fetchAndStorePullRequestDetails' reviewsUpToDate check): ANY non-null reviews_synced_at is trusted as +-- "reviews are fully synced, skip refetching" until a `pull_request_review` webhook invalidates it. +-- +-- Impact of leaving existing rows as-is: a PR whose review sync previously failed (rate limit, transient +-- network error, etc.) already has a reviews_synced_at timestamp from that failed attempt. The new cache would +-- treat that PR's reviews as permanently up to date -- silently serving incomplete/stale review data forever, +-- unless that specific PR happens to receive a NEW review action after this deploys (the only thing that clears +-- the marker going forward). +-- +-- Repair strategy: there is no reliable way to tell, from the stored row alone, whether a PARTICULAR existing +-- reviews_synced_at value came from a pass where reviews specifically succeeded (status reflects the aggregate +-- outcome across files/reviews/checks together, not reviews alone). Clear it for every row instead of trying to +-- guess -- the one-time cost is a single redundant review refetch for PRs that were already correctly synced, +-- which is cheap and bounded; the alternative (leaving any ambiguous row as-is) risks silently trusting bad data +-- indefinitely. The next sync pass for every tracked PR re-populates it correctly under the new, +-- only-stamp-on-success semantics. +UPDATE pull_request_detail_sync_state + SET reviews_synced_at = NULL + WHERE reviews_synced_at IS NOT NULL; diff --git a/test/unit/pr-detail-sync-reviews-repair-migration.test.ts b/test/unit/pr-detail-sync-reviews-repair-migration.test.ts new file mode 100644 index 0000000000..9fb4119399 --- /dev/null +++ b/test/unit/pr-detail-sync-reviews-repair-migration.test.ts @@ -0,0 +1,142 @@ +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { DatabaseSync } from "node:sqlite"; +import { describe, expect, it } from "vitest"; + +// Regression coverage for migrations/0095, the one-time repair for pull_request_detail_sync_state.reviews_synced_at +// (#2595 review fix). Every pre-existing writer (backfillOpenPullRequestDetails / refreshPullRequestDetails / +// backfillRepository, all in src/github/backfill.ts) stamped this column UNCONDITIONALLY on every sync pass, +// success or failure -- so an existing row's marker cannot be trusted once the new durable review cache starts +// treating ANY non-null value as "reviews are fully synced, never refetch." These tests pin the repair +// migration's exact behavior against the REAL migration file so a future edit can't silently narrow or widen +// what gets cleared. +const migrationsDir = join(dirname(fileURLToPath(import.meta.url)), "..", "..", "migrations"); +const migrationSql = (name: string) => readFileSync(join(migrationsDir, name), "utf8"); + +type Row = { + id: string; + status: string; + files_synced_at: string | null; + reviews_synced_at: string | null; + checks_synced_at: string | null; + head_sha: string | null; +}; + +function freshDb(): DatabaseSync { + const db = new DatabaseSync(":memory:"); + db.exec(migrationSql("0006_open_data_completeness.sql")); + db.exec(migrationSql("0092_pull_request_detail_sync_head_sha.sql")); + db.exec(migrationSql("0094_pull_request_detail_sync_pr_state.sql")); + return db; +} + +function insert( + db: DatabaseSync, + row: { + id: string; + repo_full_name: string; + pull_number: number; + status?: string; + files_synced_at?: string | null; + reviews_synced_at?: string | null; + checks_synced_at?: string | null; + head_sha?: string | null; + }, +): void { + db.prepare( + "INSERT INTO pull_request_detail_sync_state (id, repo_full_name, pull_number, status, files_synced_at, reviews_synced_at, checks_synced_at, head_sha) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + ).run( + row.id, + row.repo_full_name, + row.pull_number, + row.status ?? "never_synced", + row.files_synced_at ?? null, + row.reviews_synced_at ?? null, + row.checks_synced_at ?? null, + row.head_sha ?? null, + ); +} + +function allRows(db: DatabaseSync): Row[] { + return db + .prepare("SELECT id, status, files_synced_at, reviews_synced_at, checks_synced_at, head_sha FROM pull_request_detail_sync_state ORDER BY id") + .all() as unknown as Row[]; +} + +describe("0095 reviews_synced_at repair migration", () => { + it("clears reviews_synced_at on every row that has it set, regardless of status, while leaving OTHER columns untouched", () => { + const db = freshDb(); + // A "complete" row (reviews genuinely succeeded on the pass that set this) -- still cleared: there is no + // reliable way to tell a trustworthy stamp from an untrustworthy one from the row alone (status reflects the + // aggregate files/reviews/checks outcome, not reviews specifically), so the migration clears unconditionally + // by design rather than guessing. + insert(db, { + id: "p1", + repo_full_name: "o/r", + pull_number: 1, + status: "complete", + files_synced_at: "2026-01-01T00:00:00Z", + reviews_synced_at: "2026-01-01T00:00:00Z", + checks_synced_at: "2026-01-01T00:00:00Z", + head_sha: "sha1", + }); + // A "partial" row -- the exact scenario the bug is about: reviews_synced_at was stamped even though this + // pass (files, reviews, or checks) had a failure. + insert(db, { + id: "p2", + repo_full_name: "o/r", + pull_number: 2, + status: "partial", + files_synced_at: "2026-01-02T00:00:00Z", + reviews_synced_at: "2026-01-02T00:00:00Z", + checks_synced_at: null, + head_sha: "sha2", + }); + // A row that never had reviews synced at all -- must remain untouched (already NULL, nothing to clear). + insert(db, { id: "p3", repo_full_name: "o/r", pull_number: 3, status: "never_synced" }); + + db.exec(migrationSql("0095_repair_reviews_synced_at.sql")); + + const rows = allRows(db); + expect(rows.find((r) => r.id === "p1")).toMatchObject({ + reviews_synced_at: null, + files_synced_at: "2026-01-01T00:00:00Z", + checks_synced_at: "2026-01-01T00:00:00Z", + head_sha: "sha1", + status: "complete", + }); + expect(rows.find((r) => r.id === "p2")).toMatchObject({ + reviews_synced_at: null, + files_synced_at: "2026-01-02T00:00:00Z", + checks_synced_at: null, + head_sha: "sha2", + status: "partial", + }); + expect(rows.find((r) => r.id === "p3")).toMatchObject({ + reviews_synced_at: null, + files_synced_at: null, + checks_synced_at: null, + head_sha: null, + status: "never_synced", + }); + }); + + it("is a no-op on a table with no rows", () => { + const db = freshDb(); + + expect(() => db.exec(migrationSql("0095_repair_reviews_synced_at.sql"))).not.toThrow(); + + expect(allRows(db)).toEqual([]); + }); + + it("is idempotent — running it twice has the same effect as running it once", () => { + const db = freshDb(); + insert(db, { id: "p1", repo_full_name: "o/r", pull_number: 1, status: "complete", reviews_synced_at: "2026-01-01T00:00:00Z" }); + + db.exec(migrationSql("0095_repair_reviews_synced_at.sql")); + + expect(() => db.exec(migrationSql("0095_repair_reviews_synced_at.sql"))).not.toThrow(); + expect(allRows(db).find((r) => r.id === "p1")?.reviews_synced_at).toBeNull(); + }); +}); From 2098f7ba02e47d9e0e822e2daa81b5f13fa63b4b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 14:45:53 -0700 Subject: [PATCH 5/7] fix(github): invalidate the durable review cache on synchronize too Addresses the blocker the gate's own AI review flagged: this branch's new durable review cache (reviewsSyncedAt) was only invalidated on a pull_request_review webhook (submitted/edited/dismissed). But when a repo's branch protection has "dismiss stale pull request approvals" enabled, GitHub also dismisses existing approvals server-side on a new push (a pull_request synchronize event) -- exactly the same moment the PR-state cache already treats as a forced live miss for mergeable_state. The review cache had no equivalent signal for that path, so a repo with that protection rule could serve a stale "reviews complete" read indefinitely after a push silently dismissed the approval it was based on. Extends the existing synchronize/closed/reopened PR-state invalidation block to also invalidate the review cache, but only for synchronize (GitHub doesn't dismiss reviews on close or reopen). Updated the now-inaccurate comment on the pull_request_review-only invalidation site accordingly, and added a regression test mirroring the existing per-action review-invalidation table test, mutation-verified to fail without the fix. --- src/queue/processors.ts | 19 ++++++++++++++----- test/unit/queue.test.ts | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index f6bd79af53..ce3434ce9b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -4251,6 +4251,15 @@ async function processGitHubWebhook( // to avoid spurious cache churn / extra writes on high-frequency low-signal actions. if (eventName === "pull_request" && (payload.action === "synchronize" || payload.action === "closed" || payload.action === "reopened")) { await invalidatePrStateCache(env, repoFullName, pr.number).catch(() => undefined); + if (payload.action === "synchronize") { + // A new push can dismiss existing approvals server-side when the repo's branch protection has + // "dismiss stale pull request approvals" enabled -- GitHub recomputes review state on synchronize + // the same way it recomputes mergeable_state, but the durable review cache (reviewsSyncedAt, + // invalidated below only for actual pull_request_review actions) has no independent signal for + // this. Treat every synchronize as a review-cache miss too rather than trusting stale approvals + // indefinitely (#2537). + await invalidatePrReviewsCache(env, repoFullName, pr.number).catch(() => undefined); + } } // Reopen-prevention (#one-shot-reopen): a CONTRIBUTOR may not reopen a PR that gittensory or a maintainer // closed — closes are one-shot (resubmit, don't reopen). If a non-maintainer reopened a PR whose last close @@ -4349,11 +4358,11 @@ async function processGitHubWebhook( installationId && shouldProcessPullRequestPublicSurface(eventName, payload.action) ) { - // #2537: reviews only actually change on a pull_request_review webhook (submitted/edited/dismissed) — not - // on every sweep tick / unrelated pull_request action — so invalidate the durable review cache HERE, - // before the refreshPullRequestDetails call below reads it, rather than on head-SHA change (reviews are - // independent of head). shouldProcessPullRequestPublicSurface already gates pull_request_review to - // exactly these 3 actions. + // #2537: reviews change on a pull_request_review webhook (submitted/edited/dismissed) -- not on every + // sweep tick / unrelated pull_request action -- so invalidate the durable review cache HERE, before the + // refreshPullRequestDetails call below reads it. (synchronize is handled separately above, since GitHub + // can also dismiss approvals server-side on a new push.) shouldProcessPullRequestPublicSurface already + // gates pull_request_review to exactly these 3 actions. if (eventName === "pull_request_review") { await invalidatePrReviewsCache(env, repoFullName, pr.number).catch(() => undefined); } diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index cfe8fd7a0d..a3c8cf15f8 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -13398,6 +13398,47 @@ describe("queue processors", () => { }, ); + it("pull_request synchronize action ALSO invalidates the durable review cache (branch protection can dismiss approvals on a new push, #2537)", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { repoFullName: "JSONbored/gittensory", gateCheckMode: "off", checkRunMode: "off", commentMode: "off", publicSurface: "off" }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 204, + status: "complete", + prMergeableState: "clean", + prState: "open", + prStateFetchedAt: new Date().toISOString(), + reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/pulls/204/files")) return Response.json([]); + if (url.includes("/pulls/204/reviews")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "invalidate-reviews-on-synchronize", + eventName: "pull_request", + payload: { + action: "synchronize", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 204, title: "PR", state: "open", user: { login: "contributor" }, head: { sha: "a204" }, labels: [], body: "" }, + }, + }); + + // Same reasoning as the pull_request_review case above: a subsequent refreshPullRequestDetails MAY + // re-stamp reviewsSyncedAt within this same pass — assert the invalidating write happened by checking + // the stale seeded value is gone, not that the field is null. + const state = await getPullRequestDetailSyncState(env, "JSONbored/gittensory", 204); + expect(state?.reviewsSyncedAt).not.toBe("2026-05-20T00:00:00.000Z"); + }); + it("a pull_request_review_comment event (sibling of pull_request_review) does NOT invalidate the durable review cache", async () => { const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "all", permissions: {}, events: [] } }); From 5ff451ecd0d414c130596d1fa6edd4c4676d59d4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:00:54 -0700 Subject: [PATCH 6/7] fix(github): bound the review cache's freshness to a max age Addresses the blocker the gate's own AI review flagged: fetchAndStorePullRequestDetails's reviewsUpToDate check trusted any non-null reviewsSyncedAt as permanently fresh. invalidatePrReviewsCache is called best-effort from the webhook handler, so a dropped invalidation write (submitted/edited/dismissed review, or now synchronize) left stale review rows in use indefinitely -- unlike the files cache, reviews have no head-SHA comparison to fall back on either. Adds PR_REVIEWS_CACHE_MAX_AGE_MS (24h) and isPrReviewsCacheFresh, mirroring the existing PR_STATE_CACHE_MAX_AGE_MS/isPrStateCacheFresh pattern for the bare PR-state cache, but with a much longer window since reviews don't need minute-level freshness -- just an eventual-consistency bound on a missed invalidation. --- src/github/backfill.ts | 40 +++++++-- .../backfill-file-hydration-scoping.test.ts | 87 ++++++++++++++++++- 2 files changed, 117 insertions(+), 10 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 87ef3dd5ab..07951249a1 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -333,6 +333,14 @@ const PR_REVIEWS_CACHE_METRIC = "gittensory_pr_reviews_cache_total"; // Safety-net max age for a webhook-invalidated PR-state cache row (a dropped/missed webhook must not pin a stale // value forever). Short enough that a missed synchronize/closed/reopened event self-heals within one sweep tick. const PR_STATE_CACHE_MAX_AGE_MS = 5 * 60 * 1000; +// Safety-net max age for the review cache's reviewsSyncedAt marker (flagged by the gate's own review of #2537): +// unlike the PR-state cache above, review invalidation (invalidatePrReviewsCache, processors.ts) is called +// best-effort on a pull_request_review webhook (and now, separately, on a synchronize event) -- a failed +// invalidation write leaves the marker in place indefinitely with no other fallback, since reviews have no +// head-SHA comparison to fall back on either. Much longer than the PR-state TTL (reviews don't need +// minute-level freshness, just an eventual-consistency bound on the worst case of a transient DB write +// failure), but still bounded rather than permanent. +const PR_REVIEWS_CACHE_MAX_AGE_MS = 24 * 60 * 60 * 1000; const CURRENT_OPEN_SCAN_MARKER = "gittensory-current-open-scan-v1"; const FRESH_TOTALS_SNAPSHOT_MS = 10 * 60 * 1000; const TOTALS_SNAPSHOT_LOOKBACK = 8; @@ -2003,11 +2011,13 @@ async function fetchAndStorePullRequestDetails( const existingState = !options.forceFiles && pr.headSha ? await getPullRequestDetailSyncState(env, repoFullName, pr.number) : null; const filesUpToDate = Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt); // Durable review cache (#2537): reviews are independent of head SHA (a force-push doesn't change who - // approved/requested-changes), so — unlike files — this is gated purely on reviewsSyncedAt being set, not a - // head comparison. reviewsSyncedAt is explicitly cleared (set to null, PARTIAL-UPDATE CONTRACT) by the - // pull_request_review webhook handler (processors.ts) on submitted/edited/dismissed, which is the ONLY thing - // that invalidates it — a head-SHA change alone does NOT invalidate reviews. - const reviewsUpToDate = !options.forceFiles && Boolean(existingState?.reviewsSyncedAt); + // approved/requested-changes), so — unlike files — this is gated on reviewsSyncedAt being set AND still + // within PR_REVIEWS_CACHE_MAX_AGE_MS, not a head comparison. reviewsSyncedAt is explicitly cleared (set to + // null, PARTIAL-UPDATE CONTRACT) by the webhook handler (processors.ts) on a pull_request_review + // submitted/edited/dismissed action or a pull_request synchronize action — but that invalidation write is + // best-effort, so isPrReviewsCacheFresh is the safety net for a dropped one (a head-SHA change ALONE does not + // invalidate reviews; only that explicit clear, or this TTL expiring, does). + const reviewsUpToDate = !options.forceFiles && isPrReviewsCacheFresh(existingState?.reviewsSyncedAt); incr(PR_REVIEWS_CACHE_METRIC, { result: reviewsUpToDate ? "hit" : "miss" }); const warningStart = warnings.length; const [files, reviews, checks] = await Promise.all([ @@ -2755,10 +2765,11 @@ export async function fetchLivePullRequest( // #2537: durable, webhook-invalidated cache for the bare PR-state read (GET /pulls/{n}). Unlike the request-local // LiveGithubFacts memo (queue/processors.ts), this survives ACROSS webhook deliveries / sweep ticks, cutting -// repeat /pulls/{n} calls for an unchanged PR at the freshness-guard/readiness/dup-winner/gate-override call -// sites. NEVER used by the act-boundary merge/close decision (planAgentMaintenanceActions's liveMergeState read, -// or its unified-comment mirror) — those force-refetch via refreshLiveMergeState by design (the #4220 fix) and -// must keep doing so; this cache exists purely for the OTHER, non-authoritative reads. +// repeat /pulls/{n} calls for an unchanged PR at the freshness-guard/readiness/dup-winner call sites. NEVER used +// by the act-boundary merge/close decision (planAgentMaintenanceActions's liveMergeState read, or its +// unified-comment mirror) or by resolveOverrideHeadSha (gate-override, queue/processors.ts) — those force- +// refetch by design (the #4220 fix; the gate-override race respectively) and must keep doing so; this cache +// exists purely for the OTHER, non-authoritative reads. function isPrStateCacheFresh(fetchedAt: string | null | undefined): boolean { if (!fetchedAt) return false; const fetchedAtMs = Date.parse(fetchedAt); @@ -2766,6 +2777,17 @@ function isPrStateCacheFresh(fetchedAt: string | null | undefined): boolean { return Date.now() - fetchedAtMs < PR_STATE_CACHE_MAX_AGE_MS; } +// #2537 follow-up (flagged by the gate's own review): a failed invalidatePrReviewsCache write (processors.ts) +// leaves reviewsSyncedAt stamped forever with nothing else to catch it -- reviews have no head-SHA comparison +// to fall back on the way files do. Bound reviewsUpToDate's trust in an old marker to PR_REVIEWS_CACHE_MAX_AGE_MS +// so a missed invalidation self-heals eventually instead of permanently pinning a stale review set. +function isPrReviewsCacheFresh(reviewsSyncedAt: string | null | undefined): boolean { + if (!reviewsSyncedAt) return false; + const syncedAtMs = Date.parse(reviewsSyncedAt); + if (!Number.isFinite(syncedAtMs)) return false; + return Date.now() - syncedAtMs < PR_REVIEWS_CACHE_MAX_AGE_MS; +} + /** Best-effort write-through for the PR-state cache fields. Always stamps prStateFetchedAt = now on a successful * live read (even when the live value itself is undefined/null — a confirmed-empty read is still a fresh read, * distinct from "never fetched"), so a run of undefined reads doesn't force every caller back to GitHub. Preserves diff --git a/test/unit/backfill-file-hydration-scoping.test.ts b/test/unit/backfill-file-hydration-scoping.test.ts index c626afd490..7b2648e32a 100644 --- a/test/unit/backfill-file-hydration-scoping.test.ts +++ b/test/unit/backfill-file-hydration-scoping.test.ts @@ -475,7 +475,7 @@ describe("GitHub PR file hydration scoping (#audit-rate-headroom)", () => { status: "complete", headSha: "head-101-old", filesSyncedAt: "2026-05-20T00:00:00.000Z", - reviewsSyncedAt: "2026-05-20T00:00:00.000Z", + reviewsSyncedAt: new Date().toISOString(), }); const urls = stubFetchTracking((url) => { if (url.includes("/pulls/101/reviews")) return new Response("must not be called", { status: 500 }); @@ -492,6 +492,91 @@ describe("GitHub PR file hydration scoping (#audit-rate-headroom)", () => { expect(urls.some((url) => url.includes("/pulls/101/files"))).toBe(true); }); + it("REGRESSION (gate-flagged follow-up): a reviewsSyncedAt older than PR_REVIEWS_CACHE_MAX_AGE_MS is treated as a cache miss, so a dropped invalidation webhook still self-heals", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 110, + title: "Open PR, stale review marker", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-110" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 110, + status: "complete", + headSha: "head-110", + filesSyncedAt: new Date().toISOString(), + reviewsSyncedAt: new Date(Date.now() - 25 * 60 * 60 * 1000).toISOString(), + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/110/reviews") ? Response.json([{ id: 4, user: { login: "reviewer4" }, state: "APPROVED", submitted_at: "2026-06-01T00:00:00.000Z" }]) : Response.json([]), + ); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 110); + + expect(urls.some((url) => url.includes("/pulls/110/reviews"))).toBe(true); + }); + + it("a reviewsSyncedAt just within PR_REVIEWS_CACHE_MAX_AGE_MS is still treated as a cache hit", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + await seedRegisteredRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 111, + title: "Open PR, review marker just inside the TTL", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-111" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 111, + status: "complete", + headSha: "head-111", + filesSyncedAt: new Date().toISOString(), + reviewsSyncedAt: new Date(Date.now() - 23 * 60 * 60 * 1000).toISOString(), + }); + const urls = stubFetchTracking((url) => (url.includes("/pulls/111/reviews") ? new Response("must not be called", { status: 500 }) : Response.json([]))); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 111); + + expect(urls.some((url) => url.includes("/pulls/111/reviews"))).toBe(false); + }); + + it("an unparseable reviewsSyncedAt string 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: 112, + title: "Open PR, unparseable review marker", + state: "open", + user: { login: "oktofeesh1" }, + head: { sha: "head-112" }, + labels: [], + body: "", + }); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "JSONbored/gittensory", + pullNumber: 112, + status: "complete", + headSha: "head-112", + filesSyncedAt: new Date().toISOString(), + reviewsSyncedAt: "not-a-date", + }); + const urls = stubFetchTracking((url) => + url.includes("/pulls/112/reviews") ? Response.json([{ id: 5, user: { login: "reviewer5" }, state: "APPROVED", submitted_at: "2026-06-01T00:00:00.000Z" }]) : Response.json([]), + ); + + await refreshPullRequestDetails(env, "JSONbored/gittensory", 112); + + expect(urls.some((url) => url.includes("/pulls/112/reviews"))).toBe(true); + }); + it("fetches fresh reviews when reviewsSyncedAt is null (invalidated)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); await seedRegisteredRepo(env); From dfbbbbb98b9144a3b6d8b1723d22cc6dfb24301f Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 15:00:54 -0700 Subject: [PATCH 7/7] fix(github): keep gate-override on a live head-SHA fetch, not the durable cache Addresses the other blocker the gate's own AI review flagged: resolveOverrideHeadSha (the security-sensitive, human-triggered @gittensory gate-override command) had been switched to cachedFetchLivePullRequestHeadSha, contradicting its own docstring ("re-fetch the LIVE head"). The whole point of this function is closing the race where a new commit lands between the override comment and its processing -- a cache hit within the durable cache's freshness window reintroduces exactly that race. Reverts the call site to the raw fetchLivePullRequestHeadSha (same as before #2537 touched it), matching the sibling PR's explicit reasoning for excluding this call site from caching, and updates the now-inaccurate comments on both resolveOverrideHeadSha and the durable PR-state cache's own doc comment. --- src/queue/processors.ts | 10 ++++++---- test/unit/resolve-override-head-sha.test.ts | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ce3434ce9b..6b7fd6fbe9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -75,7 +75,6 @@ import { backfillOpenPullRequestDetails, backfillRegisteredRepositories, backfillRepositorySegment, - cachedFetchLivePullRequestHeadSha, cachedFetchLivePullRequestMergeState, cachedFetchLivePullRequestState, enqueueRepositoryOpenDataBackfill, @@ -7436,8 +7435,11 @@ async function recordGithubProductUsage( * 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. - * #2537: durable-cached — a separate `issue_comment` webhook handler with no request-local `liveFacts`, so the - * cross-webhook cache is a pure win here (not the act-boundary merge/close decision). + * #2537: deliberately NOT routed through the durable head-SHA cache (flagged by the gate's own review) — this + * is the same class of security-sensitive, human-triggered re-check as the act-boundary merge/close decision, + * wanting the literal current commit rather than a value that can be up to PR_STATE_CACHE_MAX_AGE_MS stale. A + * commit landing inside that freshness window right after the override comment is exactly the race this + * function exists to close; a cache hit would silently reintroduce it. */ export async function resolveOverrideHeadSha( env: Env, @@ -7450,7 +7452,7 @@ export async function resolveOverrideHeadSha( () => undefined, )) ?? env.GITHUB_PUBLIC_TOKEN; const admissionKey = githubAdmissionKeyForToken(env, installationId, token); - const liveHeadSha = await cachedFetchLivePullRequestHeadSha( + const liveHeadSha = await fetchLivePullRequestHeadSha( env, repoFullName, pr.number, diff --git a/test/unit/resolve-override-head-sha.test.ts b/test/unit/resolve-override-head-sha.test.ts index 6ed575aecc..9fc69bc23b 100644 --- a/test/unit/resolve-override-head-sha.test.ts +++ b/test/unit/resolve-override-head-sha.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { resolveOverrideHeadSha } from "../../src/queue/processors"; import { createInstallationToken } from "../../src/github/app"; +import { upsertPullRequestDetailSyncState } from "../../src/db/repositories"; import type { PullRequestRecord } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; @@ -59,4 +60,18 @@ describe("resolveOverrideHeadSha (#16 / gate-override stale head)", () => { stubLiveHead(null); expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("stale-sha"); }); + + it("REGRESSION (#2537, gate-flagged): a FRESH durable PR-state cache row for this PR must NOT short-circuit the live fetch — a commit landing inside the cache's freshness window right after the override comment is exactly the race this function exists to close, so it must always hit GitHub directly rather than trust a recent-but-possibly-already-stale cached headSha", async () => { + const env = createTestEnv(); + mockedToken.mockResolvedValue("inst-tok"); + await upsertPullRequestDetailSyncState(env, { + repoFullName: "owner/repo", + pullNumber: 90, + status: "complete", + headSha: "cached-sha", + prStateFetchedAt: new Date().toISOString(), + }); + stubLiveHead("brand-new-live-sha"); + expect(await resolveOverrideHeadSha(env, 123, "owner/repo", makePr("stale-sha"))).toBe("brand-new-live-sha"); + }); });