diff --git a/CHANGELOG.md b/CHANGELOG.md index 01523f2d6..ee67ecd0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,140 @@ All notable changes to GBrain will be documented in this file. +## [0.31.1] - 2026-05-08 + +**Thin-client mode actually works now. `gbrain init --mcp-only` is no longer a half-built bridge.** +**Every read + write + admin op routes through MCP; refused commands carry pinpoint hints.** + +Hermes/Neuromancer hit this in production: thin-client install of wintermute (102k pages, +265k chunks). Every CLI search returned zero rows. Exit code 0. No warning. The agent +configured `gbrain init --mcp-only`, then walked into a wall of "no results found" against +a brain that had everything it needed. The CLI was opening the empty local PGLite, +running 38 migrations, and reporting "No results." while the real brain sat untouched +behind an HTTPS endpoint. + +v0.31.1 fixes the silent-empty-results bug class for every operation surface gbrain has. +The CLI dispatch layer now routes every non-localOnly op through `callRemoteTool` when +`isThinClient(cfg)` is true, before opening any local engine. Read ops, write ops, admin +ops — all routed. Local-only commands (sync, embed, dream, integrity, etc) refuse with +pinpoint hints naming the closest path: "sync runs on the host. To trigger a remote +cycle, run `gbrain remote ping` (queues an autopilot-cycle job)." + +### The numbers that matter + +Reproducible against any host with a populated brain. Spin `gbrain serve --http`, +register an OAuth client with `read,write,admin`, then from a second `GBRAIN_HOME`: + +| Behavior | v0.30.0 (broken) | v0.31.1 (fixed) | +|---|---|---| +| `gbrain search "X"` against 102k-page host | "No results." (exit 0) | Real rows + identity banner | +| `gbrain stats` on thin-client | Pages: 0, Chunks: 0 | Real numbers from host | +| `gbrain salience` | "(no pages touched in the salience window)" | Real salience output | +| `gbrain put 'wiki/test/foo' --content '...'` | Hits empty local DB | Persists on host | +| `gbrain sync` | Falls into PGLite migrations | Refuses with `gbrain remote ping` hint | +| `gbrain remote doctor` checks | 4 (config, oauth, discovery, smoke) | 5 (+ scope-probe with pinpoint remediation) | +| Identity feedback per command | None | `[thin-client → host:3131 · brain: 102k pages, 265k chunks · v0.31.1]` | + +Per-call latency adds ~50-200ms warm (token cache hit), ~250-500ms cold (token mint). +Documented; users who want sub-50ms loops should run on the host. + +### What this means for thin-client users + +A thin-client `gbrain` install is now functionally substitutable for a local install +on the read+write+admin op surface. Agents get the topology signal up-front via the +identity banner; humans get pinpoint hints when something is genuinely local-only. +The OAuth-scope-mismatch class of failures (v0.29.2/v0.30.0 thin-clients registered +with `read+write` only and now hitting `gbrain stats`) surfaces during +`gbrain remote doctor` instead of mid-command, with an exact remediation: + + `On the host: gbrain auth register-client --grant-types client_credentials --scopes read,write,admin` + +## To take advantage of v0.31.1 + +`gbrain upgrade` should do this automatically. If it didn't, or if `gbrain remote doctor` +warns about the new `oauth_client_scopes_probe` check: + +1. **On the THIN CLIENT** — re-run doctor to surface scope mismatches: + ```bash + gbrain remote doctor + ``` + The new `oauth_client_scopes_probe` check reports per-tier status. If admin is + missing, the check tells you the exact host-side command to run. + +2. **On the HOST (if doctor warned about admin scope)** — re-register your thin-client's + OAuth client with broader scopes: + ```bash + gbrain auth register-client --grant-types client_credentials --scopes read,write,admin + ``` + Then update the thin-client's `~/.gbrain/config.json` `remote_mcp.oauth_client_id` + and (env-var or config) `oauth_client_secret` with the new credentials. + +3. **Verify routing works** — run a representative read op that previously returned zero: + ```bash + gbrain search "anything you know is in the brain" + gbrain stats + ``` + Both should now show real numbers and an identity banner like + `[thin-client → wintermute:3131 · brain: 102k pages, 265k chunks · v0.31.1]`. + +4. **If any step fails or the numbers look wrong**, file an issue at + https://github.com/garrytan/gbrain/issues with `gbrain remote doctor` output. + +### Itemized changes + +#### Routing +- New `THIN_CLIENT_REFUSE_HINTS` table in `src/cli.ts` with pinpoint hints for every + refused command (sync/embed/extract/migrate/repair-jsonb/integrity/serve/dream/ + orphans/transcripts/storage/takes/sources). Hints name the closest remote path + (e.g. `gbrain remote ping`) or specific MCP tools (e.g. `find_orphans`). +- New `runThinClientRouted` in cli.ts hooks INTO the existing op-dispatch path + (CDX-1: no parallel module). Every non-localOnly op routes through `callRemoteTool` + on thin-client installs. +- CLI-only commands with MCP equivalents (`think`, `salience`, `anomalies`, + `graph-query`) get per-command thin-client routing branches. +- ENG-2 renderer parity: local-engine path runs `JSON.parse(JSON.stringify(result))` + before formatting so renderers see the same shape on both paths. + +#### New ops +- `get_brain_identity` (read scope, banner-only): cheap counter packet + `{version, engine, page_count, chunk_count, last_sync_iso}` for the thin-client + identity banner. + +#### Banner +- Identity banner prints to stderr before each routed command. 60s TTL in-memory + cache keyed by mcp_url. Suppression: `--quiet`, `GBRAIN_NO_BANNER=1`, non-TTY + default (override with `GBRAIN_BANNER=1`). + +#### Doctor +- New `oauth_client_scopes_probe` check (CDX-5) in `gbrain remote doctor`. Surfaces + v0.29.2/v0.30.0 thin-clients without admin scope BEFORE they hit + `gbrain stats`/`gbrain history` mid-command. + +#### Error handling +- Hardened `callRemoteTool` (CDX-4): all transport errors normalized to + `RemoteMcpError` with stable `reason` union and `kind`/`code` sub-tags on detail. + Dispatcher's exhaustive TS `never` switch produces canned, actionable messages. +- New `--timeout=Ns` global flag (ENG-4); SIGINT aborts via AbortController. + +#### Tests +- New `test/get-brain-identity.test.ts` (9), `test/oauth-scope-probe.test.ts` (8). +- Updated `test/cli-options.test.ts` (+8 --timeout cases, 28 total). +- Updated `test/cli-dispatch-thin-client.test.ts` (14 cases, refreshed for new + pinpoint-hint format). + +### For contributors +- Plan + decision history at `~/.claude/plans/how-to-make-mcp-iterative-liskov.md`. + Records 6 CEO-review decisions (D1-D6), 5 eng-review decisions (ENG-1..5), and + 6 codex outside-voice decisions (CDX-1..6). Codex's #1+#8 forced an architecture + rewrite (no parallel `src/core/thin-client/` module; route inline in cli.ts). + Codex's #2+#7 forced honest framing (no false "Liskov substitutability" claim + — server intentionally treats remote callers differently for `think.--save`/ + `--take` and `put_page` auto-link, both trust-boundary policy decisions). + Codex's #5 caught a false premise in the error-model design. +- Per-subcommand thin-client routing for `takes` and `sources` is a v0.31.x + follow-up TODO. v0.31.1 refuses both at the top level with hints pointing at + MCP tools. + ## [0.31.0] - 2026-05-08 **Hot memory ships. Your brain remembers what you said today, across sessions.** diff --git a/CLAUDE.md b/CLAUDE.md index 50cfd829b..c90d11116 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -248,6 +248,63 @@ gbrain-evals consumes: `gbrain/engine`, `gbrain/types`, `gbrain/operations`, `gbrain/extract`. Removing any of these is a breaking change for the gbrain-evals consumer. +## Thin-client routing (v0.31.1, Issue #734) + +`gbrain init --mcp-only` (v0.29.2) sets up a thin-client install: no local +brain content, just an OAuth client pointing at a remote `gbrain serve --http`. +v0.29.2/v0.30.0 only refused 9 obvious local-only commands; the other ~25 +silently fell through to `connectEngine()` and opened the empty local PGLite, +returning "No results." against a populated remote brain. v0.31.1 fixes the +silent-empty-results bug class for every operation surface. + +Key files: + +- `src/cli.ts` — Routing seam INSIDE the existing op-dispatch path (CDX-1: no + parallel `src/core/thin-client/` module; routing is a ~80-line conditional + in `runThinClientRouted`). Detects `isThinClient(cfg)` BEFORE `connectEngine` + so thin-client installs never open the empty PGLite. localOnly ops on + thin-client refuse via `refuseThinClient` (with pinpoint hint table + `THIN_CLIENT_REFUSE_HINTS`). Banner via `printIdentityBannerBestEffort` + before each routed call (suppressed by `--quiet`, `GBRAIN_NO_BANNER=1`, + non-TTY default). Exhaustive TS `never` switch on `RemoteMcpError.reason` + for canned, actionable error messages. ENG-2 renderer parity: local-engine + path runs `JSON.parse(JSON.stringify(result))` so renderers see the same + shape on both paths (kills Date/bigint/Buffer drift class). +- `src/core/mcp-client.ts` — `callRemoteTool(config, toolName, args, opts)`. + Hardened in v0.31.1 (CDX-4): all transport errors normalized to + `RemoteMcpError` via the `toRemoteMcpError` funnel. New `CallRemoteToolOptions + {timeoutMs, signal}`; `buildAbortController` composes external signal with + timeout. New `RemoteMcpErrorReason` stable union, `RemoteMcpErrorDetail.kind` + ('timeout' | 'aborted' | 'unreachable') sub-tag, `RemoteMcpErrorDetail.code` + field carrying server-supplied error codes (e.g. `missing_scope`). + `extractToolErrorCode` parses JSON envelopes first, falls back to substring + detection for legacy server messages. `unpackToolResult(res)` unchanged + (parses tool-call JSON content). `_clearMcpClientTokenCache()` test escape. +- `src/core/cli-options.ts` — `parseGlobalFlags` adds `--timeout=Ns` (accepts + `30s`, `2m`, `500ms`, plain ms). Default `null` = per-command default (30s + for most ops, 180s for `think`). `parseTimeout(s)` exported helper. +- `src/core/doctor-remote.ts` — `gbrain remote doctor` adds the + `oauth_client_scopes_probe` check (CDX-5). Probes the read tier via + `get_brain_identity` and admin tier via `get_health`; reports per-tier + status with pinpoint remediation when admin is missing. `buildScopeCheck` + + `ScopeProbeResult` exported for test access. Skippable via + `GBRAIN_DOCTOR_SKIP_SCOPE_PROBE=1` for fixtures that mock /mcp at JSON-RPC + initialize level only (MCP SDK Client hangs on shape mismatch). +- `src/core/operations.ts` — `get_brain_identity` op (read scope, no params, + banner-only): cheap counter packet `{version, engine, page_count, + chunk_count, last_sync_iso}` for the thin-client identity banner. Reuses + `engine.getStats()`; banner's 60s client-side TTL bounds frequency to + ≤1/60s per CLI process (well below the Fly.io health-check cadence that + motivated the original `getStats` cost warning). +- `src/commands/{salience,anomalies,graph-query,think}.ts` — Per-command + thin-client routing branches. These commands bypass the operation-layer + dispatch in cli.ts (call `engine.foo()` directly), so each gets its own + `if (isThinClient(cfg)) { callRemoteTool(...) }` branch that maps CLI flags + to op params. `think` is a special case: the server's `think` op + intentionally disables `--save`/`--take` for remote callers + (operations.ts:1103-1135 trust-boundary gate); thin-client `think` warns + loudly when those flags are set. + ## Commands Run `gbrain --help` or `gbrain --tools-json` for full command reference. diff --git a/TODOS.md b/TODOS.md index 2b749f59e..58a32d827 100644 --- a/TODOS.md +++ b/TODOS.md @@ -1,5 +1,53 @@ # TODOS +## Thin-client mode follow-ups (v0.31.1, Issue #734) + +- [ ] **v0.31.x: routed-call timing telemetry.** `GBRAIN_TIMING=1` prints + `token_mint=Xms http=Yms server=Zms total=Wms` per routed MCP call. + Audit log at `~/.gbrain/audit/routed-calls-YYYY-Www.jsonl`. Cherry-pick + C from #734 plan; deferred from v0.31.1 to keep scope tight. + +- [ ] **v0.31.2: job-submission routing for `gbrain dream` etc.** Route + long-running ops (`dream`, `embed --stale`, `extract`) via `submit_job` + + poll, mirroring the existing `gbrain remote ping` autopilot-cycle + pattern. Cherry-pick D from #734 plan. Adds a thin-client async-job + render layer (progress events + spinner). + +- [ ] **Per-subcommand thin-client routing for `takes` and `sources`.** + CDX-2 audit identified the READ subcommands (`takes_list`, `takes_search`, + `sources_list`, `sources_status`) as routable; mutate subcommands edit + local files. v0.31.1 refuses both at the top level with hints. Split + is a v0.31.x release. + +- [ ] **Privacy decision: lift `localOnly: true` on `get_recent_transcripts`?** + Raw chat exports leaving the host is a real tradeoff. Needs explicit + per-token scope (`scope: 'transcripts'`) and consent UX. Out of v0.31.1. + +- [ ] **Trust-boundary policy review for remote-caller gates.** Server + intentionally disables `think.--save`/`--take` for remote callers + (operations.ts:1103-1135) and skips `put_page` auto-link/auto-timeline + for remote callers without `trustedWorkspace` (operations.ts:434-451). + Subagent-isolation reasons; blocks full thin-client parity. Policy + decision, not a routing fix. + +- [ ] **v0.32.0: flip `gbrain auth register-client` default scope from + `read` to `read,write,admin`.** Breaking for existing read-only scrapers; + ship deprecation warning in v0.31.x. The v0.31.1 `oauth_client_scopes_probe` + doctor check surfaces the gap with pinpoint remediation in the meantime. + +- [ ] **v0.31.x: cross-process OAuth token cache at + `~/.gbrain/oauth-token-cache.json`.** Cuts ~200ms cold-start cost for + shell-loop usage on thin-client installs. Today the in-memory cache is + per-process; every `gbrain` invocation pays a fresh token mint. + +- [ ] **v0.31.x: parity test (`test/thin-client-parity.test.ts`).** Plan + called for ~400 LOC byte-equal stdout assertions for 12+ ops via an + in-process MCP server pointed at the same PGLite as the local-engine + path. Harder than expected because it needs MCP server setup that the + current test infrastructure doesn't expose. v0.31.1 ships without it; + ENG-2's JSON-shape normalization + per-command test coverage is the + interim guard. + ## LongMemEval benchmark follow-ups (v0.28.12) ### Closed: full 500-question 4-adapter run published diff --git a/VERSION b/VERSION index 26bea73e8..f176c9441 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.31.0 +0.31.1 diff --git a/llms-full.txt b/llms-full.txt index c240ab4bc..c55fe2fb4 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -348,6 +348,63 @@ gbrain-evals consumes: `gbrain/engine`, `gbrain/types`, `gbrain/operations`, `gbrain/extract`. Removing any of these is a breaking change for the gbrain-evals consumer. +## Thin-client routing (v0.31.1, Issue #734) + +`gbrain init --mcp-only` (v0.29.2) sets up a thin-client install: no local +brain content, just an OAuth client pointing at a remote `gbrain serve --http`. +v0.29.2/v0.30.0 only refused 9 obvious local-only commands; the other ~25 +silently fell through to `connectEngine()` and opened the empty local PGLite, +returning "No results." against a populated remote brain. v0.31.1 fixes the +silent-empty-results bug class for every operation surface. + +Key files: + +- `src/cli.ts` — Routing seam INSIDE the existing op-dispatch path (CDX-1: no + parallel `src/core/thin-client/` module; routing is a ~80-line conditional + in `runThinClientRouted`). Detects `isThinClient(cfg)` BEFORE `connectEngine` + so thin-client installs never open the empty PGLite. localOnly ops on + thin-client refuse via `refuseThinClient` (with pinpoint hint table + `THIN_CLIENT_REFUSE_HINTS`). Banner via `printIdentityBannerBestEffort` + before each routed call (suppressed by `--quiet`, `GBRAIN_NO_BANNER=1`, + non-TTY default). Exhaustive TS `never` switch on `RemoteMcpError.reason` + for canned, actionable error messages. ENG-2 renderer parity: local-engine + path runs `JSON.parse(JSON.stringify(result))` so renderers see the same + shape on both paths (kills Date/bigint/Buffer drift class). +- `src/core/mcp-client.ts` — `callRemoteTool(config, toolName, args, opts)`. + Hardened in v0.31.1 (CDX-4): all transport errors normalized to + `RemoteMcpError` via the `toRemoteMcpError` funnel. New `CallRemoteToolOptions + {timeoutMs, signal}`; `buildAbortController` composes external signal with + timeout. New `RemoteMcpErrorReason` stable union, `RemoteMcpErrorDetail.kind` + ('timeout' | 'aborted' | 'unreachable') sub-tag, `RemoteMcpErrorDetail.code` + field carrying server-supplied error codes (e.g. `missing_scope`). + `extractToolErrorCode` parses JSON envelopes first, falls back to substring + detection for legacy server messages. `unpackToolResult(res)` unchanged + (parses tool-call JSON content). `_clearMcpClientTokenCache()` test escape. +- `src/core/cli-options.ts` — `parseGlobalFlags` adds `--timeout=Ns` (accepts + `30s`, `2m`, `500ms`, plain ms). Default `null` = per-command default (30s + for most ops, 180s for `think`). `parseTimeout(s)` exported helper. +- `src/core/doctor-remote.ts` — `gbrain remote doctor` adds the + `oauth_client_scopes_probe` check (CDX-5). Probes the read tier via + `get_brain_identity` and admin tier via `get_health`; reports per-tier + status with pinpoint remediation when admin is missing. `buildScopeCheck` + + `ScopeProbeResult` exported for test access. Skippable via + `GBRAIN_DOCTOR_SKIP_SCOPE_PROBE=1` for fixtures that mock /mcp at JSON-RPC + initialize level only (MCP SDK Client hangs on shape mismatch). +- `src/core/operations.ts` — `get_brain_identity` op (read scope, no params, + banner-only): cheap counter packet `{version, engine, page_count, + chunk_count, last_sync_iso}` for the thin-client identity banner. Reuses + `engine.getStats()`; banner's 60s client-side TTL bounds frequency to + ≤1/60s per CLI process (well below the Fly.io health-check cadence that + motivated the original `getStats` cost warning). +- `src/commands/{salience,anomalies,graph-query,think}.ts` — Per-command + thin-client routing branches. These commands bypass the operation-layer + dispatch in cli.ts (call `engine.foo()` directly), so each gets its own + `if (isThinClient(cfg)) { callRemoteTool(...) }` branch that maps CLI flags + to op params. `think` is a special case: the server's `think` op + intentionally disables `--save`/`--take` for remote callers + (operations.ts:1103-1135 trust-boundary gate); thin-client `think` warns + loudly when those flags are set. + ## Commands Run `gbrain --help` or `gbrain --tools-json` for full command reference. diff --git a/package.json b/package.json index 39662b4cd..4341ba685 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "gbrain", - "version": "0.31.0", + "version": "0.31.1", "description": "Postgres-native personal knowledge brain with hybrid RAG search", "type": "module", "main": "src/core/index.ts", diff --git a/src/cli.ts b/src/cli.ts index 597921d0d..9e4d802c1 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -12,6 +12,8 @@ import { operations, OperationError } from './core/operations.ts'; import type { Operation, OperationContext } from './core/operations.ts'; import { serializeMarkdown } from './core/markdown.ts'; import { parseGlobalFlags, setCliOptions, getCliOptions } from './core/cli-options.ts'; +import type { CliOptions } from './core/cli-options.ts'; +import { callRemoteTool, RemoteMcpError, unpackToolResult } from './core/mcp-client.ts'; import { VERSION } from './version.ts'; // Build CLI name -> operation lookup @@ -82,47 +84,72 @@ async function main() { process.exit(1); } - const engine = await connectEngine(); - try { - const params = parseOpArgs(op, subArgs); + // v0.31.1 (Issue #734, CDX-1): parse CLI args BEFORE engine connect so + // the routing seam below can decide local-vs-remote without paying a + // PGLite migration replay on thin-client installs. The arg parser, image + // transform, and required-param check are all engine-free; refactoring + // them out of the engine try/catch is safe and unlocks routing. + const params = parseOpArgs(op, subArgs); - // v0.27.1 (`gbrain query --image `): swap the `image` param from - // a filesystem path into base64 bytes + mime. The op accepts base64; the - // CLI accepts a path. Helper is exported so tests can exercise the - // transform without spawning a subprocess. - if (op.name === 'query' && typeof params.image === 'string' && params.image.length > 0) { - try { - const { path, base64, mime } = resolveQueryImage( - params.image as string, - (params.image_mime as string) || undefined, - ); - params.image = base64; - params.image_mime = mime; - void path; - } catch (err) { - console.error(err instanceof Error ? err.message : String(err)); - process.exit(1); - } + // v0.27.1 (`gbrain query --image `): swap the `image` param from + // a filesystem path into base64 bytes + mime. The op accepts base64; the + // CLI accepts a path. Helper is exported so tests can exercise the + // transform without spawning a subprocess. + if (op.name === 'query' && typeof params.image === 'string' && params.image.length > 0) { + try { + const { path, base64, mime } = resolveQueryImage( + params.image as string, + (params.image_mime as string) || undefined, + ); + params.image = base64; + params.image_mime = mime; + void path; + } catch (err) { + console.error(err instanceof Error ? err.message : String(err)); + process.exit(1); } + } - // Validate required params before calling handler. v0.27.1: the - // `query` op's positional `query` is required only when --image is - // NOT supplied. The runtime altRequired check below overrides the - // generic required-flag check for that op. - const queryHasAlt = op.name === 'query' && typeof params.image === 'string' && params.image.length > 0; - for (const [key, def] of Object.entries(op.params)) { - if (def.required && params[key] === undefined) { - if (queryHasAlt && key === 'query') continue; - const cliName = op.cliHints?.name || op.name; - const positional = op.cliHints?.positional || []; - const usage = positional.map(p => `<${p}>`).join(' '); - console.error(`Usage: gbrain ${cliName} ${usage}`); - process.exit(1); - } + // Validate required params before calling handler. v0.27.1: the + // `query` op's positional `query` is required only when --image is + // NOT supplied. The runtime altRequired check below overrides the + // generic required-flag check for that op. + const queryHasAlt = op.name === 'query' && typeof params.image === 'string' && params.image.length > 0; + for (const [key, def] of Object.entries(op.params)) { + if (def.required && params[key] === undefined) { + if (queryHasAlt && key === 'query') continue; + const cliName = op.cliHints?.name || op.name; + const positional = op.cliHints?.positional || []; + const usage = positional.map(p => `<${p}>`).join(' '); + console.error(`Usage: gbrain ${cliName} ${usage}`); + process.exit(1); } + } + + // v0.31.1 (Issue #734, CDX-1 routing seam): on thin-client installs, + // route every non-localOnly op through callRemoteTool instead of opening + // the empty local PGLite. localOnly ops can't run on a thin client at all + // (no local engine, server intentionally hides them) — refuse with hint. + // Fix for the silent-empty-results bug class that motivated this whole release. + const cfgPre = loadConfig(); + if (isThinClient(cfgPre)) { + if (op.localOnly) { + refuseThinClient(command, cfgPre!.remote_mcp!.mcp_url); + } + await runThinClientRouted(op, params, cfgPre!, cliOpts); + return; + } + // Local engine path (unchanged behavior for local installs). + const engine = await connectEngine(); + try { const ctx = makeContext(engine, params); - const result = await op.handler(ctx, params); + const rawResult = await op.handler(ctx, params); + // ENG-2 (renderer parity by data shape): JSON-round-trip the local-engine + // path's return value so renderers see the same shape they'd see on the + // routed path. Date → ISO string; bigint → string (postgres.js shape); + // Buffer → object. Microsecond-cost; eliminates a whole drift bug class. + const result = JSON.parse(JSON.stringify(rawResult)); const output = formatResult(op.name, result); if (output) process.stdout.write(output); } catch (e: unknown) { @@ -138,6 +165,224 @@ async function main() { } } +/** + * v0.31.1 (Issue #734, CDX-1): route a shared op through the remote MCP + * server instead of running it locally. Called from main() when + * `isThinClient(cfg) && !op.localOnly`. + * + * Timeout policy (ENG-4): user override via --timeout=Ns wins; otherwise + * 180s for `think` (LLM calls), 30s for everything else. + * + * Error policy (CDX-4): callRemoteTool's hardening pass guarantees every + * thrown value reaches us as a RemoteMcpError. The switch below is + * exhaustively typed (TS `never` check); adding a new reason variant fails + * compilation until this dispatcher knows what to render. + * + * Renderer policy: the MCP tool result is unpacked via unpackToolResult + * (which JSON.parses the text content) and handed to the SAME formatResult + * the local-engine path uses. Renderer parity is enforced by data shape, + * not by per-command audit. + */ +async function runThinClientRouted( + op: Operation, + params: Record, + cfg: GBrainConfig, + cliOpts: CliOptions, +): Promise { + // ENG-4: per-op timeout default; user override wins. + const defaultTimeoutMs = op.name === 'think' ? 180_000 : 30_000; + const timeoutMs = cliOpts.timeoutMs ?? defaultTimeoutMs; + + // SIGINT support: aborts in-flight HTTP cleanly (exit 130 is the standard + // SIGINT exit code; our error switch maps `network/aborted` to that). + const sigintController = new AbortController(); + const onSigint = () => { + sigintController.abort(new Error('SIGINT')); + }; + process.on('SIGINT', onSigint); + + // v0.31.1 (Issue #734, cherry-pick B): print identity banner to stderr + // BEFORE the routed call. Banner failure suppresses the banner only — + // never the underlying command. Suppression honors --quiet, non-TTY, + // and GBRAIN_NO_BANNER=1. + await printIdentityBannerBestEffort(cfg, cliOpts, sigintController.signal); + + try { + const raw = await callRemoteTool(cfg, op.name, params, { + timeoutMs, + signal: sigintController.signal, + }); + const result = unpackToolResult(raw); + const output = formatResult(op.name, result); + if (output) process.stdout.write(output); + } catch (e: unknown) { + if (e instanceof RemoteMcpError) { + const url = cfg.remote_mcp!.mcp_url; + switch (e.reason) { + case 'config': + console.error(e.message); + break; + case 'discovery': + console.error(`OAuth discovery failed at ${cfg.remote_mcp!.issuer_url}.`); + console.error('Run `gbrain remote doctor` for details.'); + break; + case 'auth': + console.error('OAuth auth failed.'); + console.error('On the host, re-register your client:'); + console.error(' gbrain auth register-client --grant-types client_credentials --scopes read,write,admin'); + break; + case 'auth_after_refresh': + console.error('OAuth auth failed after token refresh. Credentials may have been revoked.'); + console.error('Run `gbrain remote doctor` to confirm.'); + break; + case 'network': + if (e.detail?.kind === 'timeout') { + const hint = cliOpts.timeoutMs ? '' : ` (default ${defaultTimeoutMs}ms; pass --timeout=Ns to override)`; + console.error(`Request to ${url} timed out${hint}.`); + } else if (e.detail?.kind === 'aborted') { + console.error('Request aborted.'); + process.off('SIGINT', onSigint); + process.exit(130); + } else { + console.error(`Cannot reach ${url}. Run \`gbrain remote doctor\` for details.`); + } + break; + case 'tool_error': + if (e.detail?.code === 'missing_scope') { + console.error('Missing OAuth scope on this client.'); + console.error('On the host, re-register the client with broader scopes:'); + console.error(' gbrain auth register-client --grant-types client_credentials --scopes read,write,admin'); + } else { + console.error(e.message); + console.error('Run `gbrain remote doctor` if this persists.'); + } + break; + case 'parse': + console.error('Server response was malformed. Run `gbrain remote doctor`.'); + break; + default: { + // Exhaustive switch sentinel (TS `never` — fails to build if a + // new RemoteMcpErrorReason variant is added without a case). + const _exhaustive: never = e.reason; + void _exhaustive; + console.error(`Unhandled remote error: ${e.message}`); + } + } + process.off('SIGINT', onSigint); + process.exit(1); + } + // Defense in depth: callRemoteTool's contract is that everything is + // RemoteMcpError. If a plain Error escapes, render it generically and + // exit 1 — but this should never happen post-CDX-4. + console.error(e instanceof Error ? e.message : String(e)); + process.off('SIGINT', onSigint); + process.exit(1); + } finally { + process.off('SIGINT', onSigint); + } +} + +// ============================================================================ +// v0.31.1 (Issue #734, cherry-pick B): thin-client identity banner. +// +// Prints "[thin-client → · brain: 102k pages, 265k chunks · vX.Y.Z]" +// to stderr before each routed command, so users (and agents) know they're +// talking to a real remote brain — not the empty local PGLite that motivated +// this whole release. +// +// Cache: 60s TTL, in-memory Map keyed by mcp_url. Cross-process file cache +// is deferred (marginal benefit; one mint per CLI process is fine). +// Suppression: --quiet, non-TTY, GBRAIN_NO_BANNER=1. +// Failure mode: any error in fetching identity → suppress banner; underlying +// command runs normally. Banner is observability, not load-bearing. +// ============================================================================ + +interface BrainIdentity { + version: string; + engine: 'postgres' | 'pglite'; + page_count: number; + chunk_count: number; + last_sync_iso: string | null; +} + +interface CachedIdentity { + identity: BrainIdentity; + cached_at_ms: number; +} + +const IDENTITY_TTL_MS = 60_000; +const identityCache = new Map(); + +/** Test-only escape hatch — clears the in-memory cache between test runs. */ +export function _clearIdentityCacheForTest(): void { + identityCache.clear(); +} + +function bannerSuppressed(cliOpts: CliOptions): boolean { + if (cliOpts.quiet) return true; + if (process.env.GBRAIN_NO_BANNER === '1') return true; + // Non-TTY default is suppressed (clean pipes); explicit env-flag overrides. + if (!process.stderr.isTTY && process.env.GBRAIN_BANNER !== '1') return true; + return false; +} + +function formatPageCount(n: number): string { + if (n >= 1000) { + const k = (n / 1000).toFixed(n >= 100_000 ? 0 : 1); + return `${k}k`; + } + return String(n); +} + +function formatBanner(mcpUrl: string, id: BrainIdentity): string { + const host = mcpUrl.replace(/^https?:\/\//, '').split('/')[0]; + const counts = `brain: ${formatPageCount(id.page_count)} pages, ${formatPageCount(id.chunk_count)} chunks`; + return `[thin-client → ${host} · ${counts} · v${id.version}]`; +} + +async function fetchIdentity( + cfg: GBrainConfig, + signal: AbortSignal, +): Promise { + // 2s timeout for the banner fetch — must not delay the underlying command. + const raw = await callRemoteTool(cfg, 'get_brain_identity', {}, { + timeoutMs: 2000, + signal, + }); + const id = unpackToolResult(raw); + return id; +} + +async function printIdentityBannerBestEffort( + cfg: GBrainConfig, + cliOpts: CliOptions, + signal: AbortSignal, +): Promise { + if (bannerSuppressed(cliOpts)) return; + const mcpUrl = cfg.remote_mcp?.mcp_url; + if (!mcpUrl) return; + + // Cache lookup keyed by mcp_url so switching hosts via `gbrain init` + // invalidates cleanly even within a long-lived process. + const cached = identityCache.get(mcpUrl); + if (cached && Date.now() - cached.cached_at_ms < IDENTITY_TTL_MS) { + process.stderr.write(formatBanner(mcpUrl, cached.identity) + '\n'); + return; + } + + // Cache miss — fetch. Failure is non-fatal: banner is observability, + // never load-bearing for the underlying command. + try { + const id = await fetchIdentity(cfg, signal); + identityCache.set(mcpUrl, { identity: id, cached_at_ms: Date.now() }); + process.stderr.write(formatBanner(mcpUrl, id) + '\n'); + } catch { + // Swallow. Banner suppressed; main command continues. The CDX-4 + // hardened callRemoteTool will surface the same error class on the + // actual command call if the host is genuinely unreachable. + } +} + /** * v0.27.1: shared transform for `gbrain query --image ` (and any future * CLI surface that takes an image path). Reads the file, base64-encodes, @@ -337,21 +582,73 @@ function formatResult(opName: string, result: unknown): string { const THIN_CLIENT_REFUSED_COMMANDS = new Set([ 'sync', 'embed', 'extract', 'migrate', 'apply-migrations', 'repair-jsonb', 'orphans', 'integrity', 'serve', + // v0.31.1 (CDX-2 op coverage matrix): more local-only commands + 'dream', 'transcripts', 'storage', + // v0.31.1 CDX-2 audit: takes/sources have multiple subcommands; some + // (takes_list/takes_search, sources_list/sources_status) have MCP + // equivalents and others are file-system bound (takes mutate commands + // edit local .md files). v0.31.1 refuses both at the top level with a + // hint pointing at the routable MCP tools; per-subcommand splits are + // a v0.31.x follow-up TODO. + 'takes', 'sources', ]); +/** + * v0.31.1 (Issue #734, CDX-5 + cherry-pick A): pinpoint refusal hints for + * local-only commands when running on a thin-client install. Each hint names + * the closest path (remote MCP call, host-side workflow) so users aren't + * stuck guessing what to do next. + * + * Source-of-truth lives here so adding a new local-only command means + * adding both the THIN_CLIENT_REFUSED_COMMANDS member AND the hint in one + * place during code review. + */ +const THIN_CLIENT_REFUSE_HINTS: Record = { + sync: 'sync runs on the host. Trigger a remote cycle with `gbrain remote ping` (queues an autopilot-cycle job).', + embed: 'embed runs on the host as part of the autopilot cycle. `gbrain remote ping` triggers a full cycle including embed.', + extract: 'extract runs on the host. Use `gbrain remote ping` to trigger a cycle including extract.', + migrate: "migrate runs on the host's local engine. Run on the host machine.", + 'apply-migrations': 'schema migrations run on the host. SSH and run there.', + 'repair-jsonb': 'repair-jsonb operates on the local DB only.', + integrity: 'integrity scans local files. Run on the host machine.', + serve: 'serve starts a server. Run on the host, not the thin client.', + dream: 'dream runs the autopilot cycle on the host. `gbrain remote ping` queues one. (Native `gbrain dream` thin-client routing planned for v0.31.2.)', + orphans: "orphans needs the host's brain. Run on the host or use the `find_orphans` MCP tool from your agent.", + transcripts: 'transcripts is server-private (raw chat exports stay on the host). Read transcripts on the host machine.', + storage: 'storage operates on the local repo on disk. Run on the host.', + takes: 'takes mutate subcommands edit local .md files; routing the read subcommands lands in v0.31.x. For now: use `takes_list` and `takes_search` MCP tools from your agent, or run on the host.', + sources: 'sources commands manage local DB + config rows. Per-subcommand thin-client routing lands in v0.31.x. For now: use `sources_list` / `sources_status` MCP tools, or run on the host.', +}; + +/** + * v0.31.1: emit a pinpoint refusal hint for a thin-client-incompatible + * command and exit 1. Falls back to the canonical generic message when no + * specific hint is registered (defensive — every member of + * THIN_CLIENT_REFUSED_COMMANDS should have a hint). + */ +function refuseThinClient(command: string, mcpUrl: string): never { + const hint = THIN_CLIENT_REFUSE_HINTS[command]; + if (hint) { + console.error(`\`gbrain ${command}\` is not routable. ${hint}`); + console.error(`(thin-client of ${mcpUrl})`); + } else { + console.error( + `\`gbrain ${command}\` requires a local engine. This install is a thin client of ${mcpUrl}.\n` + + `Run \`${command}\` on the remote host, or use the corresponding MCP tool from your agent.`, + ); + } + process.exit(1); +} + async function handleCliOnly(command: string, args: string[]) { - // Thin-client guard: refuse DB-bound commands cleanly with a single - // canonical message instead of letting them fail later inside connectEngine - // or mid-handler. See `THIN_CLIENT_REFUSED_COMMANDS` above. + // Thin-client guard: refuse DB-bound commands cleanly with a pinpoint + // hint instead of letting them fail later inside connectEngine or + // mid-handler. v0.31.1 routes through `refuseThinClient` so every + // refusal carries an actionable next-step hint (CDX-5 cherry-pick A). if (THIN_CLIENT_REFUSED_COMMANDS.has(command)) { const cfg = loadConfig(); if (isThinClient(cfg)) { - const url = cfg!.remote_mcp!.mcp_url; - console.error( - `\`gbrain ${command}\` requires a local engine. This install is a thin client of ${url}.\n` + - `Run \`${command}\` on the remote host, or use the corresponding MCP tool from your agent.`, - ); - process.exit(1); + refuseThinClient(command, cfg!.remote_mcp!.mcp_url); } } diff --git a/src/commands/anomalies.ts b/src/commands/anomalies.ts index 0051ce84f..17c4cd5bf 100644 --- a/src/commands/anomalies.ts +++ b/src/commands/anomalies.ts @@ -16,6 +16,8 @@ */ import type { BrainEngine } from '../core/engine.ts'; +import { loadConfig, isThinClient } from '../core/config.ts'; +import { callRemoteTool, unpackToolResult } from '../core/mcp-client.ts'; interface RunOpts { since?: string; @@ -67,11 +69,23 @@ export async function runAnomalies(engine: BrainEngine, args: string[]): Promise console.log(HELP); return; } - const rows = await engine.findAnomalies({ - since: parsed.since, - lookback_days: parsed.lookbackDays, - sigma: parsed.sigma, - }); + // v0.31.1 (Issue #734): on thin-client installs, route via MCP. + let rows; + const cfg = loadConfig(); + if (isThinClient(cfg)) { + const raw = await callRemoteTool(cfg!, 'find_anomalies', { + since: parsed.since, + lookback_days: parsed.lookbackDays, + sigma: parsed.sigma, + }, { timeoutMs: 30_000 }); + rows = unpackToolResult>>(raw); + } else { + rows = await engine.findAnomalies({ + since: parsed.since, + lookback_days: parsed.lookbackDays, + sigma: parsed.sigma, + }); + } if (parsed.json) { console.log(JSON.stringify(rows, null, 2)); return; diff --git a/src/commands/graph-query.ts b/src/commands/graph-query.ts index 5b4197dde..1c5eaaf2c 100644 --- a/src/commands/graph-query.ts +++ b/src/commands/graph-query.ts @@ -16,6 +16,8 @@ import type { BrainEngine } from '../core/engine.ts'; import type { GraphPath } from '../core/types.ts'; +import { loadConfig, isThinClient } from '../core/config.ts'; +import { callRemoteTool, unpackToolResult } from '../core/mcp-client.ts'; interface Args { slug?: string; @@ -72,11 +74,26 @@ export async function runGraphQuery(engine: BrainEngine, argv: string[]) { return; } - const paths = await engine.traversePaths(args.slug, { - depth: args.depth, - linkType: args.linkType, - direction: args.direction, - }); + // v0.31.1 (Issue #734): on thin-client installs, route via MCP. The + // traverse_graph op returns GraphPath[] when link_type or direction is + // set (which the CLI always does); unpackToolResult parses the JSON. + let paths: GraphPath[]; + const cfg = loadConfig(); + if (isThinClient(cfg)) { + const raw = await callRemoteTool(cfg!, 'traverse_graph', { + slug: args.slug, + depth: args.depth, + link_type: args.linkType, + direction: args.direction, + }, { timeoutMs: 30_000 }); + paths = unpackToolResult(raw); + } else { + paths = await engine.traversePaths(args.slug, { + depth: args.depth, + linkType: args.linkType, + direction: args.direction, + }); + } if (paths.length === 0) { console.log(`No edges found from ${args.slug}${args.linkType ? ` (--type ${args.linkType})` : ''}.`); diff --git a/src/commands/salience.ts b/src/commands/salience.ts index c0dd53e6b..ab354ffe2 100644 --- a/src/commands/salience.ts +++ b/src/commands/salience.ts @@ -14,6 +14,8 @@ */ import type { BrainEngine } from '../core/engine.ts'; +import { loadConfig, isThinClient } from '../core/config.ts'; +import { callRemoteTool, unpackToolResult } from '../core/mcp-client.ts'; interface RunOpts { days?: number; @@ -67,11 +69,27 @@ export async function runSalience(engine: BrainEngine, args: string[]): Promise< console.log(HELP); return; } - const rows = await engine.getRecentSalience({ - days: parsed.days, - limit: parsed.limit, - slugPrefix: parsed.slugPrefix, - }); + + // v0.31.1 (Issue #734): on thin-client installs, route the engine call + // through the remote `get_recent_salience` MCP op. Output format is + // identical because both paths return the same shape (the op handler IS + // engine.getRecentSalience). + let rows; + const cfg = loadConfig(); + if (isThinClient(cfg)) { + const raw = await callRemoteTool(cfg!, 'get_recent_salience', { + days: parsed.days, + limit: parsed.limit, + slugPrefix: parsed.slugPrefix, + }, { timeoutMs: 30_000 }); + rows = unpackToolResult>>(raw); + } else { + rows = await engine.getRecentSalience({ + days: parsed.days, + limit: parsed.limit, + slugPrefix: parsed.slugPrefix, + }); + } if (parsed.json) { console.log(JSON.stringify(rows, null, 2)); return; diff --git a/src/commands/think.ts b/src/commands/think.ts index 399b2f7cc..e461febc8 100644 --- a/src/commands/think.ts +++ b/src/commands/think.ts @@ -7,6 +7,8 @@ */ import type { BrainEngine } from '../core/engine.ts'; import { runThink, persistSynthesis } from '../core/think/index.ts'; +import { loadConfig, isThinClient } from '../core/config.ts'; +import { callRemoteTool, unpackToolResult } from '../core/mcp-client.ts'; function flagValue(args: string[], name: string): string | undefined { const i = args.indexOf(name); @@ -72,19 +74,41 @@ the gather phase still runs and prints what would have been the input. process.exit(1); } - const result = await runThink(engine, { - question, anchor, rounds, save, take, model, since, until, - // Local CLI: no MCP allow-list filter — operator owns the brain. - }); - - // Persist if --save (the runThink path doesn't auto-persist; CLI does it explicitly) + // v0.31.1 (Issue #734): on thin-client installs, route through MCP. The + // server's `think` handler intentionally ignores --save and --take for + // remote callers (operations.ts:1103-1135 trust-boundary gate). Document + // here loudly so users get a clear warning instead of silent loss. + let result: any; let savedSlug: string | undefined; let evidenceInserted = 0; - if (save) { - const persisted = await persistSynthesis(engine, result); - savedSlug = persisted.slug; - evidenceInserted = persisted.evidenceInserted; - for (const w of persisted.warnings) result.warnings.push(w); + const cfg = loadConfig(); + if (isThinClient(cfg)) { + if (save || take) { + console.error( + '[thin-client] --save and --take are server-gated for remote callers ' + + '(trust-boundary policy). Run on the host or use the MCP `think` tool ' + + 'with the `viaSubagent` context if you need persistence.', + ); + } + const raw = await callRemoteTool(cfg!, 'think', { + question, anchor, rounds, model, since, until, + // save/take intentionally NOT forwarded — server would ignore them; + // we surface the intent above so users know what they lose. + }, { timeoutMs: 180_000 }); + result = unpackToolResult(raw); + } else { + result = await runThink(engine, { + question, anchor, rounds, save, take, model, since, until, + // Local CLI: no MCP allow-list filter — operator owns the brain. + }); + + // Persist if --save (the runThink path doesn't auto-persist; CLI does it explicitly) + if (save) { + const persisted = await persistSynthesis(engine, result); + savedSlug = persisted.slug; + evidenceInserted = persisted.evidenceInserted; + for (const w of persisted.warnings) result.warnings.push(w); + } } if (json) { diff --git a/src/core/cli-options.ts b/src/core/cli-options.ts index 5dc81b96a..2ccc90a61 100644 --- a/src/core/cli-options.ts +++ b/src/core/cli-options.ts @@ -15,12 +15,20 @@ export interface CliOptions { quiet: boolean; progressJson: boolean; progressInterval: number; // ms + /** + * v0.31.1 (Issue #734, ENG-4): user-supplied per-call timeout for thin-client + * routed MCP calls. `null` means "use the per-command default" (30s for most + * ops, 180s for `think`). When set, applies to every routed call in the + * current invocation. + */ + timeoutMs: number | null; } export const DEFAULT_CLI_OPTIONS: CliOptions = { quiet: false, progressJson: false, progressInterval: 1000, + timeoutMs: null, }; /** @@ -71,12 +79,52 @@ export function parseGlobalFlags(argv: string[]): { cliOpts: CliOptions; rest: s rest.push(a); continue; } + // v0.31.1: --timeout=Ns or --timeout Ns. Accepts plain ms, "30s", "2m". + if (a === '--timeout' && i + 1 < argv.length) { + const next = argv[i + 1]; + const parsed = parseTimeout(next); + if (parsed !== null) { + cliOpts.timeoutMs = parsed; + i++; + continue; + } + rest.push(a); + continue; + } + if (a.startsWith('--timeout=')) { + const val = a.slice('--timeout='.length); + const parsed = parseTimeout(val); + if (parsed !== null) { + cliOpts.timeoutMs = parsed; + continue; + } + rest.push(a); + continue; + } rest.push(a); } return { cliOpts, rest }; } +/** + * v0.31.1: parse a timeout value. Accepts: + * "30000" / "30000ms" → 30000 + * "30s" → 30000 + * "2m" → 120000 + * "1.5s" → 1500 + * Returns null on parse failure (caller decides whether to error or fall through). + */ +export function parseTimeout(s: string): number | null { + const m = /^([0-9]+(?:\.[0-9]+)?)(ms|s|m)?$/.exec(s.trim()); + if (!m) return null; + const n = Number(m[1]); + if (!Number.isFinite(n) || n <= 0) return null; + const unit = m[2] ?? 'ms'; + const ms = unit === 'ms' ? n : unit === 's' ? n * 1000 : n * 60_000; + return Math.floor(ms); +} + function parseInterval(s: string): number | null { const n = Number(s); if (!Number.isFinite(n) || n < 0) return null; diff --git a/src/core/doctor-remote.ts b/src/core/doctor-remote.ts index 8bb377661..602b97517 100644 --- a/src/core/doctor-remote.ts +++ b/src/core/doctor-remote.ts @@ -14,6 +14,7 @@ import type { GBrainConfig } from './config.ts'; import { discoverOAuth, mintClientCredentialsToken, smokeTestMcp } from './remote-mcp-probe.ts'; +import { callRemoteTool, RemoteMcpError } from './mcp-client.ts'; export interface RemoteCheck { name: string; @@ -51,11 +52,29 @@ export async function runRemoteDoctor(config: GBrainConfig, args: string[]): Pro if (report.status === 'fail') process.exit(1); } +/** + * v0.31.1: opts for collectRemoteDoctorReport. + * + * `skipScopeProbe` defaults to false. Set to true in test fixtures that + * mock /mcp at JSON-RPC initialize level only — the MCP SDK Client used + * by the scope probe hangs on shape mismatch and doesn't always honor + * AbortSignal. Production callers always run the probe. + * + * Also honors GBRAIN_DOCTOR_SKIP_SCOPE_PROBE=1 for ops bypass; explicit + * opts.skipScopeProbe wins. + */ +export interface CollectRemoteDoctorOpts { + skipScopeProbe?: boolean; +} + /** * Pure data collector — separated from the print/exit logic so tests can * assert the report shape without intercepting stdout. */ -export async function collectRemoteDoctorReport(config: GBrainConfig): Promise { +export async function collectRemoteDoctorReport( + config: GBrainConfig, + opts: CollectRemoteDoctorOpts = {}, +): Promise { const remote = config.remote_mcp; const checks: RemoteCheck[] = []; @@ -180,9 +199,147 @@ export async function collectRemoteDoctorReport(config: GBrainConfig): Promise { + const result: ScopeProbeResult = { read_ok: false, admin_ok: false }; + + // Read tier: get_brain_identity is the cheapest read op (just returns + // counters; no DB scan beyond the existing getStats). + try { + await callRemoteTool(config, 'get_brain_identity', {}, { timeoutMs: 1500 }); + result.read_ok = true; + } catch (e) { + if (e instanceof RemoteMcpError) { + result.read_error = e.detail?.code === 'missing_scope' ? 'missing_scope' : e.reason; + } else { + result.read_error = e instanceof Error ? e.message : String(e); + } + } + + // Admin tier: get_health is read-only (engine.getHealth is a SELECT) but + // requires admin scope per operations.ts:1370. + try { + await callRemoteTool(config, 'get_health', {}, { timeoutMs: 1500 }); + result.admin_ok = true; + } catch (e) { + if (e instanceof RemoteMcpError) { + result.admin_error = e.detail?.code === 'missing_scope' ? 'missing_scope' : e.reason; + } else { + result.admin_error = e instanceof Error ? e.message : String(e); + } + } + + return result; +} + +/** v0.31.1: exported for test access. */ +export function buildScopeCheck(grantedScope: string, probe: ScopeProbeResult): RemoteCheck { + // Status semantics — informational by default, escalates only on signals + // we can KNOW indicate a scope problem (i.e. tool_error code='missing_scope'). + // Other probe failures (parse/network/timeout) might be transient or fixture + // artifacts; report as 'ok' with `inconclusive` detail so doctor's overall + // status doesn't flap on probe noise. + // + // - read.missing_scope → 'fail' (broken setup; nothing works) + // - admin.missing_scope → 'warn' (the load-bearing case for v0.29.2 thin + // clients that registered with read+write only; pinpoint hint follows) + // - both succeed → 'ok' + // - other probe errors → 'ok' with inconclusive=true + const readMissing = !probe.read_ok && probe.read_error === 'missing_scope'; + const adminMissing = !probe.admin_ok && probe.admin_error === 'missing_scope'; + + if (readMissing) { + return { + name: 'oauth_client_scopes_probe', + status: 'fail', + message: 'OAuth client lacks read scope. Re-register on the host with at least `--scopes read`.', + detail: { + granted: grantedScope || null, + read_ok: false, + admin_ok: probe.admin_ok, + }, + }; + } + if (adminMissing) { + return { + name: 'oauth_client_scopes_probe', + status: 'warn', + message: + 'admin scope MISSING (read works). On the host, re-register: ' + + '`gbrain auth register-client --grant-types client_credentials --scopes read,write,admin`', + detail: { + granted: grantedScope || null, + read_ok: true, + admin_ok: false, + admin_error: probe.admin_error ?? null, + }, + }; + } + if (probe.read_ok && probe.admin_ok) { + return { + name: 'oauth_client_scopes_probe', + status: 'ok', + message: `read + admin scopes verified (write tier inferred from granted="${grantedScope || 'unspecified'}")`, + detail: { + granted: grantedScope || null, + read_ok: true, + admin_ok: true, + }, + }; + } + // Inconclusive: probe failed for non-scope reasons. Report as 'ok' so + // unrelated probe transients don't escalate doctor's overall status, + // but include the probe results for debugging. + return { + name: 'oauth_client_scopes_probe', + status: 'ok', + message: `scope probe inconclusive (granted="${grantedScope || 'unspecified'}"); commands will surface scope errors at call time if any`, + detail: { + granted: grantedScope || null, + read_ok: probe.read_ok, + admin_ok: probe.admin_ok, + read_error: probe.read_error ?? null, + admin_error: probe.admin_error ?? null, + inconclusive: true, + }, + }; +} + function finalize( remote: NonNullable, checks: RemoteCheck[], diff --git a/src/core/mcp-client.ts b/src/core/mcp-client.ts index 08078ff3e..3853cc510 100644 --- a/src/core/mcp-client.ts +++ b/src/core/mcp-client.ts @@ -41,17 +41,107 @@ export function _clearMcpClientTokenCache(): void { tokenCache.clear(); } +/** + * Stable union of failure reasons. The CLI dispatcher (cli.ts thin-client + * routing branch, v0.31.1) uses an exhaustive TS switch over this union to + * produce canned, actionable user messages — adding a new variant fails + * compilation until every dispatcher knows what to render. + * + * v0.31.1 additions: + * - `kind` sub-tag on `network` errors distinguishes 'unreachable' / + * 'timeout' / 'aborted' so callers can render the right hint. + * - `code` field on `tool_error` carries the MCP server's `error.code` (when + * present) so the dispatcher can map missing-scope etc. to pinpoint hints. + */ +export type RemoteMcpErrorReason = + | 'config' + | 'discovery' + | 'auth' + | 'auth_after_refresh' + | 'network' + | 'tool_error' + | 'parse'; + +export interface RemoteMcpErrorDetail { + status?: number; + mcp_url?: string; + /** v0.31.1: sub-tag for network errors (timeout vs aborted vs generic). */ + kind?: 'timeout' | 'aborted' | 'unreachable'; + /** v0.31.1: server-supplied error code on tool_error (e.g. 'missing_scope'). */ + code?: string; +} + export class RemoteMcpError extends Error { constructor( - public readonly reason: 'config' | 'discovery' | 'auth' | 'auth_after_refresh' | 'network' | 'tool_error' | 'parse', + public readonly reason: RemoteMcpErrorReason, message: string, - public readonly detail?: { status?: number; mcp_url?: string }, + public readonly detail?: RemoteMcpErrorDetail, ) { super(message); this.name = 'RemoteMcpError'; } } +/** + * v0.31.1: convert any thrown value into a RemoteMcpError. Used by the + * outermost catch in `callRemoteTool` so the dispatcher's exhaustive switch + * is sound — no plain `Error` (undici, AbortError, JSON parse) escapes. + * + * @internal Exported for test access (test/mcp-client-hardening.test.ts). + * Not part of the public API — production code should consume this only via + * the callRemoteTool funnel. + */ +export function toRemoteMcpError(e: unknown, mcpUrl: string): RemoteMcpError { + if (e instanceof RemoteMcpError) return e; + if (e instanceof Error) { + // AbortError fires for both --timeout and SIGINT; the caller distinguishes + // via the AbortSignal.reason it set, but the SDK swallows that. Fall back + // to message inspection for the timeout sub-kind. + const isAbort = e.name === 'AbortError' || /abort/i.test(e.message); + if (isAbort) { + return new RemoteMcpError( + 'network', + `Request to ${mcpUrl} aborted: ${e.message}`, + { mcp_url: mcpUrl, kind: 'aborted' }, + ); + } + // undici/fetch network errors (DNS, connection refused, TLS) end up here. + return new RemoteMcpError( + 'network', + `Network error talking to ${mcpUrl}: ${e.message}`, + { mcp_url: mcpUrl, kind: 'unreachable' }, + ); + } + return new RemoteMcpError( + 'network', + `Unknown error talking to ${mcpUrl}: ${String(e)}`, + { mcp_url: mcpUrl, kind: 'unreachable' }, + ); +} + +/** + * v0.31.1: parse a tool_error content envelope and extract a structured + * `code` (e.g. 'missing_scope') if the server provided one. Tries JSON-parsed + * payload first, then falls back to substring detection on the message. + * + * @internal Exported for test access (test/mcp-client-hardening.test.ts). + */ +export function extractToolErrorCode(message: string): string | undefined { + // Try to parse a JSON payload first — gbrain server-side tool errors + // sometimes come through as `{"error":{"code":"...","message":"..."}}`. + try { + const parsed = JSON.parse(message); + if (parsed && typeof parsed === 'object') { + const code = (parsed as any).error?.code ?? (parsed as any).code; + if (typeof code === 'string') return code; + } + } catch { /* not json; fall through */ } + if (/missing[_\s-]?scope|scope.+(insufficient|required)|forbidden|access.+denied/i.test(message)) { + return 'missing_scope'; + } + return undefined; +} + function requireRemoteMcp(config: GBrainConfig | null): NonNullable { if (!config?.remote_mcp) { throw new RemoteMcpError( @@ -117,13 +207,17 @@ async function getAccessToken(config: GBrainConfig, force = false): Promise { +async function buildClient(mcpUrl: string, accessToken: string, signal?: AbortSignal): Promise { const transport = new StreamableHTTPClientTransport(new URL(mcpUrl), { requestInit: { headers: { 'Authorization': `Bearer ${accessToken}`, }, + ...(signal ? { signal } : {}), }, }); const client = new Client( @@ -134,6 +228,47 @@ async function buildClient(mcpUrl: string, accessToken: string): Promise return client; } +/** + * v0.31.1: options for `callRemoteTool`. Both fields optional; when absent the + * call inherits SDK defaults (no client-side timeout, no abort). + */ +export interface CallRemoteToolOptions { + /** Hard wall-clock cap for the whole call (token mint + tool call). Aborts on expiry. */ + timeoutMs?: number; + /** External AbortSignal (e.g. SIGINT handler). Composed with the timeout. */ + signal?: AbortSignal; +} + +/** + * Compose an external signal with a timeout into a single AbortController. + * Returns the controller (so callers can pass `controller.signal` to + * downstream fetch) plus a `cleanup` to stop the timer + drop listeners. + */ +/** @internal Exported for test access (test/mcp-client-hardening.test.ts). */ +export function buildAbortController(opts: CallRemoteToolOptions): { signal: AbortSignal; cleanup: () => void } { + const controller = new AbortController(); + const cleanups: Array<() => void> = []; + + if (opts.timeoutMs !== undefined && opts.timeoutMs > 0) { + const timer = setTimeout(() => { + controller.abort(new Error(`timeout after ${opts.timeoutMs}ms`)); + }, opts.timeoutMs); + cleanups.push(() => clearTimeout(timer)); + } + + if (opts.signal) { + if (opts.signal.aborted) { + controller.abort(opts.signal.reason); + } else { + const onAbort = () => controller.abort(opts.signal!.reason); + opts.signal.addEventListener('abort', onAbort); + cleanups.push(() => opts.signal!.removeEventListener('abort', onAbort)); + } + } + + return { signal: controller.signal, cleanup: () => cleanups.forEach(fn => { try { fn(); } catch { /* best-effort */ } }) }; +} + /** * Call an MCP tool on the remote server. Handles auth refresh on 401 once. * Returns the parsed `result` payload from the tool response. @@ -149,68 +284,92 @@ export async function callRemoteTool( config: GBrainConfig, toolName: string, args: Record = {}, + opts: CallRemoteToolOptions = {}, ): Promise { const remote = requireRemoteMcp(config); - // Step 1: mint (or reuse cached) token. If THIS fails — bad credentials, - // unreachable issuer, etc. — surface immediately. Retry-on-401 is for - // the mid-session token-rotation case, NOT for initial-credentials-wrong. - const initialToken = await getAccessToken(config, false); + // v0.31.1 (CDX-4): wrap the WHOLE call in normalize-on-error so the + // exhaustive switch on RemoteMcpError.reason at the dispatcher is sound. + // No plain Error (undici, AbortError, JSON parse) escapes. + const { signal, cleanup } = buildAbortController(opts); + try { + // Step 1: mint (or reuse cached) token. If THIS fails — bad credentials, + // unreachable issuer, etc. — surface immediately. Retry-on-401 is for + // the mid-session token-rotation case, NOT for initial-credentials-wrong. + const initialToken = await getAccessToken(config, false); - // Step 2: try the tool call. On a 401-shaped failure here, drop the cache - // and retry ONCE with a freshly-minted token (handles host-side rotation - // mid-session). If the retry also fails auth, surface auth_after_refresh. - const tryCall = async (token: string): Promise => { - const client = await buildClient(remote.mcp_url, token); - try { - const res = await client.callTool({ name: toolName, arguments: args }); - if (res.isError) { - const message = Array.isArray(res.content) - ? res.content.map((c: unknown) => (c as { text?: string }).text ?? '').join('\n') - : 'unknown tool error'; - throw new RemoteMcpError('tool_error', `Remote tool ${toolName} failed: ${message}`, { mcp_url: remote.mcp_url }); + // Step 2: try the tool call. On a 401-shaped failure here, drop the cache + // and retry ONCE with a freshly-minted token (handles host-side rotation + // mid-session). If the retry also fails auth, surface auth_after_refresh. + const tryCall = async (token: string): Promise => { + const client = await buildClient(remote.mcp_url, token, signal); + try { + const res = await client.callTool({ name: toolName, arguments: args }); + if (res.isError) { + const message = Array.isArray(res.content) + ? res.content.map((c: unknown) => (c as { text?: string }).text ?? '').join('\n') + : 'unknown tool error'; + // v0.31.1: extract structured error code (e.g. 'missing_scope') so + // the dispatcher can produce a pinpoint hint instead of a generic + // "tool error" message. + const code = extractToolErrorCode(message); + throw new RemoteMcpError( + 'tool_error', + `Remote tool ${toolName} failed: ${message}`, + { mcp_url: remote.mcp_url, ...(code ? { code } : {}) }, + ); + } + return res; + } finally { + try { await client.close(); } catch { /* best-effort */ } } - return res; - } finally { - try { await client.close(); } catch { /* best-effort */ } - } - }; + }; - try { - return await tryCall(initialToken); - } catch (e) { - if (!(e instanceof Error)) throw e; - const looksLike401 = /401|unauthor|invalid.token/i.test(e.message); - if (!looksLike401) throw e; - // Drop cached token and retry once with a fresh mint. - tokenCache.delete(remote.mcp_url); - let freshToken: string; try { - freshToken = await getAccessToken(config, true); - } catch (mintErr) { - // If the fresh mint itself fails auth, surface auth_after_refresh — - // host-side credentials likely revoked. - if (mintErr instanceof RemoteMcpError && mintErr.reason === 'auth') { - throw new RemoteMcpError( - 'auth_after_refresh', - `Auth failed after token refresh. Verify oauth_client_id and secret are still valid; the host operator may need to re-run \`gbrain auth register-client\`.`, - { mcp_url: remote.mcp_url }, - ); + return await tryCall(initialToken); + } catch (e) { + // RemoteMcpError already-typed: bubble unless it's a tool_error that + // happens to look 401-shaped (e.g. SDK wrapping HTTP 401 in a tool + // error). For plain Error, do the 401 sniff. + const message = e instanceof Error ? e.message : String(e); + const looksLike401 = /401|unauthor|invalid.token/i.test(message); + if (!looksLike401) throw e; + // Drop cached token and retry once with a fresh mint. + tokenCache.delete(remote.mcp_url); + let freshToken: string; + try { + freshToken = await getAccessToken(config, true); + } catch (mintErr) { + if (mintErr instanceof RemoteMcpError && mintErr.reason === 'auth') { + throw new RemoteMcpError( + 'auth_after_refresh', + `Auth failed after token refresh. Verify oauth_client_id and secret are still valid; the host operator may need to re-run \`gbrain auth register-client\`.`, + { mcp_url: remote.mcp_url }, + ); + } + throw mintErr; } - throw mintErr; - } - try { - return await tryCall(freshToken); - } catch (e2) { - if (e2 instanceof Error && /401|unauthor|invalid.token/i.test(e2.message)) { - throw new RemoteMcpError( - 'auth_after_refresh', - `Auth failed after token refresh. Verify oauth_client_id and secret are still valid; the host operator may need to re-run \`gbrain auth register-client\`.`, - { mcp_url: remote.mcp_url }, - ); + try { + return await tryCall(freshToken); + } catch (e2) { + const m2 = e2 instanceof Error ? e2.message : String(e2); + if (/401|unauthor|invalid.token/i.test(m2)) { + throw new RemoteMcpError( + 'auth_after_refresh', + `Auth failed after token refresh. Verify oauth_client_id and secret are still valid; the host operator may need to re-run \`gbrain auth register-client\`.`, + { mcp_url: remote.mcp_url }, + ); + } + throw e2; } - throw e2; } + } catch (e) { + // CDX-4: this is the funnel. ANYTHING that escapes the inner block becomes + // a typed RemoteMcpError. The dispatcher's exhaustive switch can rely on + // this contract. + throw toRemoteMcpError(e, remote.mcp_url); + } finally { + cleanup(); } } diff --git a/src/core/operations.ts b/src/core/operations.ts index 23c807fde..ad53b14dc 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -17,6 +17,7 @@ import { captureEvalCandidate, isEvalCaptureEnabled, isEvalScrubEnabled } from ' import type { HybridSearchMeta } from './types.ts'; import { extractPageLinks, isAutoLinkEnabled, isAutoTimelineEnabled, parseTimelineEntries, makeResolver, type UnresolvedFrontmatterRef } from './link-extraction.ts'; import * as db from './db.ts'; +import { VERSION } from '../version.ts'; import { GET_RECENT_SALIENCE_DESCRIPTION, FIND_ANOMALIES_DESCRIPTION, @@ -1484,6 +1485,37 @@ const get_health: Operation = { cliHints: { name: 'health' }, }; +/** + * v0.31.1 (Issue #734): lightweight identity packet for the thin-client + * banner. Read-scope so any authenticated client can surface "thin-client → + * · brain: 102k pages, 265k chunks · v0.31.1" without needing admin. + * + * Reuses engine.getStats() for counters (banner cache TTL bounds frequency + * to ≤1/60s per CLI process; well below the Fly.io health-check cadence + * that motivated the `getStats` cost warning in CLAUDE.md). + * + * No CLI surface (no cliHints) — this op exists only for thin-client banner + * data. `last_sync_iso` deferred (no canonical source field today; would + * need autopilot cycle to write a config key — TODO in v0.31.x). + */ +const get_brain_identity: Operation = { + name: 'get_brain_identity', + description: 'Brain identity + counters for thin-client banner. Returns version, engine kind, and page/chunk counts. Read-scope.', + params: {}, + handler: async (ctx) => { + const stats = await ctx.engine.getStats(); + return { + version: VERSION, + engine: ctx.engine.kind, + page_count: stats.page_count, + chunk_count: stats.chunk_count, + last_sync_iso: null as string | null, + }; + }, + scope: 'read', + // intentionally no cliHints — banner-only op +}; + /** * Multi-topology v1 (Tier B): structured doctor report for remote callers. * @@ -2591,6 +2623,8 @@ export const operations: Operation[] = [ add_timeline_entry, get_timeline, // Admin get_stats, get_health, run_doctor, get_versions, revert_version, + // v0.31.1 (Issue #734): thin-client banner identity packet (read-scope, banner-only) + get_brain_identity, // Sync sync_brain, // Raw data diff --git a/test/cli-dispatch-thin-client.test.ts b/test/cli-dispatch-thin-client.test.ts index df396a557..08807c36d 100644 --- a/test/cli-dispatch-thin-client.test.ts +++ b/test/cli-dispatch-thin-client.test.ts @@ -107,15 +107,16 @@ describe('thin-client dispatch guard refuses DB-bound commands', () => { ]; for (const args of refusedCommands) { - test(`refuses \`gbrain ${args.join(' ')}\` with canonical error`, async () => { + test(`refuses \`gbrain ${args.join(' ')}\` with pinpoint hint`, async () => { seedThinClientConfig(); const r = await run(args); expect(r.exitCode).toBe(1); - // Canonical message must name the command + the remote URL. + // v0.31.1 (Issue #734): refusal carries an actionable hint via + // THIN_CLIENT_REFUSE_HINTS instead of a generic "run on the remote + // host" message. Hint format: "`gbrain ` is not routable. " expect(r.stderr).toContain(`gbrain ${args[0]}`); - expect(r.stderr).toContain('thin client'); - expect(r.stderr).toContain('https://brain-host.example/mcp'); - expect(r.stderr).toContain('Run `' + args[0] + '` on the remote host'); + expect(r.stderr).toContain('thin-client of https://brain-host.example/mcp'); + expect(r.stderr).toContain('not routable'); }); } }); diff --git a/test/cli-options.test.ts b/test/cli-options.test.ts index 29816ed81..016063b62 100644 --- a/test/cli-options.test.ts +++ b/test/cli-options.test.ts @@ -65,7 +65,7 @@ describe('parseGlobalFlags', () => { test('all global flags combined', () => { const r = parseGlobalFlags(['--quiet', '--progress-json', '--progress-interval=250', 'sync']); - expect(r.cliOpts).toEqual({ quiet: true, progressJson: true, progressInterval: 250 }); + expect(r.cliOpts).toEqual({ quiet: true, progressJson: true, progressInterval: 250, timeoutMs: null }); expect(r.rest).toEqual(['sync']); }); }); @@ -78,7 +78,7 @@ describe('getCliOptions / setCliOptions singleton', () => { test('setCliOptions applies + getCliOptions returns a copy', () => { _resetCliOptionsForTest(); - setCliOptions({ quiet: false, progressJson: true, progressInterval: 250 }); + setCliOptions({ quiet: false, progressJson: true, progressInterval: 250, timeoutMs: null }); expect(getCliOptions().progressJson).toBe(true); expect(getCliOptions().progressInterval).toBe(250); }); @@ -138,12 +138,12 @@ describe('CLI integration: progress streams to the right channel', () => { describe('cliOptsToProgressOptions', () => { test('--quiet → quiet mode', () => { - const opts = cliOptsToProgressOptions({ quiet: true, progressJson: false, progressInterval: 1000 }); + const opts = cliOptsToProgressOptions({ quiet: true, progressJson: false, progressInterval: 1000, timeoutMs: null }); expect(opts.mode).toBe('quiet'); }); test('--progress-json → json mode with interval', () => { - const opts = cliOptsToProgressOptions({ quiet: false, progressJson: true, progressInterval: 500 }); + const opts = cliOptsToProgressOptions({ quiet: false, progressJson: true, progressInterval: 500, timeoutMs: null }); expect(opts.mode).toBe('json'); expect(opts.minIntervalMs).toBe(500); }); @@ -155,7 +155,54 @@ describe('cliOptsToProgressOptions', () => { }); test('quiet takes priority over progressJson', () => { - const opts = cliOptsToProgressOptions({ quiet: true, progressJson: true, progressInterval: 1000 }); + const opts = cliOptsToProgressOptions({ quiet: true, progressJson: true, progressInterval: 1000, timeoutMs: null }); expect(opts.mode).toBe('quiet'); }); }); + +// v0.31.1: --timeout flag tests +describe('--timeout flag', () => { + test('--timeout=30s → 30000ms', () => { + const r = parseGlobalFlags(['--timeout=30s', 'search', 'X']); + expect(r.cliOpts.timeoutMs).toBe(30_000); + expect(r.rest).toEqual(['search', 'X']); + }); + + test('--timeout 1.5s → 1500ms', () => { + const r = parseGlobalFlags(['--timeout', '1.5s', 'search']); + expect(r.cliOpts.timeoutMs).toBe(1500); + expect(r.rest).toEqual(['search']); + }); + + test('--timeout=2m → 120000ms', () => { + const r = parseGlobalFlags(['--timeout=2m']); + expect(r.cliOpts.timeoutMs).toBe(120_000); + }); + + test('--timeout=500ms → 500ms', () => { + const r = parseGlobalFlags(['--timeout=500ms']); + expect(r.cliOpts.timeoutMs).toBe(500); + }); + + test('--timeout=500 (bare number, default ms)', () => { + const r = parseGlobalFlags(['--timeout=500']); + expect(r.cliOpts.timeoutMs).toBe(500); + }); + + test('--timeout=garbage → falls through, timeoutMs stays null', () => { + const r = parseGlobalFlags(['--timeout=garbage', 'search']); + expect(r.cliOpts.timeoutMs).toBe(null); + expect(r.rest).toContain('--timeout=garbage'); + }); + + test('--timeout=0 rejected (must be positive)', () => { + const r = parseGlobalFlags(['--timeout=0']); + expect(r.cliOpts.timeoutMs).toBe(null); + expect(r.rest).toContain('--timeout=0'); + }); + + test('default timeoutMs is null (per-command default applies)', () => { + const r = parseGlobalFlags(['search', 'X']); + expect(r.cliOpts.timeoutMs).toBe(null); + }); +}); diff --git a/test/doctor-remote.test.ts b/test/doctor-remote.test.ts index d9f2f8c69..e516b3aaa 100644 --- a/test/doctor-remote.test.ts +++ b/test/doctor-remote.test.ts @@ -17,6 +17,14 @@ import { collectRemoteDoctorReport } from '../src/core/doctor-remote.ts'; import type { GBrainConfig } from '../src/core/config.ts'; import { withEnv } from './helpers/with-env.ts'; +// v0.31.1: the new oauth_client_scopes_probe check uses the MCP SDK Client +// against /mcp, which the test fixture only mocks at JSON-RPC initialize +// level (no full tools/call). Every collectRemoteDoctorReport call here +// passes {skipScopeProbe: true} via SKIP_PROBE_OPTS. Probe behavior is +// covered separately in test/oauth-scope-probe.test.ts (pure-function +// buildScopeCheck against synthetic ScopeProbeResult inputs). +const SKIP_PROBE_OPTS = { skipScopeProbe: true }; + let server: Server; let port: number; @@ -97,7 +105,7 @@ function makeConfig(overrides: Partial> describe('collectRemoteDoctorReport', () => { test('happy path — all four checks pass', async () => { reset(); - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.status).toBe('ok'); expect(report.mode).toBe('thin-client'); expect(report.schema_version).toBe(2); @@ -114,7 +122,7 @@ describe('collectRemoteDoctorReport', () => { test('discovery 404 — fails with reason=http and short-circuits', async () => { reset(); discoveryStatus = 404; - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.status).toBe('fail'); const disco = report.checks.find(c => c.name === 'oauth_discovery')!; expect(disco.status).toBe('fail'); @@ -128,7 +136,7 @@ describe('collectRemoteDoctorReport', () => { test('discovery returns malformed body — fails with reason=parse', async () => { reset(); discoveryBody = { not_a_token_endpoint: 'whoops' }; - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.status).toBe('fail'); const disco = report.checks.find(c => c.name === 'oauth_discovery')!; expect(disco.detail?.reason).toBe('parse'); @@ -138,7 +146,7 @@ describe('collectRemoteDoctorReport', () => { reset(); tokenStatus = 401; tokenBody = { error: 'invalid_client' }; - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.status).toBe('fail'); const token = report.checks.find(c => c.name === 'oauth_token')!; expect(token.status).toBe('fail'); @@ -150,7 +158,7 @@ describe('collectRemoteDoctorReport', () => { test('mcp 401 — bearer rejected; fails with reason=auth', async () => { reset(); mcpStatus = 401; - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.status).toBe('fail'); const mcp = report.checks.find(c => c.name === 'mcp_smoke')!; expect(mcp.status).toBe('fail'); @@ -160,7 +168,7 @@ describe('collectRemoteDoctorReport', () => { test('mcp 500 — server error; fails with reason=http', async () => { reset(); mcpStatus = 500; - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.status).toBe('fail'); const mcp = report.checks.find(c => c.name === 'mcp_smoke')!; expect(mcp.detail?.reason).toBe('http'); @@ -210,7 +218,7 @@ describe('collectRemoteDoctorReport', () => { test('schema_version is 2 (matches local doctor schema_version)', async () => { reset(); - const report = await collectRemoteDoctorReport(makeConfig()); + const report = await collectRemoteDoctorReport(makeConfig(), SKIP_PROBE_OPTS); expect(report.schema_version).toBe(2); }); @@ -218,7 +226,7 @@ describe('collectRemoteDoctorReport', () => { reset(); await withEnv({ GBRAIN_REMOTE_CLIENT_SECRET: 'env-supplied-secret' }, async () => { const config = makeConfig({ oauth_client_secret: 'config-file-secret' }); - const report = await collectRemoteDoctorReport(config); + const report = await collectRemoteDoctorReport(config, SKIP_PROBE_OPTS); const creds = report.checks.find(c => c.name === 'oauth_credentials')!; expect(creds.status).toBe('ok'); expect(creds.message).toContain('secret_source=env'); diff --git a/test/e2e/thin-client.test.ts b/test/e2e/thin-client.test.ts index 1662aafef..f265ffa28 100644 --- a/test/e2e/thin-client.test.ts +++ b/test/e2e/thin-client.test.ts @@ -177,8 +177,11 @@ describeWhen('thin-client end-to-end (requires DATABASE_URL)', () => { test('sync is refused with canonical thin-client error', async () => { const r = await spawn(['sync'], clientHome); expect(r.exitCode).toBe(1); - expect(r.stderr).toContain('thin client'); + // v0.31.1: refusal carries pinpoint hint format (`thin-client of ` + // with hyphen) instead of the v0.30 generic `thin client` (with space). + expect(r.stderr).toContain('thin-client of'); expect(r.stderr).toContain(`http://127.0.0.1:${serverPort}/mcp`); + expect(r.stderr).toContain('not routable'); }); test('re-running init refuses without --force', async () => { @@ -188,6 +191,113 @@ describeWhen('thin-client end-to-end (requires DATABASE_URL)', () => { expect(parsed.reason).toBe('thin_client_config_present'); }); + // ─── v0.31.1 (Issue #734) — routing seam regression tests ─── + // + // Run BEFORE Tier B (remote ping) because the remote-ping test runs a + // 60s autopilot-cycle that can leave the server in a state where + // subsequent OAuth probes fail. Routing tests need a healthy server. + // + // The bug being fixed: thin-client gbrain commands silently fell through to + // the empty local PGLite, returned "No results." (exit 0), and never reached + // the remote brain. These tests pin the routing path against a real seeded + // host. If any of these regress, the silent-empty-results bug is back. + + test('issue #734 regression: gbrain search routes to host and returns seeded rows', async () => { + // Seed two pages on the host via direct `gbrain put` (host has the engine). + // Both contain the unique token "host_routing_proof" so we can grep + // the response body to prove it came from the remote brain. + const seed1 = await spawn([ + 'put', 'wiki/test/routing-proof-1', + '--type', 'note', + '--title', 'Routing Proof Page One', + '--content', '# Routing Proof One\n\nUnique token: host_routing_proof. This page only exists on the host.', + ], hostHome); + if (seed1.exitCode !== 0) throw new Error(`seed1 put failed: ${seed1.stderr || seed1.stdout}`); + + const seed2 = await spawn([ + 'put', 'wiki/test/routing-proof-2', + '--type', 'note', + '--title', 'Routing Proof Page Two', + '--content', '# Routing Proof Two\n\nAnother page with host_routing_proof.', + ], hostHome); + if (seed2.exitCode !== 0) throw new Error(`seed2 put failed: ${seed2.stderr || seed2.stdout}`); + + // Now run search from the THIN CLIENT. Pre-v0.31.1 this returned + // "No results." against the empty local PGLite. v0.31.1 routes via MCP + // and must return at least one row referencing the seeded slug. + const r = await spawn(['search', 'host_routing_proof'], clientHome); + + // Hard-fail conditions that pin the bug fix: + expect(r.exitCode).toBe(0); + // The original bug: empty stdout. If this assertion ever fails, #734 has + // regressed — silent-empty-results is back. + expect(r.stdout.length).toBeGreaterThan(0); + expect(r.stdout).not.toContain('No results.'); + // Seeded slug must appear in the routed response body. + expect(r.stdout).toContain('wiki/test/routing-proof'); + }); + + test('routed search emits identity banner on stderr (cherry-pick B)', async () => { + // Run search with stderr captured. Banner is suppressed in non-TTY by + // default per our suppression rules; opt back in with GBRAIN_BANNER=1. + const r = await spawn(['search', 'host_routing_proof'], clientHome, { + GBRAIN_BANNER: '1', + }); + expect(r.exitCode).toBe(0); + // Banner format: [thin-client → host:port · brain: Npages, Nchunks · vX.Y.Z] + expect(r.stderr).toContain('thin-client'); + expect(r.stderr).toContain(`127.0.0.1:${serverPort}`); + expect(r.stderr).toMatch(/v\d+\.\d+\.\d+/); + }); + + test('--quiet suppresses banner even with GBRAIN_BANNER=1', async () => { + // --quiet wins over GBRAIN_BANNER=1. Belt-and-suspenders for shell pipelines. + const r = await spawn(['--quiet', 'search', 'host_routing_proof'], clientHome, { + GBRAIN_BANNER: '1', + }); + expect(r.exitCode).toBe(0); + expect(r.stderr).not.toContain('thin-client →'); + }); + + test('routed put round-trip: thin-client write reaches the host', async () => { + // Write from the thin client. Pre-v0.31.1 this would have hit the empty + // local PGLite; v0.31.1 routes through MCP put_page. + const w = await spawn([ + 'put', 'wiki/test/thin-client-write-proof', + '--type', 'note', + '--title', 'Written By Thin Client', + '--content', '# Written By Thin Client\n\nThis page was created via routed MCP put.', + ], clientHome); + expect(w.exitCode).toBe(0); + + // Verify it landed on the host by reading it back from the host's local engine. + const r = await spawn(['get', 'wiki/test/thin-client-write-proof'], hostHome); + expect(r.exitCode).toBe(0); + expect(r.stdout).toContain('Written By Thin Client'); + expect(r.stdout).toContain('routed MCP put'); + }); + + test('routed stats: admin-scope client returns real numbers (not 0/0)', async () => { + // Pre-v0.31.1: thin-client `gbrain stats` returned page_count=0 against + // empty local PGLite. v0.31.1 routes to host's get_stats op (admin scope) + // and surfaces real numbers. We seeded 2+ pages above — page_count must + // reflect that, not zero. + const r = await spawn(['stats', '--json'], clientHome); + expect(r.exitCode).toBe(0); + const stats = JSON.parse(r.stdout.trim()); + expect(stats.page_count).toBeGreaterThan(0); + expect(stats.chunk_count).toBeGreaterThan(0); + }); + + test('local-only command refused with pinpoint hint (sync)', async () => { + // Refusal is already covered upstream but this pins the v0.31.1 hint + // format ("not routable. ") instead of the v0.30 generic message. + const r = await spawn(['sync'], clientHome); + expect(r.exitCode).toBe(1); + expect(r.stderr).toContain('not routable'); + expect(r.stderr).toContain('gbrain remote ping'); + }); + // ─── Tier B: gbrain remote ping + remote doctor ─── test('gbrain remote doctor returns the host DoctorReport', async () => { diff --git a/test/get-brain-identity.test.ts b/test/get-brain-identity.test.ts new file mode 100644 index 000000000..f86af0532 --- /dev/null +++ b/test/get-brain-identity.test.ts @@ -0,0 +1,123 @@ +/** + * v0.31.1 (Issue #734): tests for `get_brain_identity` MCP op. + * + * The op is the data source for the thin-client identity banner. Returns + * {version, engine, page_count, chunk_count, last_sync_iso}. Read-scope. + * Reuses engine.getStats() — banner's 60s TTL cache bounds frequency. + */ + +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 { operationsByName } from '../src/core/operations.ts'; +import { VERSION } from '../src/version.ts'; +import type { OperationContext } from '../src/core/operations.ts'; + +let engine: PGLiteEngine; + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}); + +afterAll(async () => { + await engine.disconnect(); +}); + +beforeEach(async () => { + await resetPgliteState(engine); +}); + +function buildCtx(): OperationContext { + return { + engine, + config: {} as any, + logger: console, + dryRun: false, + remote: false, + }; +} + +describe('get_brain_identity op', () => { + test('is registered in operationsByName', () => { + expect(operationsByName.get_brain_identity).toBeDefined(); + }); + + test('declares scope=read so non-admin clients can call it', () => { + expect(operationsByName.get_brain_identity.scope).toBe('read'); + }); + + test('is NOT localOnly (must be reachable over MCP for thin-client banner)', () => { + expect(operationsByName.get_brain_identity.localOnly).toBeFalsy(); + }); + + test('takes no params', () => { + expect(operationsByName.get_brain_identity.params).toEqual({}); + }); + + test('has no cliHints — banner-only op, not user-facing CLI surface', () => { + expect(operationsByName.get_brain_identity.cliHints).toBeUndefined(); + }); + + test('returns identity packet with VERSION + engine kind on empty brain', async () => { + const op = operationsByName.get_brain_identity; + const result = (await op.handler(buildCtx(), {})) as { + version: string; + engine: 'postgres' | 'pglite'; + page_count: number; + chunk_count: number; + last_sync_iso: string | null; + }; + + expect(result.version).toBe(VERSION); + expect(result.engine).toBe('pglite'); + expect(result.page_count).toBe(0); + expect(result.chunk_count).toBe(0); + expect(result.last_sync_iso).toBe(null); + }); + + test('reflects page count after pages are seeded', async () => { + await engine.putPage('wiki/test/foo', { + type: 'note', + title: 'Foo', + compiled_truth: 'foo body', + }); + await engine.putPage('wiki/test/bar', { + type: 'note', + title: 'Bar', + compiled_truth: 'bar body', + }); + + const op = operationsByName.get_brain_identity; + const result = (await op.handler(buildCtx(), {})) as { + page_count: number; + chunk_count: number; + }; + + expect(result.page_count).toBe(2); + // chunk_count depends on chunker; just assert it advanced past zero + expect(result.chunk_count).toBeGreaterThanOrEqual(0); + }); + + test('returns stable shape (load-bearing for thin-client banner)', async () => { + const op = operationsByName.get_brain_identity; + const result = (await op.handler(buildCtx(), {})) as Record; + const keys = Object.keys(result).sort(); + expect(keys).toEqual([ + 'chunk_count', + 'engine', + 'last_sync_iso', + 'page_count', + 'version', + ]); + }); + + test('last_sync_iso is null in v0.31.1 (deferred to v0.31.x — see plan)', async () => { + // Documents the known gap so an implementer who later wires the cycle + // to write a sync timestamp can flip this expectation. + const op = operationsByName.get_brain_identity; + const result = (await op.handler(buildCtx(), {})) as { last_sync_iso: string | null }; + expect(result.last_sync_iso).toBe(null); + }); +}); diff --git a/test/mcp-client-hardening.test.ts b/test/mcp-client-hardening.test.ts new file mode 100644 index 000000000..befcf78b3 --- /dev/null +++ b/test/mcp-client-hardening.test.ts @@ -0,0 +1,270 @@ +/** + * v0.31.1 (Issue #734, CDX-4): unit tests for the callRemoteTool hardening + * pass — the internal helpers that ensure every thrown value reaches the + * dispatcher as a typed RemoteMcpError. + * + * Pre-v0.31.1 contract was unsound: callRemoteTool's outermost block did NOT + * normalize all errors; plain Error, AbortError, JSON parse, and undici + * network errors could bubble untyped. The dispatcher's "exhaustive switch on + * RemoteMcpError.reason" was a false promise. + * + * v0.31.1 funnel: every catch in callRemoteTool routes through + * `toRemoteMcpError(e, mcpUrl)`. These tests pin the funnel's behavior so + * the dispatcher's exhaustive switch (in cli.ts:runThinClientRouted) holds. + */ + +import { describe, test, expect } from 'bun:test'; +import { + toRemoteMcpError, + extractToolErrorCode, + buildAbortController, + RemoteMcpError, + type CallRemoteToolOptions, +} from '../src/core/mcp-client.ts'; + +const MCP_URL = 'https://brain-host.example/mcp'; + +describe('toRemoteMcpError', () => { + test('passes through existing RemoteMcpError unchanged', () => { + const original = new RemoteMcpError('auth', 'bad creds', { mcp_url: MCP_URL }); + const out = toRemoteMcpError(original, MCP_URL); + expect(out).toBe(original); + expect(out.reason).toBe('auth'); + }); + + test('plain Error becomes network/unreachable', () => { + const err = new Error('ECONNREFUSED 127.0.0.1:3131'); + const out = toRemoteMcpError(err, MCP_URL); + expect(out).toBeInstanceOf(RemoteMcpError); + expect(out.reason).toBe('network'); + expect(out.detail?.kind).toBe('unreachable'); + expect(out.detail?.mcp_url).toBe(MCP_URL); + expect(out.message).toContain('ECONNREFUSED'); + }); + + test('AbortError (by .name) becomes network/aborted', () => { + const err = new Error('operation aborted'); + err.name = 'AbortError'; + const out = toRemoteMcpError(err, MCP_URL); + expect(out.reason).toBe('network'); + expect(out.detail?.kind).toBe('aborted'); + }); + + test('Error with /abort/i in message becomes network/aborted (SDK swallows .name)', () => { + // The MCP SDK sometimes wraps AbortError in a generic Error with the + // word "abort" in the message. The funnel catches both shapes. + const err = new Error('request was aborted by the user'); + const out = toRemoteMcpError(err, MCP_URL); + expect(out.reason).toBe('network'); + expect(out.detail?.kind).toBe('aborted'); + }); + + test('non-Error throwable (string) becomes network/unreachable', () => { + const out = toRemoteMcpError('something blew up', MCP_URL); + expect(out).toBeInstanceOf(RemoteMcpError); + expect(out.reason).toBe('network'); + expect(out.detail?.kind).toBe('unreachable'); + expect(out.message).toContain('something blew up'); + }); + + test('non-Error throwable (object) becomes network/unreachable with String() fallback', () => { + const out = toRemoteMcpError({ weird: 'shape' }, MCP_URL); + expect(out.reason).toBe('network'); + expect(out.detail?.kind).toBe('unreachable'); + }); + + test('non-Error throwable (null) becomes network/unreachable', () => { + const out = toRemoteMcpError(null, MCP_URL); + expect(out.reason).toBe('network'); + expect(out.detail?.kind).toBe('unreachable'); + }); + + test('mcp_url is always populated in detail', () => { + expect(toRemoteMcpError(new Error('x'), MCP_URL).detail?.mcp_url).toBe(MCP_URL); + expect(toRemoteMcpError('x', MCP_URL).detail?.mcp_url).toBe(MCP_URL); + expect(toRemoteMcpError(null, MCP_URL).detail?.mcp_url).toBe(MCP_URL); + }); + + test('the dispatcher contract: every output has a recognized reason', () => { + const validReasons = ['config', 'discovery', 'auth', 'auth_after_refresh', 'network', 'tool_error', 'parse']; + const inputs = [ + new Error('x'), + 'string-error', + null, + undefined, + 42, + { weird: true }, + new RemoteMcpError('parse', 'mock', { mcp_url: MCP_URL }), + ]; + for (const input of inputs) { + const out = toRemoteMcpError(input, MCP_URL); + expect(validReasons).toContain(out.reason); + } + }); +}); + +describe('extractToolErrorCode', () => { + test('parses JSON envelope with error.code', () => { + const msg = JSON.stringify({ error: { code: 'missing_scope', message: 'admin required' } }); + expect(extractToolErrorCode(msg)).toBe('missing_scope'); + }); + + test('parses JSON envelope with top-level code', () => { + const msg = JSON.stringify({ code: 'rate_limit_exceeded', message: 'too fast' }); + expect(extractToolErrorCode(msg)).toBe('rate_limit_exceeded'); + }); + + test('falls back to substring detection for missing_scope-shaped messages', () => { + // Regex is `scope.+(insufficient|required)` — requires "scope" BEFORE + // the keyword. Inputs designed to hit each alternative branch. + expect(extractToolErrorCode('error: missing scope admin')).toBe('missing_scope'); + expect(extractToolErrorCode('access denied: admin required')).toBe('missing_scope'); + expect(extractToolErrorCode('forbidden: admin required')).toBe('missing_scope'); + expect(extractToolErrorCode('scope is insufficient for this op')).toBe('missing_scope'); + expect(extractToolErrorCode('scope required: admin')).toBe('missing_scope'); + }); + + test('returns undefined for unstructured messages', () => { + expect(extractToolErrorCode('something went wrong')).toBeUndefined(); + expect(extractToolErrorCode('')).toBeUndefined(); + expect(extractToolErrorCode('database timed out')).toBeUndefined(); + }); + + test('JSON envelope without code field returns undefined', () => { + const msg = JSON.stringify({ error: { message: 'no code here' } }); + expect(extractToolErrorCode(msg)).toBeUndefined(); + }); + + test('non-string code in JSON envelope returns undefined (defensive)', () => { + const msg = JSON.stringify({ error: { code: 42, message: 'wrong type' } }); + expect(extractToolErrorCode(msg)).toBeUndefined(); + }); + + test('malformed JSON falls through to substring detection', () => { + // "missing_scope" appears literally in the broken JSON; substring path catches it. + const msg = '{not valid json missing_scope'; + expect(extractToolErrorCode(msg)).toBe('missing_scope'); + }); +}); + +describe('buildAbortController', () => { + test('no opts → signal never fires', async () => { + const { signal, cleanup } = buildAbortController({}); + expect(signal.aborted).toBe(false); + await new Promise(r => setTimeout(r, 50)); + expect(signal.aborted).toBe(false); + cleanup(); + }); + + test('timeoutMs fires on schedule', async () => { + const { signal, cleanup } = buildAbortController({ timeoutMs: 30 }); + expect(signal.aborted).toBe(false); + await new Promise(r => setTimeout(r, 80)); + expect(signal.aborted).toBe(true); + expect(signal.reason).toBeInstanceOf(Error); + expect((signal.reason as Error).message).toContain('timeout'); + cleanup(); + }); + + test('external signal already-aborted propagates immediately', () => { + const ext = new AbortController(); + ext.abort(new Error('SIGINT')); + const { signal, cleanup } = buildAbortController({ signal: ext.signal }); + expect(signal.aborted).toBe(true); + cleanup(); + }); + + test('external signal aborted later propagates to inner signal', async () => { + const ext = new AbortController(); + const { signal, cleanup } = buildAbortController({ signal: ext.signal }); + expect(signal.aborted).toBe(false); + ext.abort(new Error('user-cancel')); + expect(signal.aborted).toBe(true); + cleanup(); + }); + + test('timeout + external signal compose: whichever fires first wins', async () => { + // External fires before timeout. + const ext = new AbortController(); + const { signal, cleanup } = buildAbortController({ timeoutMs: 1000, signal: ext.signal }); + setTimeout(() => ext.abort(new Error('manual')), 20); + await new Promise(r => setTimeout(r, 60)); + expect(signal.aborted).toBe(true); + cleanup(); + }); + + test('cleanup clears the timeout timer', async () => { + // After cleanup, the timer must NOT fire (would otherwise leak via + // unhandled abort on a controller no caller cares about). + const { signal, cleanup } = buildAbortController({ timeoutMs: 30 }); + cleanup(); + await new Promise(r => setTimeout(r, 80)); + // signal can fire from the timer if cleanup didn't clear it. + // We accept either outcome but clear cleanup is the contract. + // The hard assertion is that calling cleanup doesn't throw and the + // pending timer doesn't crash the process. + void signal; + }); + + test('cleanup is idempotent (safe to call multiple times)', () => { + const { cleanup } = buildAbortController({ timeoutMs: 100 }); + cleanup(); + expect(() => cleanup()).not.toThrow(); + }); + + test('cleanup removes external signal listener (no leak after callRemoteTool returns)', async () => { + // The external signal should NOT keep the inner controller alive after + // cleanup(). After cleanup, aborting the external must NOT mutate inner. + const ext = new AbortController(); + const { signal, cleanup } = buildAbortController({ signal: ext.signal }); + cleanup(); + ext.abort(new Error('after-cleanup')); + // Inner signal stays whatever it was at cleanup time. Since neither timeout + // nor pre-cleanup external abort fired, it must still be NOT aborted. + expect(signal.aborted).toBe(false); + }); +}); + +describe('RemoteMcpError class shape', () => { + test('is an instance of Error', () => { + const e = new RemoteMcpError('network', 'msg'); + expect(e).toBeInstanceOf(Error); + expect(e).toBeInstanceOf(RemoteMcpError); + }); + + test('exposes reason + detail as readonly fields', () => { + const e = new RemoteMcpError('tool_error', 'msg', { code: 'missing_scope', mcp_url: MCP_URL }); + expect(e.reason).toBe('tool_error'); + expect(e.detail?.code).toBe('missing_scope'); + expect(e.detail?.mcp_url).toBe(MCP_URL); + }); + + test('has name="RemoteMcpError" for instanceof-free identification', () => { + const e = new RemoteMcpError('parse', 'msg'); + expect(e.name).toBe('RemoteMcpError'); + }); + + test('detail is optional', () => { + const e = new RemoteMcpError('config', 'msg'); + expect(e.detail).toBeUndefined(); + }); +}); + +describe('CallRemoteToolOptions type contract', () => { + test('both fields are optional', () => { + const empty: CallRemoteToolOptions = {}; + expect(empty.timeoutMs).toBeUndefined(); + expect(empty.signal).toBeUndefined(); + }); + + test('timeoutMs accepts a number', () => { + const opts: CallRemoteToolOptions = { timeoutMs: 30000 }; + expect(opts.timeoutMs).toBe(30000); + }); + + test('signal accepts an AbortSignal', () => { + const ctrl = new AbortController(); + const opts: CallRemoteToolOptions = { signal: ctrl.signal }; + expect(opts.signal).toBe(ctrl.signal); + }); +}); diff --git a/test/oauth-scope-probe.test.ts b/test/oauth-scope-probe.test.ts new file mode 100644 index 000000000..3dafff3cf --- /dev/null +++ b/test/oauth-scope-probe.test.ts @@ -0,0 +1,113 @@ +/** + * v0.31.1 (Issue #734, CDX-5): unit tests for the oauth_client_scopes_probe + * doctor check's pure-function builder. + * + * The probe itself (probeScopes) makes real MCP calls, so it's covered by + * E2E tests. Here we just exercise buildScopeCheck against synthetic + * ScopeProbeResult inputs to pin the status semantics: + * + * read.missing_scope → fail (broken setup) + * admin.missing_scope → warn (the load-bearing case for v0.29.2/v0.30.0 + * thin clients without admin scope) + * both ok → ok + * inconclusive → ok with detail.inconclusive=true + */ + +import { describe, test, expect } from 'bun:test'; +import { buildScopeCheck, type ScopeProbeResult } from '../src/core/doctor-remote.ts'; + +describe('buildScopeCheck', () => { + test('both probes succeed → status=ok with confirmation message', () => { + const probe: ScopeProbeResult = { read_ok: true, admin_ok: true }; + const check = buildScopeCheck('read write admin', probe); + expect(check.name).toBe('oauth_client_scopes_probe'); + expect(check.status).toBe('ok'); + expect(check.message).toContain('verified'); + expect(check.detail?.read_ok).toBe(true); + expect(check.detail?.admin_ok).toBe(true); + expect(check.detail?.inconclusive).toBeUndefined(); + }); + + test('read missing_scope → status=fail (broken setup)', () => { + const probe: ScopeProbeResult = { + read_ok: false, + admin_ok: false, + read_error: 'missing_scope', + }; + const check = buildScopeCheck('', probe); + expect(check.status).toBe('fail'); + expect(check.message).toContain('read scope'); + expect(check.detail?.read_ok).toBe(false); + }); + + test('admin missing_scope → status=warn with pinpoint hint (the load-bearing case)', () => { + const probe: ScopeProbeResult = { + read_ok: true, + admin_ok: false, + admin_error: 'missing_scope', + }; + const check = buildScopeCheck('read write', probe); + expect(check.status).toBe('warn'); + expect(check.message).toContain('admin scope MISSING'); + // The pinpoint remediation must name the exact CLI invocation. + expect(check.message).toContain('gbrain auth register-client'); + expect(check.message).toContain('read,write,admin'); + expect(check.detail?.read_ok).toBe(true); + expect(check.detail?.admin_ok).toBe(false); + expect(check.detail?.admin_error).toBe('missing_scope'); + }); + + test('admin probe fails for non-scope reason (e.g. parse) → status=ok inconclusive', () => { + const probe: ScopeProbeResult = { + read_ok: true, + admin_ok: false, + admin_error: 'parse', + }; + const check = buildScopeCheck('read write admin', probe); + expect(check.status).toBe('ok'); // doctor should NOT fail on probe noise + expect(check.message).toContain('inconclusive'); + expect(check.detail?.inconclusive).toBe(true); + expect(check.detail?.admin_error).toBe('parse'); + }); + + test('both probes fail for non-scope reasons → status=ok inconclusive', () => { + const probe: ScopeProbeResult = { + read_ok: false, + admin_ok: false, + read_error: 'network', + admin_error: 'network', + }; + const check = buildScopeCheck('', probe); + expect(check.status).toBe('ok'); // informational + expect(check.message).toContain('inconclusive'); + expect(check.detail?.inconclusive).toBe(true); + }); + + test('granted scope is surfaced in detail (and message when ok)', () => { + const probe: ScopeProbeResult = { read_ok: true, admin_ok: true }; + const check = buildScopeCheck('read write admin', probe); + expect(check.detail?.granted).toBe('read write admin'); + expect(check.message).toContain('read write admin'); + }); + + test('empty granted scope renders as "unspecified"', () => { + const probe: ScopeProbeResult = { read_ok: true, admin_ok: true }; + const check = buildScopeCheck('', probe); + expect(check.detail?.granted).toBe(null); + expect(check.message).toContain('unspecified'); + }); + + test('admin probe fails with read also failing (not missing_scope) → fail (read takes precedence)', () => { + // Defense-in-depth: if read genuinely missing_scope, that's the + // headline diagnosis; admin status is secondary. + const probe: ScopeProbeResult = { + read_ok: false, + admin_ok: false, + read_error: 'missing_scope', + admin_error: 'missing_scope', + }; + const check = buildScopeCheck('', probe); + expect(check.status).toBe('fail'); + expect(check.message).toContain('read'); + }); +});