Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/review/rag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ export async function retrieveContextWithMetrics(
const p = (m.metadata?.path as string) ?? "";
return p && !exclude.has(p) && (typeof m.score !== "number" || m.score >= configuredMinScore);
});
const texts = matches.length > 0 ? await readChunkTexts(storage, matches.map((m) => m.id)) : new Map<string, string>();
const texts = matches.length > 0 ? await readChunkTexts(storage, opts.project, opts.repo, matches.map((m) => m.id)) : new Map<string, string>();
let chunks = matches
.map((m) => ({
// the `path ?? ""` leg is unreachable — surviving matches already passed the filter's `p && …` so metadata.path is a truthy string here
Expand Down Expand Up @@ -609,13 +609,13 @@ export function formatRetrievedContext(chunks: Array<{ path: string; text: strin
return lines.join("\n");
}

export async function readChunkTexts(storage: StorageAdapter, ids: string[]): Promise<Map<string, string>> {
export async function readChunkTexts(storage: StorageAdapter, project: string, repo: string, ids: string[]): Promise<Map<string, string>> {
const map = new Map<string, string>();
if (ids.length === 0) return map;
try {
const placeholders = ids.map(() => "?").join(",");
const rows = await storage.prepare(`SELECT id, text FROM repo_chunks WHERE id IN (${placeholders})`)
.bind(...ids)
const rows = await storage.prepare(`SELECT id, text FROM repo_chunks WHERE project = ? AND repo = ? AND id IN (${placeholders})`)
.bind(project, repo, ...ids)
.all<{ id: string; text: string }>();
for (const r of rows.results ?? []) map.set(r.id, r.text);
} catch (error) {
Expand Down
48 changes: 45 additions & 3 deletions test/unit/rag.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,48 @@ describe("rag: fail-safe (never throws; degrades to no context)", () => {
expect(queryEmbedBatch).toBe(1); // a single query string, regardless of the configured batch size
});

it("retrieveContextWithMetrics drops vector matches whose chunk rows belong to a colliding repo namespace", async () => {
const requested = { project: "acme", repo: `${"r".repeat(59)}-public` };
const victim = { project: "acme", repo: `${"r".repeat(59)}-private` };
expect(ragNamespace(requested.project, requested.repo)).toBe(ragNamespace(victim.project, victim.repo));

const matches = [{ id: "victim::0", score: 0.95, metadata: { path: "internal/security/secret-runbook.md" } }];
const vector = { query: async () => ({ matches }) } as unknown as VectorAdapter;
const storage = {
prepare: (query: string) => ({
bind: (...values: unknown[]) => ({
first: async () => ({ n: 1 }),
all: async () => ({
results:
query.includes("AND id IN") && values[0] === victim.project && values[1] === victim.repo
? [{ id: "victim::0", text: "private incident response runbook" }]
: [],
}),
run: async () => undefined,
}),
}),
batch: async () => undefined,
} as unknown as StorageAdapter;

const out = await retrieveContextWithMetrics(
{ storage, vector, inference: ai1024 },
{
project: requested.project,
repo: requested.repo,
queryText: "incident response runbook token rotation coverage",
reranker: "off",
},
);

expect(out.context).toBe("");
expect(out.metrics).toMatchObject({
candidates: 1,
kept: 0,
paths: [],
injectedChars: 0,
});
});

it("retrieveContextWithMetrics reports candidates, injected chars, and unique retrieved paths", async () => {
const matches = [
{ id: "src/a.ts::0", score: 0.9, metadata: { path: "src/a.ts" } },
Expand Down Expand Up @@ -701,7 +743,7 @@ describe("rag: storage/inference catch paths return their fail-safe defaults", (
it("readChunkTexts returns an empty Map when the storage read throws", async () => {
const errSpy = vi.spyOn(console, "error").mockImplementation(() => {});
const storage = { prepare: () => { throw new Error("d1 down"); }, batch: async () => undefined } as unknown as StorageAdapter;
const map = await readChunkTexts(storage, ["id-1"]);
const map = await readChunkTexts(storage, "p", "o/r", ["id-1"]);
expect(map.size).toBe(0);
// #3894: previously a no-level console.log, invisible to Sentry.
const parsed = errSpy.mock.calls.map((c) => JSON.parse(c[0] as string));
Expand All @@ -710,7 +752,7 @@ describe("rag: storage/inference catch paths return their fail-safe defaults", (
});

it("readChunkTexts short-circuits on an empty id list", async () => {
expect((await readChunkTexts(storageStub(), [])).size).toBe(0);
expect((await readChunkTexts(storageStub(), "p", "o/r", [])).size).toBe(0);
});

it("readChunkTexts yields an empty Map when the SELECT returns no `results` key (the `rows.results ?? []` fallback)", async () => {
Expand All @@ -719,7 +761,7 @@ describe("rag: storage/inference catch paths return their fail-safe defaults", (
prepare: () => ({ bind: () => ({ all: async () => ({}) }) }),
batch: async () => undefined,
} as unknown as StorageAdapter;
const map = await readChunkTexts(storage, ["id-1", "id-2"]);
const map = await readChunkTexts(storage, "p", "o/r", ["id-1", "id-2"]);
expect(map.size).toBe(0);
});
});
Expand Down