diff --git a/migrations/0094_pull_request_detail_sync_pr_state.sql b/migrations/0094_pull_request_detail_sync_pr_state.sql new file mode 100644 index 0000000000..342aefe6e4 --- /dev/null +++ b/migrations/0094_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/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/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..07951249a1 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -326,6 +326,21 @@ 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; +// 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; @@ -616,13 +631,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 +713,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 +1837,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 +2007,26 @@ 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 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([ 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 +2044,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 +2763,206 @@ 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 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); + if (!Number.isFinite(fetchedAtMs)) return false; + 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 + * 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); +} + +/** + * 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 + * 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 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, + 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 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/shared-fetch 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 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 + * 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 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 + * (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..6b7fd6fbe9 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -75,6 +75,8 @@ import { backfillOpenPullRequestDetails, backfillRegisteredRepositories, backfillRepositorySegment, + cachedFetchLivePullRequestMergeState, + cachedFetchLivePullRequestState, enqueueRepositoryOpenDataBackfill, fetchAndStorePullRequestFilesForReview, fetchLinkedIssueFacts, @@ -90,6 +92,9 @@ import { fetchLivePullRequestState, fetchOpenPullRequestNumbersForCommit, fetchRequiredStatusContexts, + invalidatePrReviewsCache, + invalidatePrStateCache, + primeDurablePrStateCache, refreshContributorActivity, refreshInstallationHealth, refreshPullRequestDetails, @@ -614,15 +619,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 +2202,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 +4243,23 @@ 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); + 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 // was by the bot / repo owner / admin, re-close it and skip the re-review. Self-closes (the contributor @@ -4322,6 +4357,14 @@ async function processGitHubWebhook( installationId && shouldProcessPullRequestPublicSurface(eventName, payload.action) ) { + // #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); + } if ( shouldCollectSlopEvidence(settings) || settings.manifestPolicyGateMode !== "off" || @@ -5615,7 +5658,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 +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: 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, 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..7b2648e32a 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,273 @@ 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: new Date().toISOString(), + }); + 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("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); + 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..ddd2006687 --- /dev/null +++ b/test/unit/pr-detail-durable-cache.test.ts @@ -0,0 +1,454 @@ +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, + }; + } + + // 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" }); + 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); + }); + + // 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(); + 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(); + 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") + }); + }); + + 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/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(); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index c5ccf7b9d2..a3c8cf15f8 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,378 @@ 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: "fake-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: "fake-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: "fake-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("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: [] } }); + 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: "fake-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: "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" }); + } + 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: "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" }); + 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") { 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"); + }); });