From ba1aedd100d0936cc97389945c4a27f7a5b5e37c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 28 Jul 2026 13:26:53 -0700 Subject: [PATCH] =?UTF-8?q?fix(sources):=20narrow=20#2928=20fix=20to=20the?= =?UTF-8?q?=20read=20path=20=E2=80=94=20keep=20tier=205.5=20write=20routin?= =?UTF-8?q?g?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI (test/sync-sole-non-default-routing.test.ts) caught that excluding explicitly-isolated sources from pickSoleNonDefaultSource redirected unqualified WRITES (sync/import) to 'default' — silent data misplacement, worse than the read bug. --no-federated governs read mixing, not write routing. Reverted tier 5.5 to master behavior. The fix now lives in localFederatedSourceIds only: an explicitly isolated resolved source (config.federated === false) is never widened into the federated set, so unqualified reads anchored on it stay scalar — its pages are not mixed with 'default' and vice versa. That helper feeds federatedSearchScope exclusively (reads); writes never touch it. Co-Authored-By: Claude Opus 5 --- src/core/source-resolver.ts | 44 ++++++++++------- test/unfederate-read-scope-2928.test.ts | 66 +++++++++++++++---------- 2 files changed, 66 insertions(+), 44 deletions(-) diff --git a/src/core/source-resolver.ts b/src/core/source-resolver.ts index 21ed63aa4..c9d6f4b14 100644 --- a/src/core/source-resolver.ts +++ b/src/core/source-resolver.ts @@ -200,32 +200,27 @@ export function resolveSourceIdEngineFree( * doesn't auto-resolve. Shared by `resolveSourceId` and `resolveSourceWithTier` * so the heuristic can't drift between the two entry points. * - * #2928: also refuses to auto-pick a source EXPLICITLY marked isolated - * (`gbrain sources unfederate ` / `--no-federated` → config.federated = - * false). Auto-routing there made an unqualified `query`/`search`/`think` - * surface the isolated source's pages — the exact thing unfederate promises - * to prevent ("only searched when explicitly named"). UNSET federated stays - * eligible: a fresh `sources add` writes no federated key, and the #1434 - * sole-source convenience must keep working for it. Narrowing-only by - * design — an isolated sole source falls through to 'default'; the tier - * never widens to some OTHER source it wouldn't have picked before. + * NOTE (#2928): this tier deliberately does NOT consult config.federated — + * `--no-federated` governs READ mixing, not write routing, and unqualified + * `sync`/`import` on a single-vault brain must keep landing in the vault + * (#1434, pinned by test/sync-sole-non-default-routing.test.ts). The + * unfederate read fix lives in `localFederatedSourceIds` below. */ async function pickSoleNonDefaultSource(engine: BrainEngine): Promise { // archived column was added in v34 (v0.26.5). Older brains may not have // it — fall back to the un-archived query in that case via try/catch. - let rows: Array<{ id: string; config: unknown }>; + let rows: Array<{ id: string }>; try { - rows = await engine.executeRaw<{ id: string; config: unknown }>( - `SELECT id, config FROM sources WHERE local_path IS NOT NULL AND id != 'default' AND archived = false`, + rows = await engine.executeRaw<{ id: string }>( + `SELECT id FROM sources WHERE local_path IS NOT NULL AND id != 'default' AND archived = false`, ); } catch { - rows = await engine.executeRaw<{ id: string; config: unknown }>( - `SELECT id, config FROM sources WHERE local_path IS NOT NULL AND id != 'default'`, + rows = await engine.executeRaw<{ id: string }>( + `SELECT id FROM sources WHERE local_path IS NOT NULL AND id != 'default'`, ); } - if (rows.length !== 1) return null; - if (parseSourceConfig(rows[0].config).federated === false) return null; - return rows[0].id; + if (rows.length === 1) return rows[0].id; + return null; } /** @@ -404,7 +399,10 @@ export async function resolveSourceWithTier( * * - explicit tiers (`flag` / `env` / `dotfile`): the user named a source; * scalar scope stands (that IS the qualified case); - * - no other federated source exists: keep the scalar fast path unchanged. + * - no other federated source exists: keep the scalar fast path unchanged; + * - #2928: the resolved source is explicitly isolated (config.federated = + * false): it must not be mixed into a cross-source read in EITHER + * direction, so the scalar scope stands. * * Archived sources are excluded (same rationale as pickSoleNonDefaultSource); * the archived column is v34+, so fall back to the un-archived query on older @@ -427,6 +425,16 @@ export async function localFederatedSourceIds( `SELECT id, config FROM sources ORDER BY id`, ); } + // #2928: an EXPLICITLY isolated anchor (`sources unfederate` / + // `--no-federated` → config.federated = false) opted out of cross-source + // read mixing — never widen it into the federated set (which would drag + // other sources' pages into its unqualified reads and vice versa). Scalar + // scope stands. UNSET federated keeps the pre-#2928 widening behavior; + // write routing (tier 5.5 above) is deliberately untouched. + const resolvedRow = rows.find((row) => row.id === sourceId); + if (resolvedRow && parseSourceConfig(resolvedRow.config).federated === false) { + return undefined; + } const ids = [ sourceId, ...rows diff --git a/test/unfederate-read-scope-2928.test.ts b/test/unfederate-read-scope-2928.test.ts index 06530e353..0823e04da 100644 --- a/test/unfederate-read-scope-2928.test.ts +++ b/test/unfederate-read-scope-2928.test.ts @@ -2,21 +2,24 @@ * #2928 — `gbrain sources unfederate ` (config.federated = false) must * keep the isolated source out of UNQUALIFIED reads. * - * Root cause is NOT the functions the issue points at (`sourceScopeOpts` / - * the dead `federatedOnly` loader flag): the read-widening path - * (`localFederatedSourceIds` → `federatedSearchScope`) already excludes - * non-federated sources. The intent is lost one step earlier, in source - * RESOLUTION: `pickSoleNonDefaultSource` (tier 5.5, #1434) auto-picks the - * single non-default source as the anchor of an unqualified call without - * consulting config.federated — so an explicitly isolated source becomes the - * scalar read scope and its pages surface in unqualified query/search/think. + * The leak: tier 5.5 (#1434) anchors an unqualified call on the sole + * non-default source — correct for writes and for the anchor itself — but + * `localFederatedSourceIds` then widened that anchor into the federated set + * (`[isolated-src, default]`), mixing the isolated source's pages with + * federated sources' pages in unqualified query/search/think, both + * directions. The fix is READ-ONLY: an explicitly isolated anchor + * (config.federated === false) is never widened; scalar scope stands. + * + * Deliberately unchanged (pinned below): + * - tier 5.5 write routing — `--no-federated` governs read mixing, not + * where unqualified sync/import land (#1434, + * test/sync-sole-non-default-routing.test.ts); + * - the unscoped-local invariant that sank #3470/#3497: an empty scope + * must stay UNSCOPED ({}), never collapse to 'default'. * * All imports here exist on master, so this file runs against an unmodified - * master checkout and fails BEHAVIORALLY there (resolution returns the - * isolated source). - * - * Also pins the unscoped-local invariant that sank #3470/#3497: an empty - * scope must stay UNSCOPED ({}), never collapse to 'default'. + * master checkout and fails BEHAVIORALLY there (the widened scope contains + * both sources). */ import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; import { mkdtempSync } from 'node:fs'; @@ -80,29 +83,36 @@ function ctxOf(overrides: Partial = {}): OperationContext { } as OperationContext; } -describe('#2928 — unfederated source and the sole_non_default tier', () => { - test('explicitly isolated sole source no longer anchors unqualified resolution', async () => { +describe('#2928 — isolated anchor is never widened into a cross-source read', () => { + test('explicitly isolated resolved source gets NO federated widening set', async () => { await addSource('isolated-src', { federated: false }); - const resolved = await resolveClean(null, outsideCwd); - // Master auto-picks the isolated source (tier sole_non_default), which - // makes an unqualified query/search return its pages — the exact thing - // `sources unfederate` promises to prevent. - expect(resolved.source_id).toBe('default'); - expect(resolved.tier).toBe('seed_default'); + // Master returns ['isolated-src', 'default'] here (the seeded default is + // federated), which mixes the isolated source's pages with default's in + // every unqualified query/search/think — the #2928 report. + const localFed = await localFederatedSourceIds(engine, 'isolated-src', 'sole_non_default'); + expect(localFed).toBeUndefined(); }); - test('full unqualified read chain excludes the isolated source from the scope', async () => { + test('full unqualified read chain stays scalar: no default pages mixed in', async () => { await addSource('isolated-src', { federated: false }); const resolved = await resolveClean(null, outsideCwd); + // Write/anchor routing is UNCHANGED: the sole vault still resolves (#1434). + expect(resolved.source_id).toBe('isolated-src'); + expect(resolved.tier).toBe('sole_non_default'); const localFed = await localFederatedSourceIds(engine, resolved.source_id, resolved.tier); const ctx = ctxOf({ sourceId: resolved.source_id, ...(localFed ? { localFederatedSourceIds: localFed } : {}), }); - const scope = federatedSearchScope(ctx); - expect(scope.sourceId).not.toBe('isolated-src'); - expect(scope.sourceIds ?? []).not.toContain('isolated-src'); - expect(scope).toEqual({ sourceId: 'default' }); + // Scalar scope: the isolated source is not mixed with 'default' (and + // vice versa). Master produces { sourceIds: ['isolated-src','default'] }. + expect(federatedSearchScope(ctx)).toEqual({ sourceId: 'isolated-src' }); + }); + + test('isolated brain_default anchor is not widened either (same seam)', async () => { + await addSource('isolated-src', { federated: false }); + const localFed = await localFederatedSourceIds(engine, 'isolated-src', 'brain_default'); + expect(localFed).toBeUndefined(); }); test('UNSET federated keeps the #1434 sole-source convenience (no over-narrowing)', async () => { @@ -110,6 +120,10 @@ describe('#2928 — unfederated source and the sole_non_default tier', () => { const resolved = await resolveClean(null, outsideCwd); expect(resolved.source_id).toBe('vault'); expect(resolved.tier).toBe('sole_non_default'); + // Only an EXPLICIT federated:false suppresses widening; unset keeps the + // pre-#2928 behavior (the seeded 'default' is federated). + const localFed = await localFederatedSourceIds(engine, 'vault', 'sole_non_default'); + expect(localFed).toEqual(['vault', 'default']); }); test('federated: true sole source still auto-resolves', async () => {