From 29b4e5a5055ea1f4ff310e2e00e9cc5d97c23b3f Mon Sep 17 00:00:00 2001
From: Jeff <158072326+jeffrey701@users.noreply.github.com>
Date: Wed, 15 Jul 2026 13:31:52 -0400
Subject: [PATCH] fix(review): hash a JSON payload for the impact-map query
 cache key to prevent collisions

---
 src/review/impact-map.ts     | 16 ++++++++++++++--
 test/unit/impact-map.test.ts | 32 +++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/src/review/impact-map.ts b/src/review/impact-map.ts
index 1322aa1341..f4fda2c94b 100644
--- a/src/review/impact-map.ts
+++ b/src/review/impact-map.ts
@@ -73,14 +73,26 @@ function impactMapQueryCacheCutoffIso(): string {
  *  are constants for this module's own calls, but are still hashed (not assumed) so this function stays
  *  correct if a future caller ever varies them. excludePaths is sorted before hashing so argument order never
  *  causes a spurious cache miss. */
-async function impactMapQueryFingerprint(input: {
+export async function impactMapQueryFingerprint(input: {
   queryText: string;
   excludePaths: string[];
   topK: number;
   minScore: number;
   reranker: string;
 }): Promise<string> {
-  const payload = [input.queryText, [...input.excludePaths].sort().join(","), String(input.topK), String(input.minScore), input.reranker].join("|");
+  // Structurally-delimited payload (mirrors linked-issue-satisfaction-cache-input.ts / ai-slop-cache-input.ts):
+  // a bare "|"-join let an unescaped "|" inside queryText -- or the "," that separates excludePaths -- shift a
+  // field boundary, so two genuinely different inputs could serialize identically and collide on one
+  // fingerprint (e.g. {queryText:"a|b", excludePaths:["c"]} and {queryText:"a", excludePaths:["b|c"]} both
+  // became "a|b|c|..."). A JSON payload escapes every field, so distinct inputs always produce distinct
+  // fingerprints. excludePaths stays sorted so argument order never causes a spurious cache miss.
+  const payload = JSON.stringify({
+    queryText: input.queryText,
+    excludePaths: [...input.excludePaths].sort(),
+    topK: input.topK,
+    minScore: input.minScore,
+    reranker: input.reranker,
+  });
   return sha256Hex(payload);
 }
 
diff --git a/test/unit/impact-map.test.ts b/test/unit/impact-map.test.ts
index 00b01ea7dd..b69dfb8701 100644
--- a/test/unit/impact-map.test.ts
+++ b/test/unit/impact-map.test.ts
@@ -1,5 +1,5 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
-import { computeImpactMap, MAX_AFFECTED_MODULES_PER_ENTRY, MAX_IMPACT_MAP_INPUT_FILES } from "../../src/review/impact-map";
+import { computeImpactMap, impactMapQueryFingerprint, MAX_AFFECTED_MODULES_PER_ENTRY, MAX_IMPACT_MAP_INPUT_FILES } from "../../src/review/impact-map";
 import * as repositoriesModule from "../../src/db/repositories";
 import { renderMetrics, resetMetrics } from "../../src/selfhost/metrics";
 import type { FileChangedSymbols } from "../../src/review/impact-symbols";
@@ -518,3 +518,33 @@ describe("computeImpactMap", () => {
     });
   });
 });
+
+describe("impactMapQueryFingerprint", () => {
+  const base = { topK: 5, minScore: 0.4, reranker: "bm25" };
+
+  it("does not collide two distinct inputs across the field delimiters", async () => {
+    // Under the old "|"-join, {queryText:"a|b", excludePaths:["c"]} and {queryText:"a", excludePaths:["b|c"]}
+    // both serialized to "a|b|c|5|0.4|bm25" -- a cache-key collision that would replay one query's cached
+    // result for a genuinely different query. A "," inside an excludePath collided the same way.
+    const a = await impactMapQueryFingerprint({ queryText: "a|b", excludePaths: ["c"], ...base });
+    const b = await impactMapQueryFingerprint({ queryText: "a", excludePaths: ["b|c"], ...base });
+    const c = await impactMapQueryFingerprint({ queryText: "a", excludePaths: ["x", "y"], ...base });
+    const d = await impactMapQueryFingerprint({ queryText: "a", excludePaths: ["x,y"], ...base });
+    expect(a).not.toBe(b);
+    expect(c).not.toBe(d);
+  });
+
+  it("is independent of excludePaths order (no spurious cache miss)", async () => {
+    const forward = await impactMapQueryFingerprint({ queryText: "q", excludePaths: ["a", "b"], ...base });
+    const reversed = await impactMapQueryFingerprint({ queryText: "q", excludePaths: ["b", "a"], ...base });
+    expect(forward).toBe(reversed);
+  });
+
+  it("changes when any retrieval parameter changes", async () => {
+    const ref = await impactMapQueryFingerprint({ queryText: "q", excludePaths: [], ...base });
+    expect(await impactMapQueryFingerprint({ queryText: "q2", excludePaths: [], ...base })).not.toBe(ref);
+    expect(await impactMapQueryFingerprint({ queryText: "q", excludePaths: [], ...base, topK: 6 })).not.toBe(ref);
+    expect(await impactMapQueryFingerprint({ queryText: "q", excludePaths: [], ...base, minScore: 0.5 })).not.toBe(ref);
+    expect(await impactMapQueryFingerprint({ queryText: "q", excludePaths: [], ...base, reranker: "none" })).not.toBe(ref);
+  });
+});