feat(github): cache bare PR reads and reviews, mirroring the head-SHA sync-state pattern#2595
feat(github): cache bare PR reads and reviews, mirroring the head-SHA sync-state pattern#2595JSONbored wants to merge 7 commits into
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 22:06:33 UTC
⏸️ Suggested Action - Manual Review
Review summary Blockers
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 #2595 +/- ##
==========================================
+ Coverage 96.05% 96.08% +0.03%
==========================================
Files 234 234
Lines 26280 26357 +77
Branches 9531 9564 +33
==========================================
+ Hits 25244 25326 +82
+ Misses 425 424 -1
+ Partials 611 607 -4
🚀 New features to boost your workflow:
|
262140e to
bbfcd04
Compare
|
Fixed the CI failure: |
2608e89 to
501f3ce
Compare
|
Fixed both flagged blockers: 1. Cross-field cache-freshness bug (real defect). 2. Possible leaked secret (false positive, confirmed via the actual scanner). Also rebased twice more to keep pace with |
501f3ce to
6738fe1
Compare
|
Fixed a third, separate defect from the latest review pass — a real backward-compatibility issue: Legacy reviews_synced_at markers from failed syncs. Confirmed by diffing against `origin/main`: every pre-existing writer of `reviews_synced_at` (`backfillOpenPullRequestDetails` / `refreshPullRequestDetails` / `backfillRepository`) stamped it unconditionally on every sync pass, success or failure — there was no gate on it at all before this PR. This PR's new durable review cache treats any non-null `reviews_synced_at` as "fully synced, never refetch" until a `pull_request_review` webhook invalidates it. So an existing row whose review sync previously failed (rate limit, transient error) already carries a timestamp from that failed attempt, and the new cache would trust that incomplete/stale review data indefinitely for any PR that doesn't happen to get a new review action after this deploys. Added migration `0095`: a one-time `UPDATE` clearing `reviews_synced_at` on every existing row. There's no reliable way to tell a trustworthy stamp from an untrustworthy one from the row alone (`status` reflects the aggregate files/reviews/checks outcome for a pass, not reviews specifically), so it clears unconditionally rather than guessing — the cost is one redundant review refetch per already-correctly-synced PR, which is cheap and bounded. Pinned the exact migration behavior with a dedicated test that applies the real migration file against a raw in-memory SQLite DB (mirroring the existing `digest-subscription-normalization.test.ts` pattern), seeding legacy-shaped rows and asserting only `reviews_synced_at` clears while every other column is untouched. Rebased again to keep pace with `main` (two more unrelated merges landed). Full local gate (`test:ci`, `npm audit`, `db:migrations:check`) green. |
… sync-state pattern Closes #2537. GET /pulls/{n} (PR state/mergeable_state) and GET /pulls/{n}/reviews were the largest and third-largest GitHub REST contributors observed in a live rate-limit trough, and neither was covered by the earlier PR-files cache (#audit-rate-headroom / #2527). The bare-PR-state read was implemented as four near-identical helpers with no caching or coalescing, called from readiness checks, the maintenance planner, duplicate-sibling reconciliation, and a gate-override command; reviews were explicitly left uncached ("more volatile than files" -- true at a fixed head, but reviews only actually change on a pull_request_review webhook). Extends the existing pull_request_detail_sync_state table (migration 0093) with prMergeableState/prState/prStateFetchedAt, and reuses the already-present reviewsSyncedAt column as the review cache's freshness marker. PR-state is event-invalidated on pull_request synchronize/closed/reopened (with a 5-minute safety-net TTL so a missed webhook self-heals within one sweep tick); reviews are invalidated on pull_request_review submitted/edited/dismissed rather than on head-SHA change, since reviews are independent of the head. The pre-merge verdict thread and its unified-comment mirror (the #4220 act-boundary) are deliberately left routed through the raw, uncached fetch -- they must always force-refetch live, since the stored/cached mergeable_state lagging GitHub's async recompute is exactly what caused #4220. This is verified by two dedicated regression tests plus direct mutation testing (temporarily rerouting the act-boundary through the cache and confirming both tests -- and no others in the suite -- catch it).
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
A second AI review pass flagged a real backward-compatibility defect in the same PR: every pre-existing writer of reviews_synced_at (backfillOpenPullRequestDetails / refreshPullRequestDetails / backfillRepository) stamped it unconditionally on every sync pass, success or failure -- confirmed by diffing against origin/main, where `reviewsSyncedAt: syncedAt` is written with no success gate at all. This PR's new durable review cache (fetchAndStorePullRequestDetails's reviewsUpToDate check) treats ANY non-null reviews_synced_at as "fully synced, never refetch" until a pull_request_review webhook invalidates it. Existing rows whose review sync previously failed (rate limit, transient error) already carry a timestamp from that failed attempt, so the new cache would trust incomplete/stale review data indefinitely for any PR that doesn't happen to receive a new review action after this deploys. Added migration 0095: a one-time UPDATE clearing reviews_synced_at on every existing row. There's no reliable way to tell a trustworthy stamp from an untrustworthy one from the row alone (status reflects the aggregate files/reviews/checks outcome for a pass, not reviews specifically), so it clears unconditionally rather than guessing -- the cost is one redundant review refetch per already-correctly-synced PR, which is cheap and bounded. Pinned the migration's exact behavior with a dedicated regression test (mirroring the existing digest-subscription-normalization.test.ts pattern: apply the real migration files against a raw in-memory SQLite DB, seed legacy-shaped rows, assert only reviews_synced_at clears and everything else is untouched). Rebased again to keep pace with main.
Addresses the blocker the gate's own AI review flagged: this branch's new durable review cache (reviewsSyncedAt) was only invalidated on a pull_request_review webhook (submitted/edited/dismissed). But when a repo's branch protection has "dismiss stale pull request approvals" enabled, GitHub also dismisses existing approvals server-side on a new push (a pull_request synchronize event) -- exactly the same moment the PR-state cache already treats as a forced live miss for mergeable_state. The review cache had no equivalent signal for that path, so a repo with that protection rule could serve a stale "reviews complete" read indefinitely after a push silently dismissed the approval it was based on. Extends the existing synchronize/closed/reopened PR-state invalidation block to also invalidate the review cache, but only for synchronize (GitHub doesn't dismiss reviews on close or reopen). Updated the now-inaccurate comment on the pull_request_review-only invalidation site accordingly, and added a regression test mirroring the existing per-action review-invalidation table test, mutation-verified to fail without the fix.
6738fe1 to
2098f7b
Compare
Addresses the blocker the gate's own AI review flagged: fetchAndStorePullRequestDetails's reviewsUpToDate check trusted any non-null reviewsSyncedAt as permanently fresh. invalidatePrReviewsCache is called best-effort from the webhook handler, so a dropped invalidation write (submitted/edited/dismissed review, or now synchronize) left stale review rows in use indefinitely -- unlike the files cache, reviews have no head-SHA comparison to fall back on either. Adds PR_REVIEWS_CACHE_MAX_AGE_MS (24h) and isPrReviewsCacheFresh, mirroring the existing PR_STATE_CACHE_MAX_AGE_MS/isPrStateCacheFresh pattern for the bare PR-state cache, but with a much longer window since reviews don't need minute-level freshness -- just an eventual-consistency bound on a missed invalidation.
…able cache
Addresses the other blocker the gate's own AI review flagged: resolveOverrideHeadSha (the security-sensitive, human-triggered @gittensory gate-override command) had been switched to cachedFetchLivePullRequestHeadSha, contradicting its own docstring ("re-fetch the LIVE head"). The whole point of this function is closing the race where a new commit lands between the override comment and its processing -- a cache hit within the durable cache's freshness window reintroduces exactly that race.
Reverts the call site to the raw fetchLivePullRequestHeadSha (same as before #2537 touched it), matching the sibling PR's explicit reasoning for excluding this call site from caching, and updates the now-inaccurate comments on both resolveOverrideHeadSha and the durable PR-state cache's own doc comment.
|
Superseded by #2633, which merged independently while this PR was still being iterated on. #2633 delivers a hardened, complete implementation of the review-caching half of #2537 — including two things this PR's own fix history was still working through: making This PR's bare PR-state caching (mergeable_state/state/head-SHA, scoped to the non-mutating freshness-guard/readiness/dup-winner reads while keeping every act-boundary read — merge/close, draft-dodge, reopen-reclose, gate-override — live) is the half #2633 explicitly deferred as its own follow-up, and isn't touched by it. That work is tested and gate-verified in this branch's history but conflicts too deeply with #2633's now-merged review-cache implementation to rebase cleanly; closing here rather than force a risky reconciliation. Worth a fresh, narrowly-scoped PR against current main if that caching is still wanted — the design and tests already exist in this branch for reference. |
Summary
Closes #2537.
GET /pulls/{n}(PR state/mergeable_state) andGET /pulls/{n}/reviewswere the largest and third-largest GitHub REST contributors observed in a live rate-limit trough — larger than the PR-files route the earlier fix (#2527) addressed, and deliberately left uncached by that fix. The bare-PR-state read was implemented as four near-identical helper functions (fetchLivePullRequestMergeState,fetchLivePullRequestState,fetchLivePullRequestHeadSha,fetchLivePullRequest) with no caching or cross-call coalescing, called from several independent read sites (readiness checks, the maintenance planner, duplicate-sibling reconciliation, a gate-override command). Reviews were explicitly excluded from #2527 ("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).What changed
pull_request_detail_sync_state(migration0093) withprMergeableState/prState/prStateFetchedAt, and reuses the already-present (previously-unused)reviewsSyncedAtcolumn as the review cache's freshness marker — no new table.src/github/backfill.ts): newcachedFetchLivePullRequestMergeState/cachedFetchLivePullRequestState/cachedFetchLivePullRequestHeadSha, each a read-through wrapper over the durable row with a 5-minute safety-net TTL (PR_STATE_CACHE_MAX_AGE_MS) so a missed webhook self-heals within one sweep tick.primeDurablePrStateCachelets an already-paid-for full-payload fetch (the sweep resync) prime the cache for other readers instead of them re-fetching moments later. Wired into every read-only call site (readiness, duplicate-sibling reconciliation, the gate-override command).fetchAndStorePullRequestDetails): gated purely onreviewsSyncedAtbeing set — no head-SHA comparison, since reviews are independent of the head (unlike files). Only stampsreviewsSyncedAtwhen the review fetch actually succeeded that pass (a failed sync must not be cached as if it succeeded).src/queue/processors.ts):invalidatePrStateCacheonpull_requestsynchronize/closed/reopened;invalidatePrReviewsCacheonpull_request_reviewsubmitted/edited/dismissed(already the exact 3 actionsshouldProcessPullRequestPublicSurfacerestricts that event to). Both explicitly null the relevant columns (PARTIAL-UPDATE CONTRACT: omitted = unchanged, explicitnull= cleared).gittensory_pr_state_cache_total{field,result}andgittensory_pr_reviews_cache_total{result}, mirroring the existinggittensory_github_pull_request_files_fetch_totalbounded-label style.runAgentMaintenancePlanAndExecute's disposition input) and its unified-comment mirror — both already documented against the feat(mcp): add gittensory_explain_gate_disposition tool (#2234) #4220 contradiction — continue to call the raw, uncachedfetchLivePullRequestMergeStateviarefreshLiveMergeState, which is untouched by this diff. They must never be routed through the new cache: a base-conflicting PR reading a stalecleanhere while the disposition reads livedirtyis exactly the bug feat(mcp): add gittensory_explain_gate_disposition tool (#2234) #4220 fixed.Correctness verification
I adversarially verified the act-boundary claim with actual mutation testing, not just a read of the code: temporarily rerouted
refreshLiveMergeStatethrough the new cache, confirmed a pre-existing, unrelated test (#audit-rate-headroom: the per-PR re-review refreshes merge state and CI...) failed as expected, then confirmed the two purpose-built#4220 regressiontests in this diff did not originally catch it — they were confounded by other same-request cache-priming/invalidation side effects that happened to clear the stale seed before the disposition ran, for unrelated reasons. Fixed both tests to no-op those confounding calls and assert directly (via pass-through spies) that the act-boundary's own read goes through the raw fetcher and never the cached one — then re-ran the same mutation to confirm both now fail correctly. Reverted the mutation; both pass cleanly on the real code.Also added dedicated tests for
primeDurablePrStateCache(previously exercised only indirectly, with no assertions on its own output), covering its early-exit, nullish-fallback, and PARTIAL-UPDATE CONTRACT branches.Known limitation
invalidatePrStateCachefires onsynchronize/closed/reopenedonly (matching the issue's own scope), not on aneditedaction that retargets a PR's base branch — a rare case where GitHub could recomputemergeable_statefor the new base without one of those three actions firing. Bounded by the 5-minute safety-net TTL; not a correctness issue for the act-boundary path (which never reads this cache). Left as a documented gap rather than expanding scope.Tests
test/unit/pr-detail-durable-cache.test.ts(new): cache miss/hit/expiry for all three PR-state fields, fail-open on a write hiccup,invalidatePrStateCache, the write-through status-preservation regression, andprimeDurablePrStateCache's three branches.test/unit/backfill-file-hydration-scoping.test.ts: cold-cache fetch, nullish-state fallback, head-independent reuse (contrasted with the files cache's head-scoped behavior), and invalidated-cache refetch for reviews.test/unit/queue.test.ts: webhook invalidation for all 3 PR-state actions and all 3 review actions (it.each), non-invalidating actions left alone, sibling-event non-invalidation, and the two mutation-tested#4220regression tests described above.Validation
npm run typechecknpx vitest run test/unit/pr-detail-durable-cache.test.ts test/unit/backfill-file-hydration-scoping.test.ts test/unit/queue.test.tsnpm run test:changednpm run test:coverage(unsharded, full suite — 100% line and branch coverage on every changed line insrc/**percoverage/lcov.info, cross-checked programmatically against the diff)npm run test:workersnpm run db:migrations:checknpm run test:ci(the full local gate, exit 0)npm audit --audit-level=moderategit diff --checkScope
site/,CNAME,**/lovable/**, orCHANGELOG.mdSafety