Skip to content

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

Description

@JSONbored

Part of #4496. P11 — low severity (bounded, indexed D1 read).

Context

applyReviewMemorySuppression itself is pure (no I/O, confirmed in review-memory-wire.ts), but its caller re-reads listReviewSuppressions from D1 on every single unified-comment render pass whenever commentGate.warnings.length > 0 (src/queue/processors.ts:10814-10820):

if (reviewMemoryEnabledForReview && commentGate.warnings.length > 0) {
  const suppressionSignals = await listReviewSuppressions(env, repoFullName);
  const { findings: suppressedWarnings, ... } = applyReviewMemorySuppression(commentGate.warnings, suppressionSignals);
  ...
}

This sits at the unified-comment publish step, AFTER the ai_review cache-hit/miss/frozen/paused branches have all already resolved aiReview — it fires on every one of those branches independently, not gated by aiReviewWasReused or any cache marker. listReviewSuppressions (src/db/repositories.ts:5289-5297) is a fresh, bounded (limit = 500) D1 SELECT with no cache layer. There are 3 independent call sites of maybePublishPrPublicSurface (processors.ts:3479, 6117, 12306, corresponding to auto re-review, webhook-triggered review, and manual panel retrigger) that can each independently invoke this code for the same repo/PR with no shared cache between them.

This is a real but low-cost/low-impact gap — a bounded, indexed D1 read, not an LLM/embedding call, and the feature is off by default.

Requirements

  • A short in-isolate TTL cache keyed by repoFullName, mirroring rag.ts's chunkCountCache pattern, so repeated webhook deliveries for the same PR within a short window reuse the same suppression set instead of re-reading D1 each time.
  • Ensure the cache correctly invalidates when a NEW suppression is recorded (e.g. via the @gittensory resolve command) — a maintainer's fresh suppression must take effect on the very next render, not be masked by a stale cached set.
  • Invariant + regression tests (non-negotiable): an invariant test asserting repeated renders within the cache TTL for the same repo make zero additional listReviewSuppressions D1 reads; a regression test confirming a freshly-recorded suppression (written between two render passes) IS reflected in the very next render, not suppressed by a stale cache entry.

Deliverables

  • Short-TTL in-isolate cache keyed by repoFullName for suppression list reads
  • Cache correctly invalidates on a fresh suppression write
  • Invariant test: repeated renders within TTL = zero extra D1 reads
  • Regression test: fresh suppression write is reflected immediately, not masked by stale cache

Expected outcome

Repeated unified-comment renders for the same repo within a short window reuse the same suppression set instead of re-reading D1 from scratch each time, without risking a maintainer's fresh suppression being masked by a stale cache entry.

References

  • src/queue/processors.ts:10814-10820 (the re-read site)
  • src/db/repositories.ts:5289-5297 (listReviewSuppressions)
  • src/review/review-memory-wire.ts:42-63 (applyReviewMemorySuppression, pure)
  • src/review/rag.ts:439 (chunkCountCache, the precedent short-TTL pattern to mirror)
  • src/queue/processors.ts:3479, 6117, 12306 (the 3 independent call sites of maybePublishPrPublicSurface)

Effort

XS

Metadata

Metadata

Assignees

Labels

gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.maintainer-onlyOwner-only work — yields no Gittensor points.orbGittensory Orb related - maintainer self-hosting analytics.

Projects

Status
Done

Relationships

None yet

Development

No branches or pull requests

Issue actions