feat(github): cache PR reviews with webhook-based invalidation

migrations/0094_pull_request_reviews_invalidated.sql

@@ -0,0 +1,16 @@
+-- PR reviews cache invalidation marker (#2537): bumped by a `pull_request_review` webhook
+-- (submitted/dismissed/edited) to signal the cached `pull_request_reviews` rows are stale. NULL (the
+-- default) means no invalidating event has been recorded, so a subsequent fetchAndStorePullRequestDetails
+-- pass can skip the `GET /pulls/{n}/reviews` call when reviews_synced_at already covers it.
+ALTER TABLE pull_request_detail_sync_state ADD COLUMN reviews_invalidated_at TEXT;
+
+-- One-time cleanup (gate review finding): reviews_synced_at has existed since migration 0006, long before
+-- today it gains any cache-skip meaning -- every sync pass since then has stamped it UNCONDITIONALLY,
+-- including passes whose review fetch itself failed (there was no per-segment success timestamp before this
+-- PR). A `status != 'complete'` row is exactly the set where SOME segment failed on its last sync (`status`
+-- only reads 'complete' when that pass recorded zero warnings across files/reviews/checks, which is a
+-- reliable historical guarantee reviews specifically succeeded); for every other status the failure could
+-- have been reviews, so trusting a stale reviews_synced_at there the moment this cache-skip logic goes live
+-- would silently skip re-fetching reviews that were never actually captured. Reset ONLY the ambiguous rows;
+-- a genuinely 'complete' row's reviews_synced_at is trustworthy as-is and is left untouched.
+UPDATE pull_request_detail_sync_state SET reviews_synced_at = NULL WHERE status != 'complete' AND reviews_synced_at IS NOT NULL;

src/db/repositories.ts

@@ -1152,6 +1152,7 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ
       headSha: state.headSha,
       filesSyncedAt: state.filesSyncedAt,
       reviewsSyncedAt: state.reviewsSyncedAt,
+      reviewsInvalidatedAt: state.reviewsInvalidatedAt,
       checksSyncedAt: state.checksSyncedAt,
       lastSyncedAt: state.lastSyncedAt,
       errorSummary: state.errorSummary,
@@ -1164,6 +1165,7 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ
         headSha: state.headSha,
         filesSyncedAt: state.filesSyncedAt,
         reviewsSyncedAt: state.reviewsSyncedAt,
+        reviewsInvalidatedAt: state.reviewsInvalidatedAt,
         checksSyncedAt: state.checksSyncedAt,
         lastSyncedAt: state.lastSyncedAt,
         errorSummary: state.errorSummary,
@@ -1172,6 +1174,50 @@ export async function upsertPullRequestDetailSyncState(env: Env, state: PullRequ
     });
 }
 
