From a46f28a63e672cff177bafbc2b49509f447cb6d4 Mon Sep 17 00:00:00 2001 From: Time Attakc <89218912+time-attack@users.noreply.github.com> Date: Mon, 20 Jul 2026 10:33:27 -0700 Subject: [PATCH 01/17] =?UTF-8?q?fix(cli):=20keep=20doctor=20--json=20stdo?= =?UTF-8?q?ut=20clean=20=E2=80=94=20v123=20migration=20handler=20printed?= =?UTF-8?q?=20to=20stdout=20(#3019)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v123 configurable-FTS migration (#2941) logged its completion notice via console.log. Migrations run lazily inside any command's first DB connect, so on the nightly heavy run (fresh Postgres service DB) the line landed as the first line of `gbrain doctor --json` stdout and broke the fm_wallclock jq parse ("Invalid numeric literal at line 1, column 7", run 29731426470). runMigrations' contract routes all migration noise to stderr; move the v123 prints (and the pre-existing v2 slug-rename print) there. Also un-vacuous the fm_wallclock harness: its register-source step used `bun run -e` (bun dumps usage with exit 0 instead of running the code) and `connect({})` (in-memory), so the source was never registered and doctor scanned nothing. It now resolves the engine the way the CLI does and registers the source in the DB doctor actually reads. Regression test: test/migrate-stdout-clean.test.ts re-runs migrations from v122 asserting zero stdout writes, plus a source-level guard that migrate.ts contains no console.log. Co-authored-by: Garry Tan Co-authored-by: Claude Fable 5 --- src/core/migrate.ts | 12 +++- test/migrate-stdout-clean.test.ts | 68 +++++++++++++++++++++++ tests/heavy/frontmatter_scan_wallclock.sh | 19 +++++-- 3 files changed, 92 insertions(+), 7 deletions(-) create mode 100644 test/migrate-stdout-clean.test.ts diff --git a/src/core/migrate.ts b/src/core/migrate.ts index 7b95a5767..1e42a958f 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -135,7 +135,10 @@ export const MIGRATIONS: Migration[] = [ } } } - if (renamed > 0) console.log(` Renamed ${renamed} slugs`); + // Migration progress goes to stderr — stdout must stay clean for + // callers parsing JSON (e.g. `gbrain doctor --json | jq`); migrations + // can run lazily inside ANY command's first DB connect. + if (renamed > 0) process.stderr.write(` Renamed ${renamed} slugs\n`); }, }, { @@ -5572,7 +5575,10 @@ export const MIGRATIONS: Migration[] = [ await engine.executeRaw(recreateChunksFn); if (lang === 'english') { - console.log(` v123: trigger functions recreated with language='english' (default — no backfill needed)`); + // stderr, NOT stdout: migrations run lazily inside any command's + // first DB connect — a console.log here polluted `doctor --json` + // stdout and broke jq consumers (heavy-tests fm_wallclock). + process.stderr.write(` v123: trigger functions recreated with language='english' (default — no backfill needed)\n`); return; } @@ -5593,7 +5599,7 @@ export const MIGRATIONS: Migration[] = [ WHERE search_vector IS NOT NULL; `); - console.log(` v123: trigger functions recreated with language='${lang}' + backfilled existing rows`); + process.stderr.write(` v123: trigger functions recreated with language='${lang}' + backfilled existing rows\n`); }, }, ]; diff --git a/test/migrate-stdout-clean.test.ts b/test/migrate-stdout-clean.test.ts new file mode 100644 index 000000000..f33e194b0 --- /dev/null +++ b/test/migrate-stdout-clean.test.ts @@ -0,0 +1,68 @@ +/** + * Migrations must never write to stdout — regression for the heavy-tests + * fm_wallclock failure (run 29731426470). + * + * Migrations run lazily inside ANY command's first DB connect (initSchema → + * runMigrations), including JSON-emitting commands like `gbrain doctor --json`. + * The v123 FTS migration (#2941) printed its completion notice via + * `console.log`, which landed as the first line of `doctor --json` stdout and + * broke every jq consumer ("Invalid numeric literal at line 1, column 7"). + * runMigrations' own contract (see the comment above its progress writes) + * routes ALL migration noise to stderr. + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { runMigrations } from '../src/core/migrate.ts'; + +describe('migration output stays off stdout', () => { + let engine: PGLiteEngine; + + beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + }); + + afterAll(async () => { + await engine.disconnect(); + }); + + test('re-running pending migrations (v122 → latest) writes nothing to stdout', async () => { + // Rewind the version stamp so the v123 handler actually re-executes — + // the exact state a CI Postgres/older brain is in when doctor connects. + await engine.setConfig('version', '122'); + + const stdoutWrites: string[] = []; + const origWrite = process.stdout.write.bind(process.stdout); + const origLog = console.log; + process.stdout.write = ((chunk: unknown, ...rest: unknown[]) => { + stdoutWrites.push(String(chunk)); + return (origWrite as (...a: unknown[]) => boolean)(chunk, ...rest); + }) as typeof process.stdout.write; + console.log = (...args: unknown[]) => { stdoutWrites.push(args.map(String).join(' ')); }; + + try { + const res = await runMigrations(engine); + // Load-bearing: the migration must have actually run for the stdout + // assertion to prove anything. + expect(res.applied).toBeGreaterThanOrEqual(1); + } finally { + process.stdout.write = origWrite; + console.log = origLog; + } + + expect(stdoutWrites).toEqual([]); + }, 60000); + + test('migrate.ts contains no console.log (all migration noise goes to stderr)', () => { + const src = readFileSync(join(import.meta.dir, '../src/core/migrate.ts'), 'utf8'); + const offenders = src + .split('\n') + .map((line, i) => ({ line, n: i + 1 })) + .filter(({ line }) => line.includes('console.log(')); + expect(offenders).toEqual([]); + }); +}); diff --git a/tests/heavy/frontmatter_scan_wallclock.sh b/tests/heavy/frontmatter_scan_wallclock.sh index b3b952069..162e4d129 100755 --- a/tests/heavy/frontmatter_scan_wallclock.sh +++ b/tests/heavy/frontmatter_scan_wallclock.sh @@ -92,11 +92,22 @@ timeout 120s bun run src/cli.ts init --pglite --yes --no-embedding >> "$LOG" 2>& # Register the brain dir as a source. Use raw SQL since `gbrain sources add` # might not exist in this version-window; the schema is what doctor reads. +# NOTE: must be `bun -e`, not `bun run -e` — `bun run` treats -e as an +# unknown script name and dumps its usage/script listing with exit 0, so the +# INSERT silently never ran and doctor scanned zero sources (vacuous pass). +# Resolve the engine the same way the CLI does (config + env), so the source +# lands in the DB doctor actually reads (Postgres when CI sets DATABASE_URL, +# the PGLite brain otherwise) — a hardcoded `connect({})` is in-memory and +# the INSERT would vanish. echo "[fm_wallclock] register source..." | tee -a "$LOG" -bun run -e " -import { PGLiteEngine } from './src/core/pglite-engine.ts'; -const e = new PGLiteEngine(); -await e.connect({}); +bun -e " +import { loadConfig, toEngineConfig } from './src/core/config.ts'; +import { createEngine } from './src/core/engine-factory.ts'; +const cfg = loadConfig(); +if (!cfg) throw new Error('no gbrain config — init failed?'); +const engineConfig = toEngineConfig(cfg); +const e = await createEngine(engineConfig); +await e.connect(engineConfig); await e.initSchema(); await e.executeRaw( \"INSERT INTO sources (id, name, local_path) VALUES ('fm-wallclock', 'Frontmatter wallclock test', \\\$1)\", From 8b325041ee0089a50a55a9f730b73d46b3a8faff Mon Sep 17 00:00:00 2001 From: raymeboltd <77594828+raymeboltd@users.noreply.github.com> Date: Mon, 20 Jul 2026 19:46:46 +0200 Subject: [PATCH 02/17] v0.42.63.0 fix: preserve configured PGLite schema database path (#3016) * fix(schema): preserve configured PGLite database path * chore: bump version and changelog (v0.42.63.0) Co-Authored-By: OpenAI Codex --------- Co-authored-by: OpenAI Codex --- CHANGELOG.md | 23 +++++++ VERSION | 2 +- docs/architecture/KEY_FILES.md | 2 +- package.json | 2 +- src/commands/schema.ts | 10 +-- test/schema-cli-database-path.serial.test.ts | 67 ++++++++++++++++++++ test/schema-cli.test.ts | 8 ++- 7 files changed, 103 insertions(+), 11 deletions(-) create mode 100644 test/schema-cli-database-path.serial.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index cd26fce76..57b52cbc8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ All notable changes to GBrain will be documented in this file. +## [0.42.63.0] - 2026-07-20 + +**Schema commands now open the local brain you actually configured.** + +If your PGLite brain lives at a custom path, commands such as `gbrain schema stats` previously ignored that path and could inspect the default brain instead. That made a healthy configured brain look empty or report the wrong schema counts. Schema commands now use the same complete database configuration as the rest of GBrain. PostgreSQL behavior is unchanged, and no migration is required. + +### How to use it + +Upgrade, then run the schema command normally: + +```bash +gbrain upgrade +gbrain schema stats --json +``` + +The reported page and type counts now come from the `database_path` in `~/.gbrain/config.json` when the engine is PGLite. + +### Itemized changes + +#### Fixed +- **Schema CLI commands preserve configured PGLite paths.** Engine construction and connection now receive the canonical complete engine configuration, including both `database_path` and `database_url` where applicable. +- **CLI tests are isolated from ambient database URLs.** Schema subprocess tests explicitly clear inherited PostgreSQL URL variables, and a persistent-PGLite regression test proves `schema stats` reads the configured database rather than the default brain. + ## [0.42.62.0] - 2026-07-17 **If your brain holds more than one source, everything now lands in the right one. Link extraction, timeline extraction, background cycles, and webhook captures used to quietly file some of their output under the default source; all of those paths now carry the correct source identity. Background agent jobs got tougher too: a failed database reconnect can no longer wedge the engine, and workers recover from dropped connections instead of crash-looping. If you run the admin dashboard behind a reverse proxy, the live activity panel finally connects. Long agent conversations cost less because repeated context is reused between turns on Anthropic calls. Local LiteLLM proxies work out of the box. Nested sources scan correctly again instead of reporting zero files. And the project's automated checks now include dependency vulnerability scanning, static code-security analysis, and signed provenance for release builds. Thirty merged changes in all, the largest batch to date, each one reviewed and verified against the live codebase before landing.** diff --git a/VERSION b/VERSION index 92762a88b..4c2cace61 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.62.0 +0.42.63.0 diff --git a/docs/architecture/KEY_FILES.md b/docs/architecture/KEY_FILES.md index a153c2b43..c81458c2a 100644 --- a/docs/architecture/KEY_FILES.md +++ b/docs/architecture/KEY_FILES.md @@ -483,7 +483,7 @@ Key files (v0.40.7.0 additions): - `src/core/schema-pack/mutate.ts` — 8-step `withMutation` skeleton (bundled-guard → lock → read → mutator → validate → atomic write → audit → invalidate). 11 mutation primitives: `addTypeToPack`, `removeTypeFromPack` (with reference check), `updateTypeOnPack`, `addAliasToType`, `removeAliasFromType`, `addPrefixToType`, `removePrefixFromType`, `addLinkTypeToPack`, `removeLinkTypeFromPack`, `setExtractableOnType`, `setExpertRoutingOnType`. Atomic write via `.tmp + fsync + rename` — the pack file on disk is NEVER partial. Inline minimal JSON→YAML emitter so YAML packs stay YAML (does NOT preserve comments — pin pack.json if you care about layout). - `src/core/schema-pack/stats.ts` — `runStatsCore(engine, opts)` returns per-source + aggregate page counts + coverage % + `dead_prefixes` (declared prefixes with zero matching pages — agent drilldown signal). Multi-source aware (`sourceIds[]` federated, `sourceId` single, or whole-brain). PGLite + Postgres parity via `executeRaw`. Empty brain → coverage:1.0 (vacuous truth). - `src/core/schema-pack/sync.ts` — `runSyncCore(engine, opts)` chunked UPDATE in 1000-row batches per declared prefix. Concurrent writers never block on a single row >100ms. Write-side scoping via `ctx.sourceId` directly (NOT `sourceScopeOpts`, which inherits OAuth read federation). Idempotent on `--apply` re-run. -- `src/commands/schema.ts` extension — 14 CLI verbs in the dispatch table: `add-type`, `remove-type`, `update-type`, `add-alias`, `remove-alias`, `add-prefix`, `remove-prefix`, `add-link-type`, `remove-link-type`, `set-extractable`, `set-expert-routing`, `stats`, `sync`, `reload`. `withConnectedEngine` defensive fix retained. Lifecycle-grouped help text (Inspection / Activation / Authoring / Discovery+repair). +- `src/commands/schema.ts` extension — 14 CLI verbs in the dispatch table: `add-type`, `remove-type`, `update-type`, `add-alias`, `remove-alias`, `add-prefix`, `remove-prefix`, `add-link-type`, `remove-link-type`, `set-extractable`, `set-expert-routing`, `stats`, `sync`, `reload`. `withConnectedEngine` routes `loadConfig()` through the canonical `toEngineConfig()` helper and passes the complete result (`database_url` and `database_path`) to factory construction and connect, so PGLite schema commands open the configured brain. Lifecycle-grouped help text (Inspection / Activation / Authoring / Discovery+repair). Pinned by `test/schema-cli-database-path.serial.test.ts`. - `src/core/operations.ts` extension — 9 MCP ops: `get_active_schema_pack`, `list_schema_packs`, `schema_stats`, `schema_lint`, `schema_graph`, `schema_explain_type`, `schema_review_orphans` (all read-scope, NOT localOnly), plus `schema_apply_mutations` (admin scope, NOT localOnly so remote agents can author packs over HTTPS MCP — batched, one MCP tool taking a `mutations[]` array atomically inside ONE `withPackLock`, audit log captures `actor: mcp:`) and `reload_schema_pack` (admin, NOT localOnly). Trust posture: per-call `schema_pack` opt STAYS rejected for remote callers via `op-trust-gate.ts`. - `src/commands/whoknows.ts` + `src/core/operations.ts:find_experts` — T1.5 wiring sites. Pack-aware via `expertTypesFromPack(pack.manifest)` from `best-effort.ts`. Pack-load failure → EMPTY filter (NOT hardcoded `['person', 'company']` defaults). A `researcher` type declared `--expert` now surfaces in `whoknows` results. - `skills/schema-author/SKILL.md` — Agent dispatcher for "evolve the schema pack." Triggers: 15+ phrasings incl. "add a page type", "my brain has untyped pages", "propose new types from my corpus", "backfill page types". Explicit Non-goals callout to `brain-taxonomist` (files one page) and `eiirp` (schema-check during iteration) so agents pick the right surface. 7-phase workflow: brain → assess → propose → apply → sync → verify → commit. Lists every gbrain schema CLI verb + every MCP op the skill uses. `brain_first: exempt` frontmatter. Required conformance sections: Contract, Anti-Patterns, Output Format. diff --git a/package.json b/package.json index a14d42cec..2f812de26 100644 --- a/package.json +++ b/package.json @@ -144,7 +144,7 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.62.0", + "version": "0.42.63.0", "overrides": { "@hono/node-server": "^1.19.13", "fast-uri": "^3.1.2", diff --git a/src/commands/schema.ts b/src/commands/schema.ts index 27c31875f..bdb0e3469 100644 --- a/src/commands/schema.ts +++ b/src/commands/schema.ts @@ -48,7 +48,7 @@ import { } from '../core/schema-pack/index.ts'; import type { SchemaPackManifest, PackPrimitive } from '../core/schema-pack/manifest-v1.ts'; import { PACK_PRIMITIVES } from '../core/schema-pack/manifest-v1.ts'; -import { gbrainPath, loadConfig, configPath } from '../core/config.ts'; +import { gbrainPath, loadConfig, configPath, toEngineConfig } from '../core/config.ts'; export async function runSchema(args: string[]): Promise { const sub = args[0]; @@ -434,16 +434,12 @@ function parseFlags(args: string[]): ParsedFlags { async function withConnectedEngine(fn: (engine: import('../core/engine.ts').BrainEngine) => Promise): Promise { const { createEngine } = await import('../core/engine-factory.ts'); - const cfg = loadConfig() ?? {}; - const engineKind = (cfg as { engine?: string }).engine === 'postgres' ? 'postgres' : 'pglite'; + const cfg = loadConfig() ?? { engine: 'pglite' as const }; // PR #1321 (closed) defensive fix retained: build the EngineConfig once and // pass it to BOTH createEngine and engine.connect. The factory captures // config at construction; explicit re-pass at connect() is defense in depth // against future engine implementations that read URL from connect-time. - const connectConfig: import('../core/types.ts').EngineConfig = { - engine: engineKind, - database_url: (cfg as { database_url?: string }).database_url, - }; + const connectConfig = toEngineConfig(cfg); const engine = await createEngine(connectConfig); await engine.connect(connectConfig); try { diff --git a/test/schema-cli-database-path.serial.test.ts b/test/schema-cli-database-path.serial.test.ts new file mode 100644 index 000000000..bd54fe197 --- /dev/null +++ b/test/schema-cli-database-path.serial.test.ts @@ -0,0 +1,67 @@ +/** + * Regression for schema CLI engine routing. + * + * Serial because it opens a persistent PGLite database and then hands that + * database to a CLI subprocess. The subprocess must read the configured path, + * not silently fall back to the default brain. + */ +import { describe, expect, test } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; + +const REPO_ROOT = join(import.meta.dir, '..'); + +describe('gbrain schema configured PGLite routing', () => { + test('schema stats reads database_path from config', async () => { + const home = mkdtempSync(join(tmpdir(), 'gbrain-schema-db-path-')); + const gbrainDir = join(home, '.gbrain'); + const dbPath = join(gbrainDir, 'configured-brain.pglite'); + mkdirSync(gbrainDir, { recursive: true }); + + const engine = new PGLiteEngine(); + try { + await engine.connect({ engine: 'pglite', database_path: dbPath }); + await engine.initSchema(); + await engine.putPage('people/alice-example', { + type: 'person', + title: 'Alice Example', + compiled_truth: 'Example page', + }); + } finally { + await engine.disconnect(); + } + + writeFileSync( + join(gbrainDir, 'config.json'), + JSON.stringify({ engine: 'pglite', database_path: dbPath, schema_pack: 'gbrain-base' }), + 'utf-8', + ); + + try { + const result = spawnSync( + 'bun', + ['run', 'src/cli.ts', 'schema', 'stats', '--json'], + { + cwd: REPO_ROOT, + encoding: 'utf-8', + env: { + ...process.env, + GBRAIN_DATABASE_URL: '', + DATABASE_URL: '', + GBRAIN_HOME: home, + }, + timeout: 60_000, + }, + ); + expect(result.status).toBe(0); + const stats = JSON.parse(result.stdout ?? ''); + expect(stats.aggregate.total_pages).toBe(1); + expect(stats.aggregate.by_type).toContainEqual({ type: 'person', count: 1 }); + } finally { + rmSync(home, { recursive: true, force: true }); + } + }, 90_000); +}); diff --git a/test/schema-cli.test.ts b/test/schema-cli.test.ts index 9ec500030..0019a1ffa 100644 --- a/test/schema-cli.test.ts +++ b/test/schema-cli.test.ts @@ -36,7 +36,13 @@ function gbrain( // bun's spawnSync does NOT inherit env mutations done via process.env = ..., // so pass env explicitly. CLAUDE.md flags this pattern as load-bearing for // any subprocess test that needs GBRAIN_HOME isolation. - const env = { ...process.env, GBRAIN_HOME: DEFAULT_GBRAIN_HOME, ...extraEnv }; + const env = { + ...process.env, + GBRAIN_DATABASE_URL: '', + DATABASE_URL: '', + GBRAIN_HOME: DEFAULT_GBRAIN_HOME, + ...extraEnv, + }; const result = spawnSync('bun', ['run', 'src/cli.ts', ...args], { cwd: REPO_ROOT, encoding: 'utf-8', From d165e99f0bbb6495809907c58dfb9185bbcfdab7 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:08:41 +0900 Subject: [PATCH 03/17] fix(brain-writer): deadline race verdict from the sentinel, not the clock the timer raced (#2947) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of #2946. The hung-COUNT deadline race derived its verdict from a post-await wall-clock re-check, which races the timer's own drift: on loaded CI runners setTimeout callbacks fire measurably EARLY relative to Date.now(), so each padding/boundary adjustment (>=, +1ms) only moved which wrong status the partial-scan test received ('scanned', then 'partial'). The race's timeout arm now resolves a module-private sentinel; the sentinel winning IS the deadline verdict (deadlineHit), consulted by the post-await check without re-reading the clock. A COUNT that resolves null (failed/absent count) stays distinguishable and does not skip the scan; a COUNT that resolves slowly without the timer winning is still caught by the retained wall-clock re-check. The +1ms pad is gone — the sentinel makes timer drift irrelevant for the hung path. Verified: partial-scan suite green 8 consecutive runs incl. the new null-vs-sentinel distinction test. Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT Co-authored-by: Claude Fable 5 --- src/core/brain-writer.ts | 59 ++++++++++++++------------ test/brain-writer-partial-scan.test.ts | 15 +++++++ 2 files changed, 48 insertions(+), 26 deletions(-) diff --git a/src/core/brain-writer.ts b/src/core/brain-writer.ts index a65e6864b..d7ff74082 100644 --- a/src/core/brain-writer.ts +++ b/src/core/brain-writer.ts @@ -409,6 +409,11 @@ export interface ScanOpts { visitDir?: (dirPath: string) => void; } +/** Timeout-arm winner for the COUNT-vs-deadline race in scanBrainSources. + * A unique object so it can never collide with a legitimate COUNT result + * (number | null). Module-private. */ +const DEADLINE_SENTINEL: unique symbol = Symbol('gbrain.scan.deadline'); + export async function scanBrainSources( engine: BrainEngine, opts: ScanOpts = {}, @@ -480,41 +485,43 @@ export async function scanBrainSources( // pool can make this await hang past the budget. Without the race, we'd // wait indefinitely AND defeat the wall-clock guarantee. let dbPageCount: number | null = null; + // Set when the deadline race's timeout arm wins: the verdict that the + // budget is spent, independent of any later Date.now() reading. Timer + // callbacks on loaded runners can fire measurably EARLY relative to the + // wall clock (a +1ms pad was drifted past in practice — see the flake + // lineage in test/brain-writer-partial-scan.test.ts and issue #2946), so + // the hung-COUNT path must not re-derive "did the deadline fire?" from + // the clock the timer just raced against. + let deadlineHit = false; if (opts.dbPageCountForSource) { try { if (opts.deadline) { const remainingMs = opts.deadline - Date.now(); if (remainingMs <= 0) { dbPageCount = null; + deadlineHit = true; } else { - // Race COUNT against the deadline so a hung query can't eat the budget. - // - // Boundary overshoot (+1ms): the post-await deadline check at line - // ~512 uses `Date.now() >= deadline`. setTimeout fires AT OR AFTER - // the requested delay, so in theory the check always passes. In - // practice on heavily-loaded CI runners (8 parallel shards × 4 - // concurrent test files = ~32 concurrent bun processes) we saw - // intermittent failures where the timer callback resolved - // microseconds BEFORE the wall-clock boundary, leaving Date.now() - // a tick below deadline and the skip-check evaluating false. The - // src-a scan then ran on a populated dir before src-b's - // between-source check caught up — causing - // `firstSource.status === 'skipped'` to receive 'scanned'. - // - // Adding 1ms guarantees the timer fires past the deadline by at - // least one millisecond regardless of runner timer drift. Cost is - // 1ms additional wall-clock latency on hung COUNT queries, which - // is operationally negligible. Flake repro: - // https://github.com/garrytan/gbrain/actions/runs/77611667786 - dbPageCount = await Promise.race([ + // Race COUNT against the deadline so a hung query can't eat the + // budget. The timeout arm resolves a private sentinel — NOT null — + // so a deadline win is distinguishable from a COUNT that resolved + // null (failed/absent count keeps its existing semantics). + const raced = await Promise.race([ opts.dbPageCountForSource(src.id), - new Promise(resolve => setTimeout(() => resolve(null), remainingMs + 1)), + new Promise(resolve => + setTimeout(() => resolve(DEADLINE_SENTINEL), remainingMs)), ]); + if (raced === DEADLINE_SENTINEL) { + dbPageCount = null; + deadlineHit = true; + } else { + dbPageCount = raced; + } } } else { dbPageCount = await opts.dbPageCountForSource(src.id); } } catch { + // A throwing COUNT is a failed count, not a deadline verdict. dbPageCount = null; } } @@ -524,11 +531,11 @@ export async function scanBrainSources( // status='partial' with files_scanned=0, which is misleading ("partial // scan" when actually nothing was scanned). Mark this source + remainder // as 'skipped' so the doctor message is honest. - // `>=` matches the between-source check above (line 445). The Promise.race - // setTimeout resolves null at exactly `remainingMs` from now, so post-await - // Date.now() often equals deadline within integer-ms precision — strict `>` - // missed those landings on CI and let the next scanOneSource run anyway. - if (opts.signal?.aborted || (opts.deadline && Date.now() >= opts.deadline)) { + // `deadlineHit` is the authoritative verdict for the hung-COUNT path (the + // sentinel above); the wall-clock re-check (`>=`, matching the + // between-source check at line ~445) still covers a COUNT that RESOLVED + // slowly enough to eat the budget without the timer winning. + if (opts.signal?.aborted || deadlineHit || (opts.deadline && Date.now() >= opts.deadline)) { if (abortedAtSource === null) { abortedAtSource = src.id; } diff --git a/test/brain-writer-partial-scan.test.ts b/test/brain-writer-partial-scan.test.ts index 611c98dc6..e9b780134 100644 --- a/test/brain-writer-partial-scan.test.ts +++ b/test/brain-writer-partial-scan.test.ts @@ -174,6 +174,21 @@ describe('scanBrainSources partial-scan state', () => { expect(report.aborted_at_source).toBe('src-a'); }); + // #2946: the deadline race's timeout arm resolves a SENTINEL, not null — + // a COUNT that legitimately resolves null (failed/absent count) before the + // deadline must NOT be mistaken for a deadline hit: the source still gets + // scanned, with db_page_count simply unavailable. + test('COUNT resolving null before the deadline is not a deadline verdict — source still scans', async () => { + const start = Date.now(); + const report = await scanBrainSources(engine, { + deadline: start + 5_000, + dbPageCountForSource: async () => null, + }); + const firstSource = report.per_source.find(r => r.source_id === 'src-a')!; + expect(firstSource.status).toBe('scanned'); + expect(firstSource.db_page_count == null).toBe(true); + }); + // Codex adversarial #4 regression: even when dbPageCountForSource itself // would hang indefinitely, the Promise.race against the deadline must // resolve null and the scan must abort cleanly. From 3a5c4c194c0c11af6cf9723c55fc65e4652d2b8d Mon Sep 17 00:00:00 2001 From: Sailesh Sivakumar <32437884+ss251@users.noreply.github.com> Date: Mon, 20 Jul 2026 23:46:21 +0530 Subject: [PATCH 04/17] =?UTF-8?q?fix(remote):=20poll=20MinionJob.status,?= =?UTF-8?q?=20not=20.state=20=E2=80=94=20ping=20now=20sees=20completion=20?= =?UTF-8?q?(#2950)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit submit_job/get_job return the MinionJob row verbatim; its lifecycle field is `status` (src/core/minions/types.ts), not `state`. remote ping typed and read `state`, so every poll saw undefined, the terminal check never matched, and ping always burned its full --timeout and exited 1 even when the autopilot-cycle had completed — printing "Job #N is still undefined." on the way out. Reads fixed to `status`; the ping's own JSON output keys (`state`, `last_state`) are unchanged for consumers. Source-audit regression test pins the field reads. Co-authored-by: Claude Fable 5 --- src/commands/remote.ts | 30 +++++++++------ test/remote-ping-status-field.test.ts | 53 +++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 12 deletions(-) create mode 100644 test/remote-ping-status-field.test.ts diff --git a/src/commands/remote.ts b/src/commands/remote.ts index 9d663d665..a62ec4374 100644 --- a/src/commands/remote.ts +++ b/src/commands/remote.ts @@ -105,13 +105,19 @@ function printHelp(): void { async function runRemotePing(config: NonNullable>, args: string[]): Promise { const { json, timeoutMs } = parseFlags(args); - let submitted: { id: number; name: string; state: string }; + // submit_job / get_job return the MinionJob row verbatim — the lifecycle + // field is `status` (src/core/minions/types.ts), not `state`. Reading + // `state` here made every poll see `undefined`, so the terminal check + // never matched and ping always exhausted its timeout (exit 1) even when + // the cycle completed. The ping's own JSON *output* keys (`state`, + // `last_state`) are kept as-is for consumers. + let submitted: { id: number; name: string; status: string }; try { const res = await callRemoteTool(config, 'submit_job', { name: 'autopilot-cycle', data: { phases: ['sync', 'extract', 'embed'] }, }); - submitted = unpackToolResult<{ id: number; name: string; state: string }>(res); + submitted = unpackToolResult<{ id: number; name: string; status: string }>(res); } catch (e) { return failPing(e, json); } @@ -122,43 +128,43 @@ async function runRemotePing(config: NonNullable>, const startMs = Date.now(); let attempt = 0; - let lastState = submitted.state; + let lastState = submitted.status; while (Date.now() - startMs < timeoutMs) { const elapsed = Date.now() - startMs; const intervalMs = elapsed < 30_000 ? 1_000 : elapsed < 5 * 60_000 + 30_000 ? 5_000 : 10_000; await sleep(intervalMs); attempt++; - let job: { id: number; state: string; failed_reason?: string }; + let job: { id: number; status: string; failed_reason?: string }; try { const res = await callRemoteTool(config, 'get_job', { id: submitted.id }); - job = unpackToolResult<{ id: number; state: string; failed_reason?: string }>(res); + job = unpackToolResult<{ id: number; status: string; failed_reason?: string }>(res); } catch (e) { // Network blip mid-poll: log and keep going. Surface only if persistent. if (!json) console.error(` poll #${attempt} failed (${e instanceof Error ? e.message : String(e)}); continuing...`); continue; } - if (job.state !== lastState) { - lastState = job.state; - if (!json) console.error(` job #${submitted.id} → ${job.state}`); + if (job.status !== lastState) { + lastState = job.status; + if (!json) console.error(` job #${submitted.id} → ${job.status}`); } const terminal = ['completed', 'failed', 'dead', 'cancelled']; - if (terminal.includes(job.state)) { - const ok = job.state === 'completed'; + if (terminal.includes(job.status)) { + const ok = job.status === 'completed'; if (json) { console.log(JSON.stringify({ status: ok ? 'success' : 'error', job_id: submitted.id, - state: job.state, + state: job.status, ...(job.failed_reason ? { failed_reason: job.failed_reason } : {}), elapsed_ms: Date.now() - startMs, })); } else { console.log(ok ? `\nautopilot-cycle complete (${Math.round((Date.now() - startMs) / 1000)}s).` - : `\nautopilot-cycle ended ${job.state}${job.failed_reason ? `: ${job.failed_reason}` : ''}.`); + : `\nautopilot-cycle ended ${job.status}${job.failed_reason ? `: ${job.failed_reason}` : ''}.`); } process.exit(ok ? 0 : 1); } diff --git a/test/remote-ping-status-field.test.ts b/test/remote-ping-status-field.test.ts new file mode 100644 index 000000000..11e74e905 --- /dev/null +++ b/test/remote-ping-status-field.test.ts @@ -0,0 +1,53 @@ +/** + * Regression guard: `gbrain remote ping` must poll the MinionJob `status` + * field, never `state`. + * + * submit_job and get_job (src/core/operations.ts) return the MinionJob row + * verbatim, whose lifecycle field is `status` + * (src/core/minions/types.ts). remote.ts once typed and read `state` + * instead: every poll then saw `undefined`, the terminal check + * (`['completed','failed','dead','cancelled'].includes(job.state)`) never + * matched, and ping exhausted its full --timeout and exited 1 even when + * the autopilot-cycle had completed — printing + * "Job #N is still undefined." on the way out. + * + * Source-audit style (same idiom as thin-client-routing-audit.test.ts): + * pins the reads without needing a live MCP transport. + */ + +import { describe, test, expect } from 'bun:test'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; + +const REMOTE_TS_PATH = join(import.meta.dir, '..', 'src', 'commands', 'remote.ts'); +const REMOTE_SOURCE = readFileSync(REMOTE_TS_PATH, 'utf8'); + +describe('remote ping polls MinionJob.status, not .state', () => { + test('no `.state` property reads on job objects remain', () => { + // Catches `submitted.state`, `job.state` — any resurrection of the + // wrong field. The ping's JSON *output* keys (`state:`, `last_state:`) + // are object-literal keys, not property reads, and don't match this. + expect(REMOTE_SOURCE).not.toMatch(/\b(?:job|submitted)\.state\b/); + }); + + test('poll loop reads job.status', () => { + expect(REMOTE_SOURCE).toMatch(/\bjob\.status\b/); + expect(REMOTE_SOURCE).toMatch(/\bsubmitted\.status\b/); + }); + + test('terminal-state check tests job.status', () => { + expect(REMOTE_SOURCE).toMatch(/terminal\.includes\(job\.status\)/); + }); + + test('unpack generics type the lifecycle field as status', () => { + // Both the submit and poll unpack sites must carry `status: string` in + // their type argument, and none may reintroduce `state: string`. + const unpackShapes = REMOTE_SOURCE.match(/unpackToolResult<\{[^}]*\}>/g) ?? []; + const jobShapes = unpackShapes.filter((s) => s.includes('id: number')); + expect(jobShapes.length).toBeGreaterThanOrEqual(2); + for (const shape of jobShapes) { + expect(shape).toContain('status: string'); + expect(shape).not.toContain('state: string'); + } + }); +}); From f1031d5a0b15b25640c66b8f1f898c598131f33d Mon Sep 17 00:00:00 2001 From: Sailesh Sivakumar <32437884+ss251@users.noreply.github.com> Date: Mon, 20 Jul 2026 23:55:46 +0530 Subject: [PATCH 05/17] fix(cli): stop thin-client jobs/config from fabricating a scratch PGLite (#2951) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `jobs list|get` have had remote MCP routing since v0.32, but the CLI shell still ran connectEngine() before dispatch — on a thin-client install that fabricates an empty scratch PGLite in the thin-client GBRAIN_HOME and replays the entire migration chain on every invocation, before the remote call even runs. Host-only jobs subcommands (work, supervisor, submit, ...) and `config` did the same instead of refusing. - cli.ts: dispatch thin-client `jobs list|get` engine-free (runJobs(null, ...)); refuse the other jobs subcommands with a pinpoint hint; add `config` to THIN_CLIENT_REFUSED_COMMANDS with a hint (it reads/writes the host brain's config plane). - jobs.ts: widen runJobs to accept a null engine, guarded so null can only reach the MCP-routed list/get branches. - tests: behavioral (no scratch store created, no migration replay, refusals carry hints) + source-audit pins in the existing idioms. Co-authored-by: Claude Fable 5 --- src/cli.ts | 31 +++++++++++++++++ src/commands/jobs.ts | 18 +++++++++- test/cli-dispatch-thin-client.test.ts | 44 ++++++++++++++++++++++++ test/thin-client-routing-audit.test.ts | 46 ++++++++++++++++++++++++++ 4 files changed, 138 insertions(+), 1 deletion(-) diff --git a/src/cli.ts b/src/cli.ts index 53622bb05..154f449b1 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -998,6 +998,13 @@ const THIN_CLIENT_REFUSED_COMMANDS = new Set([ // - `code-def`/`code-refs`/`code-callers`/`code-callees` have NO MCP ops // in operations.ts:2630-2671; cannot be "fixed by routing" yet 'pages', 'files', 'eval', 'code-def', 'code-refs', 'code-callers', 'code-callees', + // scratch-DB audit: `config` get/set operate on the host brain's config + // plane (DB rows / host file-plane). On a thin client they fabricated an + // ephemeral local PGLite (full migration replay per call) and read/wrote + // config nobody would ever see. NOTE: `jobs` is deliberately NOT here — + // it gets a partial dispatch (list/get route over MCP engine-free, the + // rest refuse) in the main dispatch before connectEngine(). + 'config', ]); /** @@ -1035,6 +1042,9 @@ const THIN_CLIENT_REFUSE_HINTS: Record = { 'code-refs': '`code-refs` has no MCP op yet. Run on the host.', 'code-callers': '`code-callers` has no MCP op yet. Run on the host.', 'code-callees': '`code-callees` has no MCP op yet. Run on the host.', + // scratch-DB audit additions + config: "config reads/writes the host brain's config plane. Edit the host's .gbrain/config.json (file-plane keys) or run on the host with GBRAIN_HOME set.", + jobs: '`jobs list` and `jobs get ` are thin-client routable; this subcommand runs against the host queue. Use the submit_job / list_jobs / get_job MCP tools from your agent, or run on the host with GBRAIN_HOME set.', }; /** @@ -1593,6 +1603,27 @@ async function handleCliOnly(command: string, args: string[]) { } } + // Thin-client `jobs` dispatch: `list` and `get` route over MCP (v0.32 + // routing branches in commands/jobs.ts) and never touch a local engine — + // but falling through to connectEngine() below fabricates an empty + // scratch PGLite in the thin-client GBRAIN_HOME and replays the entire + // migration chain on every invocation before the remote call even runs. + // Dispatch them engine-free here; every other jobs subcommand is + // host-queue-bound, so refuse with a pinpoint hint instead of building + // the scratch store. + if (command === 'jobs') { + const cfgJobs = loadConfig(); + if (isThinClient(cfgJobs)) { + const jobsSub = args[0]; + if (jobsSub === 'list' || jobsSub === 'get') { + const { runJobs } = await import('./commands/jobs.ts'); + await runJobs(null, args); + return; + } + refuseThinClient('jobs', cfgJobs!.remote_mcp!.mcp_url); + } + } + // All remaining CLI-only commands need a DB connection const engine = await connectEngine(); try { diff --git a/src/commands/jobs.ts b/src/commands/jobs.ts index 32902b55a..52ba691ce 100644 --- a/src/commands/jobs.ts +++ b/src/commands/jobs.ts @@ -132,9 +132,23 @@ function formatJobDetail(job: MinionJob): string { return lines.join('\n'); } -export async function runJobs(engine: BrainEngine, args: string[]): Promise { +export async function runJobs(engineOrNull: BrainEngine | null, args: string[]): Promise { const sub = args[0]; + // Thin-client dispatch (cli.ts) passes engine=null for the subcommands + // with remote MCP routing (`list`, `get`) so no scratch local engine is + // ever built. Any other subcommand arriving with a null engine is a + // routing bug upstream of this function — refuse instead of crashing + // inside MinionQueue. + if (!engineOrNull && sub !== 'list' && sub !== 'get') { + console.error(`\`gbrain jobs ${sub ?? ''}\` needs a local engine and cannot run on a thin client.`); + process.exit(1); + } + // Null only ever reaches the MCP-routed `list`/`get` branches, which + // never touch the engine — narrowed once here so the host-only cases + // below typecheck unchanged. + const engine = engineOrNull as BrainEngine; + if (!sub || sub === '--help' || sub === '-h') { console.log(`gbrain jobs — Minions job queue @@ -217,6 +231,8 @@ HANDLER TYPES (built in) return; } + // The constructor just stores the reference; on the null (thin-client + // list/get) paths no queue method is ever reached. const queue = new MinionQueue(engine); switch (sub) { diff --git a/test/cli-dispatch-thin-client.test.ts b/test/cli-dispatch-thin-client.test.ts index 08807c36d..568e1b2db 100644 --- a/test/cli-dispatch-thin-client.test.ts +++ b/test/cli-dispatch-thin-client.test.ts @@ -174,3 +174,47 @@ describe('regression — local config still passes through normally', () => { expect(r.stdout).not.toContain('"mode":"thin-client"'); }); }); + +describe('thin-client scratch-DB guard — jobs partial dispatch + config refusal', () => { + test('`gbrain config set x y` is refused with pinpoint hint', async () => { + seedThinClientConfig(); + const r = await run(['config', 'set', 'search.reranker.enabled', 'false']); + expect(r.exitCode).toBe(1); + expect(r.stderr).toContain('gbrain config'); + expect(r.stderr).toContain('not routable'); + expect(r.stderr).toContain('thin-client of https://brain-host.example/mcp'); + }); + + test('`gbrain jobs work` is refused with pinpoint hint (host-queue-bound)', async () => { + seedThinClientConfig(); + const r = await run(['jobs', 'work']); + expect(r.exitCode).toBe(1); + expect(r.stderr).toContain('gbrain jobs'); + expect(r.stderr).toContain('not routable'); + expect(r.stderr).toContain('thin-client of https://brain-host.example/mcp'); + }); + + test('`gbrain jobs get` never fabricates a scratch local engine', async () => { + // The regression this pins: on a thin-client install with a PGLite + // engine key, `jobs get` connected a LOCAL engine before its remote + // routing branch ran — creating an empty scratch PGLite store in the + // thin-client GBRAIN_HOME and replaying the entire migration chain + // ("Schema version 1 → N") on every invocation. The remote call to + // brain-host.example will fail (unreachable) — irrelevant here. What + // matters: no local store is created and no migration replay runs. + seedThinClientConfig({ engine: 'pglite' }); + const r = await run(['jobs', 'get', '999']); + const { existsSync } = await import('fs'); + expect(existsSync(join(tmp, '.gbrain', 'brain.pglite'))).toBe(false); + expect(r.stdout + r.stderr).not.toContain('Schema version'); + expect(r.stdout + r.stderr).not.toContain('migration(s) pending'); + }); + + test('`gbrain jobs list` never fabricates a scratch local engine', async () => { + seedThinClientConfig({ engine: 'pglite' }); + const r = await run(['jobs', 'list']); + const { existsSync } = await import('fs'); + expect(existsSync(join(tmp, '.gbrain', 'brain.pglite'))).toBe(false); + expect(r.stdout + r.stderr).not.toContain('Schema version'); + }); +}); diff --git a/test/thin-client-routing-audit.test.ts b/test/thin-client-routing-audit.test.ts index b9a4e45db..0c2bcba10 100644 --- a/test/thin-client-routing-audit.test.ts +++ b/test/thin-client-routing-audit.test.ts @@ -127,3 +127,49 @@ describe('thin-client routing audit — v0.32 ROUTE additions wire callRemoteToo expect(src).toContain(`callRemoteTool(cfg!, 'get_job'`); }); }); + +describe('thin-client routing audit — scratch-DB additions (jobs partial dispatch + config refusal)', () => { + // `jobs list|get` route over MCP but the CLI shell still connected a + // local engine first, fabricating an empty scratch PGLite in the + // thin-client GBRAIN_HOME and replaying the full migration chain on + // every invocation. `config` did the same with no remote path at all. + + test('cli.ts dispatches thin-client jobs list/get engine-free (runJobs(null, ...))', () => { + expect(CLI_SOURCE).toMatch(/command === 'jobs'/); + expect(CLI_SOURCE).toMatch(/runJobs\(null, args\)/); + }); + + test('cli.ts refuses non-routable jobs subcommands on thin clients via refuseThinClient', () => { + const dispatchStart = CLI_SOURCE.indexOf("if (command === 'jobs') {"); + expect(dispatchStart).toBeGreaterThan(-1); + const dispatchBlock = CLI_SOURCE.slice(dispatchStart, dispatchStart + 900); + expect(dispatchBlock).toContain('isThinClient'); + expect(dispatchBlock).toContain("refuseThinClient('jobs'"); + }); + + test("'config' is in THIN_CLIENT_REFUSED_COMMANDS with a hint", () => { + const setStart = CLI_SOURCE.indexOf('const THIN_CLIENT_REFUSED_COMMANDS = new Set(['); + const setEnd = CLI_SOURCE.indexOf(']);', setStart); + expect(CLI_SOURCE.slice(setStart, setEnd)).toContain("'config'"); + const hintsStart = CLI_SOURCE.indexOf( + 'const THIN_CLIENT_REFUSE_HINTS: Record = {', + ); + const hintsEnd = CLI_SOURCE.indexOf('};', hintsStart); + expect(/\bconfig\s*:/.test(CLI_SOURCE.slice(hintsStart, hintsEnd))).toBe(true); + }); + + test("'jobs' is NOT in THIN_CLIENT_REFUSED_COMMANDS (partial dispatch owns it)", () => { + const setStart = CLI_SOURCE.indexOf('const THIN_CLIENT_REFUSED_COMMANDS = new Set(['); + const setEnd = CLI_SOURCE.indexOf(']);', setStart); + expect(CLI_SOURCE.slice(setStart, setEnd)).not.toContain("'jobs'"); + }); + + test('jobs.ts guards the null-engine path to list/get only', () => { + const src = readFileSync( + join(import.meta.dir, '..', 'src', 'commands', 'jobs.ts'), + 'utf8', + ); + expect(src).toContain('engineOrNull: BrainEngine | null'); + expect(src).toMatch(/if \(!engineOrNull && sub !== 'list' && sub !== 'get'\)/); + }); +}); From 89f226eb38c18138d0d71cd4666ad8fd2ed1d268 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:35:02 +0900 Subject: [PATCH 06/17] =?UTF-8?q?fix(search):=20classify=20cache=20hit/mis?= =?UTF-8?q?s=20in=20telemetry=20=E2=80=94=20hits=20were=20invisible,=20mis?= =?UTF-8?q?ses=20unclassified=20(#2953)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(search): classify cache hit/miss in telemetry — hits were invisible, misses unclassified (#2952) search stats reported 0 hit / 0 miss forever: recordSearchTelemetry fired only from bare hybridSearch, whose meta never carries a cache field, and a cache HIT returned from hybridSearchCached before any record at all — so hit searches also vanished from count/results/tokens/rank-1. - HybridSearchOpts: internal _telemetryCacheStatus ('miss' | 'disabled') threaded from hybridSearchCached into the inner hybridSearch (same pattern as _queryEmbedDeadline), folded into the RECORDED meta only — onMeta payloads unchanged, count/sum_tokens/budget_dropped/rank-1 behavior byte-identical for the miss/disabled paths - hit path: record once from hybridSearchCached with the already-built cachedMeta (cache.status='hit'), post-slice/budget result count, tokens from the budget pass, and the same rank-1 rule as the inner paths - bare hybridSearch direct callers (think/gather, brainstorm, enrich, evals, ...) keep recording exactly as before, with no cache field - test: serial wiring test drives a real store-then-hit roundtrip through hybridSearchCached (mocked embedQuery, real PGLite SemanticQueryCache) and pins the decision matrix (miss / hit / consult-skipped / bare); revert-checked red on pre-fix source at the miss classification Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT * fix(search): harden cache-hit telemetry per review — mode-gated tokens, embed-failure coverage - hit-path tokens_estimate now gated on the MODE-resolved budget, mirroring the inner paths' resolvedMode.tokenBudget > 0 meta condition (a tokenmax budget-off brain would otherwise record real tokens on hits but 0 on misses, inflating avg-tokens as the hit rate rises) - test: exact token-delta parity assertion (hit contributes the same tokens as the miss that stored the served set) — catches the class of hit/miss accounting asymmetry the increase-only check accepted - test: embed-provider-failure flavor of the disabled path (consult degrades via catch, keyword fallback serves, neither counter bumps) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT --------- Co-authored-by: Claude Fable 5 --- src/core/search/hybrid.ts | 44 +++- ...arch-telemetry-cache-wiring.serial.test.ts | 238 ++++++++++++++++++ 2 files changed, 281 insertions(+), 1 deletion(-) create mode 100644 test/search-telemetry-cache-wiring.serial.test.ts diff --git a/src/core/search/hybrid.ts b/src/core/search/hybrid.ts index d4669a1cc..4f2feb856 100644 --- a/src/core/search/hybrid.ts +++ b/src/core/search/hybrid.ts @@ -740,6 +740,21 @@ export interface HybridSearchOpts extends SearchOpts { * a fresh per-call deadline. Not part of the public contract. */ _queryEmbedDeadline?: QueryEmbedDeadline; + + /** + * INTERNAL — cache-consult outcome threaded from `hybridSearchCached` into + * the inner `hybridSearch` so the ONE telemetry record per search (emitted + * by the inner function) carries the cache classification: 'miss' when the + * semantic cache was consulted and had no row, 'disabled' when the consult + * was skipped (cache off, walk/near-symbol/non-default-column/adaptive + * skip, or the lookup embed failed). Folded into the RECORDED meta only — + * `onMeta` payloads are unchanged. Direct `hybridSearch` callers leave it + * undefined and keep recording with no cache field (they never consulted + * the cache). The cache-HIT record is emitted by `hybridSearchCached` + * itself, since the inner function never runs on a hit. Not part of the + * public contract. + */ + _telemetryCacheStatus?: 'miss' | 'disabled'; } /** @@ -943,7 +958,15 @@ export async function hybridSearch( // swallow — capture telemetry is best-effort } try { - recordSearchTelemetry(engine, meta, { results_count: lastResultsCount, rank1_score: lastRank1Score }); + // #2952 — fold the cache-consult outcome (threaded by hybridSearchCached) + // into the RECORDED meta only. None of the inner return paths set a + // `cache` field themselves, so this is the sole source of the miss / + // disabled classification; `onMeta` consumers above still receive the + // meta unchanged (the cached wrapper emits its own merged meta to them). + const recordedMeta = opts?._telemetryCacheStatus + ? { ...meta, cache: { status: opts._telemetryCacheStatus } } + : meta; + recordSearchTelemetry(engine, recordedMeta, { results_count: lastResultsCount, rank1_score: lastRank1Score }); } catch { // swallow — telemetry must never break the search hot path. } @@ -1753,6 +1776,21 @@ export async function hybridSearchCached( } catch { // swallow — telemetry is best-effort } + // #2952 — a cache hit never reaches the inner hybridSearch (the only + // other telemetry site), so record the search HERE or it vanishes from + // stats entirely (count, results, tokens, rank-1 — not just the hit + // counter). Same rank-1 rule as the inner return paths. Tokens are + // gated on the MODE-resolved budget, mirroring the inner paths' `if + // (resolvedMode.tokenBudget > 0)` meta condition — otherwise a + // tokenmax (budget-off) brain would record real tokens on hits but 0 + // on misses, skewing avg-tokens upward as the hit rate rises (codex). + recordSearchTelemetry(engine, cachedMeta, { + results_count: budgeted.length, + ...(resolvedForCache.tokenBudget && resolvedForCache.tokenBudget > 0 + ? { tokens_estimate: budgetMeta.used } + : {}), + rank1_score: budgeted[0] ? (budgeted[0].base_score ?? budgeted[0].score) : undefined, + }); return budgeted; } } @@ -1768,6 +1806,10 @@ export async function hybridSearchCached( // v0.42.20.0 (Fix 3) — share the query-embed deadline so the inner embed // doesn't start a fresh 6s budget after the cache-lookup already spent it. _queryEmbedDeadline: queryEmbedDl, + // #2952 — classify this search's telemetry record (emitted by the inner + // function) with the cache-consult outcome. 'hit' already returned above, + // so only miss/disabled reach this call. + _telemetryCacheStatus: cacheStatus === 'disabled' ? 'disabled' : 'miss', onMeta: (m) => { innerMetaBox.current = m; // Do NOT call userOnMeta here — we'll emit a merged meta below diff --git a/test/search-telemetry-cache-wiring.serial.test.ts b/test/search-telemetry-cache-wiring.serial.test.ts new file mode 100644 index 000000000..320e2708d --- /dev/null +++ b/test/search-telemetry-cache-wiring.serial.test.ts @@ -0,0 +1,238 @@ +/** + * Regression wiring test for #2952 — cache classification reaches telemetry. + * + * Pre-fix, `recordSearchTelemetry` fired only from bare `hybridSearch`, whose + * meta never carries a `cache` field, and a cache HIT returned from + * `hybridSearchCached` before any record at all. Net effect on a live brain: + * `search stats` reported `0 hit / 0 miss` forever while the `query_cache` + * table grew, and hit searches vanished from count/results/tokens/rank-1. + * + * This file drives the REAL pipeline (PGLite brain, real SemanticQueryCache + * store→lookup roundtrip, mocked `embedQuery` for a deterministic vector) and + * pins the decision matrix: + * + * - consulted + no row → recorded once with cache_miss + * - consulted + row → recorded once with cache_hit (plus results/rank-1) + * - consult skipped → recorded once with neither counter + * - bare hybridSearch → recorded once with neither counter (unchanged) + * + * Serial: mock.module + gateway/global-env mutation (isolation guard R2). + */ + +import { afterAll, beforeAll, beforeEach, describe, expect, mock, test } from 'bun:test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import * as realEmbedding from '../src/core/embedding.ts'; + +/** Deterministic 1536d unit vector — same for every call, so an identical + * query's second consult matches its first write at cosine 1.0. */ +function fixedEmbedding(): Float32Array { + const arr = new Float32Array(1536); + for (let i = 0; i < 1536; i++) arr[i] = Math.sin(1 + i * 0.001); + let norm = 0; + for (let i = 0; i < 1536; i++) norm += arr[i] * arr[i]; + norm = Math.sqrt(norm); + if (norm > 0) for (let i = 0; i < 1536; i++) arr[i] /= norm; + return arr; +} + +// Pluggable behavior so individual tests can simulate an embed-provider +// failure (the 'disabled'-via-catch flavor). null → deterministic vector. +let embedBehavior: (() => Promise) | null = null; + +// Mock the embedding seam BEFORE importing hybrid.ts so both the cache-lookup +// embed and the inner vector-arm embed resolve without a provider call. Spread +// the real module so every other export stays live. +mock.module('../src/core/embedding.ts', () => ({ + ...realEmbedding, + embed: async () => (embedBehavior ? embedBehavior() : fixedEmbedding()), + embedQuery: async () => (embedBehavior ? embedBehavior() : fixedEmbedding()), +})); + +// Import AFTER mocking. +const { hybridSearch, hybridSearchCached, awaitPendingSearchCacheWrites, _resetPendingSearchCacheWritesForTests } = + await import('../src/core/search/hybrid.ts'); +const { getTelemetryWriter, _resetTelemetryWriterForTest } = await import('../src/core/search/telemetry.ts'); +const { configureGateway, resetGateway } = await import('../src/core/ai/gateway.ts'); +const { PGLiteEngine } = await import('../src/core/pglite-engine.ts'); + +let engine: InstanceType; +let tmpHome: string; +const savedGbrainHome = process.env.GBRAIN_HOME; + +interface Counters { + c: number; + hit: number; + miss: number; + rank1: number; + results: number; + tokens: number; +} + +/** Flush the writer and read the summed counters back from the table. */ +async function readCounters(): Promise { + await getTelemetryWriter().flush(); + const rows = await engine.executeRaw( + `SELECT COALESCE(SUM(count), 0)::int AS c, + COALESCE(SUM(cache_hit), 0)::int AS hit, + COALESCE(SUM(cache_miss), 0)::int AS miss, + COALESCE(SUM(count_rank1), 0)::int AS rank1, + COALESCE(SUM(sum_results), 0)::int AS results, + COALESCE(SUM(sum_tokens), 0)::int AS tokens + FROM search_telemetry`, + ); + return rows[0]; +} + +beforeAll(async () => { + // Hermetic config home so the developer's real ~/.gbrain/config.json can't + // leak an embedding_model that flips isCacheSafe → 'disabled'. + tmpHome = mkdtempSync(join(tmpdir(), 'gbrain-cache-telemetry-')); + process.env.GBRAIN_HOME = tmpHome; + + // Pin the gateway to a 1536d provider BEFORE initSchema so the + // query_cache.embedding column is sized for the mock vectors, and so + // isAvailable('embedding') lets the cache consult proceed. The fake key is + // never used — embedQuery is mocked above. (Pattern: + // test/query-cache-knobs-hash.serial.test.ts.) + resetGateway(); + configureGateway({ + embedding_model: 'openai:text-embedding-3-large', + embedding_dimensions: 1536, + env: { OPENAI_API_KEY: 'sk-fake' }, + }); + + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + + // Keyword-findable fixtures so the inner search returns rows (a non-empty + // result set is what arms the cache writeback). searchKeyword joins + // content_chunks, so pages need explicit chunks — putPage alone leaves the + // chunk table empty (pattern: test/chunk-grain-fts.test.ts). + await engine.putPage('alice-foo', { + type: 'person', + title: 'Alice Foo', + compiled_truth: 'Alice Foo is a builder who ships search telemetry fixtures.', + }); + await engine.upsertChunks('alice-foo', [ + { chunk_index: 0, chunk_text: 'Alice Foo is a builder who ships search telemetry fixtures.', chunk_source: 'compiled_truth' }, + ]); + await engine.putPage('bob-bar', { + type: 'person', + title: 'Bob Bar', + compiled_truth: 'Bob Bar is a builder who reviews cache wiring fixtures.', + }); + await engine.upsertChunks('bob-bar', [ + { chunk_index: 0, chunk_text: 'Bob Bar is a builder who reviews cache wiring fixtures.', chunk_source: 'compiled_truth' }, + ]); +}); + +afterAll(async () => { + if (savedGbrainHome === undefined) delete process.env.GBRAIN_HOME; + else process.env.GBRAIN_HOME = savedGbrainHome; + try { await engine.disconnect(); } catch { /* ignore */ } + resetGateway(); + try { rmSync(tmpHome, { recursive: true, force: true }); } catch { /* ignore */ } +}); + +beforeEach(async () => { + embedBehavior = null; + _resetTelemetryWriterForTest(); + _resetPendingSearchCacheWritesForTests(); + await engine.executeRaw('DELETE FROM search_telemetry'); + await engine.executeRaw('DELETE FROM query_cache'); +}); + +describe('hybridSearchCached — telemetry carries the cache outcome', () => { + test('miss then hit: one record per search, classified, hit keeps results/rank-1 telemetry', async () => { + // Call 1 — cache consulted, empty → miss. + const first = await hybridSearchCached(engine, 'alice telemetry fixtures', { limit: 5 }); + expect(first.length).toBeGreaterThan(0); + await awaitPendingSearchCacheWrites(); + + // Sanity: the writeback actually landed, so call 2 exercises a REAL hit + // (a broken writeback would otherwise fail the hit assertion ambiguously). + const cacheRows = await engine.executeRaw<{ n: number }>( + 'SELECT COUNT(*)::int AS n FROM query_cache', + ); + expect(cacheRows[0].n).toBeGreaterThan(0); + + const afterMiss = await readCounters(); + expect(afterMiss.c).toBe(1); + expect(afterMiss.miss).toBe(1); + expect(afterMiss.hit).toBe(0); + expect(afterMiss.rank1).toBe(1); + expect(afterMiss.results).toBeGreaterThan(0); + + // Call 2 — identical query + knobs, deterministic embedding → hit. + let meta: import('../src/core/types.ts').HybridSearchMeta | undefined; + const second = await hybridSearchCached(engine, 'alice telemetry fixtures', { + limit: 5, + onMeta: (m) => { meta = m; }, + }); + expect(meta?.cache?.status).toBe('hit'); + expect(second.length).toBeGreaterThan(0); + + const afterHit = await readCounters(); + // Pre-fix both sides of this were wrong: hit stayed 0 forever AND the hit + // search was missing from count entirely (c would read 1, not 2). + expect(afterHit.c).toBe(2); + expect(afterHit.miss).toBe(1); + expect(afterHit.hit).toBe(1); + // The hit search contributes results/rank-1/tokens telemetry too. + expect(afterHit.rank1).toBe(2); + expect(afterHit.results).toBeGreaterThan(afterMiss.results); + // Token parity (codex): the hit serves the SAME result set the miss + // stored, so its token contribution must EQUAL the miss's — a hit/miss + // accounting asymmetry (e.g. hits counting tokens the miss convention + // skips) would break this exact-delta check. + expect(afterHit.tokens - afterMiss.tokens).toBe(afterMiss.tokens); + }); + + test('lookup-embed failure: consult degrades to disabled — recorded once, neither counter', async () => { + embedBehavior = async () => { throw new Error('embed provider down'); }; + // The failed consult must not break the search: keyword fallback serves. + const results = await hybridSearchCached(engine, 'bob cache wiring', { limit: 5 }); + expect(results.length).toBeGreaterThan(0); + + const counters = await readCounters(); + expect(counters.c).toBe(1); + expect(counters.hit).toBe(0); + expect(counters.miss).toBe(0); + expect(counters.rank1).toBe(1); + }); + + test('consult skipped (useCache:false): recorded once, neither counter', async () => { + const results = await hybridSearchCached(engine, 'bob cache wiring', { limit: 5, useCache: false }); + expect(results.length).toBeGreaterThan(0); + + const counters = await readCounters(); + expect(counters.c).toBe(1); + expect(counters.hit).toBe(0); + expect(counters.miss).toBe(0); + // Telemetry otherwise unchanged: the search still counts fully. + expect(counters.rank1).toBe(1); + expect(counters.results).toBeGreaterThan(0); + }); +}); + +describe('bare hybridSearch — direct callers unchanged', () => { + test('records once with no cache classification', async () => { + let meta: import('../src/core/types.ts').HybridSearchMeta | undefined; + const results = await hybridSearch(engine, 'bob cache wiring', { + limit: 5, + onMeta: (m) => { meta = m; }, + }); + expect(results.length).toBeGreaterThan(0); + // The onMeta contract is untouched: no cache field is injected into the + // caller-visible meta (the fold happens on the recorded copy only). + expect(meta?.cache).toBeUndefined(); + + const counters = await readCounters(); + expect(counters.c).toBe(1); + expect(counters.hit).toBe(0); + expect(counters.miss).toBe(0); + }); +}); From 912407bef1343cf8cb04c8beb9dbec0fb34ac780 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 03:49:47 +0900 Subject: [PATCH 07/17] fix(search): per-call token-budget meta masked the real cut on both cache paths; restore vacuous search-lite coverage (#2954) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(search): per-call token-budget meta no longer masks the real cut; restore vacuous search-lite coverage The search-lite integration tests were structurally vacuous: putPage never creates chunks and searchKeyword joins content_chunks, so every fixture query returned zero rows on every machine. The tight-budget cut test's defensive skip ('keyword search may dedupe by page') silently returned before its assertions had ever executed anywhere, and the two budget-meta tests ran against empty result sets. Restoring the fixture (upsertChunks per page) and hardening the assertions immediately exposed a real meta bug: with a per-call tokenBudget, the inner hybridSearch enforces the same resolved budget (per-call wins in resolveSearchMode) and its meta carries the true dropped count — but hybridSearchCached re-applies the budget to the already-cut set and published THAT pass's meta, which always reads dropped=0. onMeta consumers saw a budget record claiming nothing was dropped while rows were; telemetry (recorded from the inner meta) disagreed with the caller-visible meta. - hybrid.ts finalMeta: prefer innerMeta.token_budget when a per-call budget is set (outer budgetMeta stays as the fallback and remains the enforcement for the cache-HIT path, where no inner run exists) - test fixture: chunk each page (pattern: chunk-grain-fts.test.ts) - cut test: defensive skip replaced with a hard >=2 precondition; results non-empty + strictly-fewer-than-unbounded + dropped>0 now actually execute (revert-checked red on the unfixed meta) - budget-meta tests: assert non-empty result sets so kept=results.length can no longer pass vacuously at 0=0 The unbounded 'builder' query returns 2 of 3 fixture pages by design — dedup Layer 3 caps any single page type at 60% of results and the fixture is all-person — which the >=2 precondition accommodates. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT * fix(search): cache-HIT budget meta prefers the stored cut record per review; exact-count assertions, mixed-type fixture - hit path had the symmetric masking (codex P2): the re-application runs on the already-trimmed stored set and read dropped=0 while the miss that produced the same result set reported the real cut. Prefer hit.meta.token_budget unconditionally — tokenBudget is folded into knobsHash ('tb='), so a hit only ever serves a lookup with the identical resolved budget as the write and the outer pass can never cut further (verified against mode.ts; this is why the reviewer's 'outer wins when it drops' branch is unreachable). budgetMeta remains the fallback for legacy rows stored without a budget record - new serial test drives a real store-then-hit roundtrip (mocked embedQuery, real PGLite cache) and pins hit token_budget == miss token_budget; revert-checked red (dropped=0) on the unfixed path - lite test: dropped asserted as the exact unbounded-minus-kept count and used>0 (dropped>0 alone accepts any wrong positive; used<=250 alone accepts a bogus zero), fixture types mixed (person/company/note) so dedup Layer-3 type-diversity policy no longer shapes the test Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01KFWV7zBZFcmD94Vek6xRDT --------- Co-authored-by: Claude Fable 5 --- src/core/search/hybrid.ts | 20 ++- ...brid-cached-hit-budget-meta.serial.test.ts | 133 ++++++++++++++++++ test/hybrid-search-lite.serial.test.ts | 47 +++++-- 3 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 test/hybrid-cached-hit-budget-meta.serial.test.ts diff --git a/src/core/search/hybrid.ts b/src/core/search/hybrid.ts index 4f2feb856..e0a6fcb40 100644 --- a/src/core/search/hybrid.ts +++ b/src/core/search/hybrid.ts @@ -1767,8 +1767,17 @@ export async function hybridSearchCached( ...(hit.meta?.embedding_column ? { embedding_column: hit.meta.embedding_column } : {}), ...(hit.meta?.adaptive_return ? { adaptive_return: hit.meta.adaptive_return } : {}), ...(hit.meta?.autocut ? { autocut: hit.meta.autocut } : {}), + // Per-call budget: prefer the STORED budget record, which carries + // the true dropped count from the write-time cut — the + // re-application above ran on an already-cut set and reads + // dropped=0 (same masking as the miss path's finalMeta). Safe + // unconditionally: tokenBudget is folded into knobsHash (`tb=`), + // so a hit only ever serves a lookup with the identical resolved + // budget as the write — the outer pass can never cut further. + // budgetMeta stays as the fallback for legacy rows stored without + // a budget record. ...(opts?.tokenBudget && opts.tokenBudget > 0 - ? { token_budget: budgetMeta } + ? { token_budget: hit.meta?.token_budget ?? budgetMeta } : {}), }; try { @@ -1836,8 +1845,15 @@ export async function hybridSearchCached( ...(innerMeta?.embedding_column ? { embedding_column: innerMeta.embedding_column } : {}), ...(innerMeta?.adaptive_return ? { adaptive_return: innerMeta.adaptive_return } : {}), ...(innerMeta?.autocut ? { autocut: innerMeta.autocut } : {}), + // Per-call budget: prefer the INNER meta's budget record. The inner + // hybridSearch already enforced the same resolved budget (per-call wins + // in resolveSearchMode), so the re-application above sees an + // already-cut set and its meta reads dropped=0 — masking the real cut + // from onMeta consumers (the `dropped` under-report the restored + // search-lite test caught). The outer pass stays as the enforcement + // for the cache-HIT path, where no inner run exists. ...(opts?.tokenBudget && opts.tokenBudget > 0 - ? { token_budget: budgetMeta } + ? { token_budget: innerMeta?.token_budget ?? budgetMeta } : {}), }; try { diff --git a/test/hybrid-cached-hit-budget-meta.serial.test.ts b/test/hybrid-cached-hit-budget-meta.serial.test.ts new file mode 100644 index 000000000..b1b86424a --- /dev/null +++ b/test/hybrid-cached-hit-budget-meta.serial.test.ts @@ -0,0 +1,133 @@ +/** + * Cache-HIT budget-meta provenance — companion to the miss-path fix. + * + * With a per-call tokenBudget, the miss path stores an already-budgeted + * result set; a subsequent HIT re-applies the same budget to that trimmed + * payload (a structural no-op: tokenBudget is folded into knobsHash, so a + * hit only ever serves a lookup with the identical resolved budget as the + * write) — and pre-fix published that no-op pass's meta, reporting + * dropped=0 while the miss that produced the very same result set reported + * the real cut. This file drives a real store→hit roundtrip (mocked + * `embedQuery` for a deterministic vector, real PGLite SemanticQueryCache) + * and pins that the hit's token_budget matches the miss's. + * + * Serial: mock.module + gateway/global-env mutation (isolation guard R2). + */ + +import { afterAll, beforeAll, describe, expect, mock, test } from 'bun:test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import * as realEmbedding from '../src/core/embedding.ts'; + +/** Deterministic 1536d unit vector — identical for every call, so the + * second consult matches the first write at cosine 1.0. */ +function fixedEmbedding(): Float32Array { + const arr = new Float32Array(1536); + for (let i = 0; i < 1536; i++) arr[i] = Math.sin(1 + i * 0.001); + let norm = 0; + for (let i = 0; i < 1536; i++) norm += arr[i] * arr[i]; + norm = Math.sqrt(norm); + if (norm > 0) for (let i = 0; i < 1536; i++) arr[i] /= norm; + return arr; +} + +// Mock BEFORE importing hybrid.ts (spread keeps every other export live). +mock.module('../src/core/embedding.ts', () => ({ + ...realEmbedding, + embed: async () => fixedEmbedding(), + embedQuery: async () => fixedEmbedding(), +})); + +// Import AFTER mocking. +const { hybridSearchCached, awaitPendingSearchCacheWrites } = + await import('../src/core/search/hybrid.ts'); +const { configureGateway, resetGateway } = await import('../src/core/ai/gateway.ts'); +const { PGLiteEngine } = await import('../src/core/pglite-engine.ts'); + +let engine: InstanceType; +let tmpHome: string; +const savedGbrainHome = process.env.GBRAIN_HOME; + +beforeAll(async () => { + // Hermetic config home so the developer's real ~/.gbrain/config.json + // can't leak an embedding_model that flips the cache consult to + // 'disabled' via isCacheSafe. + tmpHome = mkdtempSync(join(tmpdir(), 'gbrain-hit-budget-meta-')); + process.env.GBRAIN_HOME = tmpHome; + + // Pin the gateway to a 1536d provider BEFORE initSchema so the + // query_cache.embedding column is sized for the mock vectors. The fake + // key is never used — embedQuery is mocked above. + resetGateway(); + configureGateway({ + embedding_model: 'openai:text-embedding-3-large', + embedding_dimensions: 1536, + env: { OPENAI_API_KEY: 'sk-fake' }, + }); + + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + + // Three keyword-findable pages, ~200 tokens each, mixed types so dedup's + // type-diversity layer keeps all of them. putPage never chunks — + // searchKeyword joins content_chunks, so chunks are explicit. + const longText = 'x'.repeat(800); + const fixtures: Array<[string, string, string]> = [ + ['alice-foo', 'Alice Foo', 'person'], + ['bob-bar', 'Bob Bar', 'company'], + ['carol-baz', 'Carol Baz', 'note'], + ]; + for (const [slug, title, type] of fixtures) { + const truth = `${title} is a builder. ${longText}`; + await engine.putPage(slug, { type, title, compiled_truth: truth }); + await engine.upsertChunks(slug, [ + { chunk_index: 0, chunk_text: truth, chunk_source: 'compiled_truth' }, + ]); + } +}); + +afterAll(async () => { + if (savedGbrainHome === undefined) delete process.env.GBRAIN_HOME; + else process.env.GBRAIN_HOME = savedGbrainHome; + try { await engine.disconnect(); } catch { /* ignore */ } + resetGateway(); + try { rmSync(tmpHome, { recursive: true, force: true }); } catch { /* ignore */ } +}); + +describe('cache HIT — token_budget provenance', () => { + test('hit reports the same cut the miss reported, not the no-op re-application', async () => { + // Miss: budget 250 keeps ~1 of 3 rows (~209 tokens each); the meta + // carries the real cut from the inner enforcement. + let missMeta: import('../src/core/types.ts').HybridSearchMeta | undefined; + const missResults = await hybridSearchCached(engine, 'builder', { + limit: 10, + tokenBudget: 250, + onMeta: (m) => { missMeta = m; }, + }); + expect(missResults.length).toBeGreaterThan(0); + expect(missMeta?.cache?.status).toBe('miss'); + expect(missMeta?.token_budget?.budget).toBe(250); + const missDropped = missMeta?.token_budget?.dropped; + expect(missDropped).toBeGreaterThan(0); + + await awaitPendingSearchCacheWrites(); + + // Hit: identical query + knobs (tokenBudget is part of knobsHash, so + // this is the ONLY kind of lookup the stored row can serve). The + // published budget record must match the miss's — pre-fix it was the + // outer no-op pass's meta with dropped=0. + let hitMeta: import('../src/core/types.ts').HybridSearchMeta | undefined; + const hitResults = await hybridSearchCached(engine, 'builder', { + limit: 10, + tokenBudget: 250, + onMeta: (m) => { hitMeta = m; }, + }); + expect(hitMeta?.cache?.status).toBe('hit'); + expect(hitResults.length).toBe(missResults.length); + expect(hitMeta?.token_budget?.budget).toBe(250); + expect(hitMeta?.token_budget?.dropped).toBe(missDropped); + expect(hitMeta?.token_budget?.kept).toBe(missMeta?.token_budget?.kept); + }); +}); diff --git a/test/hybrid-search-lite.serial.test.ts b/test/hybrid-search-lite.serial.test.ts index 857db692c..f3705a7c1 100644 --- a/test/hybrid-search-lite.serial.test.ts +++ b/test/hybrid-search-lite.serial.test.ts @@ -41,7 +41,11 @@ beforeAll(async () => { { slug: 'bob-bar', page: { - type: 'person', + // Mixed types across the fixture keep dedup Layer 3 (no page type + // above 60% of results) out of this test's way — an all-person set + // would be capped to 2 of 3 and couple these assertions to the + // diversity policy. + type: 'company', title: 'Bob Bar', compiled_truth: `Bob Bar is a builder. ${longText}`, }, @@ -49,7 +53,7 @@ beforeAll(async () => { { slug: 'carol-baz', page: { - type: 'person', + type: 'note', title: 'Carol Baz', compiled_truth: `Carol Baz is a builder. ${longText}`, }, @@ -57,6 +61,13 @@ beforeAll(async () => { ]; for (const p of pages) { await engine.putPage(p.slug, p.page); + // putPage never chunks — searchKeyword joins content_chunks, so a + // page without explicit chunks is invisible to the keyword arm and + // every result-dependent assertion below runs against an empty set. + // (Pattern: test/chunk-grain-fts.test.ts.) + await engine.upsertChunks(p.slug, [ + { chunk_index: 0, chunk_text: p.page.compiled_truth!, chunk_source: 'compiled_truth' }, + ]); } // Force keyword-only fallback by unsetting the embedding provider key. delete process.env.OPENAI_API_KEY; @@ -103,10 +114,10 @@ describe('hybridSearchCached \u2014 token budget', () => { limit: 10, onMeta: (m) => { meta = m; }, }); - // Don't assert non-empty here — keyword tokenization depends on the - // pglite analyzer config. What matters: meta is shaped right and - // budget metadata is absent when budget isn't set. - expect(results).toBeDefined(); + // Non-empty matters: pre-fix the fixture had no chunks, so this ran + // against an empty result set and the absent-budget assertion was + // trivially true. + expect(results.length).toBeGreaterThan(0); expect(meta?.token_budget).toBeUndefined(); }); @@ -117,19 +128,21 @@ describe('hybridSearchCached \u2014 token budget', () => { tokenBudget: 250, onMeta: (m) => { meta = m; }, }); + expect(results.length).toBeGreaterThan(0); expect(meta?.token_budget).toBeDefined(); expect(meta?.token_budget?.budget).toBe(250); expect(meta?.token_budget?.kept).toBe(results.length); }); test('tight budget cuts the result set', async () => { - // First find out the result count without a budget so the assertion - // is robust to the fixture’s actual chunking. + // All three fixture pages match 'builder' (mixed types, so dedup's + // type-diversity layer keeps all of them), and the unbounded set MUST + // have enough rows for the cut to be observable. Pre-fix this was a + // silent `return` when fewer than 2 rows came back — and with no + // chunks in the fixture, zero rows ALWAYS came back, so the cut + // assertions below had never executed anywhere. const unbounded = await hybridSearchCached(engine, 'builder', { limit: 10 }); - // Skip the cut test if the fixture happens to return only one row - // (keyword search may dedupe by page); the budget enforcement itself - // is exhaustively unit-tested in test/token-budget.test.ts. - if (unbounded.length < 2) return; + expect(unbounded.length).toBeGreaterThanOrEqual(2); let meta: HybridSearchMeta | undefined; const results = await hybridSearchCached(engine, 'builder', { @@ -137,10 +150,16 @@ describe('hybridSearchCached \u2014 token budget', () => { tokenBudget: 250, // enough for ~1 row of fixture data onMeta: (m) => { meta = m; }, }); + expect(results.length).toBeGreaterThan(0); + expect(results.length).toBeLessThan(unbounded.length); expect(meta?.token_budget?.budget).toBe(250); expect(meta?.token_budget?.kept).toBe(results.length); - expect(meta?.token_budget?.dropped).toBeGreaterThan(0); - // The budget must hold: cumulative cost <= budget. + // Exact accounting: every row the budget removed is a reported drop — + // dropped > 0 alone would accept any wrong positive count (codex). + expect(meta?.token_budget?.dropped).toBe(unbounded.length - results.length); + // The budget must hold with a real (non-zero) cost: cumulative cost + // <= budget, and used=0 would mean the accounting never ran. + expect(meta?.token_budget?.used).toBeGreaterThan(0); expect(meta?.token_budget?.used).toBeLessThanOrEqual(250); }); }); From 184b6cb8a10bde6e9e916c0857966996635a8ab3 Mon Sep 17 00:00:00 2001 From: Cossackx <121278003+Cossackx@users.noreply.github.com> Date: Mon, 20 Jul 2026 14:57:31 -0400 Subject: [PATCH 08/17] fix search: title candidate arm + gated OR fallback for lexical recall (#2956) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pages were unreachable by their own exact titles: FTS indexed only chunk body text while the title-weighted pages.search_vector (GIN-indexed since its introduction) was never queried by any search path, and websearch_to_tsquery AND-at-chunk-grain semantics meant one non-matching token zeroed keyword recall with no fallback — long or acronym-bearing titles (e.g. "IAWG ... AAR-LL deck") fell through to the vector arm alone and missed. - searchTitles (both engines): page-grain candidate arm over pages.search_vector (title 'A' + compiled_truth 'B' + timeline 'C'), ts_rank_cd ranked, representative-chunk LATERAL join, full filter parity with searchKeyword (visibility, soft-delete, source grants, hard-excludes, dates, types); fused as a weighted RRF list at the keyword arm's intent-effective k on all three hybrid return paths; fail-open with warnOncePerProcess. No schema changes — the index already existed, dark. - AND->OR one-retry fallback for the keyword arm, gated behind SearchOpts.orFallback (only hybridSearch opts in; countMentions, link resolution, eval, and keyword-only MCP callers keep the strict-AND contract). Refused for queries carrying websearch operators (negation, quoted phrases). searchTitles carries its own page-grain fallback. - Lexical arms parallelized (Promise.all) on the main path. Verified: typecheck clean; 18 hermetic PGLite tests + 2 engine-parity e2e cases (CI Postgres); consumer regression enrichment 18/0 + link-extraction 127/0; independent live QA on a 10,664-page brain — exact-title target miss -> rank 1 (exact_title_match), controls held, negation/quoted guards proven, strict-consumer contract pinned. Diagnosed from a 3-lane read-only diagnostic; adversarial review round closed findings on fallback scope, Postgres test coverage, and operator handling before this commit. Co-authored-by: Aleksei Razsadin Co-authored-by: Claude Fable 5 --- src/core/engine.ts | 21 ++ src/core/pglite-engine.ts | 140 ++++++++++- src/core/postgres-engine.ts | 164 ++++++++++++- src/core/search/hybrid.ts | 69 ++++-- src/core/search/sql-ranking.ts | 45 ++++ src/core/types.ts | 13 ++ test/e2e/engine-parity.test.ts | 60 +++++ test/search/title-retrieval-arm.test.ts | 296 ++++++++++++++++++++++++ 8 files changed, 784 insertions(+), 24 deletions(-) create mode 100644 test/search/title-retrieval-arm.test.ts diff --git a/src/core/engine.ts b/src/core/engine.ts index 4a8d27046..a389d3091 100644 --- a/src/core/engine.ts +++ b/src/core/engine.ts @@ -936,6 +936,27 @@ export interface BrainEngine { // Search searchKeyword(query: string, opts?: SearchOpts): Promise; + /** + * fix/title-retrieval-arm (D1): page-grain title candidate arm. + * + * content_chunks.search_vector never includes the page TITLE (it is + * doc_comment + symbol_name_qualified + chunk_text), so a page whose + * title tokens are absent from its body is unreachable by searchKeyword. + * This arm queries the PAGE-GRAIN DOCUMENT vector pages.search_vector — + * NOT titles alone: per trg_pages_search_vector it is title (weight 'A') + * + compiled_truth ('B') + timeline text ('C'). Ranked by ts_rank_cd, + * the 'A'-weighted title dominates, but body/timeline matches also + * produce (lower-ranked) candidates. Returns page-grain hits joined to + * ONE representative chunk per page (compiled_truth preferred, else + * lowest chunk_index) so rows are shaped like searchKeyword's output and + * can enter RRF fusion in hybridSearch. + * + * Deliberately NO query-length gating — unlike the alias hop (≤6-token + * guard) and the title-phrase re-rank boost, this arm must GENERATE + * candidates for long exact-title queries, which is exactly where + * chunk-grain AND FTS is weakest. + */ + searchTitles(query: string, opts?: SearchOpts): Promise; searchVector(embedding: Float32Array, opts?: SearchOpts): Promise; /** * Hydrate embeddings for chunks already known by id. v0.36 (D9): diff --git a/src/core/pglite-engine.ts b/src/core/pglite-engine.ts index 8239ec354..d084079af 100644 --- a/src/core/pglite-engine.ts +++ b/src/core/pglite-engine.ts @@ -55,7 +55,7 @@ import { GBrainError, PAGE_SORT_SQL, ENRICH_ORDER_SQL } from './types.ts'; import { finalizeLastSeen } from './chronicle/last-seen.ts'; import { computeAnomaliesFromBuckets } from './cycle/anomaly.ts'; import { resolveBoostMap, resolveHardExcludes } from './search/source-boost.ts'; -import { buildSourceFactorCase, buildHardExcludeClause, buildVisibilityClause, buildRecencyComponentSql, buildBestPerPagePoolCte } from './search/sql-ranking.ts'; +import { buildSourceFactorCase, buildHardExcludeClause, buildVisibilityClause, buildRecencyComponentSql, buildBestPerPagePoolCte, buildOrFallbackWebsearchQuery } from './search/sql-ranking.ts'; import { normalizeEngineColumn, buildVectorCastFragment, @@ -1630,7 +1630,7 @@ export class PGLiteEngine implements BrainEngine { // — safe to interpolate into raw SQL. const ftsLang = getFtsLanguage(); - const { rows } = await this.db.query( + const keywordSql = `WITH ranked AS ( SELECT p.slug, p.id as page_id, p.title, p.type, p.source_id, @@ -1654,10 +1654,140 @@ export class PGLiteEngine implements BrainEngine { ${buildBestPerPagePoolCte('ranked')} SELECT * FROM best_per_page ORDER BY score DESC, page_id ASC, chunk_id ASC - LIMIT $3 OFFSET $4`, - params - ); + LIMIT $3 OFFSET $4`; + let { rows } = await this.db.query(keywordSql, params); + // D2 fix (fix/title-retrieval-arm): websearch AND semantics at chunk + // grain mean one non-co-occurring token zeroes keyword recall. When the + // strict query returns nothing, retry ONCE with OR-of-terms. Strict-AND + // results always win when non-empty (no change for working queries). + // Opt-in via SearchOpts.orFallback (Reviewer F1): only hybridSearch's + // recall arm relaxes; precision consumers (countMentions, + // link-extraction, eval) keep the strict-AND contract. + if (rows.length === 0 && opts?.orFallback) { + const orQuery = buildOrFallbackWebsearchQuery(query); + if (orQuery) { + const fallbackParams = [...params]; + fallbackParams[0] = orQuery; + ({ rows } = await this.db.query(keywordSql, fallbackParams)); + } + } + + return (rows as Record[]).map(rowToSearchResult); + } + + /** + * fix/title-retrieval-arm (D1): page-grain title candidate arm. See the + * BrainEngine interface doc for the full contract. Queries + * pages.search_vector (title weight 'A' dominates ts_rank_cd by + * construction) with the same page-grain filters the keyword arm applies + * (type/types/excludeSlugs/date/source scoping, hard-excludes, + * visibility), joined to one representative chunk per page. Applies the + * same AND→OR recall fallback as searchKeyword. NO query-length gate — + * long exact-title queries are the case this arm exists for. + * + * CJK queries fall through to websearch FTS here (a single-token CJK + * query CAN exact-match a single-token CJK title); the richer CJK ILIKE + * fallback stays keyword-arm-only. + */ + async searchTitles(query: string, opts?: SearchOpts): Promise { + // language/symbolKind are chunk-grain code filters with no page-grain + // meaning; a code-scoped query gets no title candidates rather than + // rows that silently violate the caller's filter. + if (opts?.language || opts?.symbolKind) return []; + const limit = clampSearchLimit(opts?.limit); + const offset = opts?.offset || 0; + const detailLow = opts?.detail === 'low'; + + if (opts?.limit && opts.limit > MAX_SEARCH_LIMIT) { + console.warn(`[gbrain] Warning: search limit clamped from ${opts.limit} to ${MAX_SEARCH_LIMIT}`); + } + + const boostMap = resolveBoostMap(); + const sourceFactorCase = buildSourceFactorCase('p.slug', boostMap, opts?.detail); + const hardExcludePrefixes = resolveHardExcludes(opts?.exclude_slug_prefixes, opts?.include_slug_prefixes); + const hardExcludeClause = buildHardExcludeClause('p.slug', hardExcludePrefixes); + const visibilityClause = buildVisibilityClause('p', 's'); + // FTS config name (e.g. 'english', 'pt_br'). Validated by getFtsLanguage() + // — safe to interpolate into raw SQL. + const ftsLang = getFtsLanguage(); + + const params: unknown[] = [query, limit, offset]; + let extraFilter = ''; + if (opts?.type) { + params.push(opts.type); + extraFilter += ` AND p.type = $${params.length}`; + } + if (opts?.types && opts.types.length > 0) { + params.push(opts.types); + extraFilter += ` AND p.type = ANY($${params.length}::text[])`; + } + if (opts?.exclude_slugs?.length) { + params.push(opts.exclude_slugs); + extraFilter += ` AND p.slug != ALL($${params.length}::text[])`; + } + if (opts?.afterDate) { + params.push(opts.afterDate); + extraFilter += ` AND COALESCE(p.effective_date, p.updated_at, p.created_at) > $${params.length}::timestamptz`; + } + if (opts?.beforeDate) { + params.push(opts.beforeDate); + extraFilter += ` AND COALESCE(p.effective_date, p.updated_at, p.created_at) < $${params.length}::timestamptz`; + } + if (opts?.sourceIds && opts.sourceIds.length > 0) { + params.push(opts.sourceIds); + extraFilter += ` AND p.source_id = ANY($${params.length}::text[])`; + } else if (opts?.sourceId) { + params.push(opts.sourceId); + extraFilter += ` AND p.source_id = $${params.length}`; + } + + // Page grain — one row per page by construction, so no best_per_page + // pooling CTE is needed. The LEFT JOIN LATERAL picks the representative + // chunk (compiled_truth first, then lowest chunk_index); COALESCEs keep + // chunkless pages retrievable (the extreme D1 case: a title with no + // body) with the alias-hop row shape (chunk_id 0, empty chunk_text). + // Accepted limitations (Reviewer F5/F6): the synthetic chunkless row + // inherits the compiled-truth RRF boost and dedups on empty chunk_text; + // and detail='low' filters only the REPRESENTATIVE — pages without a + // compiled_truth chunk still surface (unlike the keyword arm's filter). + const titlesSql = + `SELECT + p.slug, p.id as page_id, p.title, p.type, p.source_id, + p.effective_date, p.effective_date_source, + COALESCE(rep.id, 0) as chunk_id, + COALESCE(rep.chunk_index, 0) as chunk_index, + COALESCE(rep.chunk_text, '') as chunk_text, + COALESCE(rep.chunk_source, 'compiled_truth') as chunk_source, + ts_rank_cd(p.search_vector, websearch_to_tsquery('${ftsLang}', $1)) * ${sourceFactorCase} AS score, + CASE WHEN p.updated_at < ( + SELECT MAX(te.created_at) FROM timeline_entries te WHERE te.page_id = p.id + ) THEN true ELSE false END AS stale + FROM pages p + JOIN sources s ON s.id = p.source_id + LEFT JOIN LATERAL ( + SELECT cc.id, cc.chunk_index, cc.chunk_text, cc.chunk_source + FROM content_chunks cc + WHERE cc.page_id = p.id + AND cc.modality = 'text' + ${detailLow ? `AND cc.chunk_source = 'compiled_truth'` : ''} + ORDER BY (cc.chunk_source = 'compiled_truth') DESC, cc.chunk_index ASC + LIMIT 1 + ) rep ON true + WHERE p.search_vector @@ websearch_to_tsquery('${ftsLang}', $1) + ${extraFilter} ${hardExcludeClause} ${visibilityClause} + ORDER BY score DESC, p.id ASC + LIMIT $2 OFFSET $3`; + + let { rows } = await this.db.query(titlesSql, params); + if (rows.length === 0) { + const orQuery = buildOrFallbackWebsearchQuery(query); + if (orQuery) { + const fallbackParams = [...params]; + fallbackParams[0] = orQuery; + ({ rows } = await this.db.query(titlesSql, fallbackParams)); + } + } return (rows as Record[]).map(rowToSearchResult); } diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index eafaf8bfa..43decdb52 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -63,7 +63,7 @@ import { ConnectionManager } from './connection-manager.ts'; import { logConnectionEvent } from './connection-audit.ts'; import { validateSlug, contentHash, rowToPage, rowToStalePage, rowToChunk, rowToSearchResult, parseEmbedding, tryParseEmbedding, takeRowToTake, takeHitRowToHit, isUndefinedTableError, warnOncePerProcess } from './utils.ts'; import { resolveBoostMap, resolveHardExcludes } from './search/source-boost.ts'; -import { buildSourceFactorCase, buildHardExcludeClause, buildVisibilityClause, buildRecencyComponentSql, buildBestPerPagePoolCte } from './search/sql-ranking.ts'; +import { buildSourceFactorCase, buildHardExcludeClause, buildVisibilityClause, buildRecencyComponentSql, buildBestPerPagePoolCte, buildOrFallbackWebsearchQuery } from './search/sql-ranking.ts'; import { DEFAULT_EMBEDDING_MODEL, DEFAULT_EMBEDDING_DIMENSIONS } from './ai/defaults.ts'; import { DELETE_BATCH_SIZE } from './engine-constants.ts'; @@ -1807,10 +1807,164 @@ export class PostgresEngine implements BrainEngine { // the GUC can never leak onto a pooled connection). Flag off → the // wrap is identical to master's; flag on → set_config('app.scopes') // shares the same transaction as the timeout. - const rows = await this.withScopedReadTransaction(opts?.sourceIds, opts?.sourceId, async (tx) => { - await tx`SET LOCAL statement_timeout = '8s'`; - return await tx.unsafe(rawQuery, params as Parameters[1]); - }, { alwaysTransaction: true }); + const runKeyword = (queryText: string) => + this.withScopedReadTransaction(opts?.sourceIds, opts?.sourceId, async (tx) => { + await tx`SET LOCAL statement_timeout = '8s'`; + const boundParams = [...params]; + boundParams[0] = queryText; + return await tx.unsafe(rawQuery, boundParams as Parameters[1]); + }, { alwaysTransaction: true }); + let rows = await runKeyword(query); + // D2 fix (fix/title-retrieval-arm): websearch AND semantics at chunk + // grain mean one non-co-occurring token zeroes keyword recall. When the + // strict query returns nothing, retry ONCE with OR-of-terms — through + // the SAME scoped wrapper (the retry is a fresh scoped transaction, so + // RLS scope binding applies identically). Strict-AND results always win + // when non-empty (no change for working queries). + // Opt-in via SearchOpts.orFallback (Reviewer F1): only hybridSearch's + // recall arm relaxes; precision consumers (countMentions, + // link-extraction, eval) keep the strict-AND contract. + if (rows.length === 0 && opts?.orFallback) { + const orQuery = buildOrFallbackWebsearchQuery(query); + if (orQuery) rows = await runKeyword(orQuery); + } + return rows.map(rowToSearchResult); + } + + /** + * fix/title-retrieval-arm (D1): page-grain title candidate arm. See the + * BrainEngine interface doc for the full contract. Queries + * pages.search_vector (title weight 'A' dominates ts_rank_cd by + * construction) with the same page-grain filters the keyword arm applies + * (type/types/excludeSlugs/date/source scoping, hard-excludes, + * visibility), joined to one representative chunk per page. Applies the + * same AND→OR recall fallback as searchKeyword. NO query-length gate — + * long exact-title queries are the case this arm exists for. + */ + async searchTitles(query: string, opts?: SearchOpts): Promise { + // language/symbolKind are chunk-grain code filters with no page-grain + // meaning; a code-scoped query gets no title candidates rather than + // rows that silently violate the caller's filter. + if (opts?.language || opts?.symbolKind) return []; + const limit = clampSearchLimit(opts?.limit); + const offset = opts?.offset || 0; + const detailLow = opts?.detail === 'low'; + + if (opts?.limit && opts.limit > MAX_SEARCH_LIMIT) { + console.warn(`[gbrain] Warning: search limit clamped from ${opts.limit} to ${MAX_SEARCH_LIMIT}`); + } + + const boostMap = resolveBoostMap(); + const sourceFactorCase = buildSourceFactorCase('p.slug', boostMap, opts?.detail); + const hardExcludePrefixes = resolveHardExcludes(opts?.exclude_slug_prefixes, opts?.include_slug_prefixes); + const hardExcludeClause = buildHardExcludeClause('p.slug', hardExcludePrefixes); + const visibilityClause = buildVisibilityClause('p', 's'); + // FTS config name (e.g. 'english', 'pt_br'). Validated by getFtsLanguage() + // — safe to interpolate into raw SQL. + const ftsLang = getFtsLanguage(); + + const params: unknown[] = [query]; + let typeClause = ''; + if (opts?.type) { + params.push(opts.type); + typeClause = `AND p.type = $${params.length}`; + } + let typesClause = ''; + if (opts?.types && opts.types.length > 0) { + params.push(opts.types); + typesClause = `AND p.type = ANY($${params.length}::text[])`; + } + let excludeSlugsClause = ''; + if (opts?.exclude_slugs?.length) { + params.push(opts.exclude_slugs); + excludeSlugsClause = `AND p.slug != ALL($${params.length}::text[])`; + } + // Date filters read COALESCE(effective_date, …) — upstream unified the + // Postgres keyword arm onto the PGLite effective-date-first convention + // (v0.29.1 parity); the title arm matches it for filter parity. + let afterDateClause = ''; + if (opts?.afterDate) { + params.push(opts.afterDate); + afterDateClause = `AND COALESCE(p.effective_date, p.updated_at, p.created_at) > $${params.length}::timestamptz`; + } + let beforeDateClause = ''; + if (opts?.beforeDate) { + params.push(opts.beforeDate); + beforeDateClause = `AND COALESCE(p.effective_date, p.updated_at, p.created_at) < $${params.length}::timestamptz`; + } + let sourceClause = ''; + if (opts?.sourceIds && opts.sourceIds.length > 0) { + params.push(opts.sourceIds); + sourceClause = `AND p.source_id = ANY($${params.length}::text[])`; + } else if (opts?.sourceId) { + params.push(opts.sourceId); + sourceClause = `AND p.source_id = $${params.length}`; + } + params.push(limit); + const limitParam = `$${params.length}`; + params.push(offset); + const offsetParam = `$${params.length}`; + + // Page grain — one row per page by construction, so no best_per_page + // pooling CTE is needed. The LEFT JOIN LATERAL picks the representative + // chunk (compiled_truth first, then lowest chunk_index); COALESCEs keep + // chunkless pages retrievable (the extreme D1 case: a title with no + // body) with the alias-hop row shape (chunk_id 0, empty chunk_text). + // Accepted limitations (Reviewer F5/F6): the synthetic chunkless row + // inherits the compiled-truth RRF boost and dedups on empty chunk_text; + // and detail='low' filters only the REPRESENTATIVE — pages without a + // compiled_truth chunk still surface (unlike the keyword arm's filter). + const rawQuery = ` + SELECT + p.slug, p.id as page_id, p.title, p.type, p.source_id, + p.effective_date, p.effective_date_source, + COALESCE(rep.id, 0) as chunk_id, + COALESCE(rep.chunk_index, 0) as chunk_index, + COALESCE(rep.chunk_text, '') as chunk_text, + COALESCE(rep.chunk_source, 'compiled_truth') as chunk_source, + ts_rank_cd(p.search_vector, websearch_to_tsquery('${ftsLang}', $1)) * ${sourceFactorCase} AS score, + false AS stale + FROM pages p + JOIN sources s ON s.id = p.source_id + LEFT JOIN LATERAL ( + SELECT cc.id, cc.chunk_index, cc.chunk_text, cc.chunk_source + FROM content_chunks cc + WHERE cc.page_id = p.id + AND cc.modality = 'text' + ${detailLow ? `AND cc.chunk_source = 'compiled_truth'` : ''} + ORDER BY (cc.chunk_source = 'compiled_truth') DESC, cc.chunk_index ASC + LIMIT 1 + ) rep ON true + WHERE p.search_vector @@ websearch_to_tsquery('${ftsLang}', $1) + ${typeClause} + ${typesClause} + ${excludeSlugsClause} + ${afterDateClause} + ${beforeDateClause} + ${sourceClause} + ${hardExcludeClause} + ${visibilityClause} + ORDER BY score DESC, p.id ASC + LIMIT ${limitParam} + OFFSET ${offsetParam} + `; + + // Same RLS scope-binding wrapper as searchKeyword (alwaysTransaction: + // the SET LOCAL statement_timeout needs a transaction regardless of the + // GBRAIN_RLS_SCOPE_BINDING flag). The OR retry re-executes through the + // same scoped wrapper. + const runTitles = (queryText: string) => + this.withScopedReadTransaction(opts?.sourceIds, opts?.sourceId, async (tx) => { + await tx`SET LOCAL statement_timeout = '8s'`; + const boundParams = [...params]; + boundParams[0] = queryText; + return await tx.unsafe(rawQuery, boundParams as Parameters[1]); + }, { alwaysTransaction: true }); + let rows = await runTitles(query); + if (rows.length === 0) { + const orQuery = buildOrFallbackWebsearchQuery(query); + if (orQuery) rows = await runTitles(orQuery); + } return rows.map(rowToSearchResult); } diff --git a/src/core/search/hybrid.ts b/src/core/search/hybrid.ts index e0a6fcb40..de0a95eb4 100644 --- a/src/core/search/hybrid.ts +++ b/src/core/search/hybrid.ts @@ -34,6 +34,7 @@ import { normalizeAlias } from './alias-normalize.ts'; import { stampEvidence } from './evidence.ts'; import { expandAnchors, hydrateChunks } from './two-pass.ts'; import { enforceTokenBudget } from './token-budget.ts'; +import { warnOncePerProcess } from '../utils.ts'; import { recordSearchTelemetry } from './telemetry.ts'; import { weightsForIntent, @@ -932,6 +933,11 @@ export async function hybridSearch( // it never has to read config. Engines normalize string-or-descriptor // via normalizeEngineColumn; the descriptor path is the strict one. embeddingColumn: resolvedCol, + // D2 fix (fix/title-retrieval-arm, Reviewer F1): the hybrid keyword arm + // is a recall arm — opt in to the engine's AND→OR zero-recall fallback. + // Direct searchKeyword consumers (countMentions, link-extraction, eval) + // do NOT set this and keep the strict-AND contract. + orFallback: true, }; // Track what actually ran for the optional onMeta callback (v0.25.0). // Caller leaves onMeta undefined → these flags are computed but never @@ -990,8 +996,31 @@ export async function hybridSearch( const earlyModality = (opts?.crossModal && opts.crossModal !== 'auto') ? opts.crossModal : (suggestions.suggestedModality ?? 'text'); - const keywordResults: SearchResult[] = - earlyModality === 'image' ? [] : await engine.searchKeyword(query, searchOpts); + // D1 fix (fix/title-retrieval-arm): page-grain title candidate arm, + // fetched CONCURRENTLY with the keyword arm (Reviewer F7 — independent + // engine queries). The chunk FTS vector never includes the page title, so + // an exact-title query can be unretrievable by keyword — this arm queries + // pages.search_vector (title weight 'A') directly. Runs regardless of + // query token count: the alias hop (≤6-token guard) and the title-phrase + // boost are re-rank-only, so LONG exact-title queries — where strict-AND + // chunk FTS is weakest — need a candidate GENERATOR. Fail-open WITH + // SIGNAL (Reviewer F2): a SQL error (e.g. a pre-search_vector brain) + // degrades to no title candidates, but warns once per process so a + // broken engine arm cannot ship dark. + const [keywordResults, titleResults]: [SearchResult[], SearchResult[]] = + earlyModality === 'image' + ? [[], []] + : await Promise.all([ + engine.searchKeyword(query, searchOpts), + engine.searchTitles(query, searchOpts).catch((err: unknown) => { + warnOncePerProcess( + 'search-titles-arm-failed', + `[gbrain] searchTitles arm failed (fail-open, title candidates skipped): ` + + `${err instanceof Error ? err.message : String(err)}`, + ); + return [] as SearchResult[]; + }), + ]); // v0.29.1: resolve salience/recency from caller (back-compat aliases for // PR #618's `recencyBoost` numeric scale) or fall back to the heuristic. @@ -1069,14 +1098,16 @@ export async function hybridSearch( if (!isAvailable('embedding', providerProbe)) { // v0.43 — fuse the relational arm with keyword so typed-edge answers // survive on the no-embedding-provider path (the relational win is most - // valuable exactly when vector is unavailable). + // valuable exactly when vector is unavailable). The title arm fuses here + // too — an exact-title lookup on a keyless install is precisely where + // chunk-grain keyword FTS alone fails (D1). let noEmbedResults = keywordResults; - if (relationalList.length > 0) { + if (relationalList.length > 0 || titleResults.length > 0) { const fk = opts?.rrfK ?? RRF_K; - noEmbedResults = rrfFusionWeighted( - [{ list: keywordResults, k: fk }, { list: relationalList, k: fk }], - detailResolved !== 'high', - ); + const noEmbedLists = [{ list: keywordResults, k: fk }]; + if (titleResults.length > 0) noEmbedLists.push({ list: titleResults, k: fk }); + if (relationalList.length > 0) noEmbedLists.push({ list: relationalList, k: fk }); + noEmbedResults = rrfFusionWeighted(noEmbedLists, detailResolved !== 'high'); } if (noEmbedResults.length > 0) { await runPostFusionStages(engine, noEmbedResults, postFusionOpts); @@ -1303,14 +1334,15 @@ export async function hybridSearch( // post-fusion stages here too — without it, salience='on' silently // does nothing on embed failures. // v0.43: fuse the relational arm with keyword via RRF so typed-edge - // answers survive even when vector is unavailable. + // answers survive even when vector is unavailable. The title arm fuses + // here too (same rationale as the no-embedding-provider path — D1). let fallbackResults = keywordResults; - if (relationalList.length > 0) { + if (relationalList.length > 0 || titleResults.length > 0) { const fk = opts?.rrfK ?? RRF_K; - fallbackResults = rrfFusionWeighted( - [{ list: keywordResults, k: fk }, { list: relationalList, k: fk }], - detail !== 'high', - ); + const fallbackLists = [{ list: keywordResults, k: fk }]; + if (titleResults.length > 0) fallbackLists.push({ list: titleResults, k: fk }); + if (relationalList.length > 0) fallbackLists.push({ list: relationalList, k: fk }); + fallbackResults = rrfFusionWeighted(fallbackLists, detail !== 'high'); } if (fallbackResults.length > 0) { await runPostFusionStages(engine, fallbackResults, postFusionOpts); @@ -1375,6 +1407,15 @@ export async function hybridSearch( { list: keywordResults, k: keywordK }, ]; + // D1 fix (fix/title-retrieval-arm) — title candidate arm as a third + // weighted list. Fuses at the keyword arm's intent-effective k (same + // lexical-evidence class, no new tunable). Mirrors the keyword list's + // inclusion rules: fetch was gated on earlyModality, so no extra modality + // check here. Empty for non-matching queries → pure no-op. + if (titleResults.length > 0) { + allLists.push({ list: titleResults, k: keywordK }); + } + // v0.43 — relational recall arm (fourth RRF arm), built above so it also // contributes on the keyword-only fallback path. Neutral weight (baseRrfK): // competes evenly with keyword/vector, not dominating. Empty for diff --git a/src/core/search/sql-ranking.ts b/src/core/search/sql-ranking.ts index 4330fcea0..5989e69f0 100644 --- a/src/core/search/sql-ranking.ts +++ b/src/core/search/sql-ranking.ts @@ -206,6 +206,51 @@ export function buildBestPerPagePoolCte(candidateCte: string): string { )`; } +// ============================================================ +// AND→OR keyword-recall fallback (fix/title-retrieval-arm, D2) +// ============================================================ + +/** + * Build a relaxed OR-of-terms websearch string for the keyword-arm recall + * fallback. + * + * `websearch_to_tsquery('english', query)` joins unquoted terms with `&` + * (AND). At chunk grain, one query token that doesn't co-occur in any + * single chunk zeroes keyword recall with no fallback. When the strict + * AND query returns zero rows, engines retry ONCE with the string this + * builder returns — the same tokens joined with websearch's `OR` keyword, + * which compiles to `|`. + * + * Why rebuild via websearch syntax instead of hand-assembling a tsquery: + * websearch_to_tsquery never raises on malformed input, applies the same + * stemming/stopword pipeline as the document side, and an all-stopword + * token list degrades to an empty tsquery (matches nothing) instead of a + * SQL error — the empty-tsquery guard comes free. + * + * Returns null when relaxation is pointless or unsafe: + * - fewer than 2 tokens survive tokenization (OR of one term is the same + * query as AND of one term); + * - the raw query uses websearch OPERATORS (Reviewer F3): a `-term` + * negation would be RESURRECTED as a positive OR term, and a quoted + * phrase would degrade to a bag of words — both invert caller intent, + * so operator queries get no fallback at all. + * Tokenization splits on non-alphanumeric runs (Unicode-aware). Literal + * OR/AND words are dropped so they can't be re-parsed as operators + * mid-list. + */ +export function buildOrFallbackWebsearchQuery(query: string): string | null { + // F3 operator guard: any double quote, or a dash LEADING a token + // (whitespace/start boundary — interior hyphens like "foo-bar" are fine). + if (query.includes('"') || /(^|\s)-\S/.test(query)) return null; + const tokens = query + .normalize('NFKC') + .split(/[^\p{L}\p{N}]+/u) + .filter(Boolean) + .filter(t => { const u = t.toUpperCase(); return u !== 'OR' && u !== 'AND'; }); + if (tokens.length < 2) return null; + return tokens.join(' OR '); +} + // ============================================================ // v0.29.1 — Recency component SQL builder // ============================================================ diff --git a/src/core/types.ts b/src/core/types.ts index f94c049e4..a9fa99394 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -974,6 +974,19 @@ export interface SearchOpts { * client) → `sourceIds`; otherwise `ctx.sourceId` (scalar) → `sourceId`. */ sourceIds?: string[]; + /** + * fix/title-retrieval-arm (D2, Reviewer F1): opt-in AND→OR keyword-recall + * fallback. When true, `searchKeyword` retries ONCE with OR-of-terms after + * the strict websearch AND query returns zero rows (strict results always + * win when non-empty). Default false/undefined = strict-AND only — the + * pre-fix contract. hybridSearch opts in for its keyword arm; precision + * consumers (enrichment countMentions, link-extraction resolution, eval + * paths) MUST NOT set this: OR-matches would inflate mention counts and + * relax link-candidate resolution ("John Smith" matching every John and + * every Smith). `searchTitles` has its own page-grain fallback and + * ignores this flag. + */ + orFallback?: boolean; /** * v0.27.1 / v0.36 (D11): target column for vector search. Two shapes: * diff --git a/test/e2e/engine-parity.test.ts b/test/e2e/engine-parity.test.ts index f0de50e98..3abe512c5 100644 --- a/test/e2e/engine-parity.test.ts +++ b/test/e2e/engine-parity.test.ts @@ -225,6 +225,66 @@ describeBoth('Engine parity — Postgres vs PGLite', () => { expect(pgChanged || pgliteChanged).toBe(true); }); + // fix/title-retrieval-arm (Reviewer F2): the title arm must behave + // identically on both engines — including the D1 case where the title + // tokens never appear in any chunk. Without this case the Postgres + // implementation would only ever execute behind hybridSearch's fail-open + // catch and a break could ship dark on the production brain. Runs in CI + // via scripts/run-e2e.sh (docker-provisioned Postgres); skips gracefully + // when DATABASE_URL is not configured. + test('searchTitles parity: exact-title hit with title tokens absent from body', async () => { + const seed = async (eng: BrainEngine) => { + await eng.putPage('wiki/title-arm-parity', { + type: 'note', + title: 'Vermilion Icebreaker Compendium', + compiled_truth: 'A document body that never mentions those words.', + timeline: '', + }); + await eng.upsertChunks('wiki/title-arm-parity', [{ + chunk_index: 0, + chunk_text: 'A document body that never mentions those words.', + chunk_source: 'compiled_truth', + embedding: basisEmbedding(33), + token_count: 9, + }] satisfies ChunkInput[]); + }; + await seed(pgEngine); + await seed(pgliteEngine); + + const q = 'Vermilion Icebreaker Compendium'; + // Premise on both engines: chunk-grain keyword cannot see the page + // (also pins the F1 contract — no orFallback flag means strict AND). + expect((await pgEngine.searchKeyword(q, { limit: 5 })).map((r: SearchResult) => r.slug)) + .not.toContain('wiki/title-arm-parity'); + expect((await pgliteEngine.searchKeyword(q, { limit: 5 })).map((r: SearchResult) => r.slug)) + .not.toContain('wiki/title-arm-parity'); + + const pg = await pgEngine.searchTitles(q, { limit: 5 }); + const pglite = await pgliteEngine.searchTitles(q, { limit: 5 }); + expect(pg.map((r: SearchResult) => r.slug)).toContain('wiki/title-arm-parity'); + expect(pglite.map((r: SearchResult) => r.slug)).toContain('wiki/title-arm-parity'); + + // Row-shape parity: identical representative chunk on both engines. + const pgHit = pg.find((r: SearchResult) => r.slug === 'wiki/title-arm-parity')!; + const pgliteHit = pglite.find((r: SearchResult) => r.slug === 'wiki/title-arm-parity')!; + expect(pgHit.chunk_source).toBe('compiled_truth'); + expect(pgliteHit.chunk_source).toBe(pgHit.chunk_source); + expect(pgliteHit.chunk_text).toBe(pgHit.chunk_text); + }); + + // fix/title-retrieval-arm (Reviewer F1): the AND→OR fallback is opt-in. + // Default searchKeyword stays strict on BOTH engines; orFallback: true + // rescues the one-bad-token query identically. + test('searchKeyword orFallback parity: default strict, opt-in rescues', async () => { + const q = 'fat code thin harness zzzabsenttoken'; + for (const eng of [pgEngine, pgliteEngine]) { + const strict = await eng.searchKeyword(q, { limit: 5 }); + expect(strict.length).toBe(0); + const relaxed = await eng.searchKeyword(q, { limit: 5, orFallback: true }); + expect(relaxed.map((r: SearchResult) => r.slug)).toContain('concepts/fat-code-thin-harness'); + } + }); + // v0.39.3.0 T3 — provenance write+read parity (WARN-8 + CV5). // Both engines must write the same 4 provenance columns (source_kind, // source_uri, ingested_via, ingested_at) on putPage AND surface them diff --git a/test/search/title-retrieval-arm.test.ts b/test/search/title-retrieval-arm.test.ts new file mode 100644 index 000000000..45d0af622 --- /dev/null +++ b/test/search/title-retrieval-arm.test.ts @@ -0,0 +1,296 @@ +/** + * fix/title-retrieval-arm — D1 title candidate arm + D2 AND→OR keyword fallback. + * + * The disease (3-lane diagnostic, 2026-07): page titles never enter the + * keyword-searchable text. content_chunks.search_vector is doc_comment + + * symbol_name_qualified + chunk_text — no title — so an exact-title query + * whose tokens are absent from the body had ZERO keyword recall, and every + * existing title mechanism (title boost, exact-match boost, alias hop) is + * re-rank-only: none can GENERATE the missing candidate. Compounding it, + * websearch_to_tsquery AND semantics at chunk grain meant one + * non-co-occurring token zeroed the whole keyword arm with no fallback. + * + * Fixes under test: + * C1 — engine.searchTitles: page-grain candidates from pages.search_vector + * (title weight 'A'), joined to one representative chunk, fused into + * hybridSearch as a keyword-class RRF list. No query-length gate. + * C2 — searchKeyword retries ONCE with OR-of-terms when strict AND + * returns zero rows; strict results always win when non-empty. + * + * Hermetic PGLite. The gateway is pinned with an EMPTY env so embedding is + * deterministically unavailable — hybridSearch takes the keyword(+title) + * no-embed path with zero network, regardless of host API keys. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../../src/core/pglite-engine.ts'; +import { resetPgliteState } from '../helpers/reset-pglite.ts'; +import { hybridSearch } from '../../src/core/search/hybrid.ts'; +import { buildOrFallbackWebsearchQuery } from '../../src/core/search/sql-ranking.ts'; +import { configureGateway } from '../../src/core/ai/gateway.ts'; + +let engine: PGLiteEngine; + +const DIM = 1536; + +beforeAll(async () => { + // Pin 1536-d (matches the preload schema default) with an EMPTY env so + // isAvailable('embedding') is false → hybridSearch never embeds. + configureGateway({ + embedding_model: 'openai:text-embedding-3-large', + embedding_dimensions: DIM, + env: {}, + }); + engine = new PGLiteEngine(); + await engine.connect({}); // in-memory + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); + // Restore the preload-equivalent gateway for sibling files in this shard. + configureGateway({ + embedding_model: 'openai:text-embedding-3-large', + embedding_dimensions: DIM, + env: { ...process.env }, + }); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +/** Page whose TITLE tokens never appear in its body/chunks (the D1 shape). */ +async function seedTitleOnlyPage(): Promise { + await engine.putPage('projects/chronomancer', { + type: 'note', + title: 'Chronomancer Codex Ledger', + compiled_truth: 'A reference document about scheduling practices and planning.', + }); + await engine.upsertChunks('projects/chronomancer', [ + { + chunk_index: 0, + chunk_text: 'A reference document about scheduling practices and planning.', + chunk_source: 'compiled_truth', + }, + ]); +} + +describe('searchTitles — D1 title candidate arm', () => { + test('exact-title query retrieves a page whose title tokens are absent from its body', async () => { + await seedTitleOnlyPage(); + + // Premise check: the chunk-grain keyword arm CANNOT see this page for + // this query, even with the OR fallback (no title token is in any chunk). + const kw = await engine.searchKeyword('Chronomancer Codex Ledger', { limit: 10 }); + expect(kw.map(r => r.slug)).not.toContain('projects/chronomancer'); + + // The title arm can. + const hits = await engine.searchTitles('Chronomancer Codex Ledger', { limit: 10 }); + expect(hits.map(r => r.slug)).toContain('projects/chronomancer'); + const hit = hits.find(r => r.slug === 'projects/chronomancer')!; + expect(hit.title).toBe('Chronomancer Codex Ledger'); + expect(hit.score).toBeGreaterThan(0); + // Shaped like a keyword-arm row: representative chunk attached. + expect(hit.chunk_text).toContain('reference document'); + expect(hit.chunk_source).toBe('compiled_truth'); + }); + + test('long 10-content-token exact-title query still retrieves (no token-count gate)', async () => { + const longTitle = 'Emerald Falcon Doctrine Quarterly Synthesis Report Alpha Bravo Charlie Delta'; + await engine.putPage('reports/emerald-falcon', { + type: 'note', + title: longTitle, + compiled_truth: 'An annual planning artifact.', + }); + await engine.upsertChunks('reports/emerald-falcon', [ + { chunk_index: 0, chunk_text: 'An annual planning artifact.', chunk_source: 'compiled_truth' }, + ]); + + const hits = await engine.searchTitles(longTitle, { limit: 10 }); + expect(hits.map(r => r.slug)).toContain('reports/emerald-falcon'); + }); + + test('representative chunk prefers compiled_truth, else lowest chunk_index', async () => { + await engine.putPage('notes/mixed-chunks', { + type: 'note', + title: 'Obsidian Waterfall Registry', + compiled_truth: 'body text here', + }); + await engine.upsertChunks('notes/mixed-chunks', [ + { chunk_index: 0, chunk_text: 'timeline entry text', chunk_source: 'timeline' }, + { chunk_index: 1, chunk_text: 'compiled body text', chunk_source: 'compiled_truth' }, + ]); + const hits = await engine.searchTitles('Obsidian Waterfall Registry', { limit: 5 }); + const hit = hits.find(r => r.slug === 'notes/mixed-chunks')!; + expect(hit.chunk_source).toBe('compiled_truth'); + expect(hit.chunk_index).toBe(1); + + await engine.putPage('notes/timeline-only', { + type: 'note', + title: 'Cobalt Meridian Atlas', + compiled_truth: 'unrelated body', + }); + await engine.upsertChunks('notes/timeline-only', [ + { chunk_index: 5, chunk_text: 'later timeline', chunk_source: 'timeline' }, + { chunk_index: 2, chunk_text: 'earlier timeline', chunk_source: 'timeline' }, + ]); + const tlHits = await engine.searchTitles('Cobalt Meridian Atlas', { limit: 5 }); + const tlHit = tlHits.find(r => r.slug === 'notes/timeline-only')!; + expect(tlHit.chunk_index).toBe(2); // lowest index when no compiled_truth chunk + }); + + test('respects soft-delete visibility and source scoping', async () => { + await seedTitleOnlyPage(); + + // Source scope that doesn't own the page → filtered out at SQL level. + const scoped = await engine.searchTitles('Chronomancer Codex Ledger', { + limit: 10, + sourceId: 'some-other-source', + }); + expect(scoped.length).toBe(0); + + // Soft-deleted pages disappear (visibility clause). + await engine.softDeletePage('projects/chronomancer'); + const afterDelete = await engine.searchTitles('Chronomancer Codex Ledger', { limit: 10 }); + expect(afterDelete.map(r => r.slug)).not.toContain('projects/chronomancer'); + }); + + test('respects hard-exclude slug prefixes (test/ is excluded by default)', async () => { + await engine.putPage('test/hidden-fixture', { + type: 'note', + title: 'Zanzibar Protocol Manifest', + compiled_truth: 'fixture body', + }); + const hits = await engine.searchTitles('Zanzibar Protocol Manifest', { limit: 10 }); + expect(hits.map(r => r.slug)).not.toContain('test/hidden-fixture'); + }); +}); + +describe('searchKeyword — D2 AND→OR fallback', () => { + async function seedQuantumPage(): Promise { + await engine.putPage('notes/quantum', { + type: 'note', + title: 'Quantum Notes', + compiled_truth: 'quantum lattice harmonics resonance experiments', + }); + await engine.upsertChunks('notes/quantum', [ + { + chunk_index: 0, + chunk_text: 'quantum lattice harmonics resonance experiments', + chunk_source: 'compiled_truth', + }, + ]); + } + + test('one bad token no longer zeroes keyword recall (orFallback: true rescues)', async () => { + await seedQuantumPage(); + // Strict AND fails ('zzzmissingtoken' is nowhere); OR fallback rescues. + const hits = await engine.searchKeyword('quantum lattice harmonics zzzmissingtoken', { + limit: 10, + orFallback: true, + }); + expect(hits.map(r => r.slug)).toContain('notes/quantum'); + }); + + test('WITHOUT the orFallback flag the one-bad-token query returns zero (F1: strict default)', async () => { + await seedQuantumPage(); + // Precision consumers (countMentions, link-extraction, eval) call + // searchKeyword without the flag — their strict-AND contract must hold. + const hits = await engine.searchKeyword('quantum lattice harmonics zzzmissingtoken', { limit: 10 }); + expect(hits.length).toBe(0); + }); + + test('strict-AND results stay preferred: no OR dilution when AND matches', async () => { + await seedQuantumPage(); + await engine.putPage('notes/partial', { + type: 'note', + title: 'Partial Overlap', + compiled_truth: 'quantum computing conference recap', + }); + await engine.upsertChunks('notes/partial', [ + { chunk_index: 0, chunk_text: 'quantum computing conference recap', chunk_source: 'compiled_truth' }, + ]); + + // All four tokens co-occur only in notes/quantum → strict AND non-empty + // → the OR retry must NOT fire (even with the flag SET), so the + // partial-overlap page stays out. + const hits = await engine.searchKeyword('quantum lattice harmonics resonance', { + limit: 10, + orFallback: true, + }); + expect(hits.map(r => r.slug)).toContain('notes/quantum'); + expect(hits.map(r => r.slug)).not.toContain('notes/partial'); + }); + + test('single unmatched token returns empty (OR of one term is pointless)', async () => { + await seedQuantumPage(); + const hits = await engine.searchKeyword('zzznothinghere', { limit: 10, orFallback: true }); + expect(hits.length).toBe(0); + }); +}); + +describe('buildOrFallbackWebsearchQuery — pure', () => { + test('joins tokens with OR', () => { + expect(buildOrFallbackWebsearchQuery('alpha beta')).toBe('alpha OR beta'); + }); + test('returns null for <2 tokens', () => { + expect(buildOrFallbackWebsearchQuery('alpha')).toBeNull(); + expect(buildOrFallbackWebsearchQuery('')).toBeNull(); + expect(buildOrFallbackWebsearchQuery(' ')).toBeNull(); + }); + test('F3: refuses queries with websearch operators (negation must not resurrect)', () => { + // A `-bar` exclusion relaxed to `foo OR bar` would MATCH the excluded + // term; a quoted phrase would degrade to a bag of words. No fallback. + expect(buildOrFallbackWebsearchQuery('foo -bar')).toBeNull(); + expect(buildOrFallbackWebsearchQuery('"alpha beta" gamma')).toBeNull(); + expect(buildOrFallbackWebsearchQuery('"alpha beta" -gamma')).toBeNull(); + }); + test('interior hyphens are not operators — still relaxed', () => { + expect(buildOrFallbackWebsearchQuery('alpha-beta gamma')).toBe('alpha OR beta OR gamma'); + }); + test('drops literal OR/AND words so they cannot re-parse as operators', () => { + expect(buildOrFallbackWebsearchQuery('alpha or beta')).toBe('alpha OR beta'); + expect(buildOrFallbackWebsearchQuery('alpha AND beta')).toBe('alpha OR beta'); + // Only operator words survive tokenization → nothing left to relax. + expect(buildOrFallbackWebsearchQuery('or and')).toBeNull(); + }); +}); + +describe('hybridSearch wiring — title arm reaches the fused result set', () => { + test('exact-title query surfaces the page through hybridSearch (keyword-only path)', async () => { + await seedTitleOnlyPage(); + const results = await hybridSearch(engine, 'Chronomancer Codex Ledger', { limit: 5 }); + expect(results.map(r => r.slug)).toContain('projects/chronomancer'); + }); + + test('long exact-title query (>=8 content tokens) surfaces through hybridSearch', async () => { + const longTitle = 'Emerald Falcon Doctrine Quarterly Synthesis Report Alpha Bravo Charlie Delta'; + await engine.putPage('reports/emerald-falcon', { + type: 'note', + title: longTitle, + compiled_truth: 'An annual planning artifact.', + }); + await engine.upsertChunks('reports/emerald-falcon', [ + { chunk_index: 0, chunk_text: 'An annual planning artifact.', chunk_source: 'compiled_truth' }, + ]); + const results = await hybridSearch(engine, longTitle, { limit: 5 }); + expect(results.map(r => r.slug)).toContain('reports/emerald-falcon'); + }); + + test('body-only queries still work (no regression from the extra arm)', async () => { + await seedTitleOnlyPage(); + const results = await hybridSearch(engine, 'scheduling practices planning', { limit: 5 }); + expect(results.map(r => r.slug)).toContain('projects/chronomancer'); + }); + + test('hybrid keyword arm still opts into the OR fallback (F1: QA-verified behavior preserved)', async () => { + await seedTitleOnlyPage(); + // One bad token against body text: direct searchKeyword (no flag) finds + // nothing, but hybridSearch sets orFallback for its recall arm. + const q = 'scheduling practices zzzmissingtoken'; + expect((await engine.searchKeyword(q, { limit: 5 })).length).toBe(0); + const results = await hybridSearch(engine, q, { limit: 5 }); + expect(results.map(r => r.slug)).toContain('projects/chronomancer'); + }); +}); From 324c3553184759bd5f1aa6ad32c368d0a96ee19e Mon Sep 17 00:00:00 2001 From: Cossackx <121278003+Cossackx@users.noreply.github.com> Date: Mon, 20 Jul 2026 15:05:04 -0400 Subject: [PATCH 09/17] =?UTF-8?q?test(e2e):=20production=20guard=20?= =?UTF-8?q?=E2=80=94=20setupDB=20refuses=20non-test=20databases=20(#2957)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setupDB() TRUNCATEs every data table on whatever DATABASE_URL points at, and run-e2e.sh deliberately preserves an exported DATABASE_URL — one stray environment variable away from wiping a production brain, with no guard of any kind (found during an independent review, 2026-07-18). assertSafeE2eDatabaseUrl (pure, unit-tested) now runs before any connection: allowed when the database name carries "test" as a word segment (the gbrain_test convention used by CI and .env.testing.example), or when GBRAIN_E2E_ALLOW_DB names the exact database intentionally. Refusal is loud and actionable. 7 unit tests, no DB required. Co-authored-by: Aleksei Razsadin Co-authored-by: Claude Fable 5 --- test/e2e/db-guard.test.ts | 65 +++++++++++++++++++++++++++++++++++++++ test/e2e/helpers.ts | 35 +++++++++++++++++++++ 2 files changed, 100 insertions(+) create mode 100644 test/e2e/db-guard.test.ts diff --git a/test/e2e/db-guard.test.ts b/test/e2e/db-guard.test.ts new file mode 100644 index 000000000..e5245a411 --- /dev/null +++ b/test/e2e/db-guard.test.ts @@ -0,0 +1,65 @@ +/** + * Unit tests for the setupDB production guard (assertSafeE2eDatabaseUrl). + * Pure — no database connection; runs with or without DATABASE_URL set. + */ + +import { describe, test, expect } from 'bun:test'; +import { assertSafeE2eDatabaseUrl } from './helpers.ts'; + +const NO_ENV = {} as Record; + +describe('assertSafeE2eDatabaseUrl', () => { + test('allows the canonical CI test database', () => { + expect(() => + assertSafeE2eDatabaseUrl('postgresql://postgres:postgres@localhost:5433/gbrain_test', NO_ENV), + ).not.toThrow(); + }); + + test('allows test as any word segment', () => { + for (const name of ['test', 'test_gbrain', 'e2e-test', 'gbrain_test_2', 'TEST_DB']) { + expect(() => + assertSafeE2eDatabaseUrl(`postgresql://u:p@localhost:5432/${name}`, NO_ENV), + ).not.toThrow(); + } + }); + + test('refuses production-looking database names', () => { + for (const name of ['gbrain', 'postgres', 'prod', 'gbrain_live', 'contest', 'latest']) { + expect(() => + assertSafeE2eDatabaseUrl(`postgresql://u:p@localhost:5432/${name}`, NO_ENV), + ).toThrow(/does not look like a test database/); + } + }); + + test('refuses a Supabase-style pooler URL with a bare postgres db', () => { + expect(() => + assertSafeE2eDatabaseUrl( + 'postgresql://postgres.ref:pw@aws-0-us-east-1.pooler.supabase.com:6543/postgres', + NO_ENV, + ), + ).toThrow(/does not look like a test database/); + }); + + test('explicit exact-name override opts a non-test database in', () => { + expect(() => + assertSafeE2eDatabaseUrl('postgresql://u:p@localhost:5432/gbrain', { + GBRAIN_E2E_ALLOW_DB: 'gbrain', + }), + ).not.toThrow(); + }); + + test('override must match the exact database name', () => { + expect(() => + assertSafeE2eDatabaseUrl('postgresql://u:p@localhost:5432/gbrain', { + GBRAIN_E2E_ALLOW_DB: 'other_db', + }), + ).toThrow(/does not look like a test database/); + }); + + test('refuses unparseable URLs and missing database names', () => { + expect(() => assertSafeE2eDatabaseUrl('not a url', NO_ENV)).toThrow(/not a parseable URL/); + expect(() => assertSafeE2eDatabaseUrl('postgresql://u:p@localhost:5432/', NO_ENV)).toThrow( + /no database name/, + ); + }); +}); diff --git a/test/e2e/helpers.ts b/test/e2e/helpers.ts index 56cec2ff5..0d0fe762b 100644 --- a/test/e2e/helpers.ts +++ b/test/e2e/helpers.ts @@ -66,6 +66,40 @@ export function hasDatabase(): boolean { return !!DATABASE_URL; } +/** + * Production guard: setupDB() TRUNCATEs every data table on whatever + * DATABASE_URL points at, and run-e2e.sh deliberately preserves an exported + * DATABASE_URL — so a developer with a production URL in their environment + * would wipe their real brain by running the suite. Refuse unless the + * database name identifies itself as a test database ("test" as a word + * segment, e.g. gbrain_test — the CI/.env.testing.example convention), or + * the operator explicitly opts the exact name in via GBRAIN_E2E_ALLOW_DB. + * + * Exported for unit testing; pure — no connection is made. + */ +export function assertSafeE2eDatabaseUrl( + url: string, + env: Record = process.env, +): void { + let dbName: string; + try { + dbName = decodeURIComponent(new URL(url).pathname.replace(/^\//, '')); + } catch { + throw new Error(`E2E guard: DATABASE_URL is not a parseable URL; refusing to run destructive setup.`); + } + if (!dbName) { + throw new Error(`E2E guard: DATABASE_URL has no database name; refusing to run destructive setup.`); + } + if (/(^|[_-])test([_-]|$)/i.test(dbName)) return; + if (env.GBRAIN_E2E_ALLOW_DB && env.GBRAIN_E2E_ALLOW_DB === dbName) return; + throw new Error( + `E2E guard: database "${dbName}" does not look like a test database ` + + `(expected "test" as a name segment, e.g. gbrain_test). setupDB() would ` + + `TRUNCATE every data table in it. If this is intentional, set ` + + `GBRAIN_E2E_ALLOW_DB=${dbName} to opt in explicitly.`, + ); +} + /** * Connect to DB, run schema init, truncate all tables. * Call in beforeAll() of each test file. @@ -74,6 +108,7 @@ export async function setupDB(): Promise { if (!DATABASE_URL) { throw new Error('DATABASE_URL not set. Copy .env.testing.example to .env.testing and configure it.'); } + assertSafeE2eDatabaseUrl(DATABASE_URL); // Disconnect any prior connection (clean slate) await db.disconnect(); From 6498b872ea54e6f78d28381f748cc6d1624c4922 Mon Sep 17 00:00:00 2001 From: Anton Senkovskiy Date: Mon, 20 Jul 2026 20:16:57 +0100 Subject: [PATCH 10/17] fix(extract): --dry-run must not write extract_rollup_7d (#2994) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `extract-conversation-facts --dry-run` promises "no DB writes, no checkpoint advance" (help text) and correctly skips the fact INSERTs, orphan delete, checkpoint advance, and receipt page. But writeRunReceiptAndRollup was called unconditionally at both exit paths, and its upsertExtractRollup always UPSERTs a row into extract_rollup_7d ("ALWAYS fire so doctor's extract_health sees the cycle ran") — so a dry run mutates the DB. Gate both writeRunReceiptAndRollup call sites on !dryRun. The writer returns void and its only non-rollup action (the receipt page) is already suppressed in dry-run via facts_inserted > 0, so gating at the call site skips nothing else. Mirrors the existing !dryRun guards on the fact-insert / checkpoint / audit paths. Regression test: a dry run leaves extract_rollup_7d empty. Fails before (row count 1), passes after. Hermetic PGLite + stubbed transports, no live LLM. Note: --dry-run still calls the extractor (LLM) by design — facts_extracted is the reported "would extract N" preview count; left unchanged. --- src/commands/extract-conversation-facts.ts | 7 +++++-- test/extract-conversation-facts.test.ts | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/commands/extract-conversation-facts.ts b/src/commands/extract-conversation-facts.ts index f1d04e4e3..65e8a4ac4 100644 --- a/src/commands/extract-conversation-facts.ts +++ b/src/commands/extract-conversation-facts.ts @@ -1069,7 +1069,8 @@ export async function runExtractConversationFactsCore( } // Fall through to receipt+rollup write so the partial run is // still observable in extract_health doctor + extracts/ pages. - await writeRunReceiptAndRollup(engine, sourceId, result, /* halted */ true); + // ...but not under --dry-run: a preview must not persist cache state. + if (!dryRun) await writeRunReceiptAndRollup(engine, sourceId, result, /* halted */ true); // Return partial result — caller (CLI / Minion) decides how to // surface. NOT a thrown failure. return result; @@ -1081,7 +1082,9 @@ export async function runExtractConversationFactsCore( // (queryable + citable per D-EXTRACT-17/19) AND UPSERTs the per-day // rollup row (best-effort cache per F-OUT-19). Both are best-effort — // failures stderr-warn but never fail the parent operation. - await writeRunReceiptAndRollup(engine, sourceId, result, /* halted */ false); + // --dry-run must not persist cache/knowledge state: skip the rollup UPSERT + + // receipt-page write so a preview leaves no extract cache row behind. + if (!dryRun) await writeRunReceiptAndRollup(engine, sourceId, result, /* halted */ false); return result; } diff --git a/test/extract-conversation-facts.test.ts b/test/extract-conversation-facts.test.ts index bcc0e4d1f..865022c13 100644 --- a/test/extract-conversation-facts.test.ts +++ b/test/extract-conversation-facts.test.ts @@ -306,6 +306,7 @@ describe('runExtractConversationFactsCore', () => { // truncation semantics than the canonical reset helper. await engine.executeRaw(`DELETE FROM facts WHERE source LIKE 'cli:extract-conversation-facts%'`); await engine.executeRaw(`DELETE FROM op_checkpoints WHERE op = 'extract-conversation-facts'`); + await engine.executeRaw(`DELETE FROM extract_rollup_7d`); await engine.executeRaw(`DELETE FROM pages WHERE slug LIKE 'conversations/%' OR slug LIKE 'people/alice%'`); // Set facts.extraction_enabled=true so kill-switch doesn't refuse. await engine.setConfig('facts.extraction_enabled', 'true'); @@ -365,6 +366,21 @@ describe('runExtractConversationFactsCore', () => { expect(result.segments_processed).toBeGreaterThanOrEqual(1); }); + test('dry-run does not write the extract_rollup_7d cache row', async () => { + // Regression: --dry-run promises "no DB writes" but writeRunReceiptAndRollup + // upsert-ed extract_rollup_7d unconditionally. A preview must not mutate the DB. + await runExtractConversationFactsCore(engine, { + sourceId: 'default', + slug: 'conversations/imessage/alice-example', + dryRun: true, + sleepMs: 0, + }); + const rows = await engine.executeRaw<{ count: string | number }>( + `SELECT COUNT(*) AS count FROM extract_rollup_7d WHERE kind = 'facts.conversation' AND source_id = 'default'`, + ); + expect(Number(rows[0]?.count ?? 0)).toBe(0); + }); + test('non-conversation pages are skipped', async () => { const result = await runExtractConversationFactsCore(engine, { sourceId: 'default', From 4528bfa79cf197c842a4da519b7ce462b7677247 Mon Sep 17 00:00:00 2001 From: Nazim22 <34912639+Nazim22@users.noreply.github.com> Date: Mon, 20 Jul 2026 15:00:16 -0500 Subject: [PATCH 11/17] feat(cli): GBRAIN_DRAIN_TIMEOUT_MS env override for the per-sink teardown drain budget (#2996) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one-shot CLI teardown drains fire-and-forget background sinks with a hardcoded 2s per-sink budget (DEFAULT_DRAIN_TIMEOUT_MS). That budget assumes a sub-second cloud chat provider; on a self-hosted provider (e.g. an ollama model at 10-20s per completion) a facts:absorb extraction can never finish inside it, so every one-shot CLI exit — sync timers especially — aborts the in-flight chat with 'pipeline_error: The operation was aborted', and the same touched pages retry-and-abort on every subsequent sync. Facts from those pages silently never land, and doctor's facts_extraction_health warns permanently. Fix: resolveDrainTimeoutMs() — GBRAIN_DRAIN_TIMEOUT_MS env override (same env-only escape-hatch pattern as GBRAIN_TEARDOWN_DEADLINE_MS and GBRAIN_FLUSH_GRACE_MS) over the 2000ms default. Explicit drainTimeoutMs from a call site still wins; computeTeardownDeadlineMs already computes the backstop from the resolved value, so the deadline scales with it. Garbage/zero/negative env values fall back to the default. Tests: default, env override, garbage/zero/negative fallback, finishCliTeardown drains with the env-resolved budget, explicit opts still win over env. Co-authored-by: Claude Fable 5 --- src/core/cli-force-exit.ts | 27 ++++++++++++-- test/cli-finish-teardown.test.ts | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 2 deletions(-) diff --git a/src/core/cli-force-exit.ts b/src/core/cli-force-exit.ts index a83edbdaf..982b7da87 100644 --- a/src/core/cli-force-exit.ts +++ b/src/core/cli-force-exit.ts @@ -114,6 +114,26 @@ function resolveFlushGraceMs(): number { /** Default per-sink drain budget (matches drainAllBackgroundWorkForCliExit). */ const DEFAULT_DRAIN_TIMEOUT_MS = 2_000; +/** + * Resolve the per-sink drain budget: `GBRAIN_DRAIN_TIMEOUT_MS` env override + * (slow-provider escape hatch, same env-only pattern as + * GBRAIN_TEARDOWN_DEADLINE_MS) over the 2000ms default. An explicit + * `drainTimeoutMs` from a call site still wins — the env replaces only the + * DEFAULT. The 2s default assumes a sub-second cloud chat provider; a + * self-hosted model (e.g. ollama at 10-20s per completion) can never finish a + * fire-and-forget facts:absorb extraction inside it, so every one-shot CLI + * exit — sync timers especially — aborts the in-flight chat and the + * extraction never lands, retrying (and re-aborting) on each subsequent sync + * of the same page. Raising the budget via env lets those installs drain + * instead of abort; computeTeardownDeadlineMs already scales the backstop + * from the resolved value, so the deadline widens with it. + */ +export function resolveDrainTimeoutMs(): number { + const env = Number(process.env.GBRAIN_DRAIN_TIMEOUT_MS); + if (Number.isFinite(env) && env > 0) return env; + return DEFAULT_DRAIN_TIMEOUT_MS; +} + /** * Backstop deadline for drain + disconnect COMBINED, computed from the bounds * it guards so it fires only when a component violated its own bound (#2084 @@ -262,7 +282,10 @@ export function flushThenExit(code: number, opts: FlushThenExitOpts = {}): void export interface FinishCliTeardownOpts { /** Engine to disconnect. A disconnect throw is warned + swallowed (D3). */ engine: { disconnect(): Promise }; - /** Per-sink drain budget. Default 2000 (the registry default). */ + /** + * Per-sink drain budget. Default: `GBRAIN_DRAIN_TIMEOUT_MS` env override, + * else 2000 (the registry default). + */ drainTimeoutMs?: number; /** Test seam — wins over the env override and the computed formula. */ deadlineMs?: number; @@ -284,7 +307,7 @@ export interface FinishCliTeardownOpts { * exit in here, and it means a component violated its own bound. */ export async function finishCliTeardown(opts: FinishCliTeardownOpts): Promise { - const drainTimeoutMs = opts.drainTimeoutMs ?? DEFAULT_DRAIN_TIMEOUT_MS; + const drainTimeoutMs = opts.drainTimeoutMs ?? resolveDrainTimeoutMs(); const warn = opts.warn ?? ((m: string) => console.warn(m)); const drain = opts.drain ?? drainAllBackgroundWorkForCliExit; const deadlineMs = diff --git a/test/cli-finish-teardown.test.ts b/test/cli-finish-teardown.test.ts index fa1b4e4df..c1ee39e73 100644 --- a/test/cli-finish-teardown.test.ts +++ b/test/cli-finish-teardown.test.ts @@ -14,6 +14,7 @@ import { finishCliTeardown, flushThenExit, computeTeardownDeadlineMs, + resolveDrainTimeoutMs, TEARDOWN_DEADLINE_FLOOR_MS, setCliExitVerdict, currentExitCode, @@ -115,6 +116,67 @@ describe('computeTeardownDeadlineMs', () => { }); }); +describe('resolveDrainTimeoutMs', () => { + test('defaults to the 2000ms registry budget', () => { + expect(resolveDrainTimeoutMs()).toBe(2_000); + }); + + test('GBRAIN_DRAIN_TIMEOUT_MS env override wins over the default', async () => { + await withEnv({ GBRAIN_DRAIN_TIMEOUT_MS: '30000' }, async () => { + expect(resolveDrainTimeoutMs()).toBe(30_000); + }); + }); + + test('garbage, zero, and negative env values fall back to the default', async () => { + await withEnv({ GBRAIN_DRAIN_TIMEOUT_MS: 'banana' }, async () => { + expect(resolveDrainTimeoutMs()).toBe(2_000); + }); + await withEnv({ GBRAIN_DRAIN_TIMEOUT_MS: '0' }, async () => { + expect(resolveDrainTimeoutMs()).toBe(2_000); + }); + await withEnv({ GBRAIN_DRAIN_TIMEOUT_MS: '-5' }, async () => { + expect(resolveDrainTimeoutMs()).toBe(2_000); + }); + }); + + test('finishCliTeardown drains with the env-resolved budget when no explicit drainTimeoutMs', async () => { + await withEnv({ GBRAIN_DRAIN_TIMEOUT_MS: '12345' }, async () => { + let drainBudget = -1; + await finishCliTeardown({ + engine: { disconnect: async () => {} }, + deadlineMs: 250, + drain: async ({ timeoutMs }) => { + drainBudget = timeoutMs; + }, + exit: () => {}, + warn: () => {}, + stdout: fakeStream(), + stderr: fakeStream(), + }); + expect(drainBudget).toBe(12_345); + }); + }); + + test('an explicit drainTimeoutMs still wins over the env override', async () => { + await withEnv({ GBRAIN_DRAIN_TIMEOUT_MS: '12345' }, async () => { + let drainBudget = -1; + await finishCliTeardown({ + engine: { disconnect: async () => {} }, + drainTimeoutMs: 777, + deadlineMs: 250, + drain: async ({ timeoutMs }) => { + drainBudget = timeoutMs; + }, + exit: () => {}, + warn: () => {}, + stdout: fakeStream(), + stderr: fakeStream(), + }); + expect(drainBudget).toBe(777); + }); + }); +}); + describe('finishCliTeardown — clean path', () => { test('drains with the injected budget, disconnects, returns; no exit, no warn', async () => { const calls: string[] = []; From f3e78fd2fb8798348ec458f80c165230d37a18b6 Mon Sep 17 00:00:00 2001 From: hhamilton-fv Date: Mon, 20 Jul 2026 13:07:27 -0700 Subject: [PATCH 12/17] fix(providers): route diagnostics through buildGatewayConfig so file-plane keys are visible (#3000) configureFromEnv() hand-assembled its own AIGatewayConfig instead of calling buildGatewayConfig(), the single seam that folds file-plane API keys (openrouter_api_key, zeroentropy_api_key, ...) into the gateway env. That let `gbrain providers list`/`test` report a provider as missing env even when it was correctly set in ~/.gbrain/config.json and the real gateway path resolved it fine. Both configureFromEnv() and runList() now build their env through buildGatewayConfig() (falling back to a bare process.env passthrough pre-init), matching what init-provider-picker.ts already does. --- src/commands/providers.ts | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/commands/providers.ts b/src/commands/providers.ts index a07fab359..68c22c7a2 100644 --- a/src/commands/providers.ts +++ b/src/commands/providers.ts @@ -9,6 +9,7 @@ import { listRecipes, getRecipe } from '../core/ai/recipes/index.ts'; import { configureGateway, embedOne, isAvailable as gwIsAvailable, chat as gwChat } from '../core/ai/gateway.ts'; import { probeOllama, probeLMStudio } from '../core/ai/probes.ts'; import { loadConfig } from '../core/config.ts'; +import { buildGatewayConfig } from '../core/ai/build-gateway-config.ts'; import { AIConfigError, AITransientError } from '../core/ai/errors.ts'; import type { Recipe } from '../core/ai/types.ts'; @@ -33,16 +34,19 @@ interface ProviderOption { function configureFromEnv(): void { const config = loadConfig(); - configureGateway({ - embedding_model: config?.embedding_model, - embedding_dimensions: config?.embedding_dimensions, - expansion_model: config?.expansion_model, - chat_model: config?.chat_model, - chat_fallback_chain: config?.chat_fallback_chain, - base_urls: config?.provider_base_urls, - provider_chat_options: config?.provider_chat_options, - env: { ...process.env }, - }); + // Route through buildGatewayConfig — the single ownership seam that folds + // file-plane API keys (openrouter_api_key, zeroentropy_api_key, ...) into + // the gateway env — instead of hand-assembling AIGatewayConfig field by + // field. Hand-building it here let this diagnostic report a provider as + // missing env even when ~/.gbrain/config.json had it and the real gateway + // path resolved it fine (#2728). Pre-init (no file-plane config yet) falls + // back to a bare env passthrough so the command still works before + // `gbrain init`. + if (config) { + configureGateway(buildGatewayConfig(config)); + return; + } + configureGateway({ env: { ...process.env } }); } export function envReady(recipe: Recipe, env: NodeJS.ProcessEnv = process.env): boolean { @@ -137,7 +141,12 @@ EXAMPLES } function runList(_args: string[]): void { - console.log(formatRecipeTable(listRecipes())); + // Same env the gateway actually sees (file-plane keys folded in), not bare + // process.env — keeps this table's STATUS column honest with what + // `providers test` (and the real init/gateway path) would report. + const cfg = loadConfig(); + const env = cfg ? buildGatewayConfig(cfg).env : process.env; + console.log(formatRecipeTable(listRecipes(), env)); } async function runTest(args: string[]): Promise { From c873ce3014439b4ec5a973c4edbec0ecfb261683 Mon Sep 17 00:00:00 2001 From: bo-developing Date: Mon, 20 Jul 2026 22:42:41 +0200 Subject: [PATCH 13/17] feat(ai): add Mistral provider recipe (#3001) Adds an EU-hosted provider covering embedding, expansion and chat on one OpenAI-compatible endpoint (https://api.mistral.ai/v1), so a brain that must stay inside EU jurisdiction does not need a US hop for any AI touchpoint. Every field is measured against the live API, not copied from docs: - mistral-embed is fixed 1024 dims and accepts no dimension parameter. Both spellings are rejected: {"dimensions": N} returns 400 extra_forbidden, {"output_dimension": N} returns 400 "does not support output_dimension". The generic openai-compatible branch of dimsProviderOptions() already falls through to `return undefined` for these model ids, so nothing is emitted. Same contract as voyage-4-nano, pinned by a negative assertion in the test. - max_batch_tokens 65536: a 65,286-token batch is accepted, 66,960 returns 400 code 3210 "Too many tokens overall, split into more batches." - chars_per_token 2: the value is a DIVISOR in splitByTokenBudget() (estTokens = text.length / charsPerToken), so lower is the conservative direction. The module default of 4 assumes English prose; a German-language corpus measured 3.58 chars/token, which the default overshoots toward overflow. codestral-embed is deliberately left out: it returns 1536 dims, and a touchpoint carries a single default_dims. Listing it under a 1024 declaration is the mixed-dim case embedding-dim-check.ts exists to catch. Co-authored-by: Claude Opus 4.8 (1M context) --- src/core/ai/recipes/index.ts | 2 + src/core/ai/recipes/mistral.ts | 84 +++++++++++++++++++++++++++++++++ src/core/embedding-pricing.ts | 3 ++ test/ai/recipe-mistral.test.ts | 85 ++++++++++++++++++++++++++++++++++ 4 files changed, 174 insertions(+) create mode 100644 src/core/ai/recipes/mistral.ts create mode 100644 test/ai/recipe-mistral.test.ts diff --git a/src/core/ai/recipes/index.ts b/src/core/ai/recipes/index.ts index 098691b46..2e6954089 100644 --- a/src/core/ai/recipes/index.ts +++ b/src/core/ai/recipes/index.ts @@ -24,6 +24,7 @@ import { azureOpenAI } from './azure-openai.ts'; import { zeroentropyai } from './zeroentropyai.ts'; import { llamaServerReranker } from './llama-server-reranker.ts'; import { moonshot } from './moonshot.ts'; +import { mistral } from './mistral.ts'; const ALL: Recipe[] = [ openai, @@ -44,6 +45,7 @@ const ALL: Recipe[] = [ azureOpenAI, zeroentropyai, moonshot, + mistral, ]; /** Map from `provider:id` key to recipe. */ diff --git a/src/core/ai/recipes/mistral.ts b/src/core/ai/recipes/mistral.ts new file mode 100644 index 000000000..c22cce549 --- /dev/null +++ b/src/core/ai/recipes/mistral.ts @@ -0,0 +1,84 @@ +import type { Recipe } from '../types.ts'; + +/** + * Mistral AI exposes an OpenAI-compatible API at https://api.mistral.ai/v1 + * (/embeddings + /chat/completions). EU-hosted — the reason this recipe + * exists: a brain that must stay inside EU jurisdiction can run embed + + * expansion + chat on a single provider without a US hop. + * + * Verified against the live API on 2026-07-19 (model catalog, embedding + * dimensions, dimension-parameter rejection, and the batch ceiling — see + * the notes on each field below). + * + * DIMENSIONS — mistral-embed is FIXED 1024 and accepts NO dimension + * parameter at all. Both spellings are rejected upstream: + * {"dimensions": 512} -> 400 extra_forbidden (not in the API schema) + * {"output_dimension": 512} -> 400 "This model does not support output_dimension" + * The generic `openai-compatible` branch of dims.ts:dimsProviderOptions() + * already falls through to `return undefined` for these model ids, so no + * dimension field is emitted. Do NOT add mistral-embed to any of the + * flexible-dim allowlists there — it would 400 every embed call. Same + * contract as voyage-4-nano, for the same reason. + * + * codestral-embed / codestral-embed-2505 are deliberately NOT listed: they + * return 1536 dims, and a touchpoint carries a single `default_dims`. + * Mixing them under a 1024 declaration is the mixed-dim footgun + * embedding-dim-check.ts exists to catch. They are code-retrieval models + * anyway; a prose brain wants mistral-embed. + */ +export const mistral: Recipe = { + id: 'mistral', + name: 'Mistral AI', + tier: 'openai-compat', + implementation: 'openai-compatible', + base_url_default: 'https://api.mistral.ai/v1', + auth_env: { + required: ['MISTRAL_API_KEY'], + setup_url: 'https://console.mistral.ai/api-keys', + }, + touchpoints: { + embedding: { + models: ['mistral-embed', 'mistral-embed-2312'], + default_dims: 1024, + // Mistral's published list price. Advisory only — canonical embedding + // spend accounting lives in src/core/embedding-pricing.ts. + cost_per_1m_tokens_usd: 0.1, + price_last_verified: '2026-07-19', + // Measured ceiling, not a doc guess: the /embeddings endpoint accepts a + // 65,286-token batch and rejects 66,960 with + // 400 code 3210 "Too many tokens overall, split into more batches." + // -> the real cap is 65,536 (64K) tokens per request. + max_batch_tokens: 65_536, + // chars_per_token is a DIVISOR in splitByTokenBudget() + // (estTokens = text.length / charsPerToken), so a LOWER value is the + // conservative direction. The module default of 4 is an English-prose + // assumption; German prose measured 3.58 here, and code/JSON/CJK runs + // denser still. 2 keeps the estimate above the real token count for + // every content shape we see. + chars_per_token: 2, + // With safety_factor 0.5 the pre-split budget is 32,768 estimated + // tokens = 65,536 chars. Worst realistic density (~1.5 chars/token) + // puts that at ~43.7K real tokens — still clear of the 64K ceiling. + safety_factor: 0.5, + }, + expansion: { + models: ['ministral-3b-latest', 'mistral-small-latest'], + price_last_verified: '2026-07-19', + }, + chat: { + models: [ + 'mistral-small-latest', 'mistral-medium-latest', 'mistral-large-latest', + 'ministral-3b-latest', 'ministral-8b-latest', 'magistral-small-latest', + ], + supports_tools: true, + // Same call as the Moonshot recipe: ordinary tool calls are fine, but + // gbrain's subagent loop stays Anthropic-pinned for stable tool_use_id + // behavior across crashes/replays. + supports_subagent_loop: false, + supports_prompt_cache: false, + max_context_tokens: 262144, + price_last_verified: '2026-07-19', + }, + }, + setup_hint: 'Get an API key at https://console.mistral.ai/api-keys, then `export MISTRAL_API_KEY=...` and use `mistral:mistral-embed` (1024 dims) for embeddings.', +}; diff --git a/src/core/embedding-pricing.ts b/src/core/embedding-pricing.ts index 1bb37375c..18774727d 100644 --- a/src/core/embedding-pricing.ts +++ b/src/core/embedding-pricing.ts @@ -37,6 +37,9 @@ export const EMBEDDING_PRICING: Record = { 'voyage:voyage-4-large': { pricePerMTok: 0.18 }, // ZeroEntropy (https://zeroentropy.dev/pricing — zembed-1) 'zeroentropyai:zembed-1': { pricePerMTok: 0.05 }, + // Mistral (https://mistral.ai/pricing/api/, verified 2026-07-19) + 'mistral:mistral-embed': { pricePerMTok: 0.10 }, + 'mistral:mistral-embed-2312': { pricePerMTok: 0.10 }, }; export type PriceLookupResult = diff --git a/test/ai/recipe-mistral.test.ts b/test/ai/recipe-mistral.test.ts new file mode 100644 index 000000000..80490cff6 --- /dev/null +++ b/test/ai/recipe-mistral.test.ts @@ -0,0 +1,85 @@ +/** + * Mistral recipe smoke. + * + * The load-bearing assertion here is the negative one: mistral-embed rejects + * every dimension parameter with HTTP 400, so dimsProviderOptions() must emit + * no dimension field for it. Same contract as voyage-4-nano, pinned the same + * way (see the negative regression assertion in test/ai/gateway.test.ts). + */ + +import { describe, expect, test } from 'bun:test'; +import { getRecipe } from '../../src/core/ai/recipes/index.ts'; +import { defaultResolveAuth } from '../../src/core/ai/gateway.ts'; +import { assertTouchpoint } from '../../src/core/ai/model-resolver.ts'; +import { AIConfigError } from '../../src/core/ai/errors.ts'; +import { dimsProviderOptions } from '../../src/core/ai/dims.ts'; +import { lookupEmbeddingPrice } from '../../src/core/embedding-pricing.ts'; + +describe('recipe: mistral', () => { + test('registered with expected OpenAI-compatible shape', () => { + const r = getRecipe('mistral'); + expect(r).toBeDefined(); + expect(r!.id).toBe('mistral'); + expect(r!.tier).toBe('openai-compat'); + expect(r!.implementation).toBe('openai-compatible'); + expect(r!.base_url_default).toBe('https://api.mistral.ai/v1'); + expect(r!.auth_env?.required).toEqual(['MISTRAL_API_KEY']); + }); + + test('embedding touchpoint pins the measured 1024 dims and 64K batch ceiling', () => { + const e = getRecipe('mistral')!.touchpoints.embedding; + expect(e).toBeDefined(); + expect(e!.models).toContain('mistral-embed'); + expect(e!.default_dims).toBe(1024); + // Measured: a 65,286-token batch is accepted, 66,960 returns 400 code 3210. + expect(e!.max_batch_tokens).toBe(65_536); + // chars_per_token is a DIVISOR in splitByTokenBudget(), so a lower value + // is the conservative direction. The module default of 4 is an English + // assumption and overshoots on denser prose. + expect(e!.chars_per_token).toBe(2); + }); + + test('NEGATIVE: no dimension parameter is emitted for mistral-embed', () => { + // Mistral rejects both spellings: + // {"dimensions": N} -> 400 extra_forbidden + // {"output_dimension": N} -> 400 "does not support output_dimension" + // If a future change adds mistral-embed to a flexible-dim allowlist in + // dims.ts, this assertion fails before it reaches users as a 400 on every + // embed call. + expect(dimsProviderOptions('openai-compatible', 'mistral-embed', 1024)).toBeUndefined(); + expect(dimsProviderOptions('openai-compatible', 'mistral-embed-2312', 1024)).toBeUndefined(); + }); + + test('embedding models resolve to a known price', () => { + // An unknown price makes the embedding spend cap fail closed. + expect(lookupEmbeddingPrice('mistral:mistral-embed').kind).toBe('known'); + expect(lookupEmbeddingPrice('mistral:mistral-embed-2312').kind).toBe('known'); + }); + + test('chat and expansion touchpoints accept their configured models', () => { + const r = getRecipe('mistral')!; + expect(r.touchpoints.chat!.supports_tools).toBe(true); + expect(r.touchpoints.chat!.supports_subagent_loop).toBe(false); + expect(() => assertTouchpoint(r, 'chat', 'mistral-small-latest')).not.toThrow(); + expect(() => assertTouchpoint(r, 'expansion', 'ministral-3b-latest')).not.toThrow(); + expect(() => assertTouchpoint(r, 'embedding', 'mistral-embed')).not.toThrow(); + }); + + test('codestral-embed is deliberately absent (1536 dims would mix under a 1024 declaration)', () => { + const e = getRecipe('mistral')!.touchpoints.embedding!; + expect(e.models).not.toContain('codestral-embed'); + expect(e.models).not.toContain('codestral-embed-2505'); + }); + + test('default auth: MISTRAL_API_KEY set -> Bearer token', () => { + const r = getRecipe('mistral')!; + const auth = defaultResolveAuth(r, { MISTRAL_API_KEY: 'fake-mistral-key' }, 'embedding'); + expect(auth.headerName).toBe('Authorization'); + expect(auth.token).toBe('Bearer fake-mistral-key'); + }); + + test('default auth: missing MISTRAL_API_KEY -> AIConfigError', () => { + const r = getRecipe('mistral')!; + expect(() => defaultResolveAuth(r, {}, 'embedding')).toThrow(AIConfigError); + }); +}); From 23e0541d9b9b44a33d07e6a998eeb945975abbe2 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 05:56:00 +0900 Subject: [PATCH 14/17] =?UTF-8?q?fix(cycle):=20budget=20the=20patterns=20s?= =?UTF-8?q?ubagent=20from=20remaining=20job=20time=20=E2=80=94=20one=20pha?= =?UTF-8?q?se's=20fixed=2035-min=20worst=20case=20defeats=20any=20interval?= =?UTF-8?q?-derived=20cycle=20budget=20(#2781)=20(#2959)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cycle): budget the patterns subagent from remaining job time, not a fixed constant (#2781) The autopilot-cycle job gets an interval-derived timeout stamped at submit, but the patterns phase submits its subagent with a fixed 30-min job timeout and waits up to 35 min — one phase's worst case exceeds ANY interval-derived budget <= 35 min, so the parent job dead-letters mid-patterns and the tail phases (consolidate -> schema-suggest) starve for days (#2781, the deeper half left open by the #2852 dispatch-floor fix). - MinionJobContext.deadlineAtMs: absolute deadline from the claim-time timeout_at stamp (the DB ground truth handleTimeouts() sweeps against; re-stamped on every claim so retries get a fresh budget). Null when the job has no per-job timeout. - worker: the per-job abort timer now derives its delay from timeout_at when present, so the in-process timer, the DB sweeper, and the handler-visible deadline agree on ONE absolute instant. - autopilot-cycle + autopilot-global-maintenance handlers thread deadlineAtMs into runCycle; CycleOpts carries it to the patterns phase. - patterns: clampSubagentBudgets() derives BOTH the child job timeout and the wait timeout from the same child deadline (parent deadline minus a 60s stop-margin reserve — enough for the wait poll + force-evict grace + cleanup, deliberately NOT a promise that tail phases complete). Under a 2-min minimum the phase skips honestly (insufficient_cycle_budget) instead of submitting a guaranteed-kill LLM call; the next cycle retries with a fresh budget. - Direct callers (gbrain dream) pass no deadline and keep the configured timeouts unchanged. Follow-up (separate PR): synthesize has the same shape plus sequential per-child waits that accumulate N x subagent_wait_timeout_ms past any parent budget; it needs per-wait remaining-time recomputation. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RaNDjecPPvhC7LRqkxkhGF * fix(cycle): address review — cancel timed-out patterns child; thread deadline through phase-wrapper handlers - P1: the child's timeout_ms clock starts at ITS claim, so a queued child could outlive the parent deadline the wait was clamped to. On wait timeout, cancelJob strips it (waiting -> cancelled; active -> lock stripped, worker abort fires next renew tick). - P2: makePhaseHandler (standalone patterns/synthesize/... minion jobs) now threads job.deadlineAtMs into runCycle like the autopilot handlers. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RaNDjecPPvhC7LRqkxkhGF --------- Co-authored-by: Claude Fable 5 --- src/commands/jobs.ts | 3 + src/core/cycle.ts | 11 ++ src/core/cycle/patterns.ts | 82 ++++++++- src/core/minions/types.ts | 6 + src/core/minions/worker.ts | 12 +- test/cycle-patterns-deadline-budget.test.ts | 168 ++++++++++++++++++ test/e2e/ingestion-roundtrip.test.ts | 1 + ...bagent-crash-replay-multi-provider.test.ts | 1 + test/e2e/subagent-gateway-path.test.ts | 1 + ...gent-gateway-resume-reconciliation.test.ts | 2 +- test/handlers-embed-backfill.test.ts | 1 + test/ingestion/ingest-capture.test.ts | 1 + test/minions-shell.test.ts | 1 + test/subagent-aggregator.test.ts | 1 + test/subagent-handler.test.ts | 1 + 15 files changed, 285 insertions(+), 7 deletions(-) create mode 100644 test/cycle-patterns-deadline-budget.test.ts diff --git a/src/commands/jobs.ts b/src/commands/jobs.ts index 52ba691ce..46f1027f5 100644 --- a/src/commands/jobs.ts +++ b/src/commands/jobs.ts @@ -1796,6 +1796,7 @@ export async function registerBuiltinHandlers( brainDir: effectiveBrainDir, pull, signal: job.signal, // propagate abort so cycle bails on timeout/cancel + deadlineAtMs: job.deadlineAtMs, // #2781: phases budget sub-work from remaining time ...(sourceId ? { sourceId } : {}), ...(requestedPhases && requestedPhases.length > 0 ? { phases: requestedPhases as any } : {}), yieldBetweenPhases: async () => { @@ -1833,6 +1834,7 @@ export async function registerBuiltinHandlers( brainDir: repoPath, pull: false, // brain-wide DB/maintenance work never git-pulls signal: job.signal, + deadlineAtMs: job.deadlineAtMs, // #2781: phases budget sub-work from remaining time phases, yieldBetweenPhases: async () => { await new Promise((r) => setImmediate(r)); }, }); @@ -1978,6 +1980,7 @@ export async function registerBuiltinHandlers( brainDir: repoPath, phases: [phase as any], signal: job.signal, + deadlineAtMs: job.deadlineAtMs, // #2781: phases budget sub-work from remaining time }); return { phase, status: report.status, report }; }; diff --git a/src/core/cycle.ts b/src/core/cycle.ts index a52efcd89..100eab2a3 100644 --- a/src/core/cycle.ts +++ b/src/core/cycle.ts @@ -479,6 +479,16 @@ export interface CycleOpts { * Validated via `assertValidSourceId` in `cycleLockIdFor` (defense-in-depth). */ sourceId?: string; + /** + * Absolute wall-clock deadline (epoch ms) of the enclosing minion job, + * from `MinionJobContext.deadlineAtMs` (the claim-time `timeout_at` + * stamp). Phases that spawn bounded sub-work (patterns' subagent) clamp + * their own timeouts to the REMAINING time so one phase's fixed + * worst-case can't blow past the job budget and dead-letter the whole + * cycle mid-phase (#2781). Unset for direct callers (`gbrain dream`) — + * phases then use their configured timeouts unchanged. + */ + deadlineAtMs?: number | null; } // ─── Lock primitives ─────────────────────────────────────────────── @@ -1888,6 +1898,7 @@ export async function runCycle( brainDir, dryRun, yieldDuringPhase: opts.yieldDuringPhase, + deadlineAtMs: opts.deadlineAtMs ?? null, })); result.duration_ms = duration_ms; phaseResults.push(result); diff --git a/src/core/cycle/patterns.ts b/src/core/cycle/patterns.ts index d96584dad..788381a63 100644 --- a/src/core/cycle/patterns.ts +++ b/src/core/cycle/patterns.ts @@ -37,6 +37,57 @@ export interface PatternsPhaseOpts { brainDir: string; dryRun: boolean; yieldDuringPhase?: () => Promise; + /** + * Absolute deadline (epoch ms) of the enclosing minion job, or null for + * direct callers (`gbrain dream`). When set, the subagent's job timeout + * and the wait timeout are clamped so the phase finishes (or times out) + * BEFORE the parent job's budget expires — a fixed 30/35-min default + * inside an interval-derived cycle budget dead-letters the whole cycle + * mid-phase and starves every tail phase (#2781). + */ + deadlineAtMs?: number | null; +} + +/** + * Stop-margin reserved under the parent deadline when clamping subagent + * budgets. NOT a promise that tail phases complete — the cycle is allowed + * to go partial and resume next tick. This only guarantees the phase's + * wait returns and the handler unwinds cleanly before the worker's abort + * fires: wait poll interval (5s) + worker force-evict grace (30s) + lock + * and DB cleanup headroom. + */ +export const CYCLE_DEADLINE_RESERVE_MS = 60 * 1000; + +/** + * Smallest remaining budget worth submitting a subagent for. Below this, + * the LLM call is near-certain to be killed mid-flight — wasted spend and + * a guaranteed-timeout child — so the phase skips honestly instead + * (`insufficient_cycle_budget`) and the next cycle retries with a fresh + * budget. + */ +export const MIN_PATTERNS_SUBAGENT_BUDGET_MS = 2 * 60 * 1000; + +/** + * Clamp the configured subagent budgets to the remaining parent-job time. + * Both timeouts derive from the SAME absolute child deadline + * (`deadlineAtMs - reserve`) so the child job's kill switch and our wait + * agree. Returns null when the remaining budget is below the minimum — + * caller should skip the phase without submitting. + */ +export function clampSubagentBudgets( + config: { subagentTimeoutMs: number; subagentWaitTimeoutMs: number }, + deadlineAtMs: number | null | undefined, + nowMs: number, +): { timeoutMs: number; waitTimeoutMs: number } | null { + if (deadlineAtMs == null) { + return { timeoutMs: config.subagentTimeoutMs, waitTimeoutMs: config.subagentWaitTimeoutMs }; + } + const childBudgetMs = deadlineAtMs - CYCLE_DEADLINE_RESERVE_MS - nowMs; + if (childBudgetMs < MIN_PATTERNS_SUBAGENT_BUDGET_MS) return null; + return { + timeoutMs: Math.min(config.subagentTimeoutMs, childBudgetMs), + waitTimeoutMs: Math.min(config.subagentWaitTimeoutMs, childBudgetMs), + }; } export async function runPhasePatterns( @@ -90,6 +141,19 @@ export async function runPhasePatterns( 'skills/_brain-filing-rules.json missing dream_synthesize_paths.globs')); } + // #2781: budget the subagent from the REMAINING parent-job time, not + // the fixed config default. Checked after the cheap gates (disabled / + // insufficient_evidence / no_provider) so a skip for budget reasons + // only fires when the phase would otherwise have submitted. + const budgets = clampSubagentBudgets(config, opts.deadlineAtMs, Date.now()); + if (budgets === null) { + return skipped( + 'insufficient_cycle_budget', + `remaining cycle budget under ${Math.round(MIN_PATTERNS_SUBAGENT_BUDGET_MS / 1000)}s ` + + `(reserve ${Math.round(CYCLE_DEADLINE_RESERVE_MS / 1000)}s); next cycle retries with a fresh budget`, + ); + } + const queue = new MinionQueue(engine); const data: SubagentHandlerData = { prompt: buildPatternsPrompt(reflections, config.minEvidence, config.outputRoot), @@ -99,7 +163,7 @@ export async function runPhasePatterns( }; const submitOpts: Partial = { max_stalled: 3, - timeout_ms: config.subagentTimeoutMs, + timeout_ms: budgets.timeoutMs, }; const job = await queue.add('subagent', data as unknown as Record, submitOpts, { allowProtectedSubmit: true, @@ -108,13 +172,23 @@ export async function runPhasePatterns( let outcome: string; try { const final = await waitForCompletion(queue, job.id, { - timeoutMs: config.subagentWaitTimeoutMs, + timeoutMs: budgets.waitTimeoutMs, pollMs: 5 * 1000, }); outcome = final.status; } catch (e) { - if (e instanceof TimeoutError) outcome = 'timeout'; - else throw e; + if (e instanceof TimeoutError) { + outcome = 'timeout'; + // The child's own timeout_ms clock starts at ITS claim, not at + // submit — a child that sat queued behind other work can outlive + // the parent deadline this wait was clamped to. Cancel it so the + // subagent can't keep spending/writing after the phase gave up + // (waiting child → cancelled immediately; active child → lock + // stripped, worker abort fires on next renew tick). + try { await queue.cancelJob(job.id); } catch { /* best-effort */ } + } else { + throw e; + } } if (opts.yieldDuringPhase) { diff --git a/src/core/minions/types.ts b/src/core/minions/types.ts index 73877bb99..7a1b5f08e 100644 --- a/src/core/minions/types.ts +++ b/src/core/minions/types.ts @@ -200,6 +200,12 @@ export interface MinionJobContext { attempts_made: number; /** AbortSignal for cooperative cancellation (fires on timeout, cancel, pause, or lock loss). */ signal: AbortSignal; + /** Absolute wall-clock deadline (epoch ms) from the claim-time `timeout_at` stamp, + * or null when the job has no per-job timeout. This is the DB's ground truth — + * the same instant handleTimeouts() dead-letters against — so handlers that + * spawn bounded sub-work (e.g. autopilot-cycle's subagent phases) can budget + * from the REMAINING time instead of a fixed constant that may exceed it. */ + deadlineAtMs: number | null; /** AbortSignal that fires only on worker process SIGTERM/SIGINT. Handlers sensitive * to deploy restarts (e.g. the shell handler, which must run a SIGTERM → 5s → SIGKILL * sequence on its child) listen to this in addition to `signal`. Most handlers can diff --git a/src/core/minions/worker.ts b/src/core/minions/worker.ts index beb3055ba..3117eed34 100644 --- a/src/core/minions/worker.ts +++ b/src/core/minions/worker.ts @@ -900,15 +900,22 @@ export class MinionWorker extends EventEmitter { // Per-job wall-clock timeout (timer-armed only if `timeout_ms` was // set on the job; the grace-evict pattern above now lives outside - // this branch). + // this branch). The delay derives from the claim-time `timeout_at` + // stamp when present so this timer, the DB sweeper (handleTimeouts), + // and the handler-visible `deadlineAtMs` all agree on ONE absolute + // deadline instead of three clocks started at slightly different + // instants. let timeoutTimer: ReturnType | null = null; if (job.timeout_ms != null) { + const delayMs = job.timeout_at != null + ? Math.max(0, job.timeout_at.getTime() - Date.now()) + : job.timeout_ms; timeoutTimer = setTimeout(() => { if (!abort.signal.aborted) { console.warn(`Job ${job.id} (${job.name}) hit per-job timeout (${job.timeout_ms}ms), aborting`); abort.abort(new Error('timeout')); } - }, job.timeout_ms); + }, delayMs); } const promise = this.executeJob(job, lockToken, abort, lockTimer) @@ -964,6 +971,7 @@ export class MinionWorker extends EventEmitter { data: job.data, attempts_made: job.attempts_made, signal: abort.signal, + deadlineAtMs: job.timeout_at != null ? job.timeout_at.getTime() : null, shutdownSignal: this.shutdownAbort.signal, updateProgress: async (progress: unknown) => { await this.queue.updateProgress(job.id, lockToken, progress); diff --git a/test/cycle-patterns-deadline-budget.test.ts b/test/cycle-patterns-deadline-budget.test.ts new file mode 100644 index 000000000..62bdf6ffe --- /dev/null +++ b/test/cycle-patterns-deadline-budget.test.ts @@ -0,0 +1,168 @@ +/** + * #2781 — patterns phase budgets its subagent from the REMAINING parent-job + * time instead of a fixed 30/35-min default that can exceed any + * interval-derived cycle budget and dead-letter the whole cycle mid-phase. + * + * Layers: + * 1. Unit tests on the exported pure `clampSubagentBudgets`. + * 2. A real-queue check that `claim` stamps `timeout_at` (the DB ground + * truth `deadlineAtMs` derives from) and leaves it null when the job + * has no per-job timeout. + * 3. Structural assertions pinning the wiring: worker → context → + * handler → runCycle → patterns (matches the house style of + * test/cycle-patterns.test.ts). + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { readFileSync } from 'fs'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { MinionQueue } from '../src/core/minions/queue.ts'; +import { + clampSubagentBudgets, + CYCLE_DEADLINE_RESERVE_MS, + MIN_PATTERNS_SUBAGENT_BUDGET_MS, +} from '../src/core/cycle/patterns.ts'; + +const CONFIG = { + subagentTimeoutMs: 30 * 60 * 1000, + subagentWaitTimeoutMs: 35 * 60 * 1000, +}; + +describe('clampSubagentBudgets', () => { + const now = 1_000_000_000_000; // fixed epoch ms; the function takes nowMs explicitly + + test('null deadline → config passthrough (direct `gbrain dream` back-compat)', () => { + expect(clampSubagentBudgets(CONFIG, null, now)).toEqual({ + timeoutMs: CONFIG.subagentTimeoutMs, + waitTimeoutMs: CONFIG.subagentWaitTimeoutMs, + }); + expect(clampSubagentBudgets(CONFIG, undefined, now)).toEqual({ + timeoutMs: CONFIG.subagentTimeoutMs, + waitTimeoutMs: CONFIG.subagentWaitTimeoutMs, + }); + }); + + test('deadline far away → config values win (no clamping)', () => { + const deadline = now + 2 * 60 * 60 * 1000; // 2h out + expect(clampSubagentBudgets(CONFIG, deadline, now)).toEqual({ + timeoutMs: CONFIG.subagentTimeoutMs, + waitTimeoutMs: CONFIG.subagentWaitTimeoutMs, + }); + }); + + test('deadline inside config window → BOTH timeouts clamp to the same child budget', () => { + const deadline = now + 10 * 60 * 1000; // 10 min out + const childBudget = deadline - CYCLE_DEADLINE_RESERVE_MS - now; // 9 min + const budgets = clampSubagentBudgets(CONFIG, deadline, now); + expect(budgets).toEqual({ timeoutMs: childBudget, waitTimeoutMs: childBudget }); + // The child's own kill switch never outlives the parent budget. + expect(budgets!.timeoutMs).toBeLessThanOrEqual(deadline - now); + }); + + test('remaining budget below minimum → null (caller skips, no submit)', () => { + const deadline = now + CYCLE_DEADLINE_RESERVE_MS + MIN_PATTERNS_SUBAGENT_BUDGET_MS - 1; + expect(clampSubagentBudgets(CONFIG, deadline, now)).toBeNull(); + }); + + test('boundary: exactly the minimum budget → submit allowed', () => { + const deadline = now + CYCLE_DEADLINE_RESERVE_MS + MIN_PATTERNS_SUBAGENT_BUDGET_MS; + expect(clampSubagentBudgets(CONFIG, deadline, now)).toEqual({ + timeoutMs: MIN_PATTERNS_SUBAGENT_BUDGET_MS, + waitTimeoutMs: MIN_PATTERNS_SUBAGENT_BUDGET_MS, + }); + }); + + test('deadline already past → null, never a negative timeout', () => { + expect(clampSubagentBudgets(CONFIG, now - 1000, now)).toBeNull(); + }); +}); + +describe('claim stamps timeout_at (deadlineAtMs ground truth)', () => { + let engine: PGLiteEngine; + let queue: MinionQueue; + + beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({ database_url: '' }); // in-memory + await engine.initSchema(); + queue = new MinionQueue(engine); + }); + + afterAll(async () => { + await engine.disconnect(); + }); + + test('job with timeout_ms → claim sets timeout_at ≈ now + timeout_ms', async () => { + const before = Date.now(); + await queue.add('sync', {}, { timeout_ms: 600_000 }); + const claimed = await queue.claim('tok-dl-1', 30000, 'default', ['sync']); + const after = Date.now(); + expect(claimed).not.toBeNull(); + expect(claimed!.timeout_at).not.toBeNull(); + const at = claimed!.timeout_at!.getTime(); + expect(at).toBeGreaterThanOrEqual(before + 600_000 - 5_000); + expect(at).toBeLessThanOrEqual(after + 600_000 + 5_000); + }); + + test('job without timeout_ms and no handler default → timeout_at stays null', async () => { + // 'sync' is not in the long-handler default set, so no stamp either way. + await queue.add('sync', { which: 'no-timeout' }); + // Drain the possibly-remaining job from the prior test first. + let claimed = await queue.claim('tok-dl-2', 30000, 'default', ['sync']); + while (claimed && claimed.timeout_ms != null) { + claimed = await queue.claim('tok-dl-2', 30000, 'default', ['sync']); + } + expect(claimed).not.toBeNull(); + expect(claimed!.timeout_ms).toBeNull(); + expect(claimed!.timeout_at).toBeNull(); + }); +}); + +describe('deadline plumbing wiring (structural)', () => { + const workerSrc = readFileSync(new URL('../src/core/minions/worker.ts', import.meta.url), 'utf-8'); + const jobsSrc = readFileSync(new URL('../src/commands/jobs.ts', import.meta.url), 'utf-8'); + const cycleSrc = readFileSync(new URL('../src/core/cycle.ts', import.meta.url), 'utf-8'); + const patternsSrc = readFileSync(new URL('../src/core/cycle/patterns.ts', import.meta.url), 'utf-8'); + + test('worker exposes deadlineAtMs from the claim-time timeout_at stamp', () => { + expect(workerSrc).toContain('deadlineAtMs: job.timeout_at != null ? job.timeout_at.getTime() : null'); + }); + + test('worker arms its abort timer from timeout_at when present (one absolute deadline)', () => { + expect(workerSrc).toContain('job.timeout_at.getTime() - Date.now()'); + }); + + test('autopilot-cycle, global-maintenance AND phase-wrapper handlers thread deadlineAtMs into runCycle', () => { + const matches = jobsSrc.match(/deadlineAtMs: job\.deadlineAtMs/g) ?? []; + expect(matches.length).toBe(3); + }); + + test('runCycle forwards deadlineAtMs to the patterns phase', () => { + expect(cycleSrc).toContain('deadlineAtMs: opts.deadlineAtMs ?? null'); + }); + + test('patterns submits + waits with the CLAMPED budgets, not raw config', () => { + expect(patternsSrc).toContain('timeout_ms: budgets.timeoutMs'); + expect(patternsSrc).toContain('timeoutMs: budgets.waitTimeoutMs'); + expect(patternsSrc).not.toContain('timeout_ms: config.subagentTimeoutMs'); + expect(patternsSrc).not.toContain('timeoutMs: config.subagentWaitTimeoutMs'); + }); + + test('patterns cancels the child on wait timeout (child clock starts at ITS claim)', () => { + // A child that sat queued can outlive the parent deadline the wait was + // clamped to; the timeout path must strip it so it can't keep spending. + expect(patternsSrc).toContain('queue.cancelJob(job.id)'); + }); + + test('patterns skips honestly when the remaining budget is too small', () => { + expect(patternsSrc).toContain('insufficient_cycle_budget'); + // Budget gate sits AFTER the provider probe so a no-provider brain + // still reports no_provider (cheaper, more actionable reason). + const probeIdx = patternsSrc.indexOf("skipped('no_provider'"); + // lastIndexOf: the doc comment on MIN_PATTERNS_SUBAGENT_BUDGET_MS + // mentions the reason string too; the CALL SITE is the later hit. + const budgetIdx = patternsSrc.lastIndexOf('insufficient_cycle_budget'); + expect(probeIdx).toBeGreaterThan(0); + expect(budgetIdx).toBeGreaterThan(probeIdx); + }); +}); diff --git a/test/e2e/ingestion-roundtrip.test.ts b/test/e2e/ingestion-roundtrip.test.ts index 732b1cf11..a0fae218c 100644 --- a/test/e2e/ingestion-roundtrip.test.ts +++ b/test/e2e/ingestion-roundtrip.test.ts @@ -49,6 +49,7 @@ function makeFakeJobCtx(data: Record): MinionJobContext { data, attempts_made: 1, signal: new AbortController().signal, + deadlineAtMs: null, shutdownSignal: new AbortController().signal, updateProgress: async () => {}, updateTokens: async () => {}, diff --git a/test/e2e/subagent-crash-replay-multi-provider.test.ts b/test/e2e/subagent-crash-replay-multi-provider.test.ts index 140350400..75935ee05 100644 --- a/test/e2e/subagent-crash-replay-multi-provider.test.ts +++ b/test/e2e/subagent-crash-replay-multi-provider.test.ts @@ -279,6 +279,7 @@ async function makeCrashedCtx(jobId: number, prompt: string, modelId: string): P data: { prompt, model: modelId }, attempts_made: 1, // crashed once signal: abortCtrl.signal, + deadlineAtMs: null, shutdownSignal: shutdownCtrl.signal, updateProgress: async () => {}, updateTokens: async () => {}, diff --git a/test/e2e/subagent-gateway-path.test.ts b/test/e2e/subagent-gateway-path.test.ts index 3fa1cedf8..98fdfdb07 100644 --- a/test/e2e/subagent-gateway-path.test.ts +++ b/test/e2e/subagent-gateway-path.test.ts @@ -92,6 +92,7 @@ async function makeFakeJob(opts: FakeJobOpts): Promise<{ jobId: number; ctx: Min data: { prompt: opts.prompt, model: opts.model, allowed_tools: opts.allowed_tools }, attempts_made: 0, signal: abortCtrl.signal, + deadlineAtMs: null, shutdownSignal: shutdownCtrl.signal, updateProgress: async () => {}, updateTokens: async (t) => { tokenSink.push(t); }, diff --git a/test/e2e/subagent-gateway-resume-reconciliation.test.ts b/test/e2e/subagent-gateway-resume-reconciliation.test.ts index d7c479a3a..cb38e3042 100644 --- a/test/e2e/subagent-gateway-resume-reconciliation.test.ts +++ b/test/e2e/subagent-gateway-resume-reconciliation.test.ts @@ -68,7 +68,7 @@ async function makeJob(prompt: string, model: string): Promise<{ jobId: number; const jobId = rows[0].id; const ctx: MinionJobContext = { id: jobId, name: 'subagent', data: { prompt, model }, attempts_made: 1, - signal: new AbortController().signal, shutdownSignal: new AbortController().signal, + signal: new AbortController().signal, deadlineAtMs: null, shutdownSignal: new AbortController().signal, updateProgress: async () => {}, updateTokens: async () => {}, log: async () => {}, isActive: async () => true, readInbox: async () => [], }; diff --git a/test/handlers-embed-backfill.test.ts b/test/handlers-embed-backfill.test.ts index 3c7e4c772..f09ff67dd 100644 --- a/test/handlers-embed-backfill.test.ts +++ b/test/handlers-embed-backfill.test.ts @@ -48,6 +48,7 @@ function fakeJob(data: Record): MinionJobContext { data, attempts_made: 0, signal: controller.signal, + deadlineAtMs: null, shutdownSignal: controller.signal, updateProgress: async () => {}, updateTokens: async () => {}, diff --git a/test/ingestion/ingest-capture.test.ts b/test/ingestion/ingest-capture.test.ts index 3262ada39..7f4bdaf94 100644 --- a/test/ingestion/ingest-capture.test.ts +++ b/test/ingestion/ingest-capture.test.ts @@ -58,6 +58,7 @@ function makeJob(data: Record): MinionJobContext { data, attempts_made: 1, signal: new AbortController().signal, + deadlineAtMs: null, shutdownSignal: new AbortController().signal, updateProgress: async () => {}, updateTokens: async () => {}, diff --git a/test/minions-shell.test.ts b/test/minions-shell.test.ts index b4f5ec117..ff1f311f9 100644 --- a/test/minions-shell.test.ts +++ b/test/minions-shell.test.ts @@ -52,6 +52,7 @@ function makeCtx( data, attempts_made: 0, signal: opts.signal ?? new AbortController().signal, + deadlineAtMs: null, shutdownSignal: opts.shutdownSignal ?? new AbortController().signal, updateProgress: async () => {}, updateTokens: async () => {}, diff --git a/test/subagent-aggregator.test.ts b/test/subagent-aggregator.test.ts index 94a36b9e0..318fb12e3 100644 --- a/test/subagent-aggregator.test.ts +++ b/test/subagent-aggregator.test.ts @@ -40,6 +40,7 @@ function ctxWithInbox( data, attempts_made: 0, signal: new AbortController().signal, + deadlineAtMs: null, shutdownSignal: new AbortController().signal, async updateProgress(p: unknown) { progress.push(p); }, async updateTokens() {}, diff --git a/test/subagent-handler.test.ts b/test/subagent-handler.test.ts index 3b50a7122..da5a830d7 100644 --- a/test/subagent-handler.test.ts +++ b/test/subagent-handler.test.ts @@ -90,6 +90,7 @@ async function makeCtx(input: unknown): Promise { data: (input as Record) ?? {}, attempts_made: 0, signal: ac.signal, + deadlineAtMs: null, shutdownSignal: shutdown.signal, async updateProgress() {}, async updateTokens() {}, From bcf3b73dcf80a45aec3162cb565c01fd18661a3e Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 06:06:21 +0900 Subject: [PATCH 15/17] fix(sync): self-heal a never-git-initialized default brain dir (#2964) (#2967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sync): self-heal a never-git-initialized default brain dir (#2964) The dream cycle's sync phase throws unconditionally on a legacy sync.repo_path-anchored default brain dir that was never git init-ed (predates git-backed sync, or was rsync'd without its .git), failing every nightly run with no recovery. doctor's sync_freshness/ sync_consolidation checks report "ok" for this exact brain, but only because they query the sources table (0 rows for a legacy default brain) — a coincidental false-negative, not a real diagnosis. Self-heal by git-initializing the dir and capturing the current on-disk state as the sync baseline, scoped to !opts.sourceId only — gbrain owns this directory outright, unlike a registered local source (sources add --path, no --url) which is the user's own external directory and should keep failing loudly. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): dry-run no-write contract, unborn-HEAD recovery, no-gpg-sign (#2964) Codex review on the initial self-heal patch (b1671ee) found 3 real gaps: - P1: the self-heal ran even under --dry-run, mutating the filesystem during what's documented as a preview-only command. Gated the whole self-heal (both discoverGitRoot and the headCommit read) on !opts.dryRun, same as the existing !opts.sourceId ownership check. - P2: if `git init` succeeded but the process died before the baseline commit landed, the next run's discoverGitRoot would succeed (`.git` exists) and skip recovery entirely, permanently wedging on "No commits in repo" forever. Added the same self-heal at the `git rev-parse HEAD` catch site, sharing a new createSyncBaselineCommit helper with the discoverGitRoot catch. - P2: the baseline commit inherited the operator's global commit.gpgSign, which can block headless cron/launchd runs on an unavailable signing agent/pinentry. Added --no-gpg-sign. Two new tests cover dry-run no-mutation and unborn-HEAD recovery. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): restrict git auto-init self-heal to the anchor-resolved path only (#2964) Second Codex review round (a10eeab) found the ownership check still too loose: - P1 (security): !opts.sourceId alone isn't proof gbrain owns repoPath. jobs.ts's `sync` job handler leaves sourceId undefined whenever job.data.repoPath doesn't match a registered source's local_path, so an admin-scope submit_job({name:'sync', data:{repoPath}}) MCP call could point the self-heal at an arbitrary directory and have it silently git-init + commit + ingest it. Gated both self-heal sites on !opts.repoPath too — only the path resolved from gbrain's own sync.repo_path anchor (never a caller-supplied one) is eligible. - P2: the unborn-HEAD recovery site calls discoverGitRoot, which walks UP from repoPath and can resolve to an ANCESTOR repo for a --src-subpath/subdir-as-repoPath sync with an unborn HEAD. Committing there would `git add -A` sibling files well outside the sync scope. Added a check that gitContextRoot === realpathSync(repoPath) before self-healing; refuses (falls through to the original error) otherwise. Tests rewritten to exercise the true self-heal-eligible path (anchor config via engine.setConfig('sync.repo_path', dir), no repoPath/sourceId passed) instead of an explicit repoPath, plus a new test asserting a caller-supplied repoPath with no sourceId still throws. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): prove self-heal ownership by anchor VALUE, not field presence (#2964) Third Codex review round (613ad5b) found the previous round's fix broke the very call site it was meant to repair, plus a second scope gap: - P1 (critical): !opts.repoPath rejected self-heal on the REAL production callers too. runPhaseSync (dream cycle's sync phase, cycle.ts) always passes `repoPath: brainDir` explicitly after resolving it upstream, and the CLI's bare `gbrain sync` resolves sourceId='default'. Both made the ownership gate rethrow, leaving `gbrain dream` and `gbrain sync` wedged on the exact non-git legacy brain this fix targets — only synthetic callers that omitted both fields ever healed. Fixed by proving ownership by VALUE instead of by field absence: a new isAnchorOwnedSyncPath() re-reads gbrain's own persisted sync.repo_path config and requires the resolved repoPath to equal it exactly, regardless of whether the caller passed it explicitly or let it default. An attacker-supplied arbitrary path (e.g. via submit_job({name:'sync', data:{repoPath}})) only self-heals if it happens to already equal gbrain's own anchor — which is the legitimate case, not an escalation. opts.sourceId and opts.srcSubpath still disqualify unconditionally (registered/subpath-scoped syncs are a different ownership context). - P2: a --src-subpath sync with an unborn parent-repo HEAD would commit the whole ancestor root, capturing sibling files outside the scope. isAnchorOwnedSyncPath's opts.srcSubpath check closes this; the existing gitContextRoot === repoPath check stays as defense in depth. - P2: manageGitignore's "warn and return" contract (a deliberate side- effect that must never kill the sync job for its OTHER callers) meant a broken gbrain.yml or unwritable .gitignore would silently let the baseline `git add -A` commit db_only content. createSyncBaselineCommit now recomputes db_only exclusion directly from loadStorageConfig and passes it to `git add` as pathspecs, independent of the .gitignore write's success — true fail-closed. (A redundant pathspec exclude for a path .gitignore ALREADY covers makes git's -A bail with "paths ignored, use -f" even though the negation is correct, so each dir is check-ignore'd first and only pathspec-excluded when NOT already covered.) Tests rewritten around the anchor-VALUE model: the critical regression case (explicit repoPath matching the anchor still heals — the exact scenario Codex proved was broken) plus a true negative (a caller-supplied path that does NOT match the anchor still throws), --src-subpath refusal, and db_only fail-closed exclusion. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): allow default-source ownership, realpath compare, generous timeout, --no-verify (#2964) Fourth Codex review round (9c1d461) found the previous round's ownership gate still didn't match the REAL installed-brain shape, plus 3 more gaps: - P1 (critical): rejecting all non-empty opts.sourceId meant self-heal still never fired on a real brain. Migration sources_table_additive seeds a 'default' source row whose local_path mirrors sync.repo_path on every brain that's run it (virtually all of them), so resolveSourceForDir (dream cycle) and bare `gbrain sync` both resolve sourceId:'default' in practice, never undefined. isAnchorOwnedSyncPath now permits sourceId undefined OR exactly 'default' (gbrain's own bootstrap identity, never something a caller names) and proves ownership by rereading the LIVE anchor for that same identity (sources.default.local_path vs config.sync.repo_path). - P2: compared raw anchor/repoPath strings, so a cosmetic difference (trailing slash, ..) between the stored anchor and dream.ts's path.resolve()-normalized brainDir would defeat the match. Now realpath-compares both sides (fail-closed on ENOENT/dangling). - P2: the shared git() helper's 30s timeout could abort the baseline `git add -A` on a large legacy brain mid-way, after `git init` already created `.git` — leaving an unborn repo every subsequent sync would retry and time out identically forever. Added an optional timeoutMs param (default unchanged at 30s); the baseline add call uses 10min. - P2: the baseline commit could trigger an operator's global core.hooksPath/init.templateDir hooks (pre-commit/commit-msg), breaking headless recovery if those hooks need project tooling or prompt. Added --no-verify. Tests: rewrote the mis-scoped "registered source" test (it used sourceId:'default', which is now correctly permitted) into two — a new regression test proving sourceId='default' + matching local_path heals (the actual production shape), and a corrected non-default-sourceId refusal test. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): defer .gitignore write past first import, rebuild index, fail closed on unparseable db_only (#2964) Fifth Codex review round (c17cd23) found the baseline-commit helper interacting badly with the pre-existing db_only storage-tiering feature: - P1 (data loss): createSyncBaselineCommit called manageGitignore BEFORE performFullSync's collectSyncableFiles ran. collectSyncableFiles enumerates via `git ls-files --cached --others --exclude-standard`, so writing db_only entries into .gitignore first would silently exclude those pages from the DATABASE, not just from git — on a brain's very first sync. This is the exact bug class runSync's existing "manage .gitignore ONLY on successful sync" ordering (this file, ~line 4540, itself a prior Codex P1 fix, comment literally says so) was written to prevent — my new code reintroduced it in a different spot. Fix: stopped calling manageGitignore inside the self-heal at all. db_only exclusion for the COMMIT still happens via the existing pathspec computation (independent of .gitignore); .gitignore itself gets written by the already-existing post-sync flow once this sync completes, same as any other sync. - P1 (data leak): the unborn-HEAD recovery site can reach createSyncBaselineCommit with a repo whose INDEX already has entries staged from some prior operation (manual `git add`, interrupted workflow) before gbrain's self-heal ever touched it. `git add -A` only adds/updates — it doesn't drop an already-staged path our exclusion pathspecs now want excluded. Added `git read-tree --empty` to reset the index before staging (no-op on a freshly-`git init`-ed repo, whose index is already empty). - P2: loadStorageConfig warns-and-returns an EMPTY config (not a throw) for syntactically-valid-but-unsupported YAML (e.g. flow-style `db_only: [dir/]` — the narrow custom parser only handles block-style lists), which would silently resolve zero exclusions from a gbrain.yml that clearly intended some. Added a sniff-test: if gbrain.yml exists and mentions db_only but nothing resolved from it, refuse the baseline commit rather than guess "genuinely empty" vs "syntax silently ignored" (git init may already have run by this point — same "unborn, retry on next sync" recovery path handles it, and will hit this same guard again until the user fixes gbrain.yml). Tests: a positive regression proving db_only markdown IS imported into the DB on first sync (the actual data-loss scenario), the sniff-test refusal, and the stale-staged-content-gets-dropped case for the index rebuild. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): post-heal .gitignore write, supabase_only alias, honor abort signal (#2964) Sixth Codex review round (e687913), 3 P2s: - Dream-cycle callers (cycle.ts:runPhaseSync) invoke performSync directly and never run runSync's CLI-only post-success manageGitignoreAtGitRoot. A brain self-healed only via the dream cycle would have db_only content correctly excluded from the baseline commit (createSyncBaselineCommit's pathspec exclusion) but no .gitignore ever written, leaving the user's own future manual git add/commit unprotected. Added performFullSyncAndMaybeGitignore, a thin wrapper around the 3 post-self-heal performFullSync call sites that writes .gitignore (same success-status gate runSync already uses) only when didSelfHeal is true — a no-op for the normal path, which still relies on runSync exactly as before. - The fail-closed sniff-test only checked the canonical `db_only` key; the deprecated-but-still-supported `supabase_only` alias (same keep-out-of-git semantics) could silently bypass it. Now checks both. - Self-heal didn't check opts.signal?.aborted before starting the (now up to 10-minute) git init + baseline commit, so a cancelled sync could still mutate disk and overrun its budget instead of returning partial. Added the check at both self-heal sites, before any git operation runs. New test proves .gitignore gets written after a bare performSync call (no runSync wrapper) — the actual dream-cycle shape. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): neutralize a leftover .gitignore during the self-heal first sync (#2964) Seventh Codex review round (27337b0) ran an actual repro and caught the primary motivating scenario still broken: a brain rsync'd from another machine without its .git can retain that machine's old auto-managed .gitignore. collectSyncableFiles (inside performFullSync) enumerates via `git ls-files --exclude-standard`, so a leftover db_only ignore rule would silently omit those pages from THIS first sync's DATABASE import — the same bug class the round-6 ordering fix prevented for a .gitignore gbrain would have written itself, just triggered by a pre-existing file this time. Fix: performFullSyncAndMaybeGitignore now neutralizes any existing .gitignore for the duration of the one first-sync call — read, delete, restore byte-for-byte immediately after (even on error) — before manageGitignore re-merges the managed db_only block onto the restored original content. This matches exactly what a truly fresh brain with no .gitignore at all already does on its first sync (nothing to suppress collection there either); db_only content stays out of the git COMMIT independently via createSyncBaselineCommit's pathspec exclusion, which never depended on .gitignore. Test proves both halves: db_only markdown IS imported despite a leftover ignore rule, AND the user's own unrelated .gitignore lines (e.g. .DS_Store) survive the restore intact. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): simplify — drop db_only-import machinery, isolate hooks fully (#2964) Eighth Codex review round (54c1e6f) found MORE problems with round 7's .gitignore-neutralization fix (deleting the whole file loses the user's own unrelated ignore rules; a multi-sync retry scenario could silently skip a still-broken db_only file while advancing the bookmark) plus 2 more issues in existing code. Rather than patch those too, stepped back and checked the actual documented semantics of db_only (docs/storage-tiering.md): it's for "bulk machine-generated content... written to disk as a local cache" — DB is the source of truth, disk is a cache populated FROM the DB (`export --restore-only` restores it), never the other way. Nothing in the docs says `gbrain sync`'s git-diff-based file collection is how db_only content is supposed to reach the database — that's ingest-specific tooling's job. Confirmed directly: `loadStorageConfig` returns the byte-identical `{db_tracked:[], db_only:[]}` for a malformed flow-style array AND a literal empty `db_only: []`, so rounds 6-7's "ensure db_only markdown gets imported on this first sync" chase was solving a problem outside sync's actual scope in the first place, on an increasingly complex, adversarially-discovered- edge-case foundation. Reverted: performFullSyncAndMaybeGitignore (the wrapper + didSelfHeal tracking + .gitignore neutralize/restore dance + post-success manageGitignore call). After self-heal, import and any subsequent .gitignore management now behave EXACTLY like any other brain, self-healed or not — runSync's existing post-success manageGitignoreAtGitRoot covers the CLI path identically either way; the dream cycle not calling it is a separate, pre-existing characteristic of the dream cycle in general (applies equally to an already-git-initialized brain going through the same path), not something this fix introduces. Kept (still correct, self-contained, don't depend on the reverted machinery): createSyncBaselineCommit's pathspec-based db_only exclusion for the COMMIT itself (matches the documented "not committed to git" requirement), the fail-closed sniff-test guard (now documents its known, structurally-unavoidable false-positive on a genuinely-empty `db_only: []` — the trade-off is deliberate: low-cost, self-resolving false positive vs. high-cost, hard-to-undo false negative), the index rebuild, and the 600s add timeout. Improved (round 8, P2): hooks isolation. --no-verify only skips pre-commit/commit-msg; added `-c core.hooksPath=/dev/null` for the baseline commit, which disables prepare-commit-msg and post-commit too (the latter runs synchronously inside the same git invocation and could otherwise hang past the timeout without even being the slow step). Tests: removed the 3 that exercised the reverted db_only-import machinery; the remaining 12 (ownership, dry-run, index rebuild, sniff test, commit-exclusion, unborn-HEAD recovery) are unaffected by the simplification. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS * fix(sync): unconditional db_only exclusion, literal pathspecs, precise sniff test (#2964) Ninth Codex review round (a6e07f6): - P1: the check-ignore pre-filter (skip pathspec-excluding a dir already covered by .gitignore) could be defeated by a pre-existing .gitignore that ignores a db_only tree with a wildcard but re-includes a child via negation (e.g. `private-cache/*` + `!private-cache/index.md`) — check-ignore on the directory still reports "ignored", so the filter skipped the pathspec exclusion, and `git add -A` staged the re-included child anyway. Fixed by making exclusion unconditional: every db_only dir is always pathspec-excluded now, never pre-filtered against .gitignore state at all — our own pathspec doesn't consult .gitignore, so no .gitignore content (negated or not) can defeat it. The advisory "paths ignored... use -f" error this can now trigger when a dir IS also already .gitignore'd (verified: git still stages everything else correctly despite the nonzero exit) is caught and swallowed by matching its exact stderr text; anything else rethrows. - P2: `:!dir` pathspec shorthand reinterprets a dir name that itself starts with a pathspec magic character (e.g. `:private/`) instead of excluding it literally. Switched to `:(exclude,literal)dir`. - P2: the fail-closed sniff-test's bare substring search on gbrain.yml's raw content could trip on a comment or unrelated prose mentioning "db_only" even when there's no real storage section at all, refusing self-heal forever on an unrelated false positive. Now requires an actual YAML key line (`db_only:`/`supabase_only:`, trimmed, ignoring `#` comments) — the round-8-documented "genuinely empty db_only: []" false positive is unchanged and remains an accepted trade-off (still structurally indistinguishable from unsupported syntax at the loadStorageConfig API boundary), but comment/prose mentions no longer false-positive. Not fixed (deliberately, documented trade-off — see PR description): Codex's other P1 this round (refuse baselining when other git refs/ history exist alongside an unborn HEAD) is a narrow, non-destructive scenario — self-heal only ever acts on the current branch ref when it's provably commit-less, never touches or deletes any other ref (remote- tracking, other branches), so at worst it creates a possibly-unexpected extra commit on an otherwise-empty branch the user hadn't checked out yet. Chasing it further trades diminishing real-world risk reduction against unbounded scope growth in what's fundamentally still the self-heal fix from round 1. Two new tests: unconditional exclusion despite a matching pre-existing .gitignore (proves the advisory-swallow path), and a comment-only gbrain.yml no longer false-positives the sniff test. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NCVEvUVm15bFuKqskWgfNS --------- Co-authored-by: Claude Sonnet 5 --- src/commands/sync.ts | 303 ++++++++++++++++++++++++++++++- test/sync-git-autoinit.test.ts | 322 +++++++++++++++++++++++++++++++++ 2 files changed, 621 insertions(+), 4 deletions(-) create mode 100644 test/sync-git-autoinit.test.ts diff --git a/src/commands/sync.ts b/src/commands/sync.ts index 0e857003b..55177421b 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -919,10 +919,10 @@ export function buildAutoEmbedArgs(slugs: string[], sourceId?: string): string[] * 100 MiB is generous but still bounded — a 100K-file diff with long * paths tops out around 10–20 MiB in practice. */ -function git(repoPath: string, args: string[], configs: string[] = []): string { +function git(repoPath: string, args: string[], configs: string[] = [], timeoutMs = 30000): string { return execFileSync('git', buildGitInvocation(repoPath, args, configs), { encoding: 'utf-8', - timeout: 30000, + timeout: timeoutMs, maxBuffer: 100 * 1024 * 1024, }).trim(); } @@ -943,6 +943,171 @@ export function discoverGitRoot(inputPath: string): string { } } +/** + * #2964: snapshot the CURRENT on-disk state of a gbrain-owned brain dir as + * a baseline commit — used both right after a self-healing `git init` (no + * `.git` at all) and to recover a repo left with `.git` but zero commits + * (an interrupted prior self-heal, or a `git init` from some other source + * that never got a first commit). Respects `.gitignore` (written first) so + * future incremental syncs diff against what's actually here rather than + * an empty tree — an empty initial commit would make every existing file + * look "added" again on the next sync, even though the full-sync pass that + * follows already imported them from disk directly. + * + * `--no-gpg-sign` + explicit `-c user.name/user.email`: this runs from a + * headless nightly cron/launchd invocation, which has no reason to have + * git signing/identity configured, and must not block on an unavailable + * signing agent or pinentry prompt. + * + * db_only exclusion is recomputed directly and passed to `git add` as + * negative pathspecs, rather than relying solely on `manageGitignore` + * having written `.gitignore` successfully: that helper is deliberately + * best-effort (a broken gbrain.yml parse, or an unwritable .gitignore, + * only warns and returns — the right default for its OTHER callers, where + * .gitignore management is a side effect that must never kill the sync + * job). For a commit we are about to create ourselves, "fail open" there + * would mean silently committing db_only content into git history. Fail + * closed instead: db_only exclusion doesn't depend on the .gitignore + * write having succeeded. `loadStorageConfig` throwing (unreadable + * gbrain.yml, or a semantic overlap) propagates — better to leave this + * self-heal wedged with a clear error than commit unknown content. + */ +function createSyncBaselineCommit(repoPath: string): void { + // #2964: db_only exclusion is computed directly from loadStorageConfig + // and passed to `git add` as pathspecs — deliberately NOT via + // manageGitignore/.gitignore, for two independent reasons: + // + // 1. Ordering (Codex review round 6, P1): `collectSyncableFiles` — the + // file enumeration `performFullSync` runs right after this function + // returns — honors `.gitignore` via `git ls-files --exclude-standard`. + // Writing db_only entries into `.gitignore` BEFORE that first import + // would silently exclude those pages from the database entirely. + // That's the exact bug class `runSync`'s existing "manage .gitignore + // ONLY on successful sync" ordering (this file, `manageGitignoreAtGitRoot` + // callers below — itself a prior Codex P1 fix) exists to prevent. Leave + // `.gitignore` untouched here; the existing post-sync flow writes it + // once this sync completes, same as it does for every other sync. + // 2. Fail-closed (rounds 5-6): `manageGitignore`'s "warn and return" on a + // broken gbrain.yml/unwritable .gitignore is the right default for its + // OTHER callers (a side effect that must never kill the sync job), but + // wrong for a commit we are creating ourselves — silently committing + // db_only content into git history. + const storageConfig = loadStorageConfig(repoPath); + const dbOnlyDirs = storageConfig?.db_only ?? []; + // Sniff-test fail-closed (round 6, P2): `loadStorageConfig` warns-and- + // returns an EMPTY config for syntactically-valid-but-unsupported YAML + // (e.g. flow-style `db_only: [dir/]` — the narrow custom parser only + // handles block-style lists), which would silently resolve zero + // exclusions from a file that clearly intended some. If gbrain.yml + // exists and mentions db_only (or its deprecated pre-v0.22.11 alias + // `supabase_only` — same keep-out-of-git semantics, still a supported + // backward-compat key per storage-config.ts) but nothing resolved from + // it, refuse rather than guess "genuinely empty" vs "syntax ignored". + // + // Known false-positive (round 8 review): a genuinely, intentionally + // empty `db_only: []` mentioning the word also refuses, and can't be + // told apart from the unsupported-syntax case — `loadStorageConfig` + // returns the IDENTICAL `{db_tracked:[],db_only:[]}` for both (verified + // directly: flow-style `[dir/]` and literal `[]` both collapse to that + // same shape). Distinguishing them would mean teaching this function + // about the parser's internal line-recognition rules, which belongs in + // storage-config.ts, not here. Accepted trade-off: the false-positive + // cost is low and self-resolving (the brain stays wedged with a clear, + // actionable error until the user drops the pointless empty stanza or + // fixes their syntax; retried on every subsequent sync); the + // false-negative this guards against — silently committing db_only + // content into permanent git history — is high-cost and hard to undo. + if (dbOnlyDirs.length === 0) { + const yamlPath = join(repoPath, 'gbrain.yml'); + const yamlContent = existsSync(yamlPath) ? readFileSync(yamlPath, 'utf-8') : ''; + // A YAML KEY line (`db_only:` / `supabase_only:`, ignoring leading + // whitespace and `#` comments), not a bare substring search — round 9, + // P2: a comment or unrelated prose value that happens to mention the + // word (e.g. `# db_only handling TBD`) must not trip this guard on an + // otherwise-genuinely-config-free gbrain.yml. + const mentionsUnresolvedKey = yamlContent.split('\n').some((line) => { + const trimmed = line.trim(); + return !trimmed.startsWith('#') && /^(db_only|supabase_only)\s*:/.test(trimmed); + }); + if (mentionsUnresolvedKey) { + throw new Error( + `${yamlPath} mentions db_only but no directories resolved from it — refusing to ` + + `auto-commit (cannot tell "genuinely empty" from "unsupported syntax silently ignored"). ` + + `Fix gbrain.yml's storage.db_only syntax, or git-init this directory manually.`, + ); + } + } + // #2964 (round 9, P1): every db_only dir is ALWAYS pathspec-excluded, + // unconditionally — never pre-filtered against what an existing + // `.gitignore` claims to already cover. An earlier version checked + // `git check-ignore -q dir` first and skipped the pathspec when it + // already reported "ignored" (to dodge the advisory error below), but + // `check-ignore` on a directory can say "ignored" even when a + // pre-existing `.gitignore` re-includes a child via negation (e.g. + // `private-cache/*` + `!private-cache/index.md`) — the filter would + // then skip excluding it via pathspec, and `git add -A` would stage + // that re-included child despite the whole directory being declared + // db_only. Our OWN pathspec exclusion is unconditional and doesn't + // consult `.gitignore` at all, so it can't be defeated by ANY + // .gitignore content, negated or not. `:(exclude,literal)dir` (not the + // `:!dir` shorthand) so a db_only dir name that itself starts with a + // pathspec magic character like `:` is excluded literally rather than + // reinterpreted (round 9, P2). + const excludePathspecs = dbOnlyDirs.map((dir) => `:(exclude,literal)${dir}`); + // Clear the index before staging (round 6, P1): the unborn-HEAD + // recovery site can reach this function with a repo whose index + // already has entries staged from some OTHER prior operation (a manual + // `git add`, an interrupted workflow) before gbrain ever touched it. + // `add -A` only adds/updates — it does not drop an already-staged path + // that our exclusion pathspecs above now want excluded. `read-tree + // --empty` resets the index without touching the working tree; a + // no-op on a freshly-`git init`-ed repo, whose index is already empty. + git(repoPath, ['read-tree', '--empty']); + try { + // #2964: 10 minutes, not the shared git() helper's 30s default — this + // full-tree `git add -A` walks a legacy brain that may hold years of + // accumulated content. A 30s timeout would abort staging after `git + // init` already created `.git`, leaving an unborn repo that every + // subsequent sync would retry (and time out identically) forever; + // the unborn-HEAD recovery path exists for OTHER causes of that + // state, not to be this one's normal first outcome. + git(repoPath, ['add', '-A', '--', '.', ...excludePathspecs], [], 600_000); + } catch (err) { + // Now that exclusion is always applied (never pre-filtered), an + // explicit pathspec exclusion for a path a pre-existing `.gitignore` + // ALSO happens to cover trips git's advice.addIgnoredFile: nonzero + // exit + "paths ignored by one of your .gitignore files, use -f", + // even though the add otherwise fully succeeded (verified directly: + // `git status --short` right after this exact error shows every + // non-excluded path staged correctly). Recognize and swallow ONLY + // this exact advisory; anything else (timeout, permission denied, + // real corruption) rethrows. + const stderr = err && typeof err === 'object' && 'stderr' in err ? String((err as { stderr: unknown }).stderr) : ''; + if (!stderr.includes('ignored by one of your .gitignore files')) throw err; + } + git( + repoPath, + // --no-verify only skips pre-commit/commit-msg — prepare-commit-msg + // and (worse, since it runs AFTER the commit object already exists, + // synchronously inside this same git invocation) post-commit are + // NOT covered by it. An operator's global core.hooksPath or + // init.templateDir can wire either, expecting project tooling, + // prompting interactively, or hanging — none of which a headless + // self-heal commit can satisfy, and a hanging post-commit hook would + // burn the 600s budget above without even being the slow step. + // `-c core.hooksPath=/dev/null` (in configs, below) makes git look + // for hook scripts inside a location that can't contain any, + // disabling the entire hooks path for this one invocation — the + // complete form of what --no-verify only partially covers, kept for + // explicitness on the two hooks it does name. + [ + 'commit', '--quiet', '--allow-empty', '--no-gpg-sign', '--no-verify', + '-m', 'gbrain: initial commit (auto-init by sync)', + ], + ['user.name=gbrain', 'user.email=gbrain@localhost', 'core.hooksPath=/dev/null'], + ); +} + /** * #774 NAV-1 TOCTOU: true only if filePath realpath-resolves inside gitRoot. * Guards symlink escape at the per-file level (a committed symlink whose @@ -1009,6 +1174,65 @@ async function readSyncAnchor( return await engine.getConfig(`sync.${which}`); } +/** + * #2964: is `repoPath` gbrain's own default-brain anchor, as opposed to a + * path some caller merely happened to pass through unchanged? + * + * `!opts.sourceId` alone is NOT sufficient — and neither is rejecting + * `opts.sourceId` outright: migration `sources_table_additive` (v20) + * seeds a `'default'` source row whose `local_path` is copied FROM + * `config.sync.repo_path` on every brain that has ever run it (i.e. + * effectively all of them by now), and `writeSyncAnchor` keeps that row's + * `local_path` current on every sync thereafter. So on a real installed + * brain, `resolveSourceForDir` (dream cycle) and the CLI's bare `gbrain + * sync` both resolve `sourceId: 'default'`, NOT `undefined` — rejecting + * all non-empty `sourceId` (an earlier, insufficiently-reviewed version + * of this check) made self-heal never fire on that real path either, + * masked in tests only because a freshly-`initSchema()`'d test brain's + * `'default'` row has a null `local_path` (Codex review round 5). + * + * The actual boundary: `'default'` is gbrain's own bootstrap identity, + * not something a caller names — a DIFFERENT, non-default `sourceId` is + * what an explicit `sources add --path ` registration (a + * user's own external directory) looks like, and that's what must keep + * failing loudly. So: permit `sourceId` when it's exactly `undefined` or + * `'default'`, reject any other id, and for BOTH permitted cases prove + * ownership by VALUE — reread the live anchor for that same identity + * (`sources.default.local_path` when sourceId='default', else + * `config.sync.repo_path`) and require the resolved `repoPath` to + * REALPATH-equal it (not raw string equality: `dream`'s `resolveBrainDir` + * normalizes via `path.resolve`, so a trailing slash or `..` in the + * stored anchor must not defeat the match — Codex review round 5, P2). + * An arbitrary caller-supplied path (e.g. an admin-scope + * `submit_job({name:'sync', data:{repoPath}})`) only passes this check + * if it already equals gbrain's own anchor by realpath identity — at + * which point self-healing it is exactly the legitimate case, not an + * escalation. + * + * `opts.srcSubpath` disqualifies unconditionally: a subpath-scoped sync + * only wants THAT subdirectory captured, but the self-heal baseline + * commit runs `git add -A` at the git root (there's no file list yet to + * scope it to — collection happens after this point) — see the P2 review + * finding on `createSyncBaselineCommit`'s callers. + */ +async function isAnchorOwnedSyncPath( + engine: BrainEngine, + opts: SyncOpts, + repoPath: string, +): Promise { + if (opts.srcSubpath) return false; + if (opts.sourceId && opts.sourceId !== 'default') return false; + const anchor = await readSyncAnchor(engine, opts.sourceId, 'repo_path'); + if (anchor === null) return false; + try { + return realpathSync(anchor) === realpathSync(repoPath); + } catch { + // Anchor or repoPath doesn't realpath-resolve (dangling/nonexistent) — + // can't prove identity, so don't self-heal. + return false; + } +} + async function writeSyncAnchor( engine: BrainEngine, sourceId: string | undefined, @@ -1628,7 +1852,33 @@ async function performSyncInner(engine: BrainEngine, opts: SyncOpts): Promise --path + * ` registration of a user's own external directory looks like), + * AND the resolved `repoPath` must realpath-equal the LIVE anchor for + * that same identity. A caller-supplied path that does not match (a + * registered non-default source, or an admin-scope + * `submit_job({name:'sync', data:{repoPath}})` MCP call with an + * unrelated path) must keep failing loudly rather than being silently + * git-initialized without consent. + */ + +import { describe, test, expect, beforeAll, afterAll, beforeEach, afterEach } from 'bun:test'; +import { mkdtempSync, writeFileSync, rmSync, existsSync } from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; + +function mdPage(title: string, body = 'Content.'): string { + return `---\ntype: note\ntitle: ${title}\n---\n\n${body}`; +} + +describe('#2964: sync auto-inits a never-git-initialized default brain dir', () => { + let engine: PGLiteEngine; + let dir: string; + + beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); + }, 60_000); + + afterAll(async () => { + await engine.disconnect(); + }, 60_000); + + beforeEach(async () => { + await resetPgliteState(engine); + dir = mkdtempSync(join(tmpdir(), 'gbrain-2964-')); + writeFileSync(join(dir, 'page1.md'), mdPage('Page 1')); + writeFileSync(join(dir, 'page2.md'), mdPage('Page 2')); + // The self-heal-eligible anchor: gbrain's own persisted config, not a + // caller-supplied --repo / job.data.repoPath (those are proven by + // VALUE against this anchor, not by mere absence — see file docstring). + await engine.setConfig('sync.repo_path', dir); + }); + + afterEach(() => { + if (dir) rmSync(dir, { recursive: true, force: true }); + }); + + test('anchor-resolved sync (no repoPath, no sourceId) on a non-git dir auto-inits git and imports files', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + expect(existsSync(join(dir, '.git'))).toBe(false); + + const result = await performSync(engine, { noPull: true, noEmbed: true, full: true }); + + expect(result.status).toBe('first_sync'); + expect(result.added).toBe(2); + expect(existsSync(join(dir, '.git'))).toBe(true); + expect(await engine.getPage('page1')).not.toBeNull(); + expect(await engine.getPage('page2')).not.toBeNull(); + }); + + test('explicit repoPath matching the anchor still auto-inits (mirrors gbrain dream\'s sync phase)', async () => { + // cycle.ts's runPhaseSync (the actual dream-cycle call site this bug + // was filed against) always passes `repoPath: brainDir` explicitly — + // it already resolved the anchor itself upstream and threads it + // through. Gating self-heal on `!opts.repoPath` would silently never + // fire here; ownership must be proven by matching the anchor's VALUE. + const { performSync } = await import('../src/commands/sync.ts'); + expect(existsSync(join(dir, '.git'))).toBe(false); + + const result = await performSync(engine, { repoPath: dir, noPull: true, noEmbed: true, full: true }); + + expect(result.status).toBe('first_sync'); + expect(result.added).toBe(2); + expect(existsSync(join(dir, '.git'))).toBe(true); + }); + + test('a second sync after auto-init sees no changes (baseline commit captured current on-disk state)', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + const first = await performSync(engine, { noPull: true, noEmbed: true, full: true }); + expect(first.added).toBe(2); + + // No new files, no explicit `full` — a real incremental sync against the + // auto-init baseline. Before this fix there was no baseline to diff + // against (sync errored outright); a naive fix that skipped the initial + // commit would make this call re-report both files as "added" again. + const second = await performSync(engine, { noPull: true, noEmbed: true }); + expect(second.status).not.toBe('first_sync'); + expect(second.added).toBe(0); + expect(second.modified).toBe(0); + }); + + test("sourceId='default' whose local_path mirrors the anchor still auto-inits (P1: the real installed-brain shape)", async () => { + // Migration sources_table_additive seeds a 'default' source row with + // local_path copied from sync.repo_path on every brain that's run it + // — i.e. this, not a bare no-sourceId call, is what runPhaseSync/CLI + // `gbrain sync` actually resolve to on a real installed brain. + await engine.executeRaw(`UPDATE sources SET local_path = $1 WHERE id = 'default'`, [dir]); + const { performSync } = await import('../src/commands/sync.ts'); + expect(existsSync(join(dir, '.git'))).toBe(false); + + const result = await performSync(engine, { + repoPath: dir, + sourceId: 'default', + noPull: true, + noEmbed: true, + full: true, + }); + + expect(result.status).toBe('first_sync'); + expect(result.added).toBe(2); + expect(existsSync(join(dir, '.git'))).toBe(true); + }); + + test('a registered non-default local source (sourceId != default, no remote_url) on a non-git dir still throws — not auto-inited', async () => { + await engine.executeRaw( + `INSERT INTO sources (id, name, local_path, config) VALUES ('mysource', 'mysource', $1, '{}'::jsonb)`, + [dir], + ); + const { performSync } = await import('../src/commands/sync.ts'); + await expect( + performSync(engine, { + repoPath: dir, + sourceId: 'mysource', + noPull: true, + noEmbed: true, + full: true, + }), + ).rejects.toThrow(/git repository/i); + expect(existsSync(join(dir, '.git'))).toBe(false); + }); + + test('a caller-supplied repoPath that does NOT match the anchor still throws (P1: MCP submit_job arbitrary-path guard)', async () => { + // Mirrors jobs.ts: submit_job({name:'sync', data:{repoPath}}) reaches + // performSyncInner with sourceId left undefined whenever repoPath + // doesn't match a registered source's local_path. Self-heal must not + // fire for a path that isn't gbrain's own anchor, even with no + // sourceId set — only exact anchor-value equality (the previous test) + // is eligible. + const other = mkdtempSync(join(tmpdir(), 'gbrain-2964-other-')); + writeFileSync(join(other, 'unrelated.md'), mdPage('Unrelated')); + try { + const { performSync } = await import('../src/commands/sync.ts'); + await expect( + performSync(engine, { repoPath: other, noPull: true, noEmbed: true, full: true }), + ).rejects.toThrow(/git repository/i); + expect(existsSync(join(other, '.git'))).toBe(false); + } finally { + rmSync(other, { recursive: true, force: true }); + } + }); + + test('--src-subpath on the anchor-resolved path still throws — not auto-inited (P2: subpath scope guard)', async () => { + // A self-heal baseline commit runs `git add -A` at the git root before + // any subpath-scoped file collection happens, so it would capture + // sibling directories a --src-subpath sync never intended to touch. + const { performSync } = await import('../src/commands/sync.ts'); + await expect( + performSync(engine, { + srcSubpath: 'wiki', + noPull: true, + noEmbed: true, + full: true, + }), + ).rejects.toThrow(/git repository/i); + expect(existsSync(join(dir, '.git'))).toBe(false); + }); + + test('--dry-run on the anchor-resolved path throws without writing anything to disk', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + await expect( + performSync(engine, { repoPath: dir, dryRun: true, noPull: true, noEmbed: true, full: true }), + ).rejects.toThrow(/git repository/i); + // The whole point of --dry-run is "preview only" — it must never git-init + // or commit on our behalf, even though this is otherwise self-heal-eligible. + expect(existsSync(join(dir, '.git'))).toBe(false); + }); + + test('unborn-HEAD recovery: a bare `git init` with zero commits (interrupted prior self-heal) still completes', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + const { execSync } = await import('child_process'); + // Simulate a self-heal that ran `git init` but died before the baseline + // commit landed (process killed, disk full, etc.) — `.git` exists so + // discoverGitRoot succeeds, but `git rev-parse HEAD` still fails. + execSync('git init -q', { cwd: dir }); + + const result = await performSync(engine, { repoPath: dir, noPull: true, noEmbed: true, full: true }); + + expect(result.status).toBe('first_sync'); + expect(result.added).toBe(2); + expect(execSync('git rev-parse HEAD', { cwd: dir }).toString().trim()).not.toBe(''); + }); + + test('db_only paths are excluded from the baseline commit even without gbrain.yml write support (P2: fail-closed exclusion)', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + const { mkdirSync } = await import('fs'); + const { execSync } = await import('child_process'); + mkdirSync(join(dir, 'private-cache')); + writeFileSync(join(dir, 'private-cache', 'secret.bin'), 'binary-ish content'); + writeFileSync( + join(dir, 'gbrain.yml'), + 'storage:\n db_only:\n - private-cache\n', + ); + + await performSync(engine, { noPull: true, noEmbed: true, full: true }); + + expect(existsSync(join(dir, '.git'))).toBe(true); + const tracked = execSync('git ls-files', { cwd: dir }).toString(); + expect(tracked).not.toContain('private-cache'); + }); + + test('db_only exclusion applies even when a pre-existing .gitignore already covers the same dir (round 9 P1: unconditional pathspec)', async () => { + // Regression for the "check-ignore pre-filter" version of this logic: + // when a dir is ALSO already covered by an existing .gitignore, git's + // `-A` bails with an advisory "paths ignored... use -f" even though + // the add otherwise succeeds. Exclusion must be unconditional and the + // advisory must not surface as a hard failure. + const { performSync } = await import('../src/commands/sync.ts'); + const { mkdirSync } = await import('fs'); + const { execSync } = await import('child_process'); + mkdirSync(join(dir, 'private-cache')); + writeFileSync(join(dir, 'private-cache', 'secret.bin'), 'binary-ish content'); + writeFileSync(join(dir, 'gbrain.yml'), 'storage:\n db_only:\n - private-cache\n'); + writeFileSync(join(dir, '.gitignore'), 'private-cache/\n'); + + const result = await performSync(engine, { noPull: true, noEmbed: true, full: true }); + + expect(result.status).toBe('first_sync'); + const tracked = execSync('git ls-files', { cwd: dir }).toString(); + expect(tracked).not.toContain('private-cache'); + }); + + test('a comment merely mentioning db_only does not false-positive the sniff test (round 9 P2)', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + // No `storage:` section at all — just a comment mentioning the word. + // A bare substring search would wrongly refuse this brain forever. + writeFileSync(join(dir, 'gbrain.yml'), '# db_only handling: TBD, not configured yet\n'); + + const result = await performSync(engine, { noPull: true, noEmbed: true, full: true }); + + expect(result.status).toBe('first_sync'); + expect(result.added).toBe(2); + }); + + test('a gbrain.yml that mentions db_only but resolves no dirs refuses the baseline commit (round 6 P2: unsupported-syntax sniff test)', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + const { execSync } = await import('child_process'); + // Flow-style array — valid YAML, but the narrow custom parser only + // handles block-style lists, so loadStorageConfig warns and resolves + // an empty db_only list rather than throwing. + writeFileSync(join(dir, 'gbrain.yml'), 'storage:\n db_only: [private-cache/]\n'); + + await expect( + performSync(engine, { noPull: true, noEmbed: true, full: true }), + ).rejects.toThrow(/db_only/i); + // `git init` (site 1's first step) already ran before the sniff-test + // guard (inside createSyncBaselineCommit) refused — that's fine, it's + // the same "unborn repo" state the round-6-P1 index-rebuild test above + // recovers from on a later retry, which would hit this same guard and + // refuse again until gbrain.yml is fixed. What must NOT happen is a + // commit landing with unknown/unexcluded content. + expect(existsSync(join(dir, '.git'))).toBe(true); + let hasCommit = true; + try { + execSync('git rev-parse HEAD', { cwd: dir, stdio: 'pipe' }); + } catch { + hasCommit = false; + } + expect(hasCommit).toBe(false); + }); + + test('unborn-HEAD recovery drops stale staged content the exclusion pathspec now wants excluded (round 6 P1: index rebuild)', async () => { + const { performSync } = await import('../src/commands/sync.ts'); + const { mkdirSync } = await import('fs'); + const { execSync } = await import('child_process'); + mkdirSync(join(dir, 'private-cache')); + writeFileSync(join(dir, 'private-cache', 'secret.bin'), 'binary-ish content'); + writeFileSync(join(dir, 'gbrain.yml'), 'storage:\n db_only:\n - private-cache\n'); + // Simulate an interrupted workflow that left this file staged in an + // unborn repo BEFORE gbrain's self-heal ever ran. + execSync('git init -q', { cwd: dir }); + execSync('git add private-cache/secret.bin', { cwd: dir }); + + await performSync(engine, { noPull: true, noEmbed: true, full: true }); + + const tracked = execSync('git ls-files', { cwd: dir }).toString(); + expect(tracked).not.toContain('private-cache'); + }); + +}); From 6ec3dd410e763f81438f441ed06840ca7f245422 Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 06:16:00 +0900 Subject: [PATCH 16/17] fix(gateway): land Anthropic cache_control breakpoints on the system block, not just the call-level auto marker (#2490) (#2981) gateway.chat() requested cacheSystem:true but never got a system-prompt cache hit on single-turn callers (page-summary, skillopt, enrich): the call-level providerOptions.anthropic.cacheControl is real (it becomes Anthropic's documented top-level "auto-cache the last cacheable block" shorthand via @ai-sdk/anthropic 3.0.47+), but for a stable system prompt paired with a different user message every call, "the last cacheable block" is that ever-varying tail -- every call writes a fresh cache entry there and never reads a prior one. Fix: pass system as a SystemModelMessage object (ai's documented shape for attaching provider options to the system block) carrying its own providerOptions.anthropic.cacheControl when cacheSystem is requested, and mirror the same marker onto the last tool def (Anthropic caches everything up to and including the last cache_control block it sees). The call-level marker is kept, not removed -- it still gives toolLoop()'s growing multi-turn conversation a rolling cache breakpoint on each turn's tail. All three markers now derive from one canonical cacheControlValue computed after provider_chat_options config merging, so a configured TTL override (e.g. ttl: '1h') applies consistently instead of only reaching the call-level marker. Verified red-before-fix by stashing the gateway.ts diff and confirming the new assertions fail on unfixed code, then restoring. Co-authored-by: Claude Sonnet 5 --- src/core/ai/gateway.ts | 64 +++++++- test/ai/gateway-cache-breakpoint.test.ts | 195 +++++++++++++++++++++++ test/ai/gateway-chat.test.ts | 8 + 3 files changed, 266 insertions(+), 1 deletion(-) create mode 100644 test/ai/gateway-cache-breakpoint.test.ts diff --git a/src/core/ai/gateway.ts b/src/core/ai/gateway.ts index 789c2db1e..a55dcc51c 100644 --- a/src/core/ai/gateway.ts +++ b/src/core/ai/gateway.ts @@ -2982,6 +2982,26 @@ export async function chat(opts: ChatOpts): Promise { const providerOptions: Record = {}; if (useCache) { + // Call-level `providerOptions.anthropic.cacheControl` is NOT a no-op: + // @ai-sdk/anthropic 3.0.47+ passes it through as a top-level + // `cache_control` field on the Anthropic request body, which the + // Messages API resolves as its documented "auto-cache the last + // cacheable block in the request" shorthand (see Anthropic's + // prompt-caching docs — "top-level auto-caching ... is the simplest + // option when you don't need fine-grained placement"). Keep it: it's + // what gives a growing multi-turn conversation (toolLoop()) a rolling + // cache breakpoint on each turn's tail for free, without us having to + // hand-roll the marker-walking logic subagent.ts's raw-SDK path uses. + // + // But "last cacheable block" is the wrong block for gbrain#2490's + // actual callers (page-summary, skillopt, enrich): those are + // single-turn calls with a STABLE system prompt and a DIFFERENT user + // message every time, so the auto-marker lands on the ever-varying + // tail — every call WRITES a fresh cache entry and never READS a prior + // one (cache_read_input_tokens stays 0 forever). Caching the stable + // prefix needs an EXPLICIT breakpoint on the system block itself, + // which is applied below via a `SystemModelMessage` (round-trips its + // own `providerOptions`) instead of a bare string. providerOptions.anthropic = { cacheControl: { type: 'ephemeral' } }; } // OpenAI prompt_cache_key (native-openai only): a stable per-prefix routing @@ -3000,6 +3020,30 @@ export async function chat(opts: ChatOpts): Promise { } applyConfiguredChatProviderOptions(providerOptions, cfg, recipe.id, modelId); + // Derive ONE canonical cache-control value AFTER config merging and reuse + // it for every breakpoint (system block, last tool def, call-level). If + // `provider_chat_options.anthropic.cacheControl` overrides the TTL (e.g. + // `{ type: 'ephemeral', ttl: '1h' }`), that override lands in + // `providerOptions.anthropic.cacheControl` via the deep-merge above — + // reusing it here (instead of hardcoding `{ type: 'ephemeral' }` per + // breakpoint) keeps every marker in the request on the same TTL. + const cacheControlValue: { type: 'ephemeral'; ttl?: '5m' | '1h' } | undefined = useCache + ? (providerOptions.anthropic?.cacheControl ?? { type: 'ephemeral' }) + : undefined; + + // Anthropic-only secondary breakpoint: mark the LAST tool def too (mirrors + // subagent.ts's raw-SDK path — Anthropic caches everything up to and + // including the last `cache_control` block it sees in the request, so + // marking the last tool extends the cached prefix through the whole tool + // list). `tool.providerOptions.anthropic.cacheControl` is the shape + // @ai-sdk/anthropic 3.x reads for tool-def breakpoints. + if (cacheControlValue && opts.tools && opts.tools.length > 0 && tools) { + const lastTool = tools[opts.tools[opts.tools.length - 1]!.name]; + if (lastTool) { + lastTool.providerOptions = { anthropic: { cacheControl: cacheControlValue } }; + } + } + let _budgetRecorded = false; const _recordBudget = (modelLabel: string, inputTokens: number, outputTokens: number): void => { if (!tracker || _budgetRecorded) return; @@ -3016,10 +3060,28 @@ export async function chat(opts: ChatOpts): Promise { } }; + // The actual Anthropic system-prompt cache breakpoint. A bare string + // `system` produces `{ role: 'system', content }` with no `providerOptions` + // field (ai@6's convertToLanguageModelPrompt), so @ai-sdk/anthropic's + // getCacheControl(providerOptions) on that block always resolves to + // nothing. Passing a `SystemModelMessage` object instead — the shape `ai` + // documents specifically for "additional provider options (e.g. for + // caching)" — round-trips `providerOptions` onto that block. Byte-identical + // to the old bare-string form when useCache is false. Reuses + // `cacheControlValue` (the config-merged value) so this breakpoint's TTL + // always matches the last-tool and call-level breakpoints. + const systemParam = cacheControlValue && opts.system + ? { + role: 'system' as const, + content: opts.system, + providerOptions: { anthropic: { cacheControl: cacheControlValue } }, + } + : opts.system; + try { const result = await _generateTextTransport({ model, - system: opts.system, + system: systemParam, messages: toModelMessages(repairToolPairing(opts.messages)) as any, tools: opts.tools && opts.tools.length > 0 ? tools : undefined, maxOutputTokens: opts.maxTokens ?? defaultMaxOutputTokens(modelStr), diff --git a/test/ai/gateway-cache-breakpoint.test.ts b/test/ai/gateway-cache-breakpoint.test.ts new file mode 100644 index 000000000..8a9b65ca9 --- /dev/null +++ b/test/ai/gateway-cache-breakpoint.test.ts @@ -0,0 +1,195 @@ +/** + * gbrain#2490 — gateway.chat() never caches a stable system prompt across + * varying single-turn calls (page-summary, skillopt, enrich). + * + * Root cause: `chat()` passed `system` as a bare string and relied solely on + * a CALL-LEVEL `providerOptions.anthropic.cacheControl`. On `ai@6` + + * `@ai-sdk/anthropic@3.x`, that call-level marker is real — it's serialized + * as a top-level `cache_control` field on the Anthropic request body, which + * the Messages API resolves via its documented "auto-cache the LAST + * cacheable block in the request" shorthand (see Anthropic's prompt-caching + * docs). For a single-turn call with a stable system prompt and a DIFFERENT + * user message every time, "the last cacheable block" is that ever-varying + * user message — every call WRITES a fresh cache entry there and never + * READS a prior one, so `cache_read_input_tokens` stays 0 forever even + * though a `cache_control` breakpoint genuinely reaches Anthropic. + * + * Fix: ALSO pass `system` as a `SystemModelMessage` object (`{ role: + * 'system', content, providerOptions }`) when caching is requested — the + * shape `ai` documents specifically for attaching provider options to the + * system block — and mark the last tool def's own `providerOptions` too + * (mirrors the already-correct raw-SDK path in `subagent.ts`). The + * call-level marker is KEPT (not removed): it's what gives `toolLoop()`'s + * growing multi-turn conversation a rolling cache breakpoint on each turn's + * tail, which the explicit system/tool markers alone don't provide. + * + * These tests pin the FIX by inspecting the exact args handed to the + * `generateText` transport (via `__setGenerateTextTransportForTests`), + * not by asserting on `providerOptions` alone — that field is exactly what + * the bug made you believe was sufficient. + */ + +import { describe, test, expect, beforeEach } from 'bun:test'; +import { + chat, + configureGateway, + resetGateway, + __setGenerateTextTransportForTests, +} from '../../src/core/ai/gateway.ts'; + +describe('gbrain#2490 — Anthropic cache breakpoint placement', () => { + beforeEach(() => { + resetGateway(); + __setGenerateTextTransportForTests(null); + }); + + async function captureTransportArgs( + opts: Partial[0]> = {}, + ): Promise { + let captured: any; + __setGenerateTextTransportForTests(async (args: any) => { + captured = args; + return { + content: [{ type: 'text', text: 'ok' }], + finishReason: 'stop', + usage: { inputTokens: 1, outputTokens: 1 }, + } as any; + }); + configureGateway({ + chat_model: 'anthropic:claude-sonnet-4-6', + env: { ANTHROPIC_API_KEY: 'fake' }, + }); + await chat({ + model: 'anthropic:claude-sonnet-4-6', + messages: [{ role: 'user', content: 'hello' }], + ...opts, + }); + return captured; + } + + test('cacheSystem:true puts a real breakpoint on the system block (SystemModelMessage, not a bare string)', async () => { + const args = await captureTransportArgs({ system: 'You are a helpful assistant.', cacheSystem: true }); + + // The regression: `system` used to stay a bare string forever, which + // carries no per-block `providerOptions` — no breakpoint could ever land. + expect(typeof args.system).not.toBe('string'); + expect(args.system).toEqual({ + role: 'system', + content: 'You are a helpful assistant.', + providerOptions: { anthropic: { cacheControl: { type: 'ephemeral' } } }, + }); + }); + + test('cacheSystem:true ALSO keeps the call-level cache_control on top-level providerOptions (rolling-conversation cache for toolLoop)', async () => { + const args = await captureTransportArgs({ system: 'SYS', cacheSystem: true }); + + // Not removed: @ai-sdk/anthropic serializes this as the Anthropic API's + // documented top-level "auto-cache the last cacheable block" shorthand, + // which is what gives a growing multi-turn toolLoop() conversation a + // rolling cache breakpoint on each turn's tail. The explicit + // system-block marker (asserted above) is what actually fixes gbrain#2490 + // for single-turn callers — the two coexist, marking different blocks. + expect(args.providerOptions?.anthropic?.cacheControl).toEqual({ type: 'ephemeral' }); + }); + + test('cacheSystem:true marks the LAST tool def with its own providerOptions.anthropic.cacheControl', async () => { + const args = await captureTransportArgs({ + system: 'SYS', + cacheSystem: true, + tools: [ + { name: 'search', description: 'search', inputSchema: { type: 'object', properties: {} } }, + { name: 'put_page', description: 'put_page', inputSchema: { type: 'object', properties: {} } }, + ], + }); + + expect(args.tools.search.providerOptions).toBeUndefined(); + expect(args.tools.put_page.providerOptions).toEqual({ + anthropic: { cacheControl: { type: 'ephemeral' } }, + }); + }); + + test('cacheSystem:false (default) leaves system a byte-identical bare string — no behavior change', async () => { + const args = await captureTransportArgs({ system: 'SYS', cacheSystem: false }); + expect(args.system).toBe('SYS'); + expect(args.providerOptions).toBeUndefined(); + }); + + test('cacheSystem omitted entirely leaves system a byte-identical bare string — no behavior change', async () => { + const args = await captureTransportArgs({ system: 'SYS' }); + expect(args.system).toBe('SYS'); + expect(args.providerOptions).toBeUndefined(); + }); + + test('cacheSystem:true with no system prompt does not synthesize an empty cached system block', async () => { + const args = await captureTransportArgs({ cacheSystem: true }); + expect(args.system).toBeUndefined(); + }); + + test('cacheSystem:true with no tools does not throw and leaves tools undefined', async () => { + const args = await captureTransportArgs({ system: 'SYS', cacheSystem: true }); + expect(args.tools).toBeUndefined(); + }); + + test('cacheSystem:true on a non-Anthropic model is silently ignored (supports_prompt_cache=false)', async () => { + let captured: any; + __setGenerateTextTransportForTests(async (args: any) => { + captured = args; + return { + content: [{ type: 'text', text: 'ok' }], + finishReason: 'stop', + usage: { inputTokens: 1, outputTokens: 1 }, + } as any; + }); + configureGateway({ + chat_model: 'openai:gpt-4o-mini', + env: { OPENAI_API_KEY: 'fake' }, + }); + await chat({ + model: 'openai:gpt-4o-mini', + system: 'SYS', + cacheSystem: true, + messages: [{ role: 'user', content: 'hello' }], + }); + // Still a bare string — the recipe doesn't support prompt caching, so + // useCache is false regardless of the caller's request. + expect(captured.system).toBe('SYS'); + }); + + test('a configured cacheControl TTL override applies to every breakpoint, not just the call-level one', async () => { + // Codex review finding: with three independently-hardcoded `{type: + // 'ephemeral'}` markers, a `provider_chat_options.anthropic.cacheControl` + // TTL override (e.g. `ttl: '1h'`) would only reach the call-level marker + // via applyConfiguredChatProviderOptions()'s deep-merge — the system and + // tool markers would stay implicit 5m, mixing TTLs across breakpoints in + // the same request. Assert all three markers derive from ONE canonical + // value instead. + let captured: any; + __setGenerateTextTransportForTests(async (args: any) => { + captured = args; + return { + content: [{ type: 'text', text: 'ok' }], + finishReason: 'stop', + usage: { inputTokens: 1, outputTokens: 1 }, + } as any; + }); + configureGateway({ + chat_model: 'anthropic:claude-sonnet-4-6', + provider_chat_options: { + anthropic: { cacheControl: { type: 'ephemeral', ttl: '1h' } }, + }, + env: { ANTHROPIC_API_KEY: 'fake' }, + }); + await chat({ + model: 'anthropic:claude-sonnet-4-6', + system: 'SYS', + cacheSystem: true, + tools: [{ name: 'search', description: 'search', inputSchema: { type: 'object', properties: {} } }], + messages: [{ role: 'user', content: 'hello' }], + }); + + const expected = { type: 'ephemeral', ttl: '1h' }; + expect(captured.providerOptions?.anthropic?.cacheControl).toEqual(expected); + expect((captured.system as any)?.providerOptions?.anthropic?.cacheControl).toEqual(expected); + expect(captured.tools?.search?.providerOptions?.anthropic?.cacheControl).toEqual(expected); + }); +}); diff --git a/test/ai/gateway-chat.test.ts b/test/ai/gateway-chat.test.ts index f2ba4ff21..4aee06a26 100644 --- a/test/ai/gateway-chat.test.ts +++ b/test/ai/gateway-chat.test.ts @@ -297,6 +297,14 @@ describe('chat touchpoint — provider_chat_options passthrough', () => { }); test('anthropic cacheControl survives provider_chat_options merging', async () => { + // gbrain#2490: this call-level cacheControl is real (not a no-op) — + // @ai-sdk/anthropic serializes it as the Anthropic API's documented + // top-level "auto-cache the last cacheable block" shorthand. It's kept + // alongside the fix (an explicit breakpoint on the system message's own + // providerOptions — see test/ai/gateway-cache-breakpoint.test.ts) because + // it's what gives toolLoop()'s growing multi-turn conversation a rolling + // cache breakpoint on each turn's tail. See gateway.ts's `useCache` block + // for the full explanation of why both markers are needed. const providerOptions = await captureProviderOptions({ chat_model: 'anthropic:claude-sonnet-4-6', provider_chat_options: { From 9f7244a77f1fa25bda2a4482b977ceed6d5b267e Mon Sep 17 00:00:00 2001 From: Masa <98894508+Masashi-Ono0611@users.noreply.github.com> Date: Tue, 21 Jul 2026 06:27:50 +0900 Subject: [PATCH 17/17] =?UTF-8?q?fix(cli):=20remove=20sync=20--install-cro?= =?UTF-8?q?n=20help=20text=20=E2=80=94=20no=20handler=20ever=20existed=20(?= =?UTF-8?q?#2795)=20(#2972)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The top-level `gbrain --help` advertised `sync --install-cron` since the line was first added, but `src/commands/sync.ts` never parsed or handled the flag — `gbrain sync --install-cron` silently ran an ordinary one-off sync instead of installing anything, manufacturing false confidence in the exact durability layer operators reach for it to secure. git blame shows the line was introduced once (v0.42.29.0 help-text scaffold) and never touched again — no design intent to recover. Implementing it would also compete with autopilot, which already owns this job: `gbrain autopilot --install` runs a self-maintaining daemon (sync+extract+embed) on a schedule, including a per-source freshness check that submits `sync` jobs on its own interval. A second, separate sync-only cron would be a competing scheduler outside the D10 cycle-lock invariant that already keeps autopilot's own targeted-submit and full-cycle paths from double-processing. Removed the misleading line and pointed sync's --watch entry at `autopilot --install`, mirroring the existing `dream` command's "See also: autopilot --install (continuous daemon)." pattern one section below. Added regression coverage to test/cli-help-discoverability.test.ts asserting the help text no longer promises install-cron and does point at autopilot. --- src/cli.ts | 2 +- test/cli-help-discoverability.test.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/cli.ts b/src/cli.ts index 154f449b1..a23881671 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -2289,7 +2289,7 @@ IMPORT/EXPORT import [--no-embed] Import markdown directory sync [--repo ] [flags] Git-to-brain incremental sync sync --watch [--interval N] Continuous sync (loops until stopped) - sync --install-cron Install persistent sync daemon + See also: autopilot --install (continuous daemon). export [--dir ./out/] Export to markdown export --restore-only [--repo

] Restore missing supabase-only files [--type T] [--slug-prefix S] With optional filters diff --git a/test/cli-help-discoverability.test.ts b/test/cli-help-discoverability.test.ts index 451c5821b..3a00b39d6 100644 --- a/test/cli-help-discoverability.test.ts +++ b/test/cli-help-discoverability.test.ts @@ -98,6 +98,33 @@ describe('WARN-6 — main `gbrain --help` lists capture/brainstorm/lsd', () => { }); }); +describe('#2795 — `sync --install-cron` help line no longer promises an unbuilt feature', () => { + test('main `gbrain --help` does not advertise install-cron', () => { + // Pre-fix: `sync --install-cron Install persistent sync daemon` was + // listed in the top-level help with no flag parsing or handler behind + // it anywhere in src/commands/sync.ts — `gbrain sync --install-cron` + // silently ran an ordinary sync instead of installing anything. + const { stdout, status } = runCli(['--help']); + expect(status).toBe(0); + expect(stdout).not.toContain('install-cron'); + expect(stdout).not.toContain('Install persistent sync daemon'); + }); + + test('main `gbrain --help` points sync users at the real continuous-daemon command', () => { + const { stdout } = runCli(['--help']); + // autopilot --install already runs sync+extract+embed on a schedule + // (docs/architecture/KEY_FILES.md); point discoverability there instead + // of promising a separate sync-only cron installer that never existed. + expect(stdout).toMatch(/sync --watch \[--interval N\][^\n]*\n\s*See also: autopilot --install/); + }); + + test('`gbrain sync --help` never listed install-cron either', () => { + const { stdout, status } = runCli(['sync', '--help']); + expect(status).toBe(0); + expect(stdout).not.toContain('install-cron'); + }); +}); + describe('#1175 — main `gbrain --help` SOURCES block matches the real subcommand set', () => { test('archive and its lifecycle siblings are listed', () => { const { stdout, status } = runCli(['--help']);