diff --git a/src/cli.ts b/src/cli.ts index 3ca8a66fd..e4f8c9a0f 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -54,7 +54,7 @@ export function bigintToStringReplacer(_key: string, value: unknown): unknown { } // CLI-only commands that bypass the operation layer -export const CLI_ONLY = new Set(['init', 'reinit-pglite', 'upgrade', 'post-upgrade', 'check-update', 'integrations', 'publish', 'check-backlinks', 'lint', 'report', 'import', 'export', 'files', 'embed', 'serve', 'call', 'config', 'doctor', 'migrate', 'eval', 'sync', 'extract', 'extract-conversation-facts', 'enrich', 'features', 'autopilot', 'graph-query', 'jobs', 'agent', 'apply-migrations', 'skillpack-check', 'skillpack', 'resolvers', 'integrity', 'repair-jsonb', 'orphans', 'maintain', 'sources', 'mounts', 'dream', 'check-resolvable', 'routing-eval', 'skillify', 'smoke-test', 'providers', 'storage', 'repos', 'code-def', 'code-refs', 'reindex', 'reindex-code', 'reindex-frontmatter', 'code-callers', 'code-callees', 'reconcile-links', 'frontmatter', 'auth', 'friction', 'claw-test', 'book-mirror', 'takes', 'think', 'salience', 'anomalies', 'calibration', 'transcripts', 'models', 'remote', 'recall', 'forget', 'edges-backfill', 'cache', 'ze-switch', 'founder', 'brainstorm', 'lsd', 'schema', 'capture', 'onboard', 'conversation-parser', 'status', 'connect', 'skillopt', 'quarantine', 'self-upgrade', 'advisor', 'watch', 'reindex-search-vector']); +export const CLI_ONLY = new Set(['init', 'reinit-pglite', 'upgrade', 'post-upgrade', 'check-update', 'integrations', 'publish', 'check-backlinks', 'lint', 'report', 'import', 'export', 'files', 'embed', 'serve', 'call', 'config', 'doctor', 'migrate', 'eval', 'sync', 'extract', 'extract-conversation-facts', 'enrich', 'features', 'autopilot', 'graph-query', 'jobs', 'agent', 'apply-migrations', 'skillpack-check', 'skillpack', 'resolvers', 'integrity', 'repair-jsonb', 'orphans', 'maintain', 'sources', 'mounts', 'dream', 'check-resolvable', 'routing-eval', 'skillify', 'smoke-test', 'providers', 'storage', 'repos', 'code-def', 'code-refs', 'reindex', 'reindex-code', 'reindex-frontmatter', 'code-callers', 'code-callees', 'reconcile-links', 'frontmatter', 'auth', 'friction', 'claw-test', 'book-mirror', 'takes', 'think', 'salience', 'anomalies', 'calibration', 'transcripts', 'models', 'remote', 'recall', 'forget', 'edges-backfill', 'backfill', 'cache', 'ze-switch', 'founder', 'brainstorm', 'lsd', 'schema', 'capture', 'onboard', 'conversation-parser', 'status', 'connect', 'skillopt', 'quarantine', 'self-upgrade', 'advisor', 'watch', 'reindex-search-vector']); // CLI-only commands whose handlers print their own --help text. These are // excluded from the generic short-circuit so detailed per-command and // per-subcommand usage stays reachable. @@ -106,6 +106,11 @@ const CLI_ONLY_SELF_HELP = new Set([ // `gbrain connect --help` prints its own usage (flags + examples) from // runConnect; route around the generic one-line short-circuit. 'connect', + // #3224 — `backfill` was missing from CLI_ONLY entirely (dispatch never + // reached runBackfillCommand). Once added there, the generic short-circuit + // still shadowed its own printHelp() (kind list + flags, same WARN-5 class + // as capture); route around it so `gbrain backfill --help` reaches it. + 'backfill', ]); // v114 (#1941): alias -> operation lookup, kept separate from `cliOps` so @@ -1007,6 +1012,17 @@ const THIN_CLIENT_REFUSED_COMMANDS = new Set([ // it gets a partial dispatch (list/get route over MCP engine-free, the // rest refuse) in the main dispatch before connectEngine(). 'config', + // #3224: `backfill` (like migrate/apply-migrations/repair-jsonb) opens + // and mutates the local engine directly with no MCP equivalent op. On a + // thin-client install it would otherwise fabricate the same kind of + // ephemeral scratch-local PGLite `config` did before this audit — + // refuse cleanly instead. This check runs before backfill's own + // pre-connectEngine dispatch branch, so `--help` is ALSO refused on a + // thin client rather than showing help — the same pre-existing tradeoff + // `sync`/`enrich` already make (both are also THIN_CLIENT_REFUSED_COMMANDS + // with their own pre-connectEngine `--help` branch further down); not a + // new inconsistency introduced here. + 'backfill', ]); /** @@ -1047,6 +1063,8 @@ const THIN_CLIENT_REFUSE_HINTS: Record = { // 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.', + // #3224 + backfill: 'backfill mutates the host brain directly with no MCP equivalent. Run `gbrain backfill` on the host machine.', }; /** @@ -1525,6 +1543,24 @@ async function handleCliOnly(command: string, args: string[]) { return; } + // #3224: `backfill` dispatches unconditionally here (not just --help). + // runBackfillCommand manages its own engine end-to-end (createEngine + + // connect at commands/backfill.ts, disconnect when done) — it takes no + // engine parameter at all. The old `case 'backfill':` further down sits + // behind this function's shared `const engine = await connectEngine()`; + // reaching it there would open a SECOND PGLite connection to the same + // database while the first is still held, and PGLite's single-writer + // lock would make every real (non---help) backfill run hang for 30s and + // fail. Dispatching before that shared connect — same as schema/init/ + // auth/remote above — avoids the double-connect entirely and, as a + // bonus, makes `--help` reachable from a fresh tmpdir with no configured + // brain (same motivation as the sync/capture/enrich branches below). + if (command === 'backfill') { + const { runBackfillCommand } = await import('./commands/backfill.ts'); + await runBackfillCommand(args); + return; + } + // v0.41.6.0 D3 (per outside-voice F1): connect-time + dispatch-time wallclock // timeouts for read-only commands whose hang would otherwise spin at 100% CPU // (the production "10-day zombie gbrain search ping" bug class). The wrap @@ -2061,13 +2097,13 @@ async function handleCliOnly(command: string, args: string[]) { await reindexFrontmatterCli(args); return; // reindexFrontmatterCli handles its own engine lifecycle } - case 'backfill': { - // v0.30.1: first-class generic backfill command. Subcommand dispatch - // is inside runBackfillCommand (kind | list | --help). - const { runBackfillCommand } = await import('./commands/backfill.ts'); - await runBackfillCommand(args); - return; - } + // #3224: `backfill` (v0.30.1's first-class generic backfill command) + // used to have its `case` right here, but runBackfillCommand manages + // its own engine end-to-end and dispatching it AFTER this switch's + // shared `connectEngine()` double-connects PGLite's single-writer + // lock to itself (hangs 30s, then fails on every real run). Moved to + // an unconditional pre-connectEngine branch above, next to schema/ + // init/auth/remote/sync/capture/enrich — see that branch's comment. case 'code-callers': { // v0.20.0 Cathedral II Layer 10 (C4): "who calls ?" const { runCodeCallers } = await import('./commands/code-callers.ts'); diff --git a/test/cli-backfill-dispatch.test.ts b/test/cli-backfill-dispatch.test.ts new file mode 100644 index 000000000..625f0aa23 --- /dev/null +++ b/test/cli-backfill-dispatch.test.ts @@ -0,0 +1,199 @@ +/** + * #3224 — `backfill` was missing from the `CLI_ONLY` set at src/cli.ts, + * so the dispatcher rejected the command with "Unknown command: backfill" + * before ever reaching the fully-implemented `case 'backfill':` inside + * `handleCliOnly`'s switch (`runBackfillCommand` in + * src/commands/backfill.ts). `edges-backfill` was registered, which is + * why the omission went unnoticed. + * + * A second, deeper bug surfaced once `backfill` became reachable: + * `runBackfillCommand` manages its own PGLite engine end-to-end + * (createEngine + connect + disconnect) and takes no engine argument at + * all. The old `case 'backfill':` sat behind handleCliOnly's SHARED + * `const engine = await connectEngine()` — dispatching there would open a + * second PGLite connection to the same data dir while the first was still + * held, and PGLite's single-writer file lock (src/core/pglite-lock.ts) + * has no same-process reentrancy check, so every real (non---help) run + * would hang for the full 30s lock timeout and then fail. The fix moves + * `backfill` dispatch to an unconditional pre-connectEngine branch (same + * spot as schema/init/auth/remote) instead of just gating `--help` there. + * + * Three things are pinned here: + * 1. The runtime repro from the issue (`gbrain backfill --help` / + * `gbrain backfill`) no longer reports "Unknown command". + * 2. A real backfill run against a configured PGLite brain completes + * quickly instead of hanging on its own lock. + * 3. The issue's own suggested class-guard: every `case` in + * handleCliOnly's command switch must have a matching `CLI_ONLY` + * entry, so the next one-word omission fails CI instead of shipping + * silently disabled. + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { spawnSync } from 'node:child_process'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const CLI_TS_PATH = fileURLToPath(new URL('../src/cli.ts', import.meta.url)); +const REPO_ROOT = fileURLToPath(new URL('..', import.meta.url)); + +function runCli( + args: string[], + gbrainHome?: string, + opts?: { timeoutMs?: number }, +): { stdout: string; stderr: string; status: number; timedOut: boolean } { + const result = spawnSync('bun', ['run', 'src/cli.ts', ...args], { + cwd: REPO_ROOT, + encoding: 'utf8', + env: { + ...process.env, + GBRAIN_HOME: gbrainHome ?? '/tmp/gbrain-test-cli-backfill-dispatch-nonexistent', + }, + // Explicit timeout: spawnSync blocks the whole (single-threaded) test + // runner, so bun:test's own per-test timeout (the 3rd `test()` arg) + // can't interrupt a hung child process — it only starts counting again + // once spawnSync itself returns. Without this, a regression that + // reintroduces the ~30s PGLite lock hang would silently wait out the + // full 30s instead of the test failing fast on a bounded timeout. + timeout: opts?.timeoutMs, + }); + return { + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + status: result.status ?? -1, + // Node sets `error.code === 'ETIMEDOUT'` (and `signal` on the result) + // when `timeout` fires and the child is killed. + timedOut: result.signal !== null || (result.error as NodeJS.ErrnoException | undefined)?.code === 'ETIMEDOUT', + }; +} + +describe('#3224 — `gbrain backfill` is dispatchable', () => { + test('`backfill --help` reaches the real HELP text, not "Unknown command"', () => { + const { stdout, stderr, status } = runCli(['backfill', '--help']); + expect(stderr).not.toContain('Unknown command'); + expect(status).toBe(0); + // runBackfillCommand's own printHelp() (src/commands/backfill.ts) — + // proves dispatch reached the real handler, not the generic CLI_ONLY + // one-line stub (the WARN-5 class: registering `backfill` in CLI_ONLY + // alone routes it behind the generic short-circuit / a `connectEngine()` + // that a fresh tmpdir can't satisfy; the fix also needed the + // pre-engine-bind `--help` short-circuit + a CLI_ONLY_SELF_HELP entry). + expect(stdout).toContain('gbrain backfill list'); + expect(stdout).toContain('--batch-size N'); + }); + + test('`-h` short flag also works', () => { + const { stdout, status } = runCli(['backfill', '-h']); + expect(status).toBe(0); + expect(stdout).toContain('gbrain backfill list'); + }); + + test('`backfill` with no brain configured fails on the config, not "Unknown command"', () => { + // Without --help, runBackfillCommand needs a real engine, so a + // fresh/nonexistent GBRAIN_HOME must still fail — but on "no brain + // configured", never on dispatch rejecting the command outright. + const { stderr, status } = runCli(['backfill']); + expect(status).not.toBe(0); + expect(stderr).not.toContain('Unknown command'); + }); +}); + +describe('#3224 — a real backfill run against a configured PGLite brain does not deadlock', () => { + let gbrainHome: string; + + beforeAll(() => { + gbrainHome = mkdtempSync(join(tmpdir(), 'gbrain-test-cli-backfill-pglite-')); + const init = runCli(['init', '--pglite', '--no-embedding'], gbrainHome); + expect(init.status).toBe(0); + }, 60000); + + afterAll(() => { + rmSync(gbrainHome, { recursive: true, force: true }); + }); + + test('`backfill effective_date --dry-run` connects, runs, and exits — no 30s PGLite lock hang', () => { + // Pre-fix (case inside the switch, behind the shared connectEngine()): + // this would hang for ~30s and then fail with "Timed out waiting for + // PGLite lock. Process has held it since ...". The explicit + // spawnSync `timeoutMs` below (well under the 30s lock timeout) is what + // actually bounds this — bun:test's own per-test timeout can't + // interrupt a blocking spawnSync call, it only resumes counting once + // spawnSync returns. + const { stdout, stderr, status, timedOut } = runCli( + ['backfill', 'effective_date', '--dry-run'], + gbrainHome, + { timeoutMs: 20_000 }, + ); + expect(timedOut).toBe(false); + expect(stderr).not.toContain('Timed out waiting for PGLite lock'); + expect(status).toBe(0); + expect(stdout).toContain('Running backfill: effective_date'); + expect(stdout).toContain('complete'); + }, 25000); +}); + +describe('#3224 class guard — every handleCliOnly switch case is CLI_ONLY-registered', () => { + test('no `case` label in the command switch is missing from CLI_ONLY (except documented pre-existing dead arms)', () => { + const src = readFileSync(CLI_TS_PATH, 'utf-8'); + + const setMatch = src.match(/export const CLI_ONLY = new Set\(\[([^\]]*)\]\);/); + expect(setMatch).not.toBeNull(); + const cliOnly = new Set([...setMatch![1].matchAll(/'([^']+)'/g)].map((m) => m[1])); + expect(cliOnly.size).toBeGreaterThan(0); + + // Scope tightly to the actual `switch (command) { ... }` body inside + // handleCliOnly — NOT the whole function. The function also has a long + // pre-engine if-chain (schema/init/auth/...) whose prose comments can + // themselves contain the literal text `case 'xyz':` when documenting + // this exact bug class (e.g. the `backfill` pre-connectEngine branch's + // own comment references the old case it replaced) — matching against + // the whole function body would misread that comment as a real, + // CLI_ONLY-registered dispatch site and silently mask a genuine gap. + const fnStartIdx = src.indexOf("async function handleCliOnly(command: string, args: string[]) {"); + expect(fnStartIdx).toBeGreaterThan(-1); + const fnEndIdx = src.indexOf('\nasync function dispatchReadOnlyCommand', fnStartIdx); + expect(fnEndIdx).toBeGreaterThan(fnStartIdx); + const fnBody = src.slice(fnStartIdx, fnEndIdx); + + const switchStartIdx = fnBody.indexOf('switch (command) {'); + expect(switchStartIdx).toBeGreaterThan(-1); + // The switch's own closing brace is immediately followed by + // ` } finally {` (the engine-teardown wrapper) — a marker specific + // enough to bound the switch precisely without a full brace-matcher. + const switchEndIdx = fnBody.indexOf('\n } finally {', switchStartIdx); + expect(switchEndIdx).toBeGreaterThan(switchStartIdx); + const switchBody = fnBody.slice(switchStartIdx, switchEndIdx); + + // Strip comments before matching so prose that merely MENTIONS a case + // label (block or line comments) can never be mistaken for a real one. + const switchBodyNoComments = switchBody + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/[^\n]*/g, ''); + + const cases = [...switchBodyNoComments.matchAll(/case '([^']+)':/g)].map((m) => m[1]); + expect(cases.length).toBeGreaterThan(30); // sanity: the switch is large + + const missing = cases.filter((c) => !cliOnly.has(c)); + + // Pre-existing switch arms that are unreachable for reasons OTHER than + // #3224's bug class: each is shadowed by an earlier, unconditional + // branch before the CLI_ONLY.has(command) dispatch gate is ever + // consulted, so adding them to CLI_ONLY would not change behavior (and + // fixing/removing the dead code is a separate, out-of-scope cleanup). + // Documented here instead of silently re-hidden, per the issue's own + // "next one-word omission" framing — if this allowlist needs to grow, + // that growth itself is the signal a case was orphaned. + const KNOWN_UNREACHABLE_DEAD_CASES = new Set([ + 'search', // shadowed by the T5 special-case (modes/stats/tune/diagnose) + the generic op fallback for free-text search + 'pages', // shadowed by the generic op fallback (list_pages) + 'notability-eval', // superseded command name; no CLI_ONLY entry ever existed for it + 'whoknows', // superseded command name (now `find_experts`); no CLI_ONLY entry ever existed for it + ]); + + const unexpected = missing.filter((c) => !KNOWN_UNREACHABLE_DEAD_CASES.has(c)); + expect(unexpected).toEqual([]); + }); +}); diff --git a/test/thin-client-routing-audit.test.ts b/test/thin-client-routing-audit.test.ts index 0c2bcba10..66de9d8ed 100644 --- a/test/thin-client-routing-audit.test.ts +++ b/test/thin-client-routing-audit.test.ts @@ -158,6 +158,22 @@ describe('thin-client routing audit — scratch-DB additions (jobs partial dispa expect(/\bconfig\s*:/.test(CLI_SOURCE.slice(hintsStart, hintsEnd))).toBe(true); }); + test("#3224 — 'backfill' is in THIN_CLIENT_REFUSED_COMMANDS with a hint", () => { + // backfill mutates the local engine directly (createEngine + connect in + // commands/backfill.ts) with no MCP equivalent — same class as `config` + // above. Without this, a thin-client install would fabricate the same + // kind of ephemeral scratch-local PGLite `config` did before its own + // v0.32 audit fix. + 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("'backfill'"); + const hintsStart = CLI_SOURCE.indexOf( + 'const THIN_CLIENT_REFUSE_HINTS: Record = {', + ); + const hintsEnd = CLI_SOURCE.indexOf('};', hintsStart); + expect(/\bbackfill\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);