From 26573e559036afb2f68e7dbef323f286c523be5a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 6 Jul 2026 17:20:58 -0700 Subject: [PATCH] feat(review): bypass the AI-review cache when impact-map is active impact-map computation queries the same live vector index RAG does (computeImpactMap issues its own retrieveContextWithMetrics calls), so it can go stale for the same head SHA without dynamicReviewFeatures' existing booleans flipping, exactly like grounding/rag/enrichment/reputation/ cultureProfile. Adds impactMap to dynamicReviewFeatures and dynamicReviewContextActive, and to AiReviewCacheInput's features shape (bumping the cache-input version so no prior fingerprint is silently reused under the new payload shape). This was implemented once already in the PR that landed the deterministic impact-map foundation, then reverted there because a safe webhook-level test was disproportionate to that PR's scope. Adds that test here. --- src/queue/processors.ts | 7 +- src/review/ai-review-cache-input.ts | 13 +++- test/unit/ai-review-cache-input.test.ts | 1 + test/unit/ai-review-cache.test.ts | 1 + test/unit/queue.test.ts | 99 ++++++++++++++++++++++--- 5 files changed, 106 insertions(+), 15 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 8fc8791851..495b84dbb6 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -8727,13 +8727,18 @@ async function maybePublishPrPublicSurface( // so a repo with it active also bypasses the AI-review result cache rather than fingerprinting a // value that can't prove freshness. cultureProfile: isRepoCultureProfileEnabled(env) && reviewCultureProfile === true, + // Impact map (#2182-#2186): queries the SAME live vector index RAG does (computeImpactMap issues + // its own retrieveContextWithMetrics calls), so it can go stale for the SAME head SHA exactly like + // RAG — a repo with it active also bypasses the AI-review result cache. + impactMap: shouldComputeImpactMap(env, reviewImpactMap === true), }; const dynamicReviewContextActive = dynamicReviewFeatures.grounding || dynamicReviewFeatures.rag || dynamicReviewFeatures.enrichment || dynamicReviewFeatures.reputation || - dynamicReviewFeatures.cultureProfile; + dynamicReviewFeatures.cultureProfile || + dynamicReviewFeatures.impactMap; const inputFingerprint = await aiReviewCacheInputFingerprint({ title: pr.title, mode: settings.aiReviewMode, diff --git a/src/review/ai-review-cache-input.ts b/src/review/ai-review-cache-input.ts index d794f64efb..026db7f4b2 100644 --- a/src/review/ai-review-cache-input.ts +++ b/src/review/ai-review-cache-input.ts @@ -5,10 +5,11 @@ import type { } from "../signals/focus-manifest"; import { sha256Hex } from "../utils/crypto"; -// Bumped v1→v2 (#2995): `features` gained a `cultureProfile` member. Every prior cached review's fingerprint was -// computed without that key, so bumping the version guarantees a clean cache miss on the first review after -// upgrade rather than silently reusing a hash computed under a different payload shape. -export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v2"; +// Bumped v1→v2 (#2995): `features` gained a `cultureProfile` member. Bumped v2→v3 (#2182-#2186): `features` +// gained an `impactMap` member. Every prior cached review's fingerprint was computed without that key, so +// bumping the version guarantees a clean cache miss on the first review after upgrade rather than silently +// reusing a hash computed under a different payload shape. +export const AI_REVIEW_CACHE_INPUT_VERSION = "ai-review-input:v3"; // #regate-churn (root cause, confirmed in production): this fingerprint USED to also hash the PR's live // `baseSha`, on the theory that a rebase/retarget can change the diff GitHub reports for an otherwise-unchanged @@ -111,6 +112,9 @@ export type AiReviewCacheInput = { // #2995: added alongside the repo quality-culture profile. Explicitly enumerated below (not passed through // raw) so a FUTURE new feature key can't silently change every existing cache entry's fingerprint again. cultureProfile: boolean; + // #2182-#2186: impact-map computation queries the same live vector index RAG does (computeImpactMap issues + // its own retrieveContextWithMetrics calls), so it can go stale for an unchanged head SHA exactly like RAG. + impactMap: boolean; }; }; @@ -197,6 +201,7 @@ export async function aiReviewCacheInputFingerprint(input: AiReviewCacheInput): enrichment: input.features.enrichment, reputation: input.features.reputation, cultureProfile: input.features.cultureProfile, + impactMap: input.features.impactMap, }, }; return `${AI_REVIEW_CACHE_INPUT_VERSION}:${await sha256Hex(stableStringify(payload))}`; diff --git a/test/unit/ai-review-cache-input.test.ts b/test/unit/ai-review-cache-input.test.ts index 853a8be991..a8ebaabfb3 100644 --- a/test/unit/ai-review-cache-input.test.ts +++ b/test/unit/ai-review-cache-input.test.ts @@ -35,6 +35,7 @@ const baseInput = (): AiReviewCacheInput => ({ enrichment: false, reputation: false, cultureProfile: false, + impactMap: false, }, }); diff --git a/test/unit/ai-review-cache.test.ts b/test/unit/ai-review-cache.test.ts index 80b8c31c92..4bc19981cf 100644 --- a/test/unit/ai-review-cache.test.ts +++ b/test/unit/ai-review-cache.test.ts @@ -34,6 +34,7 @@ const baseFingerprintInput = (): AiReviewCacheInput => ({ enrichment: false, reputation: false, cultureProfile: false, + impactMap: false, }, }); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 3704ab9759..0808bcb28f 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3531,7 +3531,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4046,7 +4046,7 @@ describe("queue processors", () => { excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4119,7 +4119,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4176,7 +4176,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4232,7 +4232,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4285,7 +4285,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4334,7 +4334,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -4386,7 +4386,7 @@ describe("queue processors", () => { aiReviewCloseConfidence: undefined, aiReviewCombine: null, aiReviewOnMerge: null, aiReviewReviewers: null, gatePack: "oss-anti-slop", reviewerPlan: env.AI_REVIEW_PLAN, selfHostProviderConfig: null, selfHostAiModelOverride: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, reviewFiles: [{ path: "src/a.ts", status: "modified", patch: "@@\n+export const ok = true;", additions: 1, deletions: 0 }], profile: null, securityFocus: false, inlineComments: false, pathInstructions: [], pathGuidance: "", repoInstructions: null, excludePaths: [], pathFilters: [], changedPaths: ["src/a.ts"], - features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false }, + features: { grounding: false, rag: false, enrichment: false, reputation: false, cultureProfile: false, impactMap: false }, }), }, }); @@ -5316,6 +5316,7 @@ describe("queue processors", () => { enrichment: false, reputation: false, cultureProfile: false, + impactMap: false, }, }); await putCachedAiReview(env, "owner/agent-repo", 7, "a7", "block", { @@ -5725,6 +5726,84 @@ describe("queue processors", () => { expect(aiCalls).toBeGreaterThan(0); }); + it("marks a cached AI review non-durable (cacheable=0) when the impact-map feature is on, even with grounding/rag/enrichment/reputation all off (#2182-#2186)", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { + run: async () => { + aiCalls += 1; + return { response: JSON.stringify({ assessment: "Looks fine.", blockers: [], nits: [], suggestions: [] }) }; + }, + } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + // Both gates on: the global capability switch, and (like culture-profile above, unlike + // grounding/enrichment/RAG/reputation which are env-only) the per-repo `.gittensory.yml` opt-in mocked + // below, so `dynamicReviewFeatures.impactMap` (src/queue/processors.ts) actually evaluates + // shouldComputeImpactMap's `&&` right-hand side true, not just short-circuits. + GITTENSORY_REVIEW_IMPACT_MAP: "true", + }); + await persistRegistrySnapshot( + env, + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + commentMode: "all_prs", + publicSurface: "comment_only", + autoLabelEnabled: false, + checkRunMode: "off", + gateCheckMode: "enabled", + aiReviewMode: "block", + gatePack: "oss-anti-slop", + }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/7/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/7")) return Response.json({ number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }); + if (url.includes("/commits/a7/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a7/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes("/issues/7/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/7/comments") && method === "POST") return Response.json({ id: 1 }, { status: 201 }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + // The repo's own review.impact_map opt-in. + if (url === "https://raw.githubusercontent.com/JSONbored/gittensory/HEAD/.gittensory.yml") { + return new Response("review:\n impact_map: true\n"); + } + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "impact-map-non-durable", + eventName: "pull_request", + payload: { + action: "opened", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" } }, + repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, + pull_request: { number: 7, title: "Clean PR", state: "open", user: { login: "contributor" }, head: { sha: "a7" }, labels: [], body: "Closes #1" }, + }, + }); + + expect(aiCalls).toBeGreaterThan(0); + const cached = await env.DB.prepare("select cacheable from ai_review_cache where repo_full_name = ? and pull_number = ? and head_sha = ?") + .bind("JSONbored/gittensory", 7, "a7") + .first<{ cacheable: number }>(); + // Never durably cacheable on its own merits, even though grounding/rag/enrichment/reputation are all off in + // this env -- impact-map alone is enough to trip dynamicReviewContextActive. + expect(cached?.cacheable).toBe(0); + }); + it("reuses a dynamic-context (grounding) AI review indefinitely once published, even long past the old cooldown window (#2119, #regate-churn)", async () => { // Grounding/RAG/enrichment/reputation each pull TIME-VARYING external context (live CI checks, the vector // index, REES/CVE data, reputation) that can change for the SAME head SHA without the feature flags @@ -14709,10 +14788,10 @@ describe("queue processors", () => { const usage = await env.DB.prepare("select feature, status from ai_usage_events where feature = ?").bind("ai_review_pr").first<{ feature: string; status: string }>(); expect(usage).toMatchObject({ feature: "ai_review_pr", status: "ok" }); expect(cacheReadSpy).toHaveBeenCalled(); - expect(cacheReadSpy.mock.calls[0]?.[5]).toMatch(/^ai-review-input:v2:/); + expect(cacheReadSpy.mock.calls[0]?.[5]).toMatch(/^ai-review-input:v3:/); expect(cacheWriteSpy).toHaveBeenCalled(); expect(cacheWriteSpy.mock.calls[0]?.[5]).toMatchObject({ - metadata: { inputFingerprint: expect.stringMatching(/^ai-review-input:v2:/) }, + metadata: { inputFingerprint: expect.stringMatching(/^ai-review-input:v3:/) }, }); cacheReadSpy.mockRestore(); cacheWriteSpy.mockRestore();