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

src/queue/processors.ts

@@ -73,7 +73,6 @@ import {
   getGateBlockOutcome,
   hasActiveReviewForHeadSha,
   isDbFrozenForRepo,
-  listReviewSuppressions,
   markGateOutcomeOverridden,
   markPullRequestLinkedIssueHardRuleViolated,
   startActiveReviewTracking,
@@ -503,7 +502,7 @@ import {
   buildRepoCultureProfileContext,
   isRepoCultureProfileEnabled,
 } from "../review/repo-culture-profile-wire";
-import { applyReviewMemorySuppression, shouldApplyReviewMemory } from "../review/review-memory-wire";
+import { applyReviewMemorySuppression, getCachedReviewSuppressions, invalidateReviewSuppressionCache, shouldApplyReviewMemory } from "../review/review-memory-wire";
 import {
   buildReviewEnrichment,
   isEnrichmentEnabled,
@@ -11048,7 +11047,10 @@ async function maybePublishPrPublicSurface(
       let renderedGate = commentGate;
       if (reviewMemoryEnabledForReview && commentGate.warnings.length > 0) {
         try {
-          const suppressionSignals = await listReviewSuppressions(env, repoFullName);
+          // #4508: cached (short in-isolate TTL, invalidated on write) — the 3 independent
+          // maybePublishPrPublicSurface call sites (auto re-review, webhook-triggered review, manual panel
+          // retrigger) no longer each force a fresh D1 read for the same repo within a short window.
+          const suppressionSignals = await getCachedReviewSuppressions(env, repoFullName, Date.now());
           const { findings: suppressedWarnings, suppressedCount, demotedCount } = applyReviewMemorySuppression(
             commentGate.warnings,
             suppressionSignals,
@@ -11649,7 +11651,7 @@ async function maybeProcessResolveCommand(env: Env, deliveryId: string, payload:
   const reviewManifest = await loadRepoFocusManifest(env, req.repoFullName).catch(() => null);
   const reviewMemoryEnabled = shouldApplyReviewMemory(env, resolveReviewMemoryManifestToggle(reviewManifest));
   let recordedSuppressionCount = 0;
-  if (reviewMemoryEnabled && selection.findings.length > 0) { const { fingerprint } = await import("../review/review-memory-match"); const { recordReviewSuppression } = await import("../db/repositories"); const suppressionWrites = selection.findings.map((finding) => ({ category: finding.code, pathGlob: "", patternHash: fingerprint({ category: finding.code, message: `${finding.title} ${finding.detail}` }) })); await Promise.all(suppressionWrites.map((write) => recordReviewSuppression(env, { repoFullName: req.repoFullName, category: write.category, pathGlob: write.pathGlob, patternHash: write.patternHash, createdBy: req.actor }))); recordedSuppressionCount = suppressionWrites.length; await recordAuditEvent(env, { eventType: "github_app.review_memory_recorded", actor: req.actor, targetKey, outcome: "completed", detail: `Recorded ${recordedSuppressionCount} review-memory suppression signal(s).`, metadata: { deliveryId, repoFullName: req.repoFullName, recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); await recordGithubProductUsage(env, "review_memory_recorded", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "completed", metadata: { recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); }
+  if (reviewMemoryEnabled && selection.findings.length > 0) { const { fingerprint } = await import("../review/review-memory-match"); const { recordReviewSuppression } = await import("../db/repositories"); const suppressionWrites = selection.findings.map((finding) => ({ category: finding.code, pathGlob: "", patternHash: fingerprint({ category: finding.code, message: `${finding.title} ${finding.detail}` }) })); await Promise.all(suppressionWrites.map((write) => recordReviewSuppression(env, { repoFullName: req.repoFullName, category: write.category, pathGlob: write.pathGlob, patternHash: write.patternHash, createdBy: req.actor }))); recordedSuppressionCount = suppressionWrites.length; invalidateReviewSuppressionCache(req.repoFullName); /* #4508: this repo's cached suppression list is stale as of this write -- the very next render must see it, not wait out the TTL. */ await recordAuditEvent(env, { eventType: "github_app.review_memory_recorded", actor: req.actor, targetKey, outcome: "completed", detail: `Recorded ${recordedSuppressionCount} review-memory suppression signal(s).`, metadata: { deliveryId, repoFullName: req.repoFullName, recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); await recordGithubProductUsage(env, "review_memory_recorded", { actor: req.actor, repoFullName: req.repoFullName, targetKey, outcome: "completed", metadata: { recordedSuppressionCount, scope: findingRef.scope, ...(findingRef.scope === "single" ? { findingCode: findingRef.findingCode } : {}) } }); }
   const resolvedLabel = findingRef.scope === "whole_pr" ? "all current advisory findings" : `\`${findingRef.findingCode}\``;
   const confirmation = sanitizePublicComment([AGENT_COMMAND_COMMENT_MARKER, "", "> [!NOTE]", `> **Review finding resolved by @${req.actor}**`, `> Marked ${resolvedLabel} as resolved for this PR. The Gate check-run is unchanged.`, ...(recordedSuppressionCount > 0 ? ["", `Recorded ${recordedSuppressionCount} review-memory suppression signal(s) for future reviews.`] : []), "", "---", gittensoryFooter()].join("\n"));
   await createOrUpdateAgentCommandComment(env, req.installationId, req.repoFullName, req.pr.number, confirmation, mode);

src/review/review-memory-wire.ts

@@ -6,6 +6,7 @@
 // review path at all (the caller guards on this flag before doing any D1 read or matching), so the review
 // stays byte-identical to today.
 
+import { listReviewSuppressions } from "../db/repositories";
 import { matchSuppressions, type ReviewMemoryFindingInput } from "./review-memory-match";
 import type { AdvisoryFinding, ReviewSuppressionRecord } from "../types";
 
@@ -26,6 +27,42 @@ export function shouldApplyReviewMemory(
 ): boolean {
   return isReviewMemoryEnabled(env) && manifestReviewMemoryEnabled;
 }
+// Short in-isolate TTL cache for listReviewSuppressions (#4508), mirroring rag.ts's chunkCountCache: repeated
+// unified-comment renders for the same repo within a short window (the 3 independent maybePublishPrPublicSurface
+// call sites -- auto re-review, webhook-triggered review, manual panel retrigger -- can each fire this
+// independently) reuse the same suppression set instead of re-reading D1 each time. Unlike chunkCountCache's
+// "only cache the positive" (cold→hot is one-way), a suppression set can grow at any time via `@gittensory
+// resolve`, so this is explicitly invalidated on every write (invalidateReviewSuppressionCache below) rather than
+// relying on TTL expiry alone -- a maintainer's fresh suppression must take effect on the very next render, not
+// be masked by a stale cached set.
+const REVIEW_SUPPRESSION_CACHE_TTL_MS = 60_000;
+const reviewSuppressionCache = new Map<string, { signals: ReviewSuppressionRecord[]; at: number }>();
+
+/** Cached read of listReviewSuppressions, keyed by repoFullName. `nowMs` is threaded in by the caller (mirrors
+ *  rag.ts's hasIndexedChunks) rather than read internally, so a caller under fake timers gets a deterministic
+ *  cache decision. */
+export async function getCachedReviewSuppressions(env: Env, repoFullName: string, nowMs: number): Promise<ReviewSuppressionRecord[]> {
+  const hit = reviewSuppressionCache.get(repoFullName);
+  if (hit && nowMs - hit.at < REVIEW_SUPPRESSION_CACHE_TTL_MS) return hit.signals;
+  const signals = await listReviewSuppressions(env, repoFullName);
+  reviewSuppressionCache.set(repoFullName, { signals, at: nowMs });
+  return signals;
+}
+
+/** Evict repoFullName's cached suppression set immediately. Called after recordReviewSuppression so the very
+ *  next render sees the fresh write, instead of waiting out the TTL. */
+export function invalidateReviewSuppressionCache(repoFullName: string): void {
+  reviewSuppressionCache.delete(repoFullName);
+}
+
+/** Test-only: clears every cached entry, mirroring clearInstallationTokenCacheForTest/
+ *  clearGitHubResponseCacheForTest. Without this, a test suite running many cases against the SAME repoFullName
+ *  under fake timers (a fixed `Date.now()` reset per test) would otherwise see one test's cached read leak into
+ *  the next. */
+export function clearReviewSuppressionCacheForTest(): void {
+  reviewSuppressionCache.clear();
+}
+
 const RESOLVE_FINDING_CODE = /^[a-z][a-z0-9_]{0,199}$/;
 export function normalizeResolveFindingRef(raw: string | null | undefined): { ok: true; scope: "whole_pr" } | { ok: true; scope: "single"; findingCode: string } | { ok: false; reason: "malformed_finding_id" } { const trimmed = (raw ?? "").trim(); if (trimmed.length === 0) return { ok: true, scope: "whole_pr" }; const normalized = trimmed.toLowerCase().replace(/^finding-/, ""); if (!RESOLVE_FINDING_CODE.test(normalized)) return { ok: false, reason: "malformed_finding_id" }; return { ok: true, scope: "single", findingCode: normalized }; }
 export function selectWarningsForResolve(warnings: ReadonlyArray<AdvisoryFinding>, ref: { ok: true; scope: "whole_pr" } | { ok: true; scope: "single"; findingCode: string }): { findings: AdvisoryFinding[]; reason?: "finding_not_found" } { if (ref.scope === "whole_pr") return { findings: [...warnings] }; const matches = warnings.filter((finding) => finding.code === ref.findingCode); if (matches.length === 0) return { findings: [], reason: "finding_not_found" }; return { findings: matches }; }

test/unit/queue.test.ts

@@ -1,6 +1,7 @@
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { generateKeyPairSync } from "node:crypto";
 import { clearInstallationTokenCacheForTest } from "../../src/github/app";
+import { clearReviewSuppressionCacheForTest } from "../../src/review/review-memory-wire";
 import { PR_PANEL_COMMENT_MARKER } from "../../src/github/comments";
 import * as backfillModule from "../../src/github/backfill";
 import * as rateLimitModule from "../../src/github/rate-limit";
@@ -106,6 +107,7 @@ describe("queue processors", () => {
   // stay deterministic regardless of when CI runs.
   beforeEach(() => {
     clearInstallationTokenCacheForTest();
+    clearReviewSuppressionCacheForTest();
     vi.mocked(fetchPullRequestFreshness).mockReset();
     vi.mocked(fetchPullRequestFreshness).mockImplementation(async (_env, args) => ({
       status: "current",

test/unit/review-memory-store.test.ts

@@ -1,5 +1,7 @@
 import { describe, expect, it, vi } from "vitest";
 import { MAX_REVIEW_SUPPRESSIONS_PER_REPO, listReviewSuppressions, recordReviewSuppression } from "../../src/db/repositories";
+import * as repositoriesModule from "../../src/db/repositories";
+import { clearReviewSuppressionCacheForTest, getCachedReviewSuppressions, invalidateReviewSuppressionCache } from "../../src/review/review-memory-wire";
 import { createTestEnv } from "../helpers/d1";
 
 // Review memory (#2178, data-model slice of #1964): insert/list repository accessors over the
@@ -205,3 +207,62 @@ describe("review-memory suppression store (#2178)", () => {
     expect(await rawCount(env, repoFullName)).toBe(MAX_REVIEW_SUPPRESSIONS_PER_REPO);
   });
 });
+
+// Short in-isolate TTL cache over listReviewSuppressions (#4508), mirroring rag.ts's chunkCountCache.
+describe("getCachedReviewSuppressions / invalidateReviewSuppressionCache (#4508)", () => {
+  it("INVARIANT: a repeated read within the TTL for the same repo makes ZERO additional D1 reads", async () => {
+    clearReviewSuppressionCacheForTest();
+    const env = createTestEnv();
+    await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-1" });
+    const t0 = 1_000_000;
+    const first = await getCachedReviewSuppressions(env, "owner/repo", t0);
+    expect(first).toHaveLength(1);
+
+    const spy = vi.spyOn(repositoriesModule, "listReviewSuppressions");
+    const second = await getCachedReviewSuppressions(env, "owner/repo", t0 + 30_000); // well within the 60s TTL
+    // Read the assertion BEFORE mockRestore() — mockRestore() also resets recorded calls.
+    expect(spy).not.toHaveBeenCalled(); // reused the cached set — no fresh listReviewSuppressions call
+    spy.mockRestore();
+
+    expect(second).toEqual(first);
+  });
+
+  it("REGRESSION: a fresh suppression recorded between two renders IS reflected in the very next render, not masked by a stale cache entry", async () => {
+    clearReviewSuppressionCacheForTest();
+    const env = createTestEnv();
+    const t0 = 2_000_000;
+    const before = await getCachedReviewSuppressions(env, "owner/live-repo", t0);
+    expect(before).toHaveLength(0); // cold cache, nothing recorded yet — this populates the cache with an empty set
+
+    // A maintainer runs `@gittensory resolve` between the two renders, well within the cache's TTL.
+    await recordReviewSuppression(env, { repoFullName: "owner/live-repo", category: "ai_review_split", patternHash: "hash-fresh" });
+    invalidateReviewSuppressionCache("owner/live-repo");
+
+    const after = await getCachedReviewSuppressions(env, "owner/live-repo", t0 + 5_000); // still within the 60s TTL
+    expect(after).toHaveLength(1); // the fresh write is visible — NOT masked by the stale empty cached set
+    expect(after[0]).toMatchObject({ patternHash: "hash-fresh" });
+  });
+
+  it("cache expires naturally past the TTL even without an explicit invalidation", async () => {
+    clearReviewSuppressionCacheForTest();
+    const env = createTestEnv();
+    const t0 = 3_000_000;
+    await getCachedReviewSuppressions(env, "owner/repo", t0); // populates the cache with an empty set
+
+    await recordReviewSuppression(env, { repoFullName: "owner/repo", category: "ai_review_split", patternHash: "hash-late" });
+    // No invalidateReviewSuppressionCache call here — relies on TTL expiry alone.
+    const afterTtl = await getCachedReviewSuppressions(env, "owner/repo", t0 + 60_001);
+    expect(afterTtl).toHaveLength(1);
+  });
+
+  it("caches independently per repoFullName", async () => {
+    clearReviewSuppressionCacheForTest();
+    const env = createTestEnv();
+    await recordReviewSuppression(env, { repoFullName: "owner/repo-a", category: "ai_review_split", patternHash: "hash-a" });
+    const t0 = 4_000_000;
+    const a = await getCachedReviewSuppressions(env, "owner/repo-a", t0);
+    const b = await getCachedReviewSuppressions(env, "owner/repo-b", t0);
+    expect(a).toHaveLength(1);
+    expect(b).toHaveLength(0);
+  });
+});