From 2e072d9e8cf4557098e1ce412b95194534a75a6f Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Tue, 28 Jul 2026 13:07:37 -0700 Subject: [PATCH] fix(sources): make the __all__ sentinel work in every resolution tier (#1712) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `__all__` sentinel had two spellings and only one worked: as a per-call source_id param, resolveRequestedScope understood it; as --source __all__ or GBRAIN_SOURCE=__all__, SOURCE_ID_RE (which forbids underscores) made all three resolver entry points throw. makeContext's blanket catch then silently fell back to sourceId 'default' — making the documented span-everything flag STRICTLY NARROWER than passing no flag at all, because the catch also discarded the #2561/#3242 federated widening: unqualified read (federated brain) -> {"sourceIds":["default","src-a","src-b"]} --source __all__ (pre-fix) -> {"sourceId":"default"} --source __all__ (post-fix) -> {} (spans the brain) Fix: - src/core/source-id.ts: export ALL_SOURCES = '__all__'. SOURCE_ID_RE itself is NOT loosened — it still guards source creation, lock ids, and path joins, and its underscore rejection is what makes the sentinel collision-free. - src/core/source-resolver.ts: the explicit and env tiers of resolveSourceId / resolveSourceIdEngineFree / resolveSourceWithTier pass the sentinel through verbatim (skipping the regex and assertSourceExists). Covers --source (#1712/#2289) and GBRAIN_SOURCE (#2140), local and thin-client alike. - src/core/operations.ts: sourceScopeOpts — the single choke point every read-side scope helper delegates to — translates ctx.sourceId === ALL_SOURCES into {} for trusted local callers (strictly remote === false) and keeps the unsatisfiable literal for remote/untrusted callers, so the sentinel can never widen past a caller's grant (fail-closed). A federated grant still wins over the sentinel. - src/cli.ts: makeContext's catch now rethrows when an explicit --source was passed — a source that genuinely fails to resolve errors loudly instead of silently becoming 'default' (the silent fallback is what turned three bug reports into debugging sessions). Tests (test/all-sources-sentinel.test.ts) fail on unmodified master (9/13, behaviorally) and pass with the fix; the 4 that pass on both sides pin invariants that must hold on both (remote fail-closed literal, grant precedence, invalid-id rejection). Closes #1712. #2289 and #2140 were closed as duplicates of it. Co-Authored-By: Claude Opus 5 --- src/cli.ts | 18 ++- src/core/operations.ts | 11 +- src/core/source-id.ts | 11 ++ src/core/source-resolver.ts | 26 ++++- test/all-sources-sentinel.test.ts | 176 ++++++++++++++++++++++++++++++ 5 files changed, 230 insertions(+), 12 deletions(-) create mode 100644 test/all-sources-sentinel.test.ts diff --git a/src/cli.ts b/src/cli.ts index 779f0c2c8..0124aa9b7 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -872,7 +872,8 @@ export function applyThinClientSourceScope( params.source_id = resolved; } -async function makeContext(engine: BrainEngine, params: Record): Promise { +// Exported for tests (same import-safety contract as applyThinClientSourceScope). +export async function makeContext(engine: BrainEngine, params: Record): Promise { // v0.31.8 (D11): resolve sourceId via the canonical 6-tier chain. Honors // --source / GBRAIN_SOURCE / .gbrain-source / path-match / brain default / // 'default'. Wrapped in try/catch so a doctor / single-source brain that @@ -884,16 +885,21 @@ async function makeContext(engine: BrainEngine, params: Record) // trusted local boundary) and consumed by federatedSearchScope in // operations.ts, which additionally gates on ctx.remote === false. let localFederated: string[] | undefined; + // params.source is set when a CLI flag was parsed for the op (rare; most + // CLI ops don't take --source). Falls through to env/dotfile/path-match. + const explicit = (params.source as string | undefined) ?? null; try { const { resolveSourceWithTier, localFederatedSourceIds } = await import('./core/source-resolver.ts'); - // params.source is set when a CLI flag was parsed for the op (rare; most - // CLI ops don't take --source). Falls through to env/dotfile/path-match. - const explicit = (params.source as string | undefined) ?? null; const resolved = await resolveSourceWithTier(engine, explicit); sourceId = resolved.source_id; localFederated = await localFederatedSourceIds(engine, resolved.source_id, resolved.tier); - } catch { - // Source resolution failed (e.g. sources table doesn't exist on a fresh + } catch (err) { + // #1712: an EXPLICIT --source that fails to resolve (invalid id, or a + // source that doesn't exist) must error loudly — the blanket swallow + // turned `--source __all__` and typos into a silent `default` scope, + // which is how three bug reports became debugging sessions. + if (explicit) throw err; + // Ambient resolution failed (e.g. sources table doesn't exist on a fresh // pre-init brain). Leave sourceId unset; engine read methods fall through // to the cross-source view (D16 back-compat path). sourceId = undefined; diff --git a/src/core/operations.ts b/src/core/operations.ts index d1ee25df6..1ef230daa 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -28,6 +28,7 @@ import { isSearchMode } from './search/mode.ts'; import { stampEvidence } from './search/evidence.ts'; import type { SearchResult } from './types.ts'; import { CJK_SLUG_CHARS, PAGE_SLUG_SEG } from './cjk.ts'; +import { ALL_SOURCES } from './source-id.ts'; import * as db from './db.ts'; import { VERSION } from '../version.ts'; import { @@ -486,6 +487,14 @@ export function sourceScopeOpts(ctx: OperationContext): { sourceId?: string; sou // value of `[]` MUST NOT widen scope to "all sources" by being interpreted // as "no filter." if (allowed && allowed.length > 0) return { sourceIds: allowed }; + // #1712: the __all__ sentinel spans the brain — but ONLY for trusted local + // callers (strictly `remote === false`). For remote/untrusted callers the + // literal stays as-is: it can never match a real source id (underscores are + // rejected at creation), so the read fail-closes to empty rather than + // widening past the caller's grant. Do NOT "simplify" this to `{}`. + if (ctx.sourceId === ALL_SOURCES) { + return ctx.remote === false ? {} : { sourceId: ctx.sourceId }; + } if (ctx.sourceId) return { sourceId: ctx.sourceId }; return {}; } @@ -553,7 +562,7 @@ export function resolveRequestedScope( sourceIdParam: string | undefined, allSourcesParam = false, ): { sourceId?: string; sourceIds?: string[] } { - const wantsAll = allSourcesParam || sourceIdParam === '__all__'; + const wantsAll = allSourcesParam || sourceIdParam === ALL_SOURCES; if (wantsAll) { return ctx.remote === false ? {} : sourceScopeOpts(ctx); } diff --git a/src/core/source-id.ts b/src/core/source-id.ts index d6da5e8d6..e9a1c414f 100644 --- a/src/core/source-id.ts +++ b/src/core/source-id.ts @@ -33,6 +33,17 @@ export const SOURCE_ID_RE = /^[a-z0-9](?:[a-z0-9-]{0,30}[a-z0-9])?$/; +/** + * Sentinel meaning "span every source" (#1712). Deliberately NOT a valid + * source id (underscores are rejected by SOURCE_ID_RE), so it can never + * collide with a real source, be created via `sources add`, or leak into + * lock ids / path joins. The resolver's explicit/env tiers pass it through + * verbatim; `sourceScopeOpts` translates it to an unscoped read for trusted + * local callers and keeps it as an unsatisfiable literal for remote callers + * (fail-closed). + */ +export const ALL_SOURCES = '__all__'; + /** Returns true if the string matches the canonical source_id regex. */ export function isValidSourceId(s: unknown): s is string { return typeof s === 'string' && SOURCE_ID_RE.test(s); diff --git a/src/core/source-resolver.ts b/src/core/source-resolver.ts index fc2587de0..f0da915e2 100644 --- a/src/core/source-resolver.ts +++ b/src/core/source-resolver.ts @@ -17,9 +17,13 @@ import { readFileSync, lstatSync, type Stats } from 'fs'; import { join, dirname, resolve } from 'path'; import type { BrainEngine } from './engine.ts'; import { isSourceFederated } from './sources-load.ts'; -import { SOURCE_ID_RE, isValidSourceId } from './source-id.ts'; +import { SOURCE_ID_RE, isValidSourceId, ALL_SOURCES } from './source-id.ts'; import { isTrustedDotfile, realpathOrResolve } from './path-confine.ts'; +// Re-export so scope-resolution call sites can import the sentinel from +// either module (#1712). +export { ALL_SOURCES }; + const DOTFILE = '.gbrain-source'; // Canonical SOURCE_ID_RE imported from `source-id.ts` (single source of truth). // Re-exported below as `__testing.SOURCE_ID_RE` for legacy test imports. @@ -83,8 +87,11 @@ export async function resolveSourceId( explicit: string | null | undefined, cwd: string = process.cwd(), ): Promise { - // 1. Explicit flag wins. + // 1. Explicit flag wins. The __all__ sentinel passes through verbatim + // (#1712) — it is not a source id, so it skips both the regex and + // assertSourceExists; sourceScopeOpts gives it span-everything semantics. if (explicit) { + if (explicit === ALL_SOURCES) return ALL_SOURCES; if (!SOURCE_ID_RE.test(explicit)) { throw new Error(`Invalid --source value "${explicit}". Must match [a-z0-9-]{1,32}.`); } @@ -92,9 +99,10 @@ export async function resolveSourceId( return explicit; } - // 2. Env var. + // 2. Env var. Same __all__ pass-through (#2140). const env = process.env.GBRAIN_SOURCE; if (env && env.length > 0) { + if (env === ALL_SOURCES) return ALL_SOURCES; if (!SOURCE_ID_RE.test(env)) { throw new Error(`Invalid GBRAIN_SOURCE value "${env}". Must match [a-z0-9-]{1,32}.`); } @@ -173,6 +181,7 @@ export function resolveSourceIdEngineFree( cwd: string = process.cwd(), ): string | null { if (explicit) { + if (explicit === ALL_SOURCES) return ALL_SOURCES; // #1712 sentinel pass-through if (!SOURCE_ID_RE.test(explicit)) { throw new Error(`Invalid --source value "${explicit}". Must match [a-z0-9-]{1,32}.`); } @@ -180,6 +189,7 @@ export function resolveSourceIdEngineFree( } const env = process.env.GBRAIN_SOURCE; if (env && env.length > 0) { + if (env === ALL_SOURCES) return ALL_SOURCES; // #2140 sentinel pass-through if (!SOURCE_ID_RE.test(env)) { throw new Error(`Invalid GBRAIN_SOURCE value "${env}". Must match [a-z0-9-]{1,32}.`); } @@ -315,8 +325,11 @@ export async function resolveSourceWithTier( explicit: string | null | undefined, cwd: string = process.cwd(), ): Promise<{ source_id: string; tier: SourceTier; detail?: string }> { - // 1. Explicit flag wins. + // 1. Explicit flag wins. __all__ sentinel passes through verbatim (#1712). if (explicit) { + if (explicit === ALL_SOURCES) { + return { source_id: ALL_SOURCES, tier: 'flag', detail: `--source ${ALL_SOURCES} (spans all sources)` }; + } if (!SOURCE_ID_RE.test(explicit)) { throw new Error(`Invalid --source value "${explicit}". Must match [a-z0-9-]{1,32}.`); } @@ -324,9 +337,12 @@ export async function resolveSourceWithTier( return { source_id: explicit, tier: 'flag', detail: `--source ${explicit}` }; } - // 2. Env var. + // 2. Env var. Same __all__ pass-through (#2140). const env = process.env.GBRAIN_SOURCE; if (env && env.length > 0) { + if (env === ALL_SOURCES) { + return { source_id: ALL_SOURCES, tier: 'env', detail: `GBRAIN_SOURCE=${ALL_SOURCES} (spans all sources)` }; + } if (!SOURCE_ID_RE.test(env)) { throw new Error(`Invalid GBRAIN_SOURCE value "${env}". Must match [a-z0-9-]{1,32}.`); } diff --git a/test/all-sources-sentinel.test.ts b/test/all-sources-sentinel.test.ts new file mode 100644 index 000000000..5bd6fa91a --- /dev/null +++ b/test/all-sources-sentinel.test.ts @@ -0,0 +1,176 @@ +/** + * #1712 (dupes #2289, #2140) — the `__all__` sentinel must work in EVERY + * resolution tier, not just as a per-call `source_id` param. + * + * The bug: SOURCE_ID_RE forbids underscores, so `--source __all__` and + * `GBRAIN_SOURCE=__all__` threw in the resolver; the CLI's makeContext + * blanket-caught that and silently fell back to `sourceId: 'default'` — + * making the documented span-everything flag STRICTLY NARROWER than passing + * no flag at all (the catch also discarded the #2561/#3242 federated + * widening). Meanwhile sourceScopeOpts treated a ctx.sourceId of '__all__' + * as an unsatisfiable literal. + * + * Uses the literal '__all__' (not the ALL_SOURCES constant) so these tests + * load and run behaviorally against pre-fix trees. + */ +import { describe, test, expect } from 'bun:test'; +import { withEnv } from './helpers/with-env.ts'; +import { + resolveSourceId, + resolveSourceIdEngineFree, + resolveSourceWithTier, +} from '../src/core/source-resolver.ts'; +import { + sourceScopeOpts, + federatedSearchScope, + type OperationContext, +} from '../src/core/operations.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +// Stub engine: registered sources + no local_path rows + no default config. +function makeStub(registeredSources: string[]): BrainEngine { + return { + kind: 'pglite', + executeRaw: async (sql: string, params?: unknown[]): Promise => { + if (sql.includes('SELECT id FROM sources WHERE id = $1')) { + const target = params?.[0]; + return registeredSources.includes(target as string) + ? [{ id: target } as unknown as T] + : []; + } + return []; + }, + getConfig: async () => null, + } as unknown as BrainEngine; +} + +function ctxOf(overrides: Partial = {}): OperationContext { + return { + engine: {} as any, + config: {} as any, + logger: console as any, + dryRun: false, + remote: true, + sourceId: 'default', + ...overrides, + }; +} + +// ── Resolver tiers pass the sentinel through verbatim ────────────────── + +describe('source-resolver — __all__ sentinel pass-through', () => { + test('resolveSourceId: explicit --source __all__ resolves (no regex throw, no existence check)', async () => { + // '__all__' is deliberately NOT in the registered set — the sentinel + // must skip assertSourceExists (it is not a source id). + const id = await resolveSourceId(makeStub(['default']), '__all__', '/nonexistent'); + expect(id).toBe('__all__'); + }); + + test('resolveSourceId: GBRAIN_SOURCE=__all__ resolves (#2140)', async () => { + await withEnv({ GBRAIN_SOURCE: '__all__' }, async () => { + const id = await resolveSourceId(makeStub(['default']), null, '/nonexistent'); + expect(id).toBe('__all__'); + }); + }); + + test('resolveSourceIdEngineFree: explicit + env __all__ (thin-client path)', async () => { + expect(resolveSourceIdEngineFree('__all__', '/nonexistent')).toBe('__all__'); + await withEnv({ GBRAIN_SOURCE: '__all__' }, () => { + expect(resolveSourceIdEngineFree(null, '/nonexistent')).toBe('__all__'); + }); + }); + + test('resolveSourceWithTier: flag and env tiers carry the sentinel', async () => { + const flag = await resolveSourceWithTier(makeStub(['default']), '__all__', '/nonexistent'); + expect(flag).toMatchObject({ source_id: '__all__', tier: 'flag' }); + await withEnv({ GBRAIN_SOURCE: '__all__' }, async () => { + const env = await resolveSourceWithTier(makeStub(['default']), null, '/nonexistent'); + expect(env).toMatchObject({ source_id: '__all__', tier: 'env' }); + }); + }); + + test('a genuinely invalid --source still throws (SOURCE_ID_RE not loosened)', async () => { + await expect(resolveSourceId(makeStub(['default']), 'my_source', '/nonexistent')) + .rejects.toThrow(/Invalid --source/); + expect(() => resolveSourceIdEngineFree('my_source', '/nonexistent')) + .toThrow(/Invalid --source/); + }); +}); + +// ── sourceScopeOpts — the single read-scope choke point ───────────────── + +describe('sourceScopeOpts — __all__ sentinel', () => { + test('trusted local (remote === false): spans the whole brain (empty scope)', () => { + expect(sourceScopeOpts(ctxOf({ remote: false, sourceId: '__all__' }))).toEqual({}); + }); + + test('remote: keeps the unsatisfiable literal — fail-closed, never widens', () => { + expect(sourceScopeOpts(ctxOf({ remote: true, sourceId: '__all__' }))) + .toEqual({ sourceId: '__all__' }); + }); + + test('anything not strictly remote === false is untrusted (fail-closed)', () => { + // undefined / missing remote must behave like remote, per the trust rule. + const ctx = ctxOf({ sourceId: '__all__' }); + (ctx as any).remote = undefined; + expect(sourceScopeOpts(ctx)).toEqual({ sourceId: '__all__' }); + }); + + test('a federated grant always wins over the sentinel', () => { + const ctx = ctxOf({ + remote: true, + sourceId: '__all__', + auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a', 'b'] } as any, + }); + expect(sourceScopeOpts(ctx)).toEqual({ sourceIds: ['a', 'b'] }); + }); +}); + +// ── Never narrower than passing no flag (#2561 regression shape) ──────── + +describe('__all__ is never narrower than an unqualified read', () => { + test('local __all__ spans the brain even when federated widening exists', () => { + // Unqualified read on a federated brain widens to the federated array… + const unqualified = ctxOf({ + remote: false, + sourceId: 'default', + localFederatedSourceIds: ['default', 'src-a', 'src-b'], + }); + expect(federatedSearchScope(unqualified)).toEqual({ + sourceIds: ['default', 'src-a', 'src-b'], + }); + // …and __all__ must be a superset of that: the whole brain ({}). + const all = ctxOf({ remote: false, sourceId: '__all__' }); + expect(federatedSearchScope(all)).toEqual({}); + }); +}); + +// ── makeContext — explicit --source failures error loudly ─────────────── + +describe('cli makeContext — no silent default fallback for explicit --source', () => { + test('--source __all__ produces ctx.sourceId __all__ (was: silent default)', async () => { + const { makeContext } = await import('../src/cli.ts'); + const ctx = await makeContext(makeStub(['default']), { source: '__all__' }); + expect(ctx.sourceId).toBe('__all__'); + expect(ctx.remote).toBe(false); + }); + + test('an explicit --source that fails to resolve throws instead of becoming default', async () => { + const { makeContext } = await import('../src/cli.ts'); + await expect(makeContext(makeStub(['default']), { source: 'ghost' })) + .rejects.toThrow(/not found/); + await expect(makeContext(makeStub(['default']), { source: 'my_source' })) + .rejects.toThrow(/Invalid --source/); + }); + + test('ambient resolution failure still falls back silently (pre-init brains)', async () => { + const { makeContext } = await import('../src/cli.ts'); + const broken = { + kind: 'pglite', + executeRaw: async () => { throw new Error('relation "sources" does not exist'); }, + getConfig: async () => { throw new Error('relation "config" does not exist'); }, + } as unknown as BrainEngine; + const ctx = await makeContext(broken, {}); + expect(ctx.sourceId).toBe('default'); + }); +});