feat(github): cache PR reviews with webhook-based invalidation - #2633
Conversation
The recently-merged head-SHA file cache (#2527) deliberately left reviews uncached ("more volatile than files"), but reviews only actually change on a pull_request_review webhook, not on every sweep tick -- unlike files, reviews are independent of head SHA entirely. fetchAndStorePullRequestDetails now skips the GET /pulls/{n}/reviews call when a prior sync already covers every review-invalidating event: the new reviews_invalidated_at column (bumped by a pull_request_review webhook with action submitted/dismissed/edited) is compared against the existing reviews_synced_at timestamp. A stored row whose last review sync itself failed is excluded from the cache-hit path so a transient failure can't poison the cache into never retrying. Broadened the existing sync-state row lookup (previously gated on `!forceFiles && headSha`, which would silently disable review caching whenever headSha was momentarily unknown) since reviews never depend on headSha or the files-only forceFiles flag. Bare PR-state caching (state/mergeable_state/head SHA), the other half of this issue, was deliberately NOT implemented after auditing every live-read call site in the codebase: each one (the freshness guard in agent-action-executor.ts, the draft-dodge-close and reopen-reclose re-checks, the gate-override command, and the sweep-resync path) is a documented act-boundary that intentionally requires a live, uncached read immediately before a mutation or before publishing review output. mergeable_state already has per-pass caching via the existing LiveGithubFacts mechanism. Caching any of these would risk exactly the regression the issue's own acceptance criteria forbids ("No regression in any decision that depends on a live PR state"). Filing as a follow-up rather than force-fitting a cache onto call sites that don't have safe surface area for one.
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 22:18:27 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 5 non-blocking
Concerns raised — review before merging
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2633 +/- ##
=======================================
Coverage 96.05% 96.06%
=======================================
Files 234 234
Lines 26280 26299 +19
Branches 9531 9535 +4
=======================================
+ Hits 25244 25264 +20
Misses 425 425
+ Partials 611 610 -1
🚀 New features to boost your workflow:
|
…ision Gate review finding on PR #2633: the sync-state row lookup was skipped entirely whenever forceFiles && headSha (the manual "refresh files" path), which zeroed out reviewsUpToDate too and forced an unrelated GET /pulls/{n}/reviews on every manual files-only force, even when reviews were already cache-current. forceFiles only ever means "force a files refetch" -- reviews caching must be completely independent of it. Now fetches the row unconditionally and applies forceFiles only to filesUpToDate, never to reviewsUpToDate.
Second round of gate findings on PR #2633: 1. fetchAndStorePullRequestDetails read existingState as a snapshot at the top of the call, but every caller then unconditionally stamped reviewsSyncedAt to "now" once the whole call finished. A pull_request_review webhook racing in after the snapshot read but before that final write would set reviewsInvalidatedAt to a moment the stamped reviewsSyncedAt would then read as already covering -- the cache could confirm itself fresh through an invalidation it never actually observed. Now returns the correct reviewsSyncedAt to the caller instead: captured BEFORE the fetch starts on a genuine success (conservative against races from that instant on), and left unchanged on a skip or a failed fetch. This also makes a stored reviewsSyncedAt trustworthy on its own, so the separate errorSummary string-matching safety net (fragile: it only ever reflects whichever of files/reviews/ checks failed LAST in a given pass, since they run concurrently) is no longer needed and has been removed. reviewsUpToDate's comparison is also now a strict `>` rather than `>=`, so a millisecond-resolution timestamp tie between a sync and a racing invalidation fails toward "still needs a refetch" rather than silently trusting the cache. 2. markPullRequestReviewsInvalidated is the sole source of the invalidation signal, so a single transient D1 write failure would permanently lose that PR's "reviews changed" event. Added a bounded 3-attempt retry -- still best-effort from the webhook handler's perspective (never blocks/retries the whole job), but absorbs a momentary blip in-process.
…he cache Third gate finding on PR #2633: reviews_synced_at has existed since migration 0006, long before this PR gives it any cache-skip meaning -- every sync pass since then has stamped it unconditionally, including passes whose review fetch itself failed. On deploy, reviewsUpToDate would immediately trust every existing row's reviews_synced_at at face value, silently skipping re-fetches for PRs whose last review sync actually failed, until some later invalidating webhook happened to arrive. Migration 0094 now resets reviews_synced_at to NULL for every row whose last sync status was not 'complete' (status only reads 'complete' when that pass recorded zero warnings across files/reviews/checks, which reliably means reviews specifically succeeded -- any other status could have been a review failure, so it's reset to force a guaranteed-safe refetch). A genuinely 'complete' row is left untouched. Adds a migration-effect test (applying the actual 0094 SQL against a scratch table shaped like the pre-#2537 schema, seeded with rows exactly as years of pre-#2537 code would have written them) proving the cleanup targets the right rows and nothing else.
…ust webhooks The durable reviews cache (#2537, PR #2633) is only invalidated by an explicit markPullRequestReviewsInvalidated write on a pull_request_review/synchronize webhook. If that write is ever dropped, nothing else re-checks it: the periodic re-gate sweep only calls refreshPullRequestDetails when slop evidence, the manifest gate, or a pre-merge check path is active, so a "quiet" PR with none of those enabled would carry a stale reviews cache indefinitely with no other convergence path. Extracts the reviewsUpToDate check out of fetchAndStorePullRequestDetails into an exported isReviewsCacheUpToDate predicate (same reviewsSyncedAt vs reviewsInvalidatedAt comparison, now a single authoritative definition), and has the per-PR sweep unit (reReviewStoredPullRequest) check it directly, forcing a refresh when stale and no other reason already triggers one. A failed cache-state read fails open toward "stale" rather than crashing the sweep.
Advances #2537. The head-SHA file cache (#2527) and the recently-merged review cache (#2633) left one sibling route uncached: a bare GET /pulls/{n} (state/mergeable_state/head SHA), implemented as four separate near-identical helper functions with no caching or cross-call coalescing, called from many independent sites within one review pass. Adds a durable, webhook-invalidated cache for this read on pull_request_detail_sync_state (prMergeableState/prState/prStateFetchedAt), capped at a 5-minute PR_STATE_CACHE_MAX_AGE_MS safety net so a missed invalidation self-heals within one sweep tick. A single GET /pulls/{n} write- throughs all three fields together under one shared fetchedAt stamp (fetchAndCachePrStateFields), so a cache miss on any one field never leaves another looking falsely fresh with an unfetched value. Wired at the freshness-guard readiness read (cachedLiveMergeState), the dup-winner reconcile (reconcileLiveDuplicateSiblings), and primed for free by the per-PR sweep's existing resync fetch (primeDurablePrStateCache) -- deliberately NOT wired into any act-boundary read: the merge/close decision (refreshLiveMergeState, unchanged) and the gate-override head-SHA resolution (resolveOverrideHeadSha, unchanged) both keep forcing a live fetch by design, since both need the literal current commit rather than a value that can be briefly stale. Test plan: - npm run typecheck clean - npm run db:migrations:check clean (0095, contiguous) - npm run test:ci full local gate green - 100% branch coverage on every changed line in backfill.ts/processors.ts/ repositories.ts/schema.ts, confirmed via lcov diff-coverage cross-check - New tests cover: cache miss/hit/expiry for all three fields, the shared- fetch write-through, webhook invalidation on synchronize/closed/reopened (and non-invalidation on unrelated actions), dup-winner reuse of a warm cache row, the sweep priming the cache from its own resync, and two mutation-verified regressions: gate-override must never route through the durable head-SHA cache (the race this function exists to close), and the sweep's resync must actually persist to the cache for later readers.
…es (#2640) * feat(github): cache bare PR-state reads at non-authoritative call sites Advances #2537. The head-SHA file cache (#2527) and the recently-merged review cache (#2633) left one sibling route uncached: a bare GET /pulls/{n} (state/mergeable_state/head SHA), implemented as four separate near-identical helper functions with no caching or cross-call coalescing, called from many independent sites within one review pass. Adds a durable, webhook-invalidated cache for this read on pull_request_detail_sync_state (prMergeableState/prState/prStateFetchedAt), capped at a 5-minute PR_STATE_CACHE_MAX_AGE_MS safety net so a missed invalidation self-heals within one sweep tick. A single GET /pulls/{n} write- throughs all three fields together under one shared fetchedAt stamp (fetchAndCachePrStateFields), so a cache miss on any one field never leaves another looking falsely fresh with an unfetched value. Wired at the freshness-guard readiness read (cachedLiveMergeState), the dup-winner reconcile (reconcileLiveDuplicateSiblings), and primed for free by the per-PR sweep's existing resync fetch (primeDurablePrStateCache) -- deliberately NOT wired into any act-boundary read: the merge/close decision (refreshLiveMergeState, unchanged) and the gate-override head-SHA resolution (resolveOverrideHeadSha, unchanged) both keep forcing a live fetch by design, since both need the literal current commit rather than a value that can be briefly stale. Test plan: - npm run typecheck clean - npm run db:migrations:check clean (0095, contiguous) - npm run test:ci full local gate green - 100% branch coverage on every changed line in backfill.ts/processors.ts/ repositories.ts/schema.ts, confirmed via lcov diff-coverage cross-check - New tests cover: cache miss/hit/expiry for all three fields, the shared- fetch write-through, webhook invalidation on synchronize/closed/reopened (and non-invalidation on unrelated actions), dup-winner reuse of a warm cache row, the sweep priming the cache from its own resync, and two mutation-verified regressions: gate-override must never route through the durable head-SHA cache (the race this function exists to close), and the sweep's resync must actually persist to the cache for later readers. * fix(github): keep the duplicate-winner reconcile on a live PR-state read Addresses the blocker the gate's own AI review flagged: reconcileLiveDuplicateSiblings was routed through cachedFetchLivePullRequestState, but that reconcile directly feeds duplicate-winner selection, which can auto-CLOSE the current PR when GITTENSORY_DUPLICATE_WINNER is on. A cached "open" read up to PR_STATE_CACHE_MAX_AGE_MS stale after a missed closed webhook would keep an already-closed sibling eligible as the winner, wrongly closing the current PR as the loser -- the same class of irreversible-actuation risk the merge/close decision and gate-override guard against. Reverts this call site to the raw fetchLivePullRequestState (unchanged from before this PR), and replaces the test that asserted cache reuse with a regression proving the opposite: a warm-but-wrong cached row must never be served here. Also replaces three test-fixture token values (fake-installation-token / installation-token) with the shorter placeholder already used elsewhere in these same tests -- both incidentally matched the repo's generic-secret-assignment scan pattern (a keyword-shaped heuristic with no placeholder-value exclusion in REES's copy of the analyzer, unlike the stricter src/review/secrets-scan.ts). No real credential was ever present; this just avoids tripping a keyword-length heuristic on new lines.
…ust webhooks The durable reviews cache (#2537, PR #2633) is only invalidated by an explicit markPullRequestReviewsInvalidated write on a pull_request_review/synchronize webhook. If that write is ever dropped, nothing else re-checks it: the periodic re-gate sweep only calls refreshPullRequestDetails when slop evidence, the manifest gate, or a pre-merge check path is active, so a "quiet" PR with none of those enabled would carry a stale reviews cache indefinitely with no other convergence path. Extracts the reviewsUpToDate check out of fetchAndStorePullRequestDetails into an exported isReviewsCacheUpToDate predicate (same reviewsSyncedAt vs reviewsInvalidatedAt comparison, now a single authoritative definition), and has the per-PR sweep unit (reReviewStoredPullRequest) check it directly, forcing a refresh when stale and no other reason already triggers one. A failed cache-state read fails open toward "stale" rather than crashing the sweep.
…ust webhooks The durable reviews cache (#2537, PR #2633) is only invalidated by an explicit markPullRequestReviewsInvalidated write on a pull_request_review/synchronize webhook. If that write is ever dropped, nothing else re-checks it: the periodic re-gate sweep only calls refreshPullRequestDetails when slop evidence, the manifest gate, or a pre-merge check path is active, so a "quiet" PR with none of those enabled would carry a stale reviews cache indefinitely with no other convergence path. Extracts the reviewsUpToDate check out of fetchAndStorePullRequestDetails into an exported isReviewsCacheUpToDate predicate (same reviewsSyncedAt vs reviewsInvalidatedAt comparison, now a single authoritative definition), and has the per-PR sweep unit (reReviewStoredPullRequest) check it directly, forcing a refresh when stale and no other reason already triggers one. A failed cache-state read fails open toward "stale" rather than crashing the sweep.
…ust webhooks (#2639) * fix(github): self-heal the reviews cache on the periodic sweep, not just webhooks The durable reviews cache (#2537, PR #2633) is only invalidated by an explicit markPullRequestReviewsInvalidated write on a pull_request_review/synchronize webhook. If that write is ever dropped, nothing else re-checks it: the periodic re-gate sweep only calls refreshPullRequestDetails when slop evidence, the manifest gate, or a pre-merge check path is active, so a "quiet" PR with none of those enabled would carry a stale reviews cache indefinitely with no other convergence path. Extracts the reviewsUpToDate check out of fetchAndStorePullRequestDetails into an exported isReviewsCacheUpToDate predicate (same reviewsSyncedAt vs reviewsInvalidatedAt comparison, now a single authoritative definition), and has the per-PR sweep unit (reReviewStoredPullRequest) check it directly, forcing a refresh when stale and no other reason already triggers one. A failed cache-state read fails open toward "stale" rather than crashing the sweep. * fix(review): avoid a generic_secret_assignment false-positive flag on the new sweep-selfheal test fixtures Two new test fixtures reused the file's existing "installation-token" literal verbatim; every other occurrence in this file is pre-existing context, not a new diff line, so this is the first time it trips the scanner. Renamed to the established "fake-installation-token" convention already used elsewhere in this suite for the same reason. Confirmed clean by running the real scanner against the diff directly. * fix(review): drop a duplicate upsertPullRequestDetailSyncState import from a rebase merge * fix(github): add a bounded-age backstop for a silently dropped reviews-cache invalidation write Gate review (second pass): isReviewsCacheUpToDate's exact reviewsSyncedAt-vs- reviewsInvalidatedAt comparison is correct when the invalidation write actually happens, but a silently DROPPED markPullRequestReviewsInvalidated write leaves reviewsInvalidatedAt null forever -- with no marker to compare against, the sweep's own quiet-PR self-heal (the prior fix in this PR) never detects that anything changed, since the marker-only check reads "up to date" indefinitely. Moves a 48h bounded-age fallback directly into isReviewsCacheUpToDate (the single shared predicate), rather than duplicating it as a second, separate check in the sweep: an old-enough reviewsSyncedAt is now treated as stale regardless of the invalidation marker, closing the gap for a signal that was never recorded in the first place. Also fixes 5 pre-existing tests in backfill-reviews-cache-scoping.test.ts that seeded a fixed 2026-05-20 sync timestamp without pinning "now," which the new bounded-age check correctly started flagging as stale against real wall-clock time.
Advances #2537
Problem
The head-SHA file cache (#2527) deliberately left
GET /pulls/{n}/reviewsuncached ("reviews are more volatile than files" — true at a fixed head, but reviews only actually change on apull_request_reviewwebhook, not on every sweep tick). Live rate-limit observation flagged this as one of the largest remaining contributors to REST call volume.Changes
0094_pull_request_reviews_invalidated.sql: purely additivereviews_invalidated_atcolumn onpull_request_detail_sync_state.fetchAndStorePullRequestDetails(src/github/backfill.ts) now skips the reviews fetch when a prior sync already covers every review-invalidating event:reviewsSyncedAtis compared againstreviewsInvalidatedAt(notheadSha, since reviews are independent of the head — the core distinction from the files cache). A row whose last review sync itself failed is excluded from the cache-hit path so a transient failure can't poison the cache into never retrying.markPullRequestReviewsInvalidated(src/db/repositories.ts), called from the webhook handler on apull_request_reviewevent with actionsubmitted/dismissed/edited— best-effort, never blocks the webhook.!forceFiles && headSha) since review caching never depends onheadShaor the files-onlyforceFilesflag — the old gate would have silently disabled review caching wheneverheadShawas momentarily unknown.Scope decision: bare PR-state caching deferred
Issue #2537 also asked for a cache on bare PR-state reads (
state/mergeable_state/head SHA). After auditing every live-PR-read call site in the codebase, each one turned out to be a documented act-boundary or freshness-guard that intentionally requires a live, uncached read:src/services/agent-action-executor.ts's freshness guard: "every supported live action mutates PR state... it must still target the reviewed, open head" — runs immediately before every merge/close/approve/label mutation.src/queue/processors.ts, both explicitly commented "Live re-check... re-verify immediately before the mutation."@gittensory gate-overridecommand's head-SHA refresh — security-sensitive, human-triggered, wants the literal current commit.mergeable_statealready has per-pass caching via the existingLiveGithubFacts/cachedLiveMergeStatemechanism.Caching any of these would risk exactly the regression issue #2537's own acceptance criteria forbids ("No regression in any decision that depends on a live PR state"). Rather than force-fit a cache onto call sites that don't have safe surface area for one, I'm filing this as a follow-up with the reasoning documented here, and shipping only the reviews cache, which has no such risk.
Test plan
npm run db:migrations:check— 97 migrations OK, contiguous through 0094npm run typecheckcleannpm run test:coverage(unsharded) green, 100% branch coverage on all new/changed linesnpm audit --audit-level=moderate— 0 vulnerabilitiesmarkPullRequestReviewsInvalidatedcreates a row when absent and updates only its own column when one exists; webhook hook fires on submitted/dismissed/edited and not on an unrelated action