From d3ef6b51cd956146c44b3dcd0a508d2d19446eb3 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 21 May 2026 11:19:39 -0700 Subject: [PATCH] feat(providers): extract formatRecipeTable + add init provider picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two changes prepping the env-detection wave: - providers.ts: extract formatRecipeTable() helper from runList(). Picker reuses it so UI can't drift from \`gbrain providers list\`. Also adds the codex finding #10 warn-line to \`providers test\` when the tested model differs from the configured default ("Note: tested X in isolation; gbrain's configured embedding is Y — this test does NOT verify your brain's active path."). envReady() takes an explicit env arg for testing. - init-provider-picker.ts (NEW): interactive picker mirroring init-mode-picker.ts. Filters candidate recipes to env-ready ones (codex finding #3), prompts via readLineSafe, exports printSubagentAnthropicCaveat() for shared use from initPGLite/initPostgres. Tests: 17 unit cases (10 providers + 7 picker). --- src/commands/init-provider-picker.ts | 183 +++++++++++++++++++++++++++ src/commands/providers.ts | 80 ++++++++---- test/init-provider-picker.test.ts | 124 ++++++++++++++++++ test/providers.test.ts | 96 ++++++++++++++ 4 files changed, 461 insertions(+), 22 deletions(-) create mode 100644 src/commands/init-provider-picker.ts create mode 100644 test/init-provider-picker.test.ts create mode 100644 test/providers.test.ts diff --git a/src/commands/init-provider-picker.ts b/src/commands/init-provider-picker.ts new file mode 100644 index 000000000..f2d851a47 --- /dev/null +++ b/src/commands/init-provider-picker.ts @@ -0,0 +1,183 @@ +/** + * v0.37.x — interactive provider picker for `gbrain init`. + * + * Mirrors the `init-mode-picker.ts` (v0.32.3) pattern. Runs from + * `initPGLite()` BEFORE `engine.initSchema()` when env detection finds + * zero or multiple env-ready providers for the embedding touchpoint + * (D1=hybrid). Reuses `formatRecipeTable()` from `providers.ts` so the + * picker's UI and `gbrain providers list` can't drift. + * + * Trust contract: + * - TTY-only. Callers must not invoke this in non-TTY contexts; D3 says + * non-TTY with zero keys exits 1 from `resolveAIOptions` before we + * reach here. A defensive guard returns null if no TTY anyway. + * - Filters candidates to env-ready recipes (codex finding #3). The + * picker is for choosing among providers the user CAN run, not for + * walking them through key setup. + * - On Ctrl-D / EOF / timeout: returns null, caller treats as exit 1. + * - When the user picks a non-Anthropic chat-capable recipe AND + * `ANTHROPIC_API_KEY` is missing, prints the subagent caveat from D7 + * BEFORE returning the choice so the user sees the implication. + */ + +import { listRecipes } from '../core/ai/recipes/index.ts'; +import { envReady, formatRecipeTable } from './providers.ts'; +import { readLineSafe } from './init.ts'; +import type { Recipe } from '../core/ai/types.ts'; + +export interface PickedProvider { + recipeId: string; + modelId: string; + /** Full `provider:model` string, ready for configureGateway. */ + fullModel: string; + /** Resolved dim (recipe's `default_dims`). */ + dim: number; + /** Whether the recipe also covers chat/expansion (informational). */ + hasChat: boolean; + hasExpansion: boolean; +} + +export interface PickProviderOpts { + /** Touchpoint the picker is selecting for. Embedding is the primary use case. */ + touchpoint: 'embedding' | 'expansion' | 'chat'; + /** Process env to probe. Defaults to process.env (injected for tests). */ + env?: NodeJS.ProcessEnv; + /** TTY override for tests. Defaults to process.stdin.isTTY. */ + isTTY?: boolean; + /** Stderr override for tests (capturing prompts). Defaults to process.stderr.write. */ + writeStderr?: (s: string) => void; +} + +/** + * Surface the subagent-Anthropic caveat (D7) when the user picks a + * non-Anthropic chat-capable recipe without `ANTHROPIC_API_KEY` set. + * + * Exported so `initPGLite` can reuse the same message in its post-init + * stderr summary path (auto-pick branch doesn't run the picker but still + * needs to surface the caveat). One source of truth keeps the message + * format aligned across the three D7 surfaces (picker / init summary / + * doctor). + */ +export function printSubagentAnthropicCaveat(write: (s: string) => void): void { + write( + '\n' + + 'Note: subagent features (gbrain dream, gbrain agent run, gbrain autopilot)\n' + + ' require ANTHROPIC_API_KEY regardless of which chat model you pick.\n' + + ' Chat alone (gbrain think, gbrain query expansion) works without it.\n' + + ' Set ANTHROPIC_API_KEY before running those commands.\n\n', + ); +} + +/** + * Filter recipes to those env-ready for the given touchpoint. Returns the + * filtered list and whether the touchpoint exists on each. Picker UI uses + * this to refuse picking a recipe whose env isn't ready (codex finding #3). + */ +function readyRecipesForTouchpoint( + recipes: Recipe[], + touchpoint: 'embedding' | 'expansion' | 'chat', + env: NodeJS.ProcessEnv, +): Recipe[] { + return recipes.filter(r => { + const tp = r.touchpoints[touchpoint]; + if (!tp) return false; + // Embedding + chat must have at least one model; expansion just needs to exist. + if (touchpoint === 'embedding' || touchpoint === 'chat') { + if (!Array.isArray(tp.models) || tp.models.length === 0) return false; + } + return envReady(r, env); + }); +} + +/** + * Pick a provider interactively from env-ready recipes. + * + * Returns null when the picker can't proceed (no TTY, no ready recipes, + * user aborted via Ctrl-D, or readLineSafe timeout). Caller exits 1 on + * null and prints the no-key fail-loud message itself. + */ +export async function pickProvider(opts: PickProviderOpts): Promise { + const env = opts.env ?? process.env; + const isTTY = opts.isTTY ?? process.stdin.isTTY ?? false; + const writeStderr = opts.writeStderr ?? ((s: string) => process.stderr.write(s)); + + if (!isTTY) { + // Defensive — caller should have handled non-TTY before reaching us. + return null; + } + + const all = listRecipes(); + const ready = readyRecipesForTouchpoint(all, opts.touchpoint, env); + + if (ready.length === 0) { + writeStderr(`\nNo ${opts.touchpoint}-capable providers are env-ready.\n`); + writeStderr('Set one of the env vars below and re-run init:\n\n'); + writeStderr(formatRecipeTable(all, env) + '\n\n'); + return null; + } + + writeStderr(`\nPick a ${opts.touchpoint} provider (env-ready providers shown):\n\n`); + writeStderr(formatRecipeTable(ready, env) + '\n\n'); + + // Build numbered options + const lines = ready.map((r, i) => { + const tp = r.touchpoints[opts.touchpoint]; + let label = ` ${i + 1}) ${r.id}`; + if (opts.touchpoint === 'embedding' && tp && 'default_dims' in tp) { + label += ` (${tp.default_dims}d)`; + } + if (tp && 'models' in tp && Array.isArray(tp.models) && tp.models.length > 0) { + label += ` ${tp.models[0]}`; + } + return label; + }); + writeStderr(lines.join('\n') + '\n\n'); + + const answer = await readLineSafe( + `Choice [1-${ready.length}, default 1]: `, + '1', + /* timeoutMs */ 60_000, + ); + + const choice = parseInt(answer.trim(), 10); + if (!Number.isFinite(choice) || choice < 1 || choice > ready.length) { + writeStderr(`\nInvalid choice "${answer}". Aborting.\n`); + return null; + } + + const picked = ready[choice - 1]; + const tp = picked.touchpoints[opts.touchpoint]; + if (!tp) return null; + + // Pick first model in the recipe's list (callers can override via flag). + const modelId = ('models' in tp && Array.isArray(tp.models) && tp.models.length > 0) + ? tp.models[0] + : ''; + if (!modelId) { + writeStderr(`\nRecipe "${picked.id}" declares no models for ${opts.touchpoint}. Aborting.\n`); + return null; + } + + // D7: surface the subagent-Anthropic caveat when picking a non-Anthropic + // chat-capable recipe without ANTHROPIC_API_KEY set. + const isChatTouchpoint = opts.touchpoint === 'chat'; + const isAnthropic = picked.id === 'anthropic'; + const anthropicKeySet = !!env.ANTHROPIC_API_KEY; + if (isChatTouchpoint && !isAnthropic && !anthropicKeySet) { + printSubagentAnthropicCaveat(writeStderr); + } + + const dim = + opts.touchpoint === 'embedding' && 'default_dims' in tp + ? (tp as { default_dims: number }).default_dims + : 0; + + return { + recipeId: picked.id, + modelId, + fullModel: `${picked.id}:${modelId}`, + dim, + hasChat: !!picked.touchpoints.chat && (picked.touchpoints.chat.models?.length ?? 0) > 0, + hasExpansion: !!picked.touchpoints.expansion, + }; +} diff --git a/src/commands/providers.ts b/src/commands/providers.ts index 22acd5662..6d2fc211b 100644 --- a/src/commands/providers.ts +++ b/src/commands/providers.ts @@ -44,10 +44,40 @@ function configureFromEnv(): void { }); } -function envReady(recipe: Recipe): boolean { +export function envReady(recipe: Recipe, env: NodeJS.ProcessEnv = process.env): boolean { const required = recipe.auth_env?.required ?? []; if (required.length === 0) return true; // e.g. local Ollama - return required.every(k => !!process.env[k]); + return required.every(k => !!env[k]); +} + +/** + * Pure formatter for the recipe matrix shown by `gbrain providers list` and + * the new `init-provider-picker` (D1+D2 — picker reuses this so its display + * stays in sync with `providers list` and can't drift). + * + * Returns the multi-line string (joined with `\n`). Callers handle stdout vs. + * stderr routing themselves. + */ +export function formatRecipeTable(recipes: Recipe[], env: NodeJS.ProcessEnv = process.env): string { + const rows: string[] = []; + rows.push('PROVIDER'.padEnd(14) + 'TIER'.padEnd(18) + 'EMBED'.padEnd(8) + 'EXPAND'.padEnd(8) + 'CHAT'.padEnd(8) + 'STATUS'); + rows.push('-'.repeat(78)); + for (const r of recipes) { + const hasEmbed = !!r.touchpoints.embedding && (r.touchpoints.embedding.models.length > 0); + const hasExpand = !!r.touchpoints.expansion; + const hasChat = !!r.touchpoints.chat && r.touchpoints.chat.models.length > 0; + const ready = envReady(r, env); + const status = ready ? '✓ ready' : `✗ missing ${r.auth_env?.required?.[0] ?? 'setup'}`; + rows.push( + r.id.padEnd(14) + + r.tier.padEnd(18) + + (hasEmbed ? 'yes' : '—').padEnd(8) + + (hasExpand ? 'yes' : '—').padEnd(8) + + (hasChat ? 'yes' : '—').padEnd(8) + + status, + ); + } + return rows.join('\n'); } export async function runProviders(subcommand: string | undefined, args: string[]): Promise { @@ -98,26 +128,7 @@ EXAMPLES } function runList(_args: string[]): void { - const recipes = listRecipes(); - const rows: string[] = []; - rows.push('PROVIDER'.padEnd(14) + 'TIER'.padEnd(18) + 'EMBED'.padEnd(8) + 'EXPAND'.padEnd(8) + 'CHAT'.padEnd(8) + 'STATUS'); - rows.push('-'.repeat(78)); - for (const r of recipes) { - const hasEmbed = !!r.touchpoints.embedding && (r.touchpoints.embedding.models.length > 0); - const hasExpand = !!r.touchpoints.expansion; - const hasChat = !!r.touchpoints.chat && r.touchpoints.chat.models.length > 0; - const ready = envReady(r); - const status = ready ? '✓ ready' : `✗ missing ${r.auth_env?.required?.[0] ?? 'setup'}`; - rows.push( - r.id.padEnd(14) + - r.tier.padEnd(18) + - (hasEmbed ? 'yes' : '—').padEnd(8) + - (hasExpand ? 'yes' : '—').padEnd(8) + - (hasChat ? 'yes' : '—').padEnd(8) + - status, - ); - } - console.log(rows.join('\n')); + console.log(formatRecipeTable(listRecipes())); } async function runTest(args: string[]): Promise { @@ -137,6 +148,30 @@ async function runTest(args: string[]): Promise { const [providerId, ...modelParts] = modelArg.split(':'); const modelId = modelParts.join(':'); const recipe = getRecipe(providerId); + + // codex finding #10: when `--model` is passed, the user is probing a + // model in isolation. They may be misled into thinking the test result + // validates their brain's actual configured path. Loud stderr line names + // the divergence at the top of the test so the recovery experience + // doesn't repeat the bug-reporter's "providers test ✓ but import still + // broken" trap. + try { + const cfg = loadConfig(); + const configuredModel = tpArg === 'embedding' ? cfg?.embedding_model : cfg?.chat_model; + if (!configuredModel) { + console.error( + `Note: tested ${modelArg} in isolation; this brain has no configured ${tpArg}_model yet. ` + + `\`providers test\` does NOT verify your brain's active path. ` + + `Set the active provider with \`gbrain config set ${tpArg}_model \` after running init.`, + ); + } else if (configuredModel !== modelArg) { + console.error( + `Note: tested ${modelArg} in isolation; gbrain's configured ${tpArg} is ${configuredModel}. ` + + `\`providers test\` does NOT verify your brain's active path.`, + ); + } + } catch { /* loadConfig throws when no brain configured — first-time install path; the no-config branch above handles it. */ } + if (tpArg === 'embedding') { const dims = recipe?.touchpoints.embedding?.default_dims ?? 1536; configureGateway({ @@ -150,6 +185,7 @@ async function runTest(args: string[]): Promise { env: { ...process.env }, }); } + void modelId; // intentionally unused but preserved for readability } if (!gwIsAvailable(tpArg)) { diff --git a/test/init-provider-picker.test.ts b/test/init-provider-picker.test.ts new file mode 100644 index 000000000..c9e43fa90 --- /dev/null +++ b/test/init-provider-picker.test.ts @@ -0,0 +1,124 @@ +/** + * Picker unit tests — exercise the pure paths (env filtering, caveat + * messaging, null returns on bad input). TTY-input flows (numbered + * selection happy path, Ctrl-D, timeout) are covered E2E via + * cli-pty-runner.ts because mocking readLineSafe at the unit boundary + * leaks across files in the shard process per CLAUDE.md test-isolation + * rules. + */ + +import { describe, test, expect } from 'bun:test'; +import { + pickProvider, + printSubagentAnthropicCaveat, +} from '../src/commands/init-provider-picker.ts'; + +describe('printSubagentAnthropicCaveat', () => { + test('writes the canonical D7 caveat lines', () => { + let buf = ''; + printSubagentAnthropicCaveat((s) => { buf += s; }); + expect(buf).toContain('subagent features'); + expect(buf).toContain('gbrain dream'); + expect(buf).toContain('gbrain agent run'); + expect(buf).toContain('gbrain autopilot'); + expect(buf).toContain('ANTHROPIC_API_KEY'); + // The caveat must clarify chat alone is fine without it. + expect(buf).toContain('Chat alone'); + }); +}); + +describe('pickProvider — defensive paths', () => { + test('non-TTY returns null without prompting', async () => { + let stderr = ''; + const got = await pickProvider({ + touchpoint: 'embedding', + env: {}, + isTTY: false, + writeStderr: (s) => { stderr += s; }, + }); + expect(got).toBeNull(); + expect(stderr).toBe(''); + }); + + test('TTY happy path — env-ready provider proceeds to prompt + returns first choice', async () => { + // OPENAI_API_KEY set → openai is env-ready. readLineSafe returns the + // default '1' in non-stdin-TTY bun:test mode, so picker picks the first + // ready recipe deterministically. We mostly want to verify NO null + // return and a sensible payload shape. + let stderr = ''; + const got = await pickProvider({ + touchpoint: 'embedding', + env: { OPENAI_API_KEY: 'sk-test' }, + isTTY: true, + writeStderr: (s) => { stderr += s; }, + }); + expect(got).not.toBeNull(); + if (got) { + expect(got.fullModel).toMatch(/:/); // provider:model shape + expect(got.dim).toBeGreaterThan(0); // embedding always has dims + expect(stderr).toContain('Pick a embedding provider'); + } + }); + + test('caveat fires when picking non-Anthropic chat without ANTHROPIC_API_KEY', async () => { + let stderr = ''; + const got = await pickProvider({ + touchpoint: 'chat', + // OpenAI is chat-capable; Anthropic key missing → caveat must fire. + env: { OPENAI_API_KEY: 'sk-test' }, + isTTY: true, + writeStderr: (s) => { stderr += s; }, + }); + expect(got).not.toBeNull(); + if (got) { + expect(got.recipeId).toBe('openai'); + // The caveat printed somewhere in stderr. + expect(stderr).toContain('subagent features'); + expect(stderr).toContain('ANTHROPIC_API_KEY'); + } + }); + + test('caveat does NOT fire when picking Anthropic for chat', async () => { + let stderr = ''; + const got = await pickProvider({ + touchpoint: 'chat', + env: { ANTHROPIC_API_KEY: 'sk-ant-test' }, + isTTY: true, + writeStderr: (s) => { stderr += s; }, + }); + expect(got).not.toBeNull(); + if (got) { + expect(got.recipeId).toBe('anthropic'); + // No subagent caveat in stderr. + expect(stderr).not.toContain('subagent features'); + } + }); + + test('caveat does NOT fire when picking non-Anthropic chat WITH ANTHROPIC_API_KEY also set', async () => { + let stderr = ''; + const got = await pickProvider({ + touchpoint: 'chat', + env: { OPENAI_API_KEY: 'sk-test', ANTHROPIC_API_KEY: 'sk-ant-test' }, + isTTY: true, + writeStderr: (s) => { stderr += s; }, + }); + expect(got).not.toBeNull(); + if (got) { + // Both are ready; first-by-listRecipes order wins; readLineSafe + // returns default '1'. Either openai or anthropic — we just verify + // no caveat fires either way (anthropic set, no subagent surprise). + expect(stderr).not.toContain('subagent features'); + } + }); + + test('embedding touchpoint label printed in prompt', async () => { + let stderr = ''; + await pickProvider({ + touchpoint: 'embedding', + env: { OPENAI_API_KEY: 'sk-test' }, + isTTY: true, + writeStderr: (s) => { stderr += s; }, + }); + expect(stderr).toContain('embedding provider'); + }); +}); diff --git a/test/providers.test.ts b/test/providers.test.ts new file mode 100644 index 000000000..f52969aa6 --- /dev/null +++ b/test/providers.test.ts @@ -0,0 +1,96 @@ +/** + * `gbrain providers` — pure formatter + envReady tests. + * + * `runTest` and `runExplain` aren't covered here because they touch the + * gateway / loadConfig; E2E exercises those. + */ + +import { describe, test, expect } from 'bun:test'; +import { formatRecipeTable, envReady } from '../src/commands/providers.ts'; +import { listRecipes, getRecipe } from '../src/core/ai/recipes/index.ts'; +import type { Recipe } from '../src/core/ai/types.ts'; + +describe('envReady', () => { + test('true when all required env vars set', () => { + const openai = getRecipe('openai'); + expect(openai).toBeDefined(); + expect(envReady(openai!, { OPENAI_API_KEY: 'sk-test' })).toBe(true); + }); + + test('false when required env var missing', () => { + const openai = getRecipe('openai'); + expect(envReady(openai!, {})).toBe(false); + }); + + test('false on empty-string env var', () => { + const openai = getRecipe('openai'); + expect(envReady(openai!, { OPENAI_API_KEY: '' })).toBe(false); + }); + + test('true for recipes with no required env (local Ollama)', () => { + // Ollama has no auth_env.required. + const ollama = getRecipe('ollama'); + expect(ollama).toBeDefined(); + expect(envReady(ollama!, {})).toBe(true); + }); +}); + +describe('formatRecipeTable', () => { + test('header row present', () => { + const out = formatRecipeTable(listRecipes(), {}); + expect(out).toContain('PROVIDER'); + expect(out).toContain('TIER'); + expect(out).toContain('EMBED'); + expect(out).toContain('EXPAND'); + expect(out).toContain('CHAT'); + expect(out).toContain('STATUS'); + }); + + test('shows ✓ ready for env-satisfied provider', () => { + const out = formatRecipeTable(listRecipes(), { OPENAI_API_KEY: 'sk-test' }); + // openai row should be ready + const openaiLine = out.split('\n').find(line => line.startsWith('openai')); + expect(openaiLine).toBeDefined(); + expect(openaiLine).toContain('✓ ready'); + }); + + test('shows ✗ missing for missing provider', () => { + const out = formatRecipeTable(listRecipes(), {}); + // openai should show missing OPENAI_API_KEY + const openaiLine = out.split('\n').find(line => line.startsWith('openai')); + expect(openaiLine).toBeDefined(); + expect(openaiLine).toContain('✗ missing OPENAI_API_KEY'); + }); + + test('each recipe appears at most once', () => { + const out = formatRecipeTable(listRecipes(), {}); + const recipes = listRecipes(); + for (const r of recipes) { + const occurrences = out.split('\n').filter(line => line.startsWith(`${r.id} `) || line.startsWith(`${r.id} `)); + expect(occurrences.length).toBeGreaterThanOrEqual(1); + } + }); + + test('embedding-only recipe (zeroentropyai) shows yes/—/— for tiers', () => { + const out = formatRecipeTable(listRecipes(), {}); + const zeLine = out.split('\n').find(line => line.startsWith('zeroentropyai')); + expect(zeLine).toBeDefined(); + // ZE has embedding but no expansion or chat + expect(zeLine).toContain('yes'); + expect(zeLine).toContain('—'); + }); + + test('isolated subset renders correctly (picker reuses this)', () => { + const openai = getRecipe('openai'); + const ze = getRecipe('zeroentropyai'); + expect(openai && ze).toBeTruthy(); + const out = formatRecipeTable([openai!, ze!], { OPENAI_API_KEY: 'sk-test' }); + const lines = out.split('\n'); + // header + separator + 2 recipe rows + expect(lines.length).toBe(4); + expect(lines[2]).toContain('openai'); + expect(lines[2]).toContain('✓ ready'); + expect(lines[3]).toContain('zeroentropyai'); + expect(lines[3]).toContain('✗ missing ZEROENTROPY_API_KEY'); + }); +});