From 48b6f88e5ca8f3faf642b58d70e5ae70f2b5241d Mon Sep 17 00:00:00 2001 From: real-venus Date: Tue, 7 Jul 2026 09:51:59 -0700 Subject: [PATCH] feat(review): render the deterministic impact map in the unified review comment (#1971) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The impact-map feature (#2182-#2186) computes a deterministic RAG-derived impact map and splices it into the AI-review prompt, and buildImpactMapCollapsible (unified-comment-bridge) already knows how to render it — but no production caller ever passed the entries to buildUnifiedCommentBody, so the map was invisible to the maintainer/contributor reading the PR comment. Thread the entries this pass already computed out of runAiReviewForAdvisory (next to inlineFindings) and render them as the "Impact map" collapsible at the publish site — reusing the SAME array, no second RAG query. Present only on a fresh review (a cache hit / frozen reuse carries none); empty or feature-off ⇒ buildImpactMapCollapsible returns null ⇒ the comment is byte-identical. Closes #1971 --- src/queue/processors.ts | 20 ++++++-- test/unit/impact-map-processor-wiring.test.ts | 5 ++ test/unit/queue.test.ts | 46 +++++++++++++++++++ 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 798aeba04d..0f0f65b03a 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -449,7 +449,7 @@ import { } from "../review/rag-wire"; import { createReviewAdapters } from "../review/adapters"; import { extractChangedSymbols } from "../review/impact-symbols"; -import { computeImpactMap } from "../review/impact-map"; +import { computeImpactMap, type ImpactMapEntry } from "../review/impact-map"; import { formatImpactMapPromptSection, shouldComputeImpactMap } from "../review/impact-map-wire"; import { shouldEmitFixHandoff } from "../review/fix-handoff"; import { buildFixHandoffBlocks } from "../review/fix-handoff-render"; @@ -6951,6 +6951,10 @@ export async function runAiReviewForAdvisory( notes: string; reviewerCount: number; inlineFindings: InlineFinding[]; + // Deterministic impact-map entries this pass computed for the AI prompt (#1971), threaded out so the + // publish site can ALSO render them as the unified comment's "Impact map" collapsible. Empty when the + // feature is off (flag/manifest) — the render arm keys on `.length`, so off ⇒ no section. + impactMap?: ImpactMapEntry[] | undefined; findings: AdvisoryFinding[]; metadata?: Record | undefined; cacheable?: boolean | undefined; @@ -7119,6 +7123,9 @@ export async function runAiReviewForAdvisory( // undefined so the prompt is byte-identical to today. Fully fail-safe (computeImpactMap never throws; a // missing/cold RAG index degrades to an empty impact map, which formats to "" and appends nothing). let impactMapContext: string | undefined; + // The computed entries are ALSO threaded out of this function (#1971) so the publish site can render the + // "Impact map" collapsible from the exact same array — no second RAG query. Empty when the feature is off. + let impactMapEntries: ImpactMapEntry[] = []; if (shouldComputeImpactMap(env, args.reviewImpactMap === true)) { const [impactMapProject, impactMapRepo] = splitRepoForRag(args.repoFullName); const changedSymbols = extractChangedSymbols( @@ -7127,12 +7134,12 @@ export async function runAiReviewForAdvisory( patch: typeof file.payload?.patch === "string" ? file.payload.patch : undefined, })), ); - const impactMap = await computeImpactMap(changedSymbols, { + impactMapEntries = await computeImpactMap(changedSymbols, { infra: createReviewAdapters(env), project: impactMapProject, repo: impactMapRepo, }); - impactMapContext = formatImpactMapPromptSection(impactMap); + impactMapContext = formatImpactMapPromptSection(impactMapEntries); } // Repo quality-culture profile (#2995, flag-gated by GITTENSORY_REVIEW_CULTURE_PROFILE AND the per-repo // `review.culture_profile` opt-in). Derives a compact reference block from the repo's OWN merge history @@ -7329,6 +7336,7 @@ export async function runAiReviewForAdvisory( notes: result.advisoryNotes!, reviewerCount: result.reviewerCount, inlineFindings: result.inlineFindings, + impactMap: impactMapEntries, findings, metadata: metadataFor(result.advisoryNotes, result.inlineFindings), }; @@ -8272,6 +8280,7 @@ async function maybePublishPrPublicSurface( notes: string; reviewerCount: number; inlineFindings?: InlineFinding[]; + impactMap?: ImpactMapEntry[] | undefined; findings?: AdvisoryFinding[]; metadata?: Record | undefined; cacheable?: boolean | undefined; @@ -10175,6 +10184,11 @@ async function maybePublishPrPublicSurface( ...(findingCategoriesEnabledForReview && aiReview?.inlineFindings?.length ? { findingCategories: aiReview.inlineFindings } : {}), + // review.impact_map render (#1971): the deterministic impact-map entries this fresh pass already computed + // for the AI prompt ALSO render here as the "Impact map" collapsible — no second RAG query. A cache hit / + // frozen reuse / skipped review carries none (undefined ⇒ []); buildImpactMapCollapsible returns null for + // an empty list, so off/empty ⇒ no section ⇒ byte-identical. `ImpactMapEntry` IS `ImpactMapSummaryInput`. + impactMap: aiReview?.impactMap ?? [], // review.fixHandoff emission (#1962): the SAME fresh inline findings feed the fix-handoff blocks — // present ONLY on a cache-miss review with inline comments enabled — so a cache hit never re-emits them, // exactly like findingCategories above. Flag-OFF ⇒ omitted ⇒ the rendered comment is byte-identical. diff --git a/test/unit/impact-map-processor-wiring.test.ts b/test/unit/impact-map-processor-wiring.test.ts index 20f628d926..119f495ea0 100644 --- a/test/unit/impact-map-processor-wiring.test.ts +++ b/test/unit/impact-map-processor-wiring.test.ts @@ -113,6 +113,11 @@ describe("impact map wired into runAiReviewForAdvisory (#2186)", () => { expect(user).toContain("IMPACT MAP"); expect(user).toContain("src/review/impact-map.ts"); expect(user).toContain("src/review/caller.ts"); + // #1971: the SAME computed entries are threaded out of the review result so the publish site can render the + // "Impact map" collapsible from them — no second RAG query. + expect(result?.impactMap?.length).toBeGreaterThan(0); + expect(result?.impactMap?.[0]?.changedModule).toBe("src/review/impact-map.ts"); + expect(result?.impactMap?.[0]?.affectedModules).toContain("src/review/caller.ts"); }); it("FLAG-OFF (operator env unset): no impact-map computation, prompt has no IMPACT MAP section", async () => { diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 76b7d8bc59..2dd1c530ce 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -18256,6 +18256,52 @@ describe("queue processors", () => { expect(unifiedCommentBody).toContain("| Security | 1 |"); }); + // #1971: a FROZEN (manual-review) PR reuses its last published AI review, which carries no impact-map entries — + // the unified comment still renders, and the impact-map render arm degrades to no section (aiReview present but + // aiReview.impactMap undefined ⇒ `aiReview?.impactMap ?? []` ⇒ [] ⇒ buildImpactMapCollapsible null). + it("renders the unified comment WITHOUT an Impact map section when a frozen review is reused (no threaded entries)", async () => { + let aiCalls = 0; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + GITTENSORY_REVIEW_UNIFIED_COMMENT: "1", + AI: { run: async () => { aiCalls += 1; return { response: JSON.stringify({ assessment: "Fresh.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); + 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" }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 77, title: "Held PR", state: "open", user: { login: "contributor" }, head: { sha: "a77" }, labels: [{ name: "manual-review" }], body: "Closes #1" }); + await upsertPullRequestDetailSyncState(env, { repoFullName: "JSONbored/gittensory", pullNumber: 77, status: "complete", reviewsSyncedAt: new Date().toISOString() }); + // A prior PUBLISHED review for this exact head — the freeze path reuses it (aiReview = frozenReview) instead of + // spending a fresh AI call. Its cached shape has notes+reviewerCount but NO impactMap, so the render arm's + // nullish arm fires. + await putCachedAiReview(env, "JSONbored/gittensory", 77, "a77", "block", { notes: "Prior published review.", reviewerCount: 1 }); + await markAiReviewPublished(env, "JSONbored/gittensory", 77, "a77"); + let unifiedCommentBody = ""; + 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: "fake-installation-token" }); + if (url.includes("/pulls/77/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/77")) return Response.json({ number: 77, title: "Held PR", state: "open", user: { login: "contributor" }, head: { sha: "a77" }, labels: [{ name: "manual-review" }], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes("/commits/a77/check-runs")) return Response.json({ total_count: 0, check_runs: [] }); + if (url.includes("/commits/a77/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/77/comments") && method === "GET") return Response.json([]); + if (url.includes("/issues/77/comments")) { unifiedCommentBody = String((JSON.parse(String(init?.body ?? "{}")) as { body?: string }).body ?? unifiedCommentBody); return Response.json({ id: 1 }, { status: 201 }); } + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); + + await processJob(env, { type: "agent-regate-pr", deliveryId: "impact-map-frozen-reuse", repoFullName: "JSONbored/gittensory", prNumber: 77, installationId: 123 }); + + expect(aiCalls).toBe(0); // frozen ⇒ reused, no fresh AI + expect(unifiedCommentBody).toContain("gittensory-pr-panel"); // the unified panel rendered from the frozen review + expect(unifiedCommentBody).not.toContain("Impact map"); // ...with no impact-map section (reused review has none) + }); + // #1962: with BOTH the operator flag and the manifest opt-in on, the review emits a "Fix handoff" collapsible — // one machine-readable block per inline finding a contributor's own local agent can consume — in the unified // comment. Flag-OFF (every other review test) ⇒ no such section.