+/** Reviews-cache invalidation stamp (#2537): a pure single-field bump of `reviewsInvalidatedAt`, leaving
+ *  every other column (`headSha`/`filesSyncedAt`/`reviewsSyncedAt`/`checksSyncedAt`/...) untouched when the
+ *  row already exists — mirrors the narrow single-field touches on `pull_requests` (markPullRequestApproved,
+ *  markPullRequestRegated). Creates the row (all other columns default/NULL) if this repo+PR has never been
+ *  synced yet, so an early review webhook is not silently dropped.
+ *
+ *  Unlike its siblings above (advisory/reporting markers with other fallback signals), this write is the SOLE
+ *  source of the reviews-cache invalidation signal (#2537 gate finding) — a single failed attempt loses that
+ *  PR's specific "reviews changed" event permanently, with nothing to naturally re-trigger it until some LATER
+ *  invalidation happens to succeed. The caller already treats this as best-effort (never blocks the webhook),
+ *  so a short bounded retry absorbs a transient D1 blip in-process rather than needing a durable retry queue
+ *  for what is still, even after this, a best-effort write. */
+export async function markPullRequestReviewsInvalidated(env: Env, repoFullName: string, pullNumber: number): Promise<void> {
+  const db = getDb(env.DB);
+  const now = nowIso();
+  const MAX_ATTEMPTS = 3;
+  let lastError: unknown;
+  for (let attempt = 1; attempt <= MAX_ATTEMPTS; attempt += 1) {
+    try {
+      await db
+        .insert(pullRequestDetailSyncState)
+        .values({
+          id: `${repoFullName}#${pullNumber}`,
+          repoFullName,
+          pullNumber,
+          status: "never_synced",
+          reviewsInvalidatedAt: now,
+          updatedAt: now,
+        })
+        .onConflictDoUpdate({
+          target: [pullRequestDetailSyncState.repoFullName, pullRequestDetailSyncState.pullNumber],
+          set: {
+            reviewsInvalidatedAt: now,
+            updatedAt: now,
+          },
+        });
+      return;
+    } catch (error) {
+      lastError = error;
+    }
+  }
+  throw lastError;
+}
+
 export async function getPullRequestDetailSyncState(env: Env, fullName: string, pullNumber: number): Promise<PullRequestDetailSyncStateRecord | null> {
   const db = getDb(env.DB);
   const [row] = await db
@@ -4226,6 +4272,7 @@ function toPullRequestDetailSyncStateRecord(row: typeof pullRequestDetailSyncSta
     headSha: row.headSha,
     filesSyncedAt: row.filesSyncedAt,
     reviewsSyncedAt: row.reviewsSyncedAt,
+    reviewsInvalidatedAt: row.reviewsInvalidatedAt,
     checksSyncedAt: row.checksSyncedAt,
     lastSyncedAt: row.lastSyncedAt,
     errorSummary: row.errorSummary,

src/db/schema.ts

@@ -231,6 +231,12 @@ export const pullRequestDetailSyncState = sqliteTable(
     headSha: text("head_sha"),
     filesSyncedAt: text("files_synced_at"),
     reviewsSyncedAt: text("reviews_synced_at"),
+    // Bumped by a `pull_request_review` webhook (submitted/dismissed/edited) to signal the cached reviews are
+    // stale. NULL, or a value <= reviewsSyncedAt, means the last sync already covers every invalidating event,
+    // so fetchAndStorePullRequestDetails can skip the `GET /pulls/{n}/reviews` call. Reviews are independent of
+    // headSha (a new commit alone does not invalidate existing review state; only an actual review-webhook event
+    // does) -- unlike the files cache, which is why this uses its own timestamp column instead of headSha matching.
+    reviewsInvalidatedAt: text("reviews_invalidated_at"),
     checksSyncedAt: text("checks_synced_at"),
     lastSyncedAt: text("last_synced_at"),
     errorSummary: text("error_summary"),

src/github/backfill.ts

@@ -613,7 +613,7 @@ export async function backfillOpenPullRequestDetails(
   await mapWithConcurrency(batch, 2, async (pr) => {
     await upsertPullRequestDetailSyncState(env, { repoFullName: repo.fullName, pullNumber: pr.number, status: "running" });
     const before = warnings.length;
-    await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details");
+    const { reviewsSyncedAt } = await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details");
     const syncedAt = nowIso();
     const newWarnings = warnings.slice(before);
     await upsertPullRequestDetailSyncState(env, {
@@ -622,7 +622,7 @@ export async function backfillOpenPullRequestDetails(
       status: newWarnings.length > 0 ? "partial" : "complete",
       headSha: pr.headSha,
       filesSyncedAt: syncedAt,
-      reviewsSyncedAt: syncedAt,
+      reviewsSyncedAt,
       checksSyncedAt: syncedAt,
       lastSyncedAt: syncedAt,
       errorSummary: newWarnings.at(-1),
@@ -689,7 +689,7 @@ export async function refreshPullRequestDetails(
   const admissionKey = repoAdmissionKeyForToken(env, repo, token);
   const warnings: string[] = [];
   await upsertPullRequestDetailSyncState(env, { repoFullName, pullNumber, status: "running" });
-  await fetchAndStorePullRequestDetails(env, repoFullName, pr, token, warnings, admissionKey, "live_review", { forceFiles: options.force });
+  const { reviewsSyncedAt } = 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";
   await upsertPullRequestDetailSyncState(env, {
@@ -698,7 +698,7 @@ export async function refreshPullRequestDetails(
     status,
     headSha: pr.headSha,
     filesSyncedAt: syncedAt,
-    reviewsSyncedAt: syncedAt,
+    reviewsSyncedAt,
     checksSyncedAt: syncedAt,
     lastSyncedAt: syncedAt,
     errorSummary: warnings.at(-1),
@@ -1807,7 +1807,7 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back
     const detailWarningStart = warnings.length;
     await mapWithConcurrency(detailTargets, limits.detailConcurrency, async (pr) => {
       const before = warnings.length;
-      await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details");
+      const { reviewsSyncedAt } = await fetchAndStorePullRequestDetails(env, repo.fullName, pr, token, warnings, admissionKey, "backfill_open_pr_details");
       // Persist the repo+PR+headSha snapshot marker (#audit-rate-headroom) so a later call through ANY
       // cache-aware path (open-PR convergence, live review) can skip refetching this PR's files while its
       // head is unchanged — without this write, fetchAndStorePullRequestDetails's cache check always misses
@@ -1820,7 +1820,7 @@ async function backfillRepository(env: Env, repo: RepositoryRecord, limits: Back
         status: newWarnings.length > 0 ? "partial" : "complete",
         headSha: pr.headSha,
         filesSyncedAt: syncedAt,
-        reviewsSyncedAt: syncedAt,
+        reviewsSyncedAt,
         checksSyncedAt: syncedAt,
         lastSyncedAt: syncedAt,
         errorSummary: newWarnings.at(-1),
@@ -1978,20 +1978,54 @@ async function fetchAndStorePullRequestDetails(
   admissionKey: GitHubRateLimitAdmissionKey | undefined,
   caller: PullRequestFilesFetchCaller,
   options: { forceFiles?: boolean | undefined } = {},
-): Promise<void> {
+): Promise<{ reviewsSyncedAt: string | null | undefined }> {
   // 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.
-  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);
+  // only files are cached here; checks are more volatile at a fixed head and still refresh every call.
+  //
+  // The row is now fetched UNCONDITIONALLY (the original files-only cache gated this on `!options.forceFiles`,
+  // skipping the read entirely on a forced refresh) because reviews caching (#2537) reuses this SAME row and
+  // does not depend on `headSha` or `forceFiles` at all -- `forceFiles` only ever forces a FILES re-fetch (see
+  // its name and its only caller, refreshPullRequestDetails's manual "force" option), so a caller asking to
+  // force-refresh files must not ALSO force an unrelated reviews refetch (gate review finding: the previous
+  // version skipped the row entirely on `forceFiles && headSha`, which zeroed out `reviewsUpToDate` too).
+  // `forceFiles` is applied ONLY to `filesUpToDate` below, never to `reviewsUpToDate`.
+  const existingState = await getPullRequestDetailSyncState(env, repoFullName, pr.number);
+  const filesUpToDate = !options.forceFiles && Boolean(existingState?.headSha) && existingState?.headSha === pr.headSha && Boolean(existingState?.filesSyncedAt);
+  // Reviews cache (#2537): independent of headSha — a new commit alone does not invalidate existing review
+  // state, only an actual `pull_request_review` webhook (submitted/dismissed/edited) does, via
+  // markPullRequestReviewsInvalidated. Up to date when a prior sync recorded reviewsSyncedAt and either no
+  // invalidation has been recorded since, or the invalidation predates that sync. STRICTLY greater-than (not
+  // >=): millisecond-resolution ISO timestamps can tie when a sync and a racing invalidation land in the same
+  // millisecond, and sub-millisecond ordering is unknowable from the stored strings — a tie must fail toward
+  // "still needs a refetch," never toward silently trusting a possibly-stale cache.
+  const reviewsSyncedAtBefore = existingState?.reviewsSyncedAt;
+  const reviewsUpToDate =
+    Boolean(reviewsSyncedAtBefore) &&
+    (!existingState?.reviewsInvalidatedAt || (reviewsSyncedAtBefore ?? "") > existingState.reviewsInvalidatedAt);
+  // Gate review finding (TOCTOU race): `existingState` above is a snapshot read at the TOP of this call. If a
+  // `pull_request_review` webhook races in AFTER that read but BEFORE this function returns, an unconditional
+  // "stamp reviewsSyncedAt to now" on the CALLER's side (the old design) would advance the timestamp PAST that
+  // webhook's invalidation without ever having fetched the reviews it invalidated -- the cache would then
+  // permanently believe it's fresh through an event it never actually observed. Captured HERE, before the
+  // fetch even starts, so it's safe: any invalidation racing in from this instant onward still leaves
+  // `reviewsInvalidatedAt` newer than whatever we return below, forcing a correct refetch on the NEXT pass.
+  const reviewFetchStartedAt = nowIso();
   const warningStart = warnings.length;
   const [files, reviews, checks] = await Promise.all([
     filesUpToDate ? Promise.resolve<GitHubFilePayload[]>([]) : fetchPullRequestFiles(env, repoFullName, pr.number, token, warnings, admissionKey, caller),
-    fetchPullRequestReviews(env, repoFullName, pr.number, token, warnings, admissionKey),
+    reviewsUpToDate ? Promise.resolve<GitHubReviewPayload[]>([]) : 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}:`));
+  // reviewsSyncedAt only ever ADVANCES on a genuine success in THIS call -- never on a cache-hit skip, and
+  // never on a failed fetch attempt. This is what makes a stored reviewsSyncedAt a trustworthy "last confirmed
+  // successful sync" marker on its own (no separate errorSummary string-matching needed: a failed or skipped
+  // pass simply preserves whatever was already known, which -- being unchanged -- correctly keeps comparing as
+  // stale against reviewsInvalidatedAt on the next pass until a real fetch actually succeeds).
+  const reviewSyncFailedThisCall = !reviewsUpToDate && warnings.slice(warningStart).some((warning) => warning.startsWith(`Review sync failed for #${pr.number}:`));
+  const reviewsSyncedAtResult = !reviewsUpToDate && !reviewSyncFailedThisCall ? reviewFetchStartedAt : reviewsSyncedAtBefore;
 
   if (!filesUpToDate && !fileSyncFailed) {
     await deletePullRequestFiles(env, repoFullName, pr.number);
@@ -2036,6 +2070,7 @@ async function fetchAndStorePullRequestDetails(
       payload: check as unknown as Record<string, JsonValue>,
     });
   }
+  return { reviewsSyncedAt: reviewsSyncedAtResult };
 }
 
 // GitHub caps list endpoints at 100 items/page, so a single `per_page=100` fetch silently truncates a

src/queue/processors.ts

@@ -43,6 +43,7 @@ import {
   getCachedAiReview,
   putCachedAiReview,
   markPullRequestsRegated,
+  markPullRequestReviewsInvalidated,
   markPullRequestSurfacePublished,
   getLatestRegatedAt,
   claimRegateFanoutSlot,
@@ -4220,6 +4221,29 @@ async function processGitHubWebhook(
           }),
         );
       });
+      // Reviews-cache invalidation (#2537): a `pull_request_review` webhook (submitted/dismissed/edited) is
+      // the ONLY event that can change the set of reviews GitHub reports for this PR, so it is the sole signal
+      // fetchAndStorePullRequestDetails's reviewsUpToDate check needs to know the cached reviews are stale.
+      // Independent of, and does not gate, any downstream processing below — best-effort like the outcome/
+      // reversal recording above, so a transient D1 failure here never blocks the webhook.
+      if (
+        eventName === "pull_request_review" &&
+        (payload.action === "submitted" || payload.action === "dismissed" || payload.action === "edited")
+      ) {
+        await markPullRequestReviewsInvalidated(env, repoFullName, payloadPullRequest.number).catch((error) => {
+          /* v8 ignore next -- best-effort: cache-invalidation stamping never blocks the webhook. */
+          console.warn(
+            JSON.stringify({
+              level: "warn",
+              event: "pull_request_reviews_invalidate_failed",
+              deliveryId,
+              repository: repoFullName,
+              pullNumber: payloadPullRequest.number,
+              error: errorMessage(error),
+            }),
+          );
+        });
+      }
       const pr = await upsertPullRequestFromGitHub(
         env,
         repoFullName,

src/types.ts

@@ -870,6 +870,7 @@ export type PullRequestDetailSyncStateRecord = {
   headSha?: string | null | undefined;
   filesSyncedAt?: string | null | undefined;
   reviewsSyncedAt?: string | null | undefined;
+  reviewsInvalidatedAt?: string | null | undefined;
   checksSyncedAt?: string | null | undefined;
   lastSyncedAt?: string | null | undefined;
   errorSummary?: string | null | undefined;