Skip to content

fix(github): self-heal the reviews cache on the periodic sweep, not just webhooks - #2639

Merged
JSONbored merged 4 commits into
mainfrom
fix/reviews-cache-sweep-selfheal
Jul 3, 2026
Merged

fix(github): self-heal the reviews cache on the periodic sweep, not just webhooks#2639
JSONbored merged 4 commits into
mainfrom
fix/reviews-cache-sweep-selfheal

Conversation

@JSONbored

@JSONbored JSONbored commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • The durable reviews cache (feat(github): extend the head-SHA snapshot cache to live PR reads and reviews #2537, merged in feat(github): cache PR reviews with webhook-based invalidation #2633) is invalidated only by an explicit 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 could carry a stale reviews cache indefinitely.
  • Extracts the reviewsUpToDate check out of fetchAndStorePullRequestDetails into an exported isReviewsCacheUpToDate predicate (same reviewsSyncedAt vs reviewsInvalidatedAt comparison, now one authoritative definition instead of a duplicated copy).
  • The per-PR sweep unit (reReviewStoredPullRequest) now checks that predicate directly and forces a refresh when the cache is stale and no other reason already triggers one. A failed cache-state read fails open toward "stale" rather than crashing the sweep.
  • Tradeoff: a failed sync-state read is intentionally treated as stale (fail-open toward re-checking), and the resulting refresh call is itself still best-effort via refreshPullRequestDetails(...).catch(() => undefined) — a D1 hiccup never crashes the sweep, but it also means that specific pass can be a no-op; convergence relies on a later sweep pass once the read succeeds.

Scope

Validation

  • npm run test:ci (full local gate: actionlint, migrations check, typecheck, unsharded coverage, workers, MCP build/pack, miner build, REES tests, OpenAPI checks, UI lint/typecheck/test/build) — green.
  • npm audit --audit-level=moderate — 0 vulnerabilities.
  • New regression tests: sweep forces a refresh on a stale reviews cache with no other trigger active; a failed sync-state read fails open without crashing the sweep. Both mutation-verified (fail without the fix).
  • 100% branch coverage on every new/changed line (confirmed directly against the coverage report, not just the aggregate summary).

Safety

  • No secrets, tokens, or private scoring values touched.
  • No auth/CORS surface touched.

@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 00:46:56 UTC

4 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change moves the review-cache freshness decision into a shared predicate and wires the periodic per-PR sweep to refresh details when that predicate says the stored review state is stale, including a bounded-age fallback for missed invalidation writes. The main path is coherent: existing invalidation ordering still fails closed, failed sweep-side sync-state reads are treated as stale, and the new tests cover stale markers, missing markers past the age window, and malformed timestamps. The notable tradeoff is that cache freshness is now time-dependent in a helper described as pure, so tests and callers need to keep controlling time where cache-hit behavior matters.

Nits — 6 non-blocking
  • nit: src/github/backfill.ts:1984 describes isReviewsCacheUpToDate as a pure predicate, but the function reads Date.now(), so either pass now in explicitly or update the comment to avoid misleading future callers.
  • nit: src/github/backfill.ts:1976 hard-codes the 48h backstop without an exported name or caller-supplied clock, which makes boundary tests and future tuning more awkward than necessary.
  • nit: test/unit/queue.test.ts:1077 adds three large near-identical fetch stubs for the sweep regression cases; a small local helper would make the scenario differences easier to audit.
  • In src/github/backfill.ts:isReviewsCacheUpToDate, consider accepting an optional nowMs/now parameter so production can default to Date.now() while tests can assert the 48h boundary without global fake timers.
  • In test/unit/queue.test.ts, extract the repeated PR seed and GitHub fetch stub setup from the new regression tests so each test highlights only the cache-state condition it is proving.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:M; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 61 registered-repo PR(s), 52 merged, 506 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 61 PR(s), 506 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 61 PR(s), 506 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 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.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@JSONbored JSONbored self-assigned this Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.09%. Comparing base (0658c41) to head (1403887).
⚠️ Report is 2 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2639   +/-   ##
=======================================
  Coverage   96.09%   96.09%           
=======================================
  Files         237      237           
  Lines       26472    26482   +10     
  Branches     9602     9606    +4     
=======================================
+ Hits        25438    25449   +11     
+ Misses        425      424    -1     
  Partials      609      609           
Files with missing lines Coverage Δ
src/github/backfill.ts 96.96% <100.00%> (+0.02%) ⬆️
src/queue/processors.ts 92.45% <100.00%> (+0.05%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored force-pushed the fix/reviews-cache-sweep-selfheal branch 2 times, most recently from 6f1c422 to 20437ef Compare July 2, 2026 23:41
JSONbored added 4 commits July 2, 2026 17:25
…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.
… 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.
…s-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.
@JSONbored
JSONbored force-pushed the fix/reviews-cache-sweep-selfheal branch from 20437ef to 1403887 Compare July 3, 2026 00:29
@JSONbored
JSONbored merged commit dfba160 into main Jul 3, 2026
12 checks passed
@JSONbored
JSONbored deleted the fix/reviews-cache-sweep-selfheal branch July 3, 2026 01:05
JSONbored added a commit that referenced this pull request Jul 3, 2026
…ypass telemetry

The new test fixtures reused the pre-existing "installation-token" literal
verbatim; since this is the first time those specific lines appear as new
diff content, the scanner flags it the same way #2639 already worked around
this exact false positive — rename the new occurrences to the established
"fake-installation-token" convention.

Also: a forced re-gate bypass was being counted under the cache-miss
metric/audit, conflating "the cache had nothing to serve" with "a caller
explicitly opted out" — split it into its own
gittensory_ai_review_force_bypass_total counter and
github_app.ai_review_force_bypass audit event, and tighten the stale
forceAiReview comment.
JSONbored added a commit that referenced this pull request Jul 3, 2026
… unchanged heads (#2667)

* fix(review): bound AI review re-spend and public-surface republish on unchanged heads

Scheduled re-gate sweeps were re-spending a full AI review on every pass for a
PR whose outcome landed in a non-cacheable state (consensus defect /
inconclusive / a dynamic-context repo with grounding or RAG enabled) — the
durable ai_review_cache correctly never stores those outcomes, so nothing
throttled the retry. Root-caused in production: one PR generated 281 AI
review calls in 24h at an unchanged head, ~92% of it from the RAG-active
unconditional-bypass path.

- ai_review_cache gains a `cacheable` column; a non-cacheable outcome (and a
  dynamic-context result, now bounded rather than unconditionally bypassing
  the cache) is still persisted for a 30-minute cooldown reuse, never as a
  durable hit. A lock-contention placeholder is still never persisted at all.
- New audit events + counters: ai_review_cache_hit/miss/write_error,
  ai_review_non_cacheable, agent.sweep.regate_ai_skipped_current,
  github_app.public_surface_publish_skipped_current. A cache write failure is
  now observable instead of a silent catch.
- A narrow public-surface no-op guard skips republishing a check-run-only
  repo's completed check when nothing provably changed since the last pass
  (head match + a live-verified completed check run + no pending refresh
  signal), falling through to a full republish on any doubt.
- `agent-regate-pr` jobs carry an optional `force` flag that bypasses both
  the cache and the cooldown for an explicit manual re-gate.

Validated against production Postgres audit_events/ai_usage_events data for
the incident repo/PR before and during the fix.

* fix(review): avoid a secret-scanner false positive and split forced-bypass telemetry

The new test fixtures reused the pre-existing "installation-token" literal
verbatim; since this is the first time those specific lines appear as new
diff content, the scanner flags it the same way #2639 already worked around
this exact false positive — rename the new occurrences to the established
"fake-installation-token" convention.

Also: a forced re-gate bypass was being counted under the cache-miss
metric/audit, conflating "the cache had nothing to serve" with "a caller
explicitly opted out" — split it into its own
gittensory_ai_review_force_bypass_total counter and
github_app.ai_review_force_bypass audit event, and tighten the stale
forceAiReview comment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

1 participant