Skip to content

fix(review): cache the review-memory suppression list read, invalidate on write (#4508) - #4599

Merged
JSONbored merged 1 commit into
mainfrom
fix/review-memory-suppression-cache-4508
Jul 10, 2026
Merged

fix(review): cache the review-memory suppression list read, invalidate on write (#4508)#4599
JSONbored merged 1 commit into
mainfrom
fix/review-memory-suppression-cache-4508

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • listReviewSuppressions was re-read from D1 on every unified-comment render whenever commentGate.warnings.length > 0 for a review-memory-enabled repo — not gated by any cache marker. The 3 independent maybePublishPrPublicSurface call sites (auto re-review, webhook-triggered review, manual panel retrigger) can each fire this independently for the same repo/PR within a short window, with no shared cache between them.
  • Adds getCachedReviewSuppressions/invalidateReviewSuppressionCache to review-memory-wire.ts: a short (60s) in-isolate TTL cache keyed by repoFullName, mirroring rag.ts's chunkCountCache pattern exactly (including threading nowMs in from the caller rather than reading Date.now() internally, for deterministic behavior under fake timers).
  • Unlike chunkCountCache's "only cache the positive" (a repo's index state is one-way, cold→hot), a suppression set can grow at any time via @gittensory resolve — so the cache is explicitly invalidated at the one write site (maybeProcessResolveCommand) immediately after recordReviewSuppression succeeds, rather than relying on TTL expiry alone. A maintainer's fresh suppression takes effect on the very next render.
  • Also adds clearReviewSuppressionCacheForTest() (mirroring the existing clearInstallationTokenCacheForTest/clearGitHubResponseCacheForTest pattern) and wires it into queue.test.ts's global beforeEach — discovered during implementation that this file's fake-timer setup resets Date.now() to the same fixed instant for every test, so without an explicit reset a module-level cache would leak one test's suppression read into the next test using the same repo name.

Closes #4508.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (Closes #4508).

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — every changed line and branch in src/queue/processors.ts and src/review/review-memory-wire.ts is covered (confirmed via precise lcov.info cross-referencing against the exact diff hunks: zero uncovered lines, zero zero-hit branch sides).
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — an invariant test (repeated reads within TTL make zero additional listReviewSuppressions calls), a regression test (a fresh write between two renders is reflected immediately, not masked by a stale cache), plus TTL-expiry and per-repo-isolation tests — all empirically verified to fail against a deliberately-broken implementation (never-caches; invalidation no-op) and pass against the real one.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no such surface touched.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface changed.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section below. (N/A — backend-only change, no visible UI.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (N/A)

…e on write (#4508)

listReviewSuppressions was re-read from D1 on every unified-comment
render whenever a repo had review-memory enabled, even though the 3
independent maybePublishPrPublicSurface call sites (auto re-review,
webhook-triggered review, manual panel retrigger) can each fire for
the same repo within a short window. Adds a short in-isolate TTL
cache mirroring rag.ts's chunkCountCache, explicitly invalidated the
moment a maintainer records a fresh suppression via `@gittensory
resolve` so the very next render sees it, not just after the TTL.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

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

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.12%. Comparing base (63d5a0f) to head (bef9162).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4599   +/-   ##
=======================================
  Coverage   94.12%   94.12%           
=======================================
  Files         430      430           
  Lines       38181    38190    +9     
  Branches    13922    13923    +1     
=======================================
+ Hits        35939    35948    +9     
  Misses       1585     1585           
  Partials      657      657           
Files with missing lines Coverage Δ
src/queue/processors.ts 95.31% <100.00%> (ø)
src/review/review-memory-wire.ts 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb

loopover-orb Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-10 07:18:34 UTC

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

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review: This PR changes guardrail-protected path(s): src/queue/processors.ts (matched src/queue/**).

Review summary
This PR adds a 60s in-isolate TTL cache (getCachedReviewSuppressions) around the previously uncached listReviewSuppressions D1 read in maybePublishPrPublicSurface, with explicit invalidation wired into the one write path (maybeProcessResolveCommand, right after recordReviewSuppression's Promise.all succeeds) rather than relying on TTL alone. The pattern faithfully mirrors rag.ts's chunkCountCache (nowMs threaded from the caller, not read internally), and the accompanying clearReviewSuppressionCacheForTest is correctly wired into queue.test.ts's beforeEach to prevent fake-timer test leakage across repos. Tests exercise cache-hit (zero D1 calls), TTL expiry, invalidation-on-write, and per-repo isolation, and the diff coheres cleanly with its own description and issue #4508.

Nits — 7 non-blocking
  • review-memory-wire.ts: getCachedReviewSuppressions returns the cached array by reference rather than a copy — if any caller (e.g. applyReviewMemorySuppression) ever mutates the returned array in place, that mutation would corrupt the shared cache entry for subsequent renders within the TTL window.
  • processors.ts: concurrent calls to getCachedReviewSuppressions for the same repoFullName before the cache is populated (the '3 independent call sites' scenario the PR describes) can still stampede into multiple simultaneous D1 reads since there's no in-flight-promise dedup — acceptable given chunkCountCache has the same gap, but worth a one-line comment noting it's a known, accepted limitation rather than an oversight.
  • review-memory-store.test.ts: the TTL-expiry test only checks the exact-boundary-plus-1ms case (t0 + 60_001); consider also asserting the still-cached case at t0 + 59_999 to pin down the boundary in both directions, though the existing 30_000 test partially covers the interior.
  • Consider documenting in review-memory-wire.ts that getCachedReviewSuppressions returns a live reference (not a defensive copy), so future callers know not to mutate the result in place.
  • In processors.ts, a brief comment noting the accepted cache-stampede window on cold cache (matching chunkCountCache's known behavior) would preempt future 'why don't we dedup in-flight reads' questions.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
  • 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 ✅ Linked #4508
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 (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 48 registered-repo PR(s), 40 merged, 334 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 48 PR(s), 334 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Linked issue satisfaction

Addressed
The PR adds getCachedReviewSuppressions with a 60s in-isolate TTL cache keyed by repoFullName mirroring rag.ts's chunkCountCache, wires it into the call site at processors.ts, and invalidates the cache immediately after recordReviewSuppression succeeds in the resolve-command path so a fresh suppression is visible on the next render.

Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 48 PR(s), 334 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
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 the manual-review Gittensor contributor context label Jul 10, 2026
@JSONbored
JSONbored merged commit 6646b87 into main Jul 10, 2026
11 checks passed
@JSONbored
JSONbored deleted the fix/review-memory-suppression-cache-4508 branch July 10, 2026 07:20
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. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

fix(review): review-memory suppression list re-read from D1 on every comment render, no cache

1 participant