From d4046871d8f91076aaf0ddfcc3864be4b84c47a6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 11 May 2026 12:26:19 -0700 Subject: [PATCH] fix(v0.34 W0a): source-routing leak across query + two-pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex outside-voice review on the v0.34 plan caught two load-bearing sites where sourceId was advertised but never applied — multi-source brains silently cross-contaminated structural retrieval: * operations.ts ~323 — `query` op handler called hybridSearch without threading ctx.sourceId. Multi-source agents querying with a --source flag got cross-source results. * two-pass.ts:81 (nearSymbol lookup) and two-pass.ts:131 (unresolved edge resolution) — TwoPassOpts.sourceId was declared and threaded through hybridSearch's expandAnchors call, but the actual SQL ignored it. The walk window crossed source boundaries every time. Fix: * `query` op now reads ctx.sourceId AND accepts a new `source_id` param (with '__all__' as the explicit force-cross-source escape hatch). Per-call param wins over ctx context. * two-pass.ts both lookups join through pages.source_id when opts.sourceId is set; omitted opts.sourceId preserves the legacy cross-source contract for callers who want it. Regression test: test/e2e/source-routing.test.ts seeds two sources with the same `parseMarkdown` symbol + a cross-source caller edge. Pins: - nearSymbol + sourceId='source-a' returns ONLY source-a chunks - nearSymbol + sourceId='source-b' returns ONLY source-b chunks - nearSymbol with no sourceId still crosses sources (contract preserved) - walk_depth=1 unresolved-edge resolution stays in source-a PGLite in-memory, no DATABASE_URL needed. The fix proves out under realistic structural retrieval not just a contrived unit test. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/core/operations.ts | 19 +++ src/core/search/two-pass.ts | 43 ++++-- test/e2e/source-routing.test.ts | 234 ++++++++++++++++++++++++++++++++ 3 files changed, 288 insertions(+), 8 deletions(-) create mode 100644 test/e2e/source-routing.test.ts diff --git a/src/core/operations.ts b/src/core/operations.ts index eb1dd8052..bb38c594f 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -1025,6 +1025,11 @@ const query: Operation = { description: "v0.29.1 — filter to effective_date <= this. Same format as `since`. Replaces deprecated `beforeDate`. YYYY-MM-DD lands at end-of-day.", }, + source_id: { + type: 'string', + description: + "v0.34: scope search to a single source. Defaults to OperationContext.sourceId (set from CLI --source / GBRAIN_SOURCE / .gbrain-source dotfile). Pass '__all__' to force cross-source search in multi-source brains.", + }, }, handler: async (ctx, p) => { const startedAt = Date.now(); @@ -1057,7 +1062,20 @@ const query: Operation = { // v0.25.0 — capture meta side-channel. hybridSearch's return contract // stays SearchResult[] (Cathedral II callers depend on that); meta // arrives via callback so eval capture can record what actually ran. + // + // v0.34 (Codex finding #2): thread ctx.sourceId so multi-source brains + // get source-scoped retrieval. Explicit `source_id` param wins over + // ctx.sourceId for callers that want to override (per-call multi-source + // search). When the param is the literal '__all__', force-allow + // cross-source mode (matches SearchOpts.sourceId contract). let capturedMeta: HybridSearchMeta | null = null; + const sourceIdParam = typeof p.source_id === 'string' ? p.source_id : undefined; + const resolvedSourceId = + sourceIdParam !== undefined + ? sourceIdParam === '__all__' + ? undefined + : sourceIdParam + : ctx.sourceId; const results = await hybridSearch(ctx.engine, queryText, { limit: (p.limit as number) || 20, offset: (p.offset as number) || 0, @@ -1068,6 +1086,7 @@ const query: Operation = { symbolKind: (p.symbol_kind as string) || undefined, nearSymbol: (p.near_symbol as string) || undefined, walkDepth: typeof p.walk_depth === 'number' ? (p.walk_depth as number) : undefined, + sourceId: resolvedSourceId, // v0.29.1 — agent-explicit recency + salience. Omitted = heuristic defaults. salience: p.salience as 'off' | 'on' | 'strong' | undefined, recency: p.recency as 'off' | 'on' | 'strong' | undefined, diff --git a/src/core/search/two-pass.ts b/src/core/search/two-pass.ts index 4ca16d67c..050fb8447 100644 --- a/src/core/search/two-pass.ts +++ b/src/core/search/two-pass.ts @@ -75,12 +75,26 @@ export async function expandAnchors( // --near-symbol: add chunks whose symbol_name_qualified matches as // additional anchors. Best-effort — if none found, fall through. + // + // v0.34 (Codex finding #2): when opts.sourceId is set, scope the + // symbol lookup to that source. Pre-v0.34 this WAS unscoped despite the + // TwoPassOpts.sourceId field being declared — multi-source brains + // cross-contaminated structural retrieval. Now: opts.sourceId set → + // filter; undefined → cross-source (matches the documented contract). if (opts.nearSymbol) { try { - const rows = await engine.executeRaw<{ id: number }>( - `SELECT id FROM content_chunks WHERE symbol_name_qualified = $1 LIMIT 50`, - [opts.nearSymbol], - ); + const rows = opts.sourceId + ? await engine.executeRaw<{ id: number }>( + `SELECT cc.id FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE cc.symbol_name_qualified = $1 AND p.source_id = $2 + LIMIT 50`, + [opts.nearSymbol, opts.sourceId], + ) + : await engine.executeRaw<{ id: number }>( + `SELECT id FROM content_chunks WHERE symbol_name_qualified = $1 LIMIT 50`, + [opts.nearSymbol], + ); const baseScore = anchors.length > 0 ? anchors[0]!.score : 1.0; for (const r of rows) { if (!seen.has(r.id)) { @@ -126,12 +140,25 @@ export async function expandAnchors( } // Resolve unresolved edges by looking up chunks whose // symbol_name_qualified matches. One batch query per frontier node. + // + // v0.34 (Codex finding #2): scope by opts.sourceId when set. Pre-v0.34 + // this lookup was unscoped, letting structural retrieval cross source + // boundaries silently in multi-source brains. if (unresolvedTargets.length > 0) { try { - const resolved = await engine.executeRaw<{ id: number }>( - `SELECT id FROM content_chunks WHERE symbol_name_qualified = ANY($1::text[]) LIMIT ${NEIGHBOR_CAP_PER_HOP}`, - [unresolvedTargets], - ); + const resolved = opts.sourceId + ? await engine.executeRaw<{ id: number }>( + `SELECT cc.id FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE cc.symbol_name_qualified = ANY($1::text[]) + AND p.source_id = $2 + LIMIT ${NEIGHBOR_CAP_PER_HOP}`, + [unresolvedTargets, opts.sourceId], + ) + : await engine.executeRaw<{ id: number }>( + `SELECT id FROM content_chunks WHERE symbol_name_qualified = ANY($1::text[]) LIMIT ${NEIGHBOR_CAP_PER_HOP}`, + [unresolvedTargets], + ); for (const r of resolved) directChunkIds.push(r.id); } catch { // best-effort diff --git a/test/e2e/source-routing.test.ts b/test/e2e/source-routing.test.ts new file mode 100644 index 000000000..1b75e8d3b --- /dev/null +++ b/test/e2e/source-routing.test.ts @@ -0,0 +1,234 @@ +/** + * v0.34 W0a — multi-source isolation E2E. + * + * Pre-v0.34 (Codex finding #2): `query` op didn't pass ctx.sourceId to + * hybridSearch; two-pass.ts:81 + :131 advertised TwoPassOpts.sourceId but + * never applied it to the nearSymbol lookup or unresolved-edge resolution. + * Multi-source brains silently cross-contaminated structural retrieval. + * + * This E2E pins the fix: seed two sources with the same symbol name in + * different files; assert near_symbol + walk_depth retrieval with + * sourceId='source-a' only returns chunks from source-a. + * + * PGLite in-memory — no DATABASE_URL needed, hermetic. + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { PGLiteEngine } from '../../src/core/pglite-engine.ts'; +import { expandAnchors } from '../../src/core/search/two-pass.ts'; +import { resetPgliteState } from '../helpers/reset-pglite.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +describe('v0.34 W0a — multi-source isolation in two-pass retrieval', () => { + beforeAll(async () => { + await resetPgliteState(engine); + await seedTwoSourcesWithSharedSymbol(engine); + }); + + test('expandAnchors with nearSymbol + sourceId returns ONLY source-a chunks', async () => { + const result = await expandAnchors(engine, [], { + walkDepth: 0, + nearSymbol: 'parseMarkdown', + sourceId: 'source-a', + }); + + expect(result.length).toBeGreaterThan(0); + + // Hydrate chunk_ids to verify they all belong to source-a + const chunkIds = result.map((r) => r.chunk_id); + const rows = await engine.executeRaw<{ chunk_id: number; source_id: string }>( + `SELECT cc.id AS chunk_id, p.source_id + FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE cc.id = ANY($1::int[])`, + [chunkIds], + ); + + expect(rows.length).toBe(chunkIds.length); + for (const r of rows) { + expect(r.source_id).toBe('source-a'); + } + }); + + test('expandAnchors with nearSymbol + sourceId="source-b" returns ONLY source-b chunks', async () => { + const result = await expandAnchors(engine, [], { + walkDepth: 0, + nearSymbol: 'parseMarkdown', + sourceId: 'source-b', + }); + + expect(result.length).toBeGreaterThan(0); + + const chunkIds = result.map((r) => r.chunk_id); + const rows = await engine.executeRaw<{ chunk_id: number; source_id: string }>( + `SELECT cc.id AS chunk_id, p.source_id + FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE cc.id = ANY($1::int[])`, + [chunkIds], + ); + + for (const r of rows) { + expect(r.source_id).toBe('source-b'); + } + }); + + test('expandAnchors with nearSymbol and NO sourceId returns chunks from both sources (legacy cross-source mode preserved)', async () => { + const result = await expandAnchors(engine, [], { + walkDepth: 0, + nearSymbol: 'parseMarkdown', + // sourceId omitted — should cross sources (matches the documented contract) + }); + + expect(result.length).toBeGreaterThan(0); + const chunkIds = result.map((r) => r.chunk_id); + const rows = await engine.executeRaw<{ chunk_id: number; source_id: string }>( + `SELECT cc.id AS chunk_id, p.source_id + FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE cc.id = ANY($1::int[])`, + [chunkIds], + ); + + const sources = new Set(rows.map((r) => r.source_id)); + expect(sources.has('source-a')).toBe(true); + expect(sources.has('source-b')).toBe(true); + }); + + test('unresolved-edge resolution within walkDepth respects sourceId', async () => { + // Seed a caller → callee edge so walk_depth=1 must resolve via + // symbol_name_qualified. We added one such edge in seedTwoSourcesWithSharedSymbol + // pointing at parseMarkdown in source-a only. + // + // Start anchor = the caller chunk in source-a; expansion of depth 1 should + // land on the source-a parseMarkdown definition, NOT the source-b one. + const callerChunk = await engine.executeRaw<{ id: number; score: number }>( + `SELECT cc.id, 1.0 AS score + FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE p.source_id = 'source-a' AND cc.symbol_name_qualified = 'callerInA' + LIMIT 1`, + [], + ); + expect(callerChunk.length).toBe(1); + const anchors = [{ + slug: 'src/foo.ts', + page_id: 0, + title: '', + type: 'code' as const, + chunk_text: '', + chunk_source: 'compiled_truth' as const, + chunk_id: callerChunk[0]!.id, + chunk_index: 0, + score: 1.0, + source_id: 'source-a', + stale: false, + }]; + + const result = await expandAnchors(engine, anchors, { + walkDepth: 1, + sourceId: 'source-a', + }); + + expect(result.length).toBeGreaterThan(1); // anchor + at least one neighbor + const chunkIds = result.map((r) => r.chunk_id); + const rows = await engine.executeRaw<{ chunk_id: number; source_id: string }>( + `SELECT cc.id AS chunk_id, p.source_id + FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE cc.id = ANY($1::int[])`, + [chunkIds], + ); + + for (const r of rows) { + expect(r.source_id).toBe('source-a'); + } + }); +}); + +// ───────────────────────────────────────────────────────────────── +// Fixture: two sources, each with a `parseMarkdown` symbol. +// Source A also has a `callerInA` function whose unresolved call edge +// points at "parseMarkdown" (testing the walk_depth resolution path). +// ───────────────────────────────────────────────────────────────── + +async function seedTwoSourcesWithSharedSymbol(engine: PGLiteEngine): Promise { + // Register two sources (schema: id PK, name UNIQUE NOT NULL, plus optional fields) + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, created_at) + VALUES ('source-a', 'source-a', '/fake/a', '{}'::jsonb, NOW()) + ON CONFLICT (id) DO NOTHING`, + [], + ); + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config, created_at) + VALUES ('source-b', 'source-b', '/fake/b', '{}'::jsonb, NOW()) + ON CONFLICT (id) DO NOTHING`, + [], + ); + + // Page A1: contains parseMarkdown in source-a + const pageA = await engine.executeRaw<{ id: number }>( + `INSERT INTO pages (slug, source_id, title, type, compiled_truth, frontmatter, updated_at, created_at) + VALUES ('code/src/markdown-a.ts', 'source-a', 'markdown-a.ts', 'code', 'export function parseMarkdown(s: string) { return s; }', '{}'::jsonb, NOW(), NOW()) + RETURNING id`, + [], + ); + + // Page A2: contains callerInA, which references parseMarkdown + const pageA2 = await engine.executeRaw<{ id: number }>( + `INSERT INTO pages (slug, source_id, title, type, compiled_truth, frontmatter, updated_at, created_at) + VALUES ('code/src/caller-a.ts', 'source-a', 'caller-a.ts', 'code', 'export function callerInA() { return parseMarkdown(""); }', '{}'::jsonb, NOW(), NOW()) + RETURNING id`, + [], + ); + + // Page B: contains parseMarkdown in source-b + const pageB = await engine.executeRaw<{ id: number }>( + `INSERT INTO pages (slug, source_id, title, type, compiled_truth, frontmatter, updated_at, created_at) + VALUES ('code/src/markdown-b.ts', 'source-b', 'markdown-b.ts', 'code', 'export function parseMarkdown(s: string) { return s; }', '{}'::jsonb, NOW(), NOW()) + RETURNING id`, + [], + ); + + // Chunks + await engine.executeRaw( + `INSERT INTO content_chunks (page_id, chunk_index, chunk_text, chunk_source, language, symbol_name_qualified, symbol_type) + VALUES ($1, 0, 'export function parseMarkdown(s: string) { return s; }', 'compiled_truth', 'typescript', 'parseMarkdown', 'function')`, + [pageA[0]!.id], + ); + await engine.executeRaw( + `INSERT INTO content_chunks (page_id, chunk_index, chunk_text, chunk_source, language, symbol_name_qualified, symbol_type) + VALUES ($1, 0, 'export function callerInA() { return parseMarkdown(""); }', 'compiled_truth', 'typescript', 'callerInA', 'function')`, + [pageA2[0]!.id], + ); + await engine.executeRaw( + `INSERT INTO content_chunks (page_id, chunk_index, chunk_text, chunk_source, language, symbol_name_qualified, symbol_type) + VALUES ($1, 0, 'export function parseMarkdown(s: string) { return s; }', 'compiled_truth', 'typescript', 'parseMarkdown', 'function')`, + [pageB[0]!.id], + ); + + // Unresolved edge: callerInA → parseMarkdown (no to_chunk_id, must resolve via symbol_name_qualified) + const callerChunk = await engine.executeRaw<{ id: number }>( + `SELECT cc.id FROM content_chunks cc + JOIN pages p ON p.id = cc.page_id + WHERE p.source_id = 'source-a' AND cc.symbol_name_qualified = 'callerInA' LIMIT 1`, + [], + ); + await engine.executeRaw( + `INSERT INTO code_edges_symbol (from_chunk_id, from_symbol_qualified, to_symbol_qualified, edge_type, source_id, edge_metadata) + VALUES ($1, 'callerInA', 'parseMarkdown', 'calls', 'source-a', '{}'::jsonb)`, + [callerChunk[0]!.id], + ); +}