From 5911072aec1f19daf1f91129bba9298b1bebe8d6 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 1 Jun 2026 21:54:13 -0700 Subject: [PATCH] =?UTF-8?q?v0.42.4.0=20fix:=20think=20--model=20fails=20lo?= =?UTF-8?q?ud=20=E2=80=94=20slash-form=20ids=20+=20never=20persist=20empty?= =?UTF-8?q?=20synthesis=20(#1698)=20(#1736)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: think --model fails loud on unresolvable model; never persist empty synthesis (#1698) Slash-form model ids (anthropic/claude-sonnet-4-6) silently degraded to the no-LLM stub and wrote empty synthesis pages with exit 0 (reporter saw 200). Three compounding defects, fixed: - normalizeModelId (src/core/model-id.ts): one shared provider:model normalizer replacing 4 colon-only inlines; slash→colon, bare→default, malformed leading-separator (:foo) returned unchanged so resolveRecipe throws loud. - validateModelId + probeChatModel (gateway.ts): shared id-validity + key probe; runThink hard-errors on an explicit --model it can't run (no silent degrade). - synthesisOk + persist-skip (think/index.ts): empty/malformed/empty-JSON synthesis is never persisted; --save with no synthesis exits 1. - auto-think (cycle/auto-think.ts): empty synthesis no longer counts complete or advances the cooldown (the autonomous third caller of persistSynthesis). - hasAnthropicKey consolidated into src/core/ai/anthropic-key.ts (3 copies → 1). MCP think op sets modelExplicit; saved_slug '' maps to null. * chore: bump version and changelog (v0.42.4.0) #1698 think --model fail-loud wave. Also files the P3 follow-up TODO for the provider-symmetric early gate (D1 accept-as-is). Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 80 +++++++++++ TODOS.md | 35 +++++ VERSION | 2 +- package.json | 2 +- src/commands/think.ts | 62 ++++++--- src/core/ai/anthropic-key.ts | 29 ++++ src/core/ai/gateway.ts | 71 ++++++++++ src/core/conversation-parser/llm-base.ts | 27 ++-- src/core/cycle/auto-think.ts | 16 ++- src/core/cycle/synthesize.ts | 55 +++----- src/core/facts/extract.ts | 6 +- src/core/model-id.ts | 30 ++++ src/core/operations.ts | 9 +- src/core/think/index.ts | 120 ++++++++++------ test/ai/anthropic-key.test.ts | 64 +++++++++ test/ai/gateway-probe-chat-model.test.ts | 105 ++++++++++++++ test/auto-think-phase.test.ts | 25 ++++ test/cycle/synthesize-gateway-adapter.test.ts | 12 ++ test/model-id.test.ts | 83 +++++++++++- test/think-gateway-adapter.test.ts | 95 +++++++++++++ test/think-pipeline.serial.test.ts | 128 ++++++++++++++++++ 21 files changed, 934 insertions(+), 122 deletions(-) create mode 100644 src/core/ai/anthropic-key.ts create mode 100644 test/ai/anthropic-key.test.ts create mode 100644 test/ai/gateway-probe-chat-model.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b37fcba1..3638b95ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,86 @@ All notable changes to GBrain will be documented in this file. +## [0.42.4.0] - 2026-06-01 + +**`gbrain think` stops writing blank pages, and a bad `--model` now fails loud instead of going quiet.** + +If you ran `gbrain think --model anthropic/claude-sonnet-4-6 --save` (with a slash +in the model name), gbrain used to quietly give up on the real model, fall back to +a "no LLM available" stub, and save an empty synthesis page anyway — exit code 0, +no error. One person ran this in a loop and got 200 blank pages before noticing. + +This release closes that whole class of silent failure: + +- **Slash-form model names work now.** `anthropic/claude-sonnet-4-6` is treated the + same as `anthropic:claude-sonnet-4-6`. A bare name like `claude-opus-4-7` still + defaults to Anthropic. +- **An explicit `--model` you typed but can't run is a hard error.** Typo the model, + pick a provider with no API key, name a model that doesn't exist — `gbrain think` + exits 1 with a clear message (and a paste-ready fix when there is one), instead of + silently degrading to the stub. Omitting `--model` keeps the old graceful behavior: + no key, no `--save` still prints the gathered context and exits 0. +- **An empty synthesis is never saved.** If the model returns nothing, malformed + output, or an empty answer, no page is written. `gbrain think --save` with no real + synthesis exits 1 and tells you nothing was saved. +- **The nightly auto-think cycle got the same guard.** An empty synthesis no longer + counts as "done" or advances the cooldown, so the next cycle retries instead of + silently skipping for days. + +If you typed `--model` and it was unusable before, you were getting a blank page with +exit 0. Now you get a real answer, or a real error. No silent middle. + +### How it works (the precise bits) + +- New `normalizeModelId` (`src/core/model-id.ts`) is the one shared `provider:model` + normalizer; it replaced four copies of a colon-only inline that mangled slash form. + A malformed leading separator (`:foo` / `/foo`) is returned unchanged so the + resolver throws loudly instead of coercing it to Anthropic. +- New `validateModelId` + `probeChatModel` (`src/core/ai/gateway.ts`) are the shared + id-validity + key probe. `runThink` calls the probe before retrieval and hard-errors + on an explicit, unusable model. +- `ThinkResult.synthesisOk` gates persistence: `persistSynthesis` returns a + `SYNTHESIS_EMPTY_NOT_PERSISTED` signal (never writes) when synthesis didn't happen. +- `hasAnthropicKey` consolidated into `src/core/ai/anthropic-key.ts` (three private + copies collapsed to one). +- The MCP `think` op enforces the same hard-error on an explicit unusable `model`. + +## To take advantage of v0.42.4.0 + +Nothing to run — the fix is automatic on upgrade (`gbrain upgrade`). To verify: + +```bash +# Slash form now works (with a real key): +gbrain think "what do we know about acme-example" --model anthropic/claude-sonnet-4-6 --save + +# A bad model is now a loud error (exit 1), not a blank page: +gbrain think "..." --model anthropic/claude-bogus-9 --save +``` + +If a bad `--model` still silently writes an empty page, file an issue with the +command you ran and `gbrain doctor` output. + +### Itemized changes + +- `src/core/model-id.ts` — `normalizeModelId(input, defaultProvider?)`; slash→colon, + bare→default, empty/whitespace/leading-separator returned unchanged. +- `src/core/ai/gateway.ts` — exported `validateModelId` + `ModelIdValidity` (registry + id-validity), `probeChatModel` + `ChatModelProbe` (validity + Anthropic-key + availability). +- `src/core/ai/anthropic-key.ts` (new) — single shared `hasAnthropicKey()`. +- `src/core/think/index.ts` — early explicit-model hard-throw, `modelExplicit` opt, + `synthesisOk` at all return sites, persist-skip signal, builder uses the shared probe. +- `src/commands/think.ts` — `modelExplicit: !!model`, try/catch → exit 1, empty-slug + save guard, updated `--model` help. +- `src/core/operations.ts` — MCP `think` op sets `modelExplicit`, maps `saved_slug` `''`→null. +- `src/core/cycle/synthesize.ts` — `makeJudgeClient` routed through `validateModelId`. +- `src/core/cycle/auto-think.ts` — empty synthesis → `partial`, no cooldown advance. +- `src/core/facts/extract.ts`, `src/core/conversation-parser/llm-base.ts` — use the + shared normalizer + `hasAnthropicKey`. +- Tests: `test/model-id.test.ts`, `test/ai/gateway-probe-chat-model.test.ts`, + `test/ai/anthropic-key.test.ts`, `test/think-gateway-adapter.test.ts`, + `test/think-pipeline.serial.test.ts`, `test/cycle/synthesize-gateway-adapter.test.ts`, + `test/auto-think-phase.test.ts`. ## [0.42.3.0] - 2026-05-30 **Search now returns the *confident handful* instead of a fixed wall of results diff --git a/TODOS.md b/TODOS.md index 3c55df82a..4e8283445 100644 --- a/TODOS.md +++ b/TODOS.md @@ -3829,3 +3829,38 @@ judgment. **Depends on:** human judgment on which historical CHANGELOG entries to leave intact vs scrub. + +### Provider-symmetric early gate for `think --model` (#1698 follow-up, P3) + +**What:** Make `runThink`'s explicit-`--model` early gate reject an explicit +NON-Anthropic model with no provider key BEFORE gather, not after. Today +`probeChatModel` (`src/core/ai/gateway.ts`) only pre-checks the Anthropic key; +non-Anthropic providers pass the early gate and hard-error at the create-callback +rethrow instead (one wasted retrieval gather). The deviation is documented as D1 +in the #1698 fix and is **accept-as-is** — pinned by the "D1 backstop" test in +`test/think-gateway-adapter.test.ts` (build succeeds, `create()` throws). + +**Why:** Symmetry — every explicit unusable model fails at one chokepoint, so the +"no silent degrade on explicit model" guarantee is provable in a single place +rather than relying on the create-callback backstop for non-Anthropic providers. +Saves one gather per failure in the rare explicit-non-Anthropic-no-key case. + +**Pros:** single validation chokepoint; explicit > clever. +**Cons:** the obvious implementation (route `probeChatModel` onto the gateway's +`isAvailable` for all providers) carries an unconfigured-gateway false-reject +footgun — `isAvailable` returns `false` when `_config` is absent even if an env +key exists, which could false-reject a *usable* model in some test/unconfigured +paths. A correct version needs a config-independent provider-general key probe +(reads each recipe's auth resolver against env+config without the gateway's +runtime `_config`), plus the full targeted-test sweep to prove no regression +across the ~13 think tests + the non-explicit `tryBuildGatewayClient` build path. + +**Context:** Surfaced by both the diff-level eng review (rated P3) and an +independent codex pass (rated P1) of the #1698 implementation. Severity tension +resolved accept-as-is: the safety property (no silent degrade on explicit unusable +model) is already met; this is a timing/symmetry improvement, not a safety fix. +Start at `probeChatModel` in `src/core/ai/gateway.ts` and the explicit gate in +`runThink` (`src/core/think/index.ts`). + +**Depends on:** a config-independent provider-general key probe (new gateway +helper) so the `isAvailable` unconfigured-gateway false-reject footgun is avoided. diff --git a/VERSION b/VERSION index 9ece4afec..85654d5e1 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.3.0 \ No newline at end of file +0.42.4.0 \ No newline at end of file diff --git a/package.json b/package.json index 464f99044..9cecdf3c0 100644 --- a/package.json +++ b/package.json @@ -142,5 +142,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.3.0" + "version": "0.42.4.0" } diff --git a/src/commands/think.ts b/src/commands/think.ts index 8b289c0b2..e3c10882f 100644 --- a/src/commands/think.ts +++ b/src/commands/think.ts @@ -29,17 +29,23 @@ Options: --rounds N Multi-pass synthesis (default 1; gap-driven loop ships in v0.29) --save Persist a synthesis page under synthesis/-.md --take Append a take row to the anchor page (requires --anchor) - --model Override the model (alias or full id) + --model Override the model: provider:model (preferred) or + provider/model or a bare alias. An explicit --model that + can't be resolved is a hard error (exit 1) — never a + silent no-LLM degrade. --since YYYY-MM-DD Start of temporal window --until YYYY-MM-DD End of temporal window --json Output as JSON --help Show this help Without --save, the synthesis is printed to stdout and discarded. With --save, -the synthesis page is persisted AND printed. +the synthesis page is persisted AND printed. If --save is given but no synthesis +was produced (no LLM available, or empty result), nothing is saved and the command +exits non-zero. -Set ANTHROPIC_API_KEY in the environment to run real synthesis. Without it, -the gather phase still runs and prints what would have been the input. +Set ANTHROPIC_API_KEY (or run: gbrain config set anthropic_api_key ...) to run +real synthesis. Without it AND without --save, the gather phase still runs and +prints what would have been the input (exit 0). `); return; } @@ -102,21 +108,41 @@ the gather phase still runs and prints what would have been the input. }, { timeoutMs: 180_000 }); result = unpackToolResult(raw); } else { - result = await runThink(engine, { - question, anchor, rounds, save, take, model, since, until, - // v0.36.1.0 (E1) — opt-in anti-bias rewrite. Falls back to baseline - // think when no profile exists, with NO_CALIBRATION_PROFILE warning. - withCalibration, - ...(calibrationHolder ? { calibrationHolder } : {}), - // Local CLI: no MCP allow-list filter — operator owns the brain. - }); + try { + result = await runThink(engine, { + question, anchor, rounds, save, take, model, since, until, + // #1698: explicit --model → hard error on an unresolvable model (no silent + // degrade to the no-LLM stub). Omitting --model keeps the graceful default path. + modelExplicit: !!model, + // v0.36.1.0 (E1) — opt-in anti-bias rewrite. Falls back to baseline + // think when no profile exists, with NO_CALIBRATION_PROFILE warning. + withCalibration, + ...(calibrationHolder ? { calibrationHolder } : {}), + // 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); + // 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 || undefined; // '' = persist-skip signal (#10) + evidenceInserted = persisted.evidenceInserted; + for (const w of persisted.warnings) result.warnings.push(w); + // #1698 (F2): --save requested but no synthesis was produced (no LLM, empty, + // or malformed) → exit non-zero. Saving nothing with exit 0 when the user + // explicitly asked to save is itself a silent failure. + if (!persisted.slug) { + console.error( + 'think: --save requested but no synthesis was produced (no LLM available ' + + 'or empty result) — nothing saved.', + ); + process.exit(1); + } + } + } catch (e) { + // #1698: an unresolvable explicit --model throws here. Clean non-zero exit + // with the actionable message, not a stack trace. + console.error((e as Error).message); + process.exit(1); } } diff --git a/src/core/ai/anthropic-key.ts b/src/core/ai/anthropic-key.ts new file mode 100644 index 000000000..62fe069f9 --- /dev/null +++ b/src/core/ai/anthropic-key.ts @@ -0,0 +1,29 @@ +/** + * v0.41.x (#1698) — single shared Anthropic key-presence probe. + * + * Consolidates three byte-identical private copies that had drifted apart over + * time (`think/index.ts`, `cycle/synthesize.ts`, `conversation-parser/llm-base.ts`). + * Same drift class as the four colon-only model-id normalizers — one source of truth. + * + * Reads BOTH env (`ANTHROPIC_API_KEY`) AND the gbrain config file + * (`anthropic_api_key` set via `gbrain config set`) so stdio MCP launches that + * don't inherit shell env keep working. `loadConfig` can throw on first-run + * installs; that is swallowed and treated as "no key available." + * + * Lives in `src/core/ai/` (not gateway.ts) to keep the gateway module's surface + * lean and to avoid any import-cycle risk — the three consumers already import + * from gateway.ts. + */ + +import { loadConfig } from '../config.ts'; + +export function hasAnthropicKey(): boolean { + if (process.env.ANTHROPIC_API_KEY) return true; + try { + const cfg = loadConfig(); + if (cfg?.anthropic_api_key) return true; + } catch { + // loadConfig may throw on first-run installs; treat as no key available. + } + return false; +} diff --git a/src/core/ai/gateway.ts b/src/core/ai/gateway.ts index 1527b3356..f871bc5d8 100644 --- a/src/core/ai/gateway.ts +++ b/src/core/ai/gateway.ts @@ -41,6 +41,7 @@ import type { EmbedMultimodalOpts, MultimodalBatchResult, MultimodalInput, + ParsedModelId, Recipe, TouchpointKind, } from './types.ts'; @@ -48,6 +49,7 @@ import { resolveRecipe, assertTouchpoint, parseModelId } from './model-resolver. import { resolveModel, TIER_DEFAULTS } from '../model-config.ts'; import type { BrainEngine } from '../engine.ts'; import { dimsProviderOptions } from './dims.ts'; +import { hasAnthropicKey } from './anthropic-key.ts'; import { AIConfigError, AITransientError, normalizeAIError } from './errors.ts'; import { runGuardrails, hasGuardrails, type GuardrailHook } from '../guardrails.ts'; @@ -2213,6 +2215,75 @@ export interface ChatOpts { cacheSystem?: boolean; } +/** + * v0.41.x (#1698) — id-validity core. Shared by `runThink`'s explicit-model gate + * (via `probeChatModel`) AND `makeJudgeClient` in `cycle/synthesize.ts`. + * + * Validates that a `provider:model` string resolves to a real recipe AND that the + * recipe supports the chat touchpoint (catches typo'd native models like + * `anthropic:claude-bogus-9`). Both checks read the recipe REGISTRY, not gateway + * `_config`, so this works before `configureGateway()` has run — which is why + * `makeJudgeClient` reuses this layer instead of the full `probeChatModel` (whose + * `isAvailable` layer would reject non-Anthropic-no-key + unconfigured-gateway). + * + * Order matters: `resolveRecipe` first (unknown_provider), then `assertTouchpoint` + * (unknown_model). `isAvailable` alone collapses both into a bare `false`. + */ +export type ModelIdValidity = + | { ok: true; parsed: ParsedModelId; recipe: Recipe } + | { ok: false; reason: 'unknown_provider' | 'unknown_model'; detail: string; fix?: string }; + +export function validateModelId(modelStr: string): ModelIdValidity { + let parsed: ParsedModelId; + let recipe: Recipe; + try { + ({ parsed, recipe } = resolveRecipe(modelStr)); + } catch (e) { + if (e instanceof AIConfigError) return { ok: false, reason: 'unknown_provider', detail: e.message, fix: e.fix }; + throw e; + } + try { + assertTouchpoint(recipe, 'chat', parsed.modelId, getExtendedModelsForProvider(parsed.providerId)); + } catch (e) { + if (e instanceof AIConfigError) return { ok: false, reason: 'unknown_model', detail: e.message, fix: e.fix }; + throw e; + } + return { ok: true, parsed, recipe }; +} + +/** + * v0.41.x (#1698) — full chat-model probe = id-validity + key availability. + * Used by `runThink`'s explicit-`--model` gate (where a model the user typed but + * can't run SHOULD hard-error, not silently degrade), AND by `tryBuildGatewayClient` + * + `makeJudgeClient`. One shared predicate, no drift. + * + * The key layer uses `hasAnthropicKey` (env OR gbrain config file), which is + * gateway-config-INDEPENDENT — it works before `configureGateway()` and in unit + * tests, and preserves the historical key-detection source (codex #6; the prior + * draft used `isAvailable`, which reads gateway `_config.env` and would have + * regressed the builder + broken every test that skips `configureGateway`). + * Non-Anthropic providers are checked LAZILY at `gateway.chat()` time (build the + * client, let the call surface AIConfigError) — matches the deliberate + * per-transcript-degrade contract (test A9: a deepseek judge with no key returns + * a client, not null). + */ +export type ChatModelProbe = + | { ok: true } + | { ok: false; reason: 'unknown_provider' | 'unknown_model' | 'unavailable'; detail: string; fix?: string }; + +export function probeChatModel(modelStr: string): ChatModelProbe { + const v = validateModelId(modelStr); + if (!v.ok) return { ok: false, reason: v.reason, detail: v.detail, fix: v.fix }; + if (v.parsed.providerId === 'anthropic' && !hasAnthropicKey()) { + return { + ok: false, + reason: 'unavailable', + detail: 'no Anthropic API key configured (set ANTHROPIC_API_KEY or run: gbrain config set anthropic_api_key ...)', + }; + } + return { ok: true }; +} + async function resolveChatProvider(modelStr: string): Promise<{ model: any; recipe: Recipe; modelId: string }> { const { parsed, recipe } = resolveRecipe(modelStr); assertTouchpoint(recipe, 'chat', parsed.modelId, getExtendedModelsForProvider(parsed.providerId)); diff --git a/src/core/conversation-parser/llm-base.ts b/src/core/conversation-parser/llm-base.ts index f119a7f07..78d0af205 100644 --- a/src/core/conversation-parser/llm-base.ts +++ b/src/core/conversation-parser/llm-base.ts @@ -33,7 +33,8 @@ import { createHash } from 'node:crypto'; import { chat as gatewayChat, type ChatOpts, type ChatResult } from '../ai/gateway.ts'; import { resolveRecipe } from '../ai/model-resolver.ts'; import { AIConfigError } from '../ai/errors.ts'; -import { loadConfig } from '../config.ts'; +import { normalizeModelId } from '../model-id.ts'; +import { hasAnthropicKey } from '../ai/anthropic-key.ts'; import type { BrainEngine } from '../engine.ts'; /** @@ -78,35 +79,23 @@ function cacheKey(shape: CallShape, modelId: string, content: string): string { return `${shape}:${modelId}:${hash}`; } -/** - * Anthropic-only key probe. Mirrors `hasAnthropicKey` in - * `src/core/cycle/synthesize.ts:811` + `src/core/think/index.ts`. - * Other providers' key checks happen lazily at `gatewayChat` time and - * surface as AIConfigError, which the caller's try/catch absorbs. - */ -function hasAnthropicKey(): boolean { - if (process.env.ANTHROPIC_API_KEY) return true; - try { - const cfg = loadConfig(); - if (cfg?.anthropic_api_key) return true; - } catch { - // loadConfig may throw on first-run; treat as no key. - } - return false; -} - /** * Construction-time provider probe. Mirrors `makeJudgeClient`'s * "return null on unavailable" semantics. Caller short-circuits on * null without spending any tokens. * + * v0.41.x (#1698): the Anthropic-only key probe is now the shared + * `hasAnthropicKey` from `src/core/ai/anthropic-key.ts` (was a private + * copy here). Other providers' key checks happen lazily at `gatewayChat` + * time and surface as AIConfigError, which the caller's try/catch absorbs. + * * Returns a normalized model id (`provider:model`) when available, or * null when: * - Unknown provider id (resolveRecipe throws AIConfigError). * - Anthropic provider with no key (env or config). */ export function probeLlmAvailability(modelStr: string): string | null { - const normalized = modelStr.includes(':') ? modelStr : `anthropic:${modelStr}`; + const normalized = normalizeModelId(modelStr); let providerId: string; try { const { parsed } = resolveRecipe(normalized); diff --git a/src/core/cycle/auto-think.ts b/src/core/cycle/auto-think.ts index b877ba20a..c0b07260e 100644 --- a/src/core/cycle/auto-think.ts +++ b/src/core/cycle/auto-think.ts @@ -152,16 +152,26 @@ export async function runPhaseAutoThink( client: opts.client, model: modelId, }); + // #1698: an empty synthesis (no LLM available / malformed output / empty-JSON answer) + // must NOT count as complete or advance the cooldown — that is the same silent-success + // the CLI + MCP think paths now guard against. runThink sets synthesisOk=false; the + // empty page is never written, and persistSynthesis returns slug '' + the + // SYNTHESIS_EMPTY_NOT_PERSISTED warning. Mark these 'partial' so `anyComplete` below + // stays false on empty-only runs and the cooldown timestamp isn't advanced (so the + // next cycle retries) — and surface the warning instead of dropping it. + const emptySynthesis = result.synthesisOk === false; + const warnings = [...result.warnings]; let slug: string | undefined; if (config.autoCommit) { const persisted = await persistSynthesis(engine, result); - slug = persisted.slug; + slug = persisted.slug || undefined; // '' = persist-skip signal (#1698) + warnings.push(...persisted.warnings); } results.push({ question: q, - status: 'complete', + status: emptySynthesis ? 'partial' : 'complete', slug, - warnings: result.warnings.length ? result.warnings : undefined, + warnings: warnings.length ? warnings : undefined, }); } catch (e) { results.push({ diff --git a/src/core/cycle/synthesize.ts b/src/core/cycle/synthesize.ts index 773f9d021..1581bc960 100644 --- a/src/core/cycle/synthesize.ts +++ b/src/core/cycle/synthesize.ts @@ -28,10 +28,10 @@ import type Anthropic from '@anthropic-ai/sdk'; import { readFileSync, existsSync, writeFileSync, mkdirSync } from 'node:fs'; -import { chat as gatewayChat, type ChatResult } from '../ai/gateway.ts'; -import { resolveRecipe } from '../ai/model-resolver.ts'; +import { chat as gatewayChat, validateModelId, type ChatResult } from '../ai/gateway.ts'; import { AIConfigError } from '../ai/errors.ts'; -import { loadConfig } from '../config.ts'; +import { normalizeModelId } from '../model-id.ts'; +import { hasAnthropicKey } from '../ai/anthropic-key.ts'; import { join, dirname, isAbsolute, resolve } from 'node:path'; import type { BrainEngine } from '../engine.ts'; import type { PhaseResult, PhaseError } from '../cycle.ts'; @@ -732,25 +732,23 @@ export interface JudgeClient { * time is caught by the verdict loop and surfaced per-transcript. */ export function makeJudgeClient(verdictModel: string): JudgeClient | null { - // Normalize: ensure provider:model shape. resolveModel returns bare - // anthropic ids (e.g. `claude-haiku-4-5-20251001`); gateway.chat needs - // `anthropic:...`. - const modelStr = verdictModel.includes(':') ? verdictModel : `anthropic:${verdictModel}`; + // Normalize: ensure provider:model shape (and slash→colon — #1698). resolveModel + // returns bare anthropic ids (e.g. `claude-haiku-4-5`); gateway.chat needs `anthropic:...`. + const modelStr = normalizeModelId(verdictModel); - // Availability probe: resolveRecipe throws AIConfigError on unknown provider. - let providerId: string; - try { - const { parsed } = resolveRecipe(modelStr); - providerId = parsed.providerId; - } catch (e) { - if (e instanceof AIConfigError) return null; - throw e; - } + // #1698 (C1): id-validity via the shared `validateModelId` core (resolveRecipe + + // assertTouchpoint) — catches unknown provider AND typo'd native model. We do NOT + // use the full `probeChatModel` here: its `isAvailable` layer would reject + // non-Anthropic-no-key providers and an unconfigured gateway, breaking the + // deliberate per-transcript-degrade contract (and test A9). validateModelId reads + // the recipe registry, not gateway _config, so it works pre-configureGateway(). + const v = validateModelId(modelStr); + if (!v.ok) return null; - // Anthropic key probe (legacy behavior preserved). Other providers' - // key checks happen lazily at chat call time and surface as - // AIConfigError, which the verdict loop catches per-transcript. - if (providerId === 'anthropic' && !hasAnthropicKey()) return null; + // Anthropic key probe (legacy behavior preserved verbatim). Other providers' key + // checks happen lazily at chat call time and surface as AIConfigError, which the + // verdict loop catches per-transcript. + if (v.parsed.providerId === 'anthropic' && !hasAnthropicKey()) return null; return { create: async (params): Promise => { @@ -802,23 +800,6 @@ export function makeJudgeClient(verdictModel: string): JudgeClient | null { }; } -/** - * Anthropic key availability probe. Reads BOTH env (`ANTHROPIC_API_KEY`) - * AND the gbrain config file (`anthropic_api_key` set via - * `gbrain config set`) so stdio MCP launches that don't inherit shell env - * keep working (mirrors `hasAnthropicKey()` in src/core/think/index.ts). - */ -function hasAnthropicKey(): boolean { - if (process.env.ANTHROPIC_API_KEY) return true; - try { - const cfg = loadConfig(); - if (cfg?.anthropic_api_key) return true; - } catch { - // loadConfig may throw on first-run installs; treat as no key. - } - return false; -} - interface VerdictResult { worth_processing: boolean; reasons: string[]; diff --git a/src/core/facts/extract.ts b/src/core/facts/extract.ts index 919667bbf..1581ff4f0 100644 --- a/src/core/facts/extract.ts +++ b/src/core/facts/extract.ts @@ -25,6 +25,7 @@ import { chat, embedOne, isAvailable } from '../ai/gateway.ts'; import type { ChatResult } from '../ai/gateway.ts'; import { INJECTION_PATTERNS } from '../think/sanitize.ts'; import { resolveModel } from '../model-config.ts'; +import { normalizeModelId } from '../model-id.ts'; import type { BrainEngine, NewFact, FactKind } from '../engine.ts'; import { normalizeMetricLabel } from './extract-from-fence.ts'; @@ -61,8 +62,9 @@ export async function getFactsExtractionModel(engine?: BrainEngine): Promise 0 ? { trajectoryBlock } : {}), }); + // #1698: true only when an actual synthesis produced a non-empty answer. Set false + // on the not-JSON branch (covers malformed output AND the buildGracefulMessage + // sentinel, which is non-JSON) and on the no-client early return below; the final + // return ANDs it with a non-empty-answer check (catches valid-but-empty JSON). + let synthesisOk = true; let response: ThinkResponse; if (opts.stubResponse) { response = opts.stubResponse; @@ -401,7 +437,7 @@ export async function runThink( // That bypassed gateway config (gbrain config set anthropic_api_key) // because the Anthropic SDK only reads process.env.ANTHROPIC_API_KEY. // Closes #952 (think over MCP returns "no LLM available"). - const client = opts.client ?? await tryBuildGatewayClient(modelUsed); + const client = opts.client ?? await tryBuildGatewayClient(modelUsed, { explicitModel: opts.modelExplicit }); if (!client) { warnings.push('NO_ANTHROPIC_API_KEY'); // Degrade gracefully: return the gather without synthesis. Better than throwing. @@ -416,6 +452,7 @@ export async function runThink( modelUsed, rounds: 0, warnings, + synthesisOk: false, // #1698: no LLM ran — never persist this diagnostics: { pagesFromHybrid: gather.diagnostics.pagesFromHybrid, takesFromKeyword: gather.diagnostics.takesFromKeyword, @@ -435,6 +472,7 @@ export async function runThink( const parsed = tryParseJSON(text); if (!parsed || typeof parsed !== 'object') { warnings.push('LLM_OUTPUT_NOT_JSON'); + synthesisOk = false; // #1698: malformed output (and the non-JSON graceful sentinel) response = { answer: text, citations: [], gaps: [] }; } else { const r = parsed as Partial; @@ -470,6 +508,9 @@ export async function runThink( modelUsed, rounds: 1, warnings, + // #1698: persistable only when a real synthesis produced a non-empty answer. + // ANDs the not-JSON/sentinel flag with a content check (catches valid-but-empty JSON). + synthesisOk: synthesisOk && response.answer.trim().length > 0, diagnostics: { pagesFromHybrid: gather.diagnostics.pagesFromHybrid, takesFromKeyword: gather.diagnostics.takesFromKeyword, @@ -487,6 +528,14 @@ export async function persistSynthesis( engine: BrainEngine, result: ThinkResult, ): Promise<{ slug: string; evidenceInserted: number; warnings: string[] }> { + // #1698: never persist an empty synthesis. Returned signal (NOT a throw, F3) so + // the MCP `think` op can return the gather result + warning instead of a bare error + // envelope; the CLI keys off this warning to exit non-zero. Guard on `=== false` so + // pre-existing/test ThinkResult literals without the field still persist (back-compat). + if (result.synthesisOk === false) { + return { slug: '', evidenceInserted: 0, warnings: ['SYNTHESIS_EMPTY_NOT_PERSISTED'] }; + } + const today = new Date().toISOString().slice(0, 10); const slugSafe = result.question .toLowerCase() @@ -576,31 +625,32 @@ async function readThinkTrajectoryEnabled(engine: BrainEngine): Promise * touchpoint not supported, etc.). Caller falls through to the graceful * "no LLM available" stub on null. */ -async function tryBuildGatewayClient(modelUsed: string): Promise { - // Normalize: ensure provider:model shape. resolveModel returns bare - // anthropic ids (e.g. `claude-opus-4-7`); gateway.chat needs `anthropic:...`. - const modelStr = modelUsed.includes(':') ? modelUsed : `anthropic:${modelUsed}`; +async function tryBuildGatewayClient( + modelUsed: string, + opts: { explicitModel?: boolean } = {}, +): Promise { + // Normalize: ensure provider:model shape (and slash→colon — #1698). resolveModel + // returns bare anthropic ids (`claude-opus-4-7`); gateway.chat needs `anthropic:...`. + const modelStr = normalizeModelId(modelUsed); - // Availability probe: resolveRecipe throws on unknown provider; assertTouchpoint - // throws if the resolved recipe doesn't support chat. Both are AIConfigError. - let providerId: string; - try { - const { parsed } = resolveRecipe(modelStr); - providerId = parsed.providerId; - } catch (e) { - if (e instanceof AIConfigError) return null; - throw e; + // #1698: ONE shared probe (resolveRecipe + assertTouchpoint + isAvailable). + // assertTouchpoint catches typo'd native models; isAvailable catches missing keys. + // For an EXPLICIT model the user typed, an unusable model is a HARD ERROR (throw) + // — never silently degrade to the no-LLM stub. For the default/configured-model + // path, return null so the caller falls through to the graceful "no LLM" stub + // (preserves the documented no-key gather-only behavior). + const probe = probeChatModel(modelStr); + if (!probe.ok) { + if (opts.explicitModel) { + throw new Error( + `think: --model "${modelUsed}" is not usable (${probe.reason}): ${probe.detail}. ` + + `Refusing to run synthesis with no model — fix the model id or omit --model.` + + (probe.fix ? ` Fix: ${probe.fix}` : ''), + ); + } + return null; } - // API-key availability probe. The gateway lazily checks keys inside - // instantiateChat at first .chat() call and throws AIConfigError on miss. - // Pre-checking here preserves the legacy "NO_ANTHROPIC_API_KEY" warning - // signal AND avoids paying for a wasted gateway call when the user clearly - // has no key configured. Reads BOTH the gbrain config file (`anthropic_api_key` - // set via `gbrain config set`) AND the process env, matching gateway's - // own loadConfig precedence. - if (providerId === 'anthropic' && !hasAnthropicKey()) return null; - return { create: async (params): Promise => { // Build ChatOpts from Anthropic.MessageCreateParamsNonStreaming. @@ -623,10 +673,13 @@ async function tryBuildGatewayClient(modelUsed: string): Promise): string { + const home = mkdtempSync(join(tmpdir(), 'gbrain-akey-')); + tmpDirs.push(home); + if (withConfig) { + const dir = join(home, '.gbrain'); + mkdirSync(dir, { recursive: true }); + writeFileSync(join(dir, 'config.json'), JSON.stringify(withConfig), 'utf8'); + } + return home; +} + +afterEach(() => { + while (tmpDirs.length) { + const d = tmpDirs.pop()!; + try { rmSync(d, { recursive: true, force: true }); } catch { /* best effort */ } + } +}); + +describe('hasAnthropicKey', () => { + test('env ANTHROPIC_API_KEY set → true (no config read needed)', async () => { + const home = freshHome(); // empty home so config can't accidentally satisfy it + await withEnv( + { ANTHROPIC_API_KEY: 'sk-test', GBRAIN_HOME: home, DATABASE_URL: undefined, GBRAIN_DATABASE_URL: undefined }, + async () => { + expect(hasAnthropicKey()).toBe(true); + }, + ); + }); + + test('gbrain config anthropic_api_key set (no env) → true', async () => { + const home = freshHome({ anthropic_api_key: 'sk-from-config' }); + await withEnv( + { ANTHROPIC_API_KEY: undefined, GBRAIN_HOME: home, DATABASE_URL: undefined, GBRAIN_DATABASE_URL: undefined }, + async () => { + expect(hasAnthropicKey()).toBe(true); + }, + ); + }); + + test('neither env nor config → false', async () => { + const home = freshHome(); // no config.json written + await withEnv( + { ANTHROPIC_API_KEY: undefined, GBRAIN_HOME: home, DATABASE_URL: undefined, GBRAIN_DATABASE_URL: undefined }, + async () => { + expect(hasAnthropicKey()).toBe(false); + }, + ); + }); +}); diff --git a/test/ai/gateway-probe-chat-model.test.ts b/test/ai/gateway-probe-chat-model.test.ts new file mode 100644 index 000000000..88db0e5c9 --- /dev/null +++ b/test/ai/gateway-probe-chat-model.test.ts @@ -0,0 +1,105 @@ +/** + * #1698 — validateModelId (C1 id-validity core) + probeChatModel (= validity + key). + * + * validateModelId reads the recipe REGISTRY (not gateway _config), so it works without + * configureGateway — that's the property makeJudgeClient + tryBuildGatewayClient rely on + * (C1 #6). probeChatModel adds the key layer via hasAnthropicKey (env OR gbrain config + * file — also gateway-config-independent). Non-Anthropic providers pass the probe (lazy + * key check deferred to gateway.chat). + * + * Hermetic: key-sensitive cases isolate env + GBRAIN_HOME via withEnv (R1) so the dev + * machine's real ~/.gbrain/config.json never leaks in. + */ + +import { describe, test, expect, afterEach } from 'bun:test'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { validateModelId, probeChatModel } from '../../src/core/ai/gateway.ts'; +import { normalizeModelId } from '../../src/core/model-id.ts'; +import { withEnv } from '../helpers/with-env.ts'; + +const REAL = 'anthropic:claude-sonnet-4-6'; + +const tmpDirs: string[] = []; +function emptyHome(): string { + const d = mkdtempSync(join(tmpdir(), 'gbrain-probe-')); + tmpDirs.push(d); + return d; +} +afterEach(() => { + while (tmpDirs.length) { + try { rmSync(tmpDirs.pop()!, { recursive: true, force: true }); } catch { /* best effort */ } + } +}); + +// No-key env: ANTHROPIC_API_KEY unset + GBRAIN_HOME pointed at an empty dir so the +// config-file branch of hasAnthropicKey finds nothing. +const noKeyEnv = () => ({ ANTHROPIC_API_KEY: undefined, GBRAIN_HOME: emptyHome() }); +const withKeyEnv = () => ({ ANTHROPIC_API_KEY: 'sk-test', GBRAIN_HOME: emptyHome() }); + +describe('validateModelId (#1698 C1 core)', () => { + test('ok for a real model id, returns parsed + recipe', () => { + const v = validateModelId(REAL); + expect(v.ok).toBe(true); + if (v.ok) { + expect(v.parsed.providerId).toBe('anthropic'); + expect(v.recipe).toBeDefined(); + } + }); + + test('works WITHOUT configureGateway (reads registry, not _config) — C1 #6 guard', () => { + // No gateway configured in this process; validateModelId must still resolve. + const v = validateModelId(REAL); + expect(v.ok).toBe(true); + }); + + test('unknown_provider when resolveRecipe throws', () => { + const v = validateModelId('bogusprovider:whatever'); + expect(v.ok).toBe(false); + if (!v.ok) expect(v.reason).toBe('unknown_provider'); + }); + + test('unknown_model for a typo native model', () => { + const v = validateModelId('anthropic:claude-bogus-9'); + expect(v.ok).toBe(false); + if (!v.ok) expect(v.reason).toBe('unknown_model'); + }); +}); + +describe('probeChatModel (#1698 = validity + key, config-independent)', () => { + test('unavailable: anthropic + no key', async () => { + await withEnv(noKeyEnv(), async () => { + const p = probeChatModel(REAL); + expect(p.ok).toBe(false); + if (!p.ok) expect(p.reason).toBe('unavailable'); + }); + }); + + test('ok: anthropic + key set (no configureGateway needed)', async () => { + await withEnv(withKeyEnv(), async () => { + expect(probeChatModel(REAL).ok).toBe(true); + }); + }); + + test('unknown_provider / unknown_model classify regardless of key (validity runs first)', async () => { + await withEnv(withKeyEnv(), async () => { + expect(probeChatModel('bogusprovider:x')).toMatchObject({ ok: false, reason: 'unknown_provider' }); + expect(probeChatModel('anthropic:claude-bogus-9')).toMatchObject({ ok: false, reason: 'unknown_model' }); + }); + }); + + test('non-anthropic provider passes the probe even with no key (lazy key check)', async () => { + // deepseek is a registered recipe; its key check is deferred to gateway.chat() + // (the per-transcript-degrade contract — A9). probe should be ok here. + await withEnv(noKeyEnv(), async () => { + expect(probeChatModel('deepseek:deepseek-chat').ok).toBe(true); + }); + }); + + test('reported repro: slash form normalizes then probes ok (with key)', async () => { + await withEnv(withKeyEnv(), async () => { + expect(probeChatModel(normalizeModelId('anthropic/claude-sonnet-4-6')).ok).toBe(true); + }); + }); +}); diff --git a/test/auto-think-phase.test.ts b/test/auto-think-phase.test.ts index 79e726c86..f46462c2e 100644 --- a/test/auto-think-phase.test.ts +++ b/test/auto-think-phase.test.ts @@ -94,6 +94,31 @@ describe('runPhaseAutoThink', () => { await engine.setConfig('dream.auto_think.enabled', 'false'); }); + // #1698 (codex #5): an empty synthesis must NOT count as complete or advance the + // cooldown — otherwise auto-think silently reports success and suppresses retry until + // the cooldown expires. An empty-answer stub drives runThink's synthesisOk=false path. + test('empty synthesis → partial, 0 synthesized, cooldown NOT advanced', async () => { + await engine.setConfig('dream.auto_think.enabled', 'true'); + await engine.setConfig('dream.auto_think.questions', JSON.stringify(['Q-empty'])); + await engine.setConfig('dream.auto_think.max_per_cycle', '1'); + await engine.setConfig('dream.auto_think.budget', '10.0'); + await engine.setConfig('dream.auto_think.auto_commit', 'false'); + await engine.setConfig('dream.auto_think.cooldown_days', '30'); + await engine.setConfig('dream.auto_think.last_completion_ts', ''); + const r = await runPhaseAutoThink(engine, { + dryRun: false, + client: makeStubClient(''), // empty answer → synthesisOk=false + auditPath: join(tmpDir, 'b-empty.jsonl'), + }); + expect(r.status).toBe('partial'); + expect((r.totals as { synthesized?: number }).synthesized).toBe(0); + // Cooldown must stay empty so the next cycle retries (no silent success). + const ts = await engine.getConfig('dream.auto_think.last_completion_ts'); + expect(ts ?? '').toBe(''); + await engine.setConfig('dream.auto_think.enabled', 'false'); + await engine.setConfig('dream.auto_think.cooldown_days', '0'); + }); + test('cooldown skips next run', async () => { await engine.setConfig('dream.auto_think.enabled', 'true'); await engine.setConfig('dream.auto_think.questions', JSON.stringify(['Q1'])); diff --git a/test/cycle/synthesize-gateway-adapter.test.ts b/test/cycle/synthesize-gateway-adapter.test.ts index 631310edc..2a5d303dd 100644 --- a/test/cycle/synthesize-gateway-adapter.test.ts +++ b/test/cycle/synthesize-gateway-adapter.test.ts @@ -78,12 +78,24 @@ describe('makeJudgeClient — construction-time provider probe', () => { // The deepseek recipe declares DEEPSEEK_API_KEY in auth_env.required; // makeJudgeClient delegates that probe to gateway.chat at call time // (where it would throw AIConfigError, caught per-transcript by the loop). + // #1698 C1: this MUST stay green — validateModelId (id-validity only) does NOT + // reject a non-anthropic provider for a missing key (no isAvailable here). await withEnv({ DEEPSEEK_API_KEY: undefined }, async () => { const judge = makeJudgeClient('deepseek:deepseek-chat'); expect(judge).not.toBeNull(); expect(typeof judge?.create).toBe('function'); }); }); + + test('A8b (#1698): typo native model → null at construction (validateModelId unknown_model)', async () => { + // Pre-#1698 makeJudgeClient only checked the provider (resolveRecipe) and would + // have returned a client that failed at call time. Now the shared validateModelId + // core runs assertTouchpoint, so a typo'd native model is rejected up front. + await withEnv({ ANTHROPIC_API_KEY: 'sk-test-A8b' }, async () => { + const judge = makeJudgeClient('anthropic:claude-bogus-9'); + expect(judge).toBeNull(); + }); + }); }); describe('JudgeClient.create — gateway routing + shape adapter', () => { diff --git a/test/model-id.test.ts b/test/model-id.test.ts index 37d9dd8f5..092dd3881 100644 --- a/test/model-id.test.ts +++ b/test/model-id.test.ts @@ -13,7 +13,12 @@ */ import { describe, test, expect } from 'bun:test'; -import { splitProviderModelId } from '../src/core/model-id.ts'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { splitProviderModelId, normalizeModelId } from '../src/core/model-id.ts'; + +const REPO_ROOT = join(import.meta.dir, '..'); +const readSrc = (rel: string) => readFileSync(join(REPO_ROOT, rel), 'utf8'); describe('splitProviderModelId', () => { describe('happy paths', () => { @@ -138,3 +143,79 @@ describe('splitProviderModelId', () => { }); }); }); + +describe('normalizeModelId (#1698)', () => { + test('slash form → colon — THE REPORTED BUG', () => { + // Pre-fix: the colon-only inline check left this as the malformed + // `anthropic:anthropic/claude-sonnet-4-6` and silently degraded to no-LLM. + expect(normalizeModelId('anthropic/claude-sonnet-4-6')).toBe('anthropic:claude-sonnet-4-6'); + }); + + test('bare → anthropic: default', () => { + expect(normalizeModelId('claude-sonnet-4-6')).toBe('anthropic:claude-sonnet-4-6'); + }); + + test('colon identity (already provider:model)', () => { + expect(normalizeModelId('anthropic:claude-sonnet-4-6')).toBe('anthropic:claude-sonnet-4-6'); + }); + + test('openrouter nested form preserved (inner slash kept)', () => { + expect(normalizeModelId('openrouter:anthropic/claude-sonnet-4.6')).toBe('openrouter:anthropic/claude-sonnet-4.6'); + }); + + test('non-anthropic bare with custom default provider', () => { + expect(normalizeModelId('gpt-5', 'openai')).toBe('openai:gpt-5'); + }); + + test('empty / whitespace returns input as-is (downstream throws loudly)', () => { + expect(normalizeModelId('')).toBe(''); + expect(normalizeModelId(' ')).toBe(' '); + }); + + // #1698 (codex #2): a malformed leading separator yields an EMPTY-STRING provider from + // splitProviderModelId. It must be returned unchanged (so resolveRecipe throws loudly), + // NOT silently coerced to the default provider — otherwise `:claude-sonnet-4-6` would run + // as `anthropic:claude-sonnet-4-6`, masking a typo as a valid Anthropic model. + test('leading-colon malformed id returns input as-is (NOT coerced to anthropic)', () => { + expect(normalizeModelId(':claude-sonnet-4-6')).toBe(':claude-sonnet-4-6'); + }); + test('leading-slash malformed id returns input as-is (NOT coerced to anthropic)', () => { + expect(normalizeModelId('/claude-sonnet-4-6')).toBe('/claude-sonnet-4-6'); + }); + test('leading separator is not coerced even with a custom default provider', () => { + expect(normalizeModelId(':gpt-5', 'openai')).toBe(':gpt-5'); + }); +}); + +describe('normalize-everywhere structural guards (#1698)', () => { + // Positive: every chat-adapter site references the shared normalizer. + const SITES = [ + 'src/core/think/index.ts', + 'src/core/cycle/synthesize.ts', + 'src/core/conversation-parser/llm-base.ts', + 'src/core/facts/extract.ts', + ]; + // The exact colon-only inline that #1698 fixed: `X.includes(':') ? X : `anthropic:`. + const COLON_ONLY_INLINE = /\.includes\(['"]:['"]\)\s*\?\s*[\w.]+\s*:\s*`anthropic:/; + + for (const site of SITES) { + test(`${site} uses normalizeModelId and not the colon-only inline`, () => { + const src = readSrc(site); + expect(src).toContain('normalizeModelId'); + expect(COLON_ONLY_INLINE.test(src)).toBe(false); + }); + } + + test('hasAnthropicKey is defined exactly once (the shared helper), not re-copied', () => { + const FORMER_COPIES = [ + 'src/core/think/index.ts', + 'src/core/cycle/synthesize.ts', + 'src/core/conversation-parser/llm-base.ts', + ]; + for (const f of FORMER_COPIES) { + expect(readSrc(f)).not.toMatch(/function hasAnthropicKey\s*\(/); + } + // The one canonical definition lives here. + expect(readSrc('src/core/ai/anthropic-key.ts')).toMatch(/export function hasAnthropicKey\s*\(/); + }); +}); diff --git a/test/think-gateway-adapter.test.ts b/test/think-gateway-adapter.test.ts index c97ff1f9c..d9788af04 100644 --- a/test/think-gateway-adapter.test.ts +++ b/test/think-gateway-adapter.test.ts @@ -17,6 +17,7 @@ import { describe, test, expect } from 'bun:test'; import { __thinkAdapter } from '../src/core/think/index.ts'; +import { resetGateway } from '../src/core/ai/gateway.ts'; import { withEnv } from './helpers/with-env.ts'; describe('think gateway adapter — response shape conversion', () => { @@ -91,6 +92,100 @@ describe('think gateway adapter — model-id normalization', () => { }); }); +describe('think gateway adapter — #1698 slash form + explicit-model fork', () => { + test('tryBuildGatewayClient accepts SLASH form (anthropic/claude-...) — the reported bug', async () => { + // Pre-fix the colon-only inline produced `anthropic:anthropic/claude-sonnet-4-6` + // and the client silently degraded. normalizeModelId fixes it → builds cleanly. + await withEnv({ ANTHROPIC_API_KEY: 'sk-test-fake' }, async () => { + const client = await __thinkAdapter.tryBuildGatewayClient('anthropic/claude-sonnet-4-6'); + expect(client).not.toBeNull(); + }); + }); + + test('explicit unresolvable model THROWS (does not degrade to null)', async () => { + await expect( + __thinkAdapter.tryBuildGatewayClient('bogusprovider:foo-1', { explicitModel: true }), + ).rejects.toThrow(/not usable.*unknown_provider/); + }); + + test('explicit typo native model THROWS (unknown_model)', async () => { + await expect( + __thinkAdapter.tryBuildGatewayClient('anthropic:claude-bogus-9', { explicitModel: true }), + ).rejects.toThrow(/not usable.*unknown_model/); + }); + + test('explicit anthropic model with no key THROWS (unavailable)', async () => { + await withEnv({ ANTHROPIC_API_KEY: undefined }, async () => { + await expect( + __thinkAdapter.tryBuildGatewayClient('anthropic:claude-sonnet-4-6', { explicitModel: true }), + ).rejects.toThrow(/not usable.*unavailable/); + }); + }); + + test('NON-explicit unresolvable model returns null (graceful, unchanged)', async () => { + const client = await __thinkAdapter.tryBuildGatewayClient('bogusprovider:foo-1', { explicitModel: false }); + expect(client).toBeNull(); + }); + + test('create-callback fork: explicit rethrows AIConfigError; non-explicit returns the sentinel', async () => { + await withEnv({ ANTHROPIC_API_KEY: 'sk-test-fake' }, async () => { + // Build valid clients (key present → probe ok) but leave the gateway UNCONFIGURED + // so gateway.chat() throws AIConfigError (requireConfig) at create() time. + resetGateway(); + const params: any = { + model: 'anthropic:claude-sonnet-4-6', + max_tokens: 16, + system: 'sys', + messages: [{ role: 'user', content: 'hi' }], + }; + + const explicitClient = await __thinkAdapter.tryBuildGatewayClient( + 'anthropic:claude-sonnet-4-6', { explicitModel: true }, + ); + expect(explicitClient).not.toBeNull(); + await expect(explicitClient!.create(params)).rejects.toThrow(); + + const gracefulClient = await __thinkAdapter.tryBuildGatewayClient( + 'anthropic:claude-sonnet-4-6', { explicitModel: false }, + ); + expect(gracefulClient).not.toBeNull(); + const msg = await gracefulClient!.create(params); + const text = msg.content.find((b: any) => b.type === 'text'); + expect(text && 'text' in text ? text.text : '').toContain('no LLM available'); + }); + }); + + // D1 BACKSTOP (codex #1, accepted-as-is): probeChatModel only PRE-checks the Anthropic + // key, so an explicit NON-anthropic model (deepseek/openai/...) passes the early gate and + // BUILDS a client even with no provider key — its key is checked lazily at chat time. The + // create-callback rethrow is then the ONLY thing standing between "explicit unusable model" + // and a silent degrade. This test locks that backstop into a contract: the client builds + // (proving the deviation), and create() HARD-ERRORS (proving it never degrades to the + // 'no LLM available' stub). A future refactor that turns this into a graceful path fails here. + test('D1 backstop: explicit non-anthropic model, no key → BUILDS then create() THROWS (never a stub)', async () => { + await withEnv( + { ANTHROPIC_API_KEY: undefined, DEEPSEEK_API_KEY: undefined, OPENAI_API_KEY: undefined }, + async () => { + resetGateway(); // unconfigured → gateway.chat() throws AIConfigError at create() + // deepseek:deepseek-chat passes validateModelId (real recipe + chat touchpoint) — the + // A9 non-anthropic model. probeChatModel returns ok (no anthropic key check) → builds. + const client = await __thinkAdapter.tryBuildGatewayClient( + 'deepseek:deepseek-chat', { explicitModel: true }, + ); + expect(client).not.toBeNull(); // proves the early gate did NOT pre-reject non-anthropic + const params: any = { + model: 'deepseek:deepseek-chat', + max_tokens: 16, + system: 'sys', + messages: [{ role: 'user', content: 'hi' }], + }; + // The backstop fires: explicit → AIConfigError rethrown, NOT the graceful sentinel. + await expect(client!.create(params)).rejects.toThrow(); + }, + ); + }); +}); + describe('think gateway adapter — graceful fallback shape', () => { test('buildGracefulMessage produces a parseable Anthropic.Message-shaped object', () => { const m = __thinkAdapter.buildGracefulMessage('anthropic:claude-opus-4-7'); diff --git a/test/think-pipeline.serial.test.ts b/test/think-pipeline.serial.test.ts index 080097021..cf8be936b 100644 --- a/test/think-pipeline.serial.test.ts +++ b/test/think-pipeline.serial.test.ts @@ -1,5 +1,6 @@ import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { operationsByName } from '../src/core/operations.ts'; import { runThink, persistSynthesis, type ThinkLLMClient } from '../src/core/think/index.ts'; import { sanitizeTakeForPrompt, renderTakesBlock } from '../src/core/think/sanitize.ts'; import { resolveCitations, parseInlineCitations, normalizeStructuredCitations } from '../src/core/think/cite-render.ts'; @@ -257,3 +258,130 @@ describe('runThink (with stub client)', () => { expect(Number(ev[0]?.count)).toBe(1); }); }); + +// #1698 — fail loud, never persist empty. +function stubClientFromText(text: string): ThinkLLMClient { + return { + create: async () => ({ + id: 'msg_1698', type: 'message', role: 'assistant', model: 'stub', + stop_reason: 'end_turn', stop_sequence: null, + usage: { input_tokens: 1, output_tokens: 1, cache_creation_input_tokens: 0, cache_read_input_tokens: 0, server_tool_use: null, service_tier: null }, + content: [{ type: 'text', text }], + }), + }; +} + +describe('runThink — #1698 explicit-model hard error', () => { + test('explicit unresolvable --model THROWS before gather (unknown_provider)', async () => { + await expect( + runThink(engine, { question: 'x', model: 'bogusprovider:foo', modelExplicit: true }), + ).rejects.toThrow(/not usable.*unknown_provider/); + }); + + test('explicit typo native --model THROWS (unknown_model)', async () => { + await expect( + runThink(engine, { question: 'x', model: 'anthropic:claude-bogus-9', modelExplicit: true }), + ).rejects.toThrow(/not usable.*unknown_model/); + }); + + test('NON-explicit bad model does NOT throw — graceful degrade (no modelExplicit)', async () => { + const origKey = process.env.ANTHROPIC_API_KEY; + delete process.env.ANTHROPIC_API_KEY; + try { + // model present but modelExplicit unset → early gate skipped; builder returns null. + const result = await runThink(engine, { question: 'nonexplicit bad', model: 'bogusprovider:foo' }); + expect(result.warnings).toContain('NO_ANTHROPIC_API_KEY'); + expect(result.synthesisOk).toBe(false); + } finally { + if (origKey) process.env.ANTHROPIC_API_KEY = origKey; + } + }); +}); + +describe('runThink + persistSynthesis — #1698 never persist empty', () => { + test('empty-but-valid-JSON answer → synthesisOk false → persist-skip signal', async () => { + const result = await runThink(engine, { + question: 'empty answer test', + client: stubClientFromText(JSON.stringify({ answer: '', citations: [], gaps: [] })), + }); + expect(result.synthesisOk).toBe(false); + + const saved = await persistSynthesis(engine, result); + expect(saved.slug).toBe(''); + expect(saved.warnings).toContain('SYNTHESIS_EMPTY_NOT_PERSISTED'); + }); + + test('malformed (not-JSON) output → synthesisOk false → persist-skip', async () => { + const result = await runThink(engine, { + question: 'malformed persist test', + client: stubClientFromText('not json at all, just prose'), + }); + expect(result.warnings).toContain('LLM_OUTPUT_NOT_JSON'); + expect(result.synthesisOk).toBe(false); + const saved = await persistSynthesis(engine, result); + expect(saved.slug).toBe(''); + expect(saved.warnings).toContain('SYNTHESIS_EMPTY_NOT_PERSISTED'); + }); + + test('valid non-empty synthesis → synthesisOk true → persists', async () => { + const result = await runThink(engine, { + question: 'nonempty persist test', + client: stubClientFromText(JSON.stringify({ answer: 'A real answer.', citations: [], gaps: [] })), + }); + expect(result.synthesisOk).toBe(true); + const saved = await persistSynthesis(engine, result); + expect(saved.slug).toContain('synthesis/nonempty-persist-test'); + }); + + test('stubResponse with empty answer → synthesisOk false; non-empty → true', async () => { + const empty = await runThink(engine, { + question: 'stub empty', stubResponse: { answer: '', citations: [], gaps: [] }, + }); + expect(empty.synthesisOk).toBe(false); + + const full = await runThink(engine, { + question: 'stub full', stubResponse: { answer: 'has content', citations: [], gaps: [] }, + }); + expect(full.synthesisOk).toBe(true); + }); + + test('pre-existing ThinkResult literal without synthesisOk still persists (back-compat)', async () => { + const legacy: any = { + question: 'legacy backcompat', answer: 'legacy body', citations: [], gaps: [], + pagesGathered: 0, takesGathered: 0, graphHits: 0, modelUsed: 'stub', rounds: 1, warnings: [], + diagnostics: { pagesFromHybrid: 0, takesFromKeyword: 0, takesFromVector: 0, graphHits: 0 }, + // NOTE: no synthesisOk field + }; + const saved = await persistSynthesis(engine, legacy); + expect(saved.slug).toContain('synthesis/legacy-backcompat'); + }); +}); + +describe('think MCP op — #1698 C3 + #10', () => { + const baseCtx = (remote: boolean) => ({ + engine, config: {} as any, dryRun: false, remote, + logger: { info() {}, warn() {}, error() {}, debug() {} } as any, + }); + + test('C3: remote caller with explicit bad model → op throws (modelExplicit wired)', async () => { + const op = operationsByName['think']; + expect(op).toBeDefined(); + await expect( + op.handler(baseCtx(true) as any, { question: 'q', model: 'bogusprovider:foo' }), + ).rejects.toThrow(/not usable.*unknown_provider/); + }); + + test('#10: local save with no synthesis → saved_slug is null, not "" + warning surfaced', async () => { + const op = operationsByName['think']; + const origKey = process.env.ANTHROPIC_API_KEY; + delete process.env.ANTHROPIC_API_KEY; + try { + // Local (remote:false), save:true, no key → graceful stub → persist-skip. + const res: any = await op.handler(baseCtx(false) as any, { question: 'op empty save test', save: true }); + expect(res.saved_slug).toBeNull(); + expect(res.warnings).toContain('SYNTHESIS_EMPTY_NOT_PERSISTED'); + } finally { + if (origKey) process.env.ANTHROPIC_API_KEY = origKey; + } + }); +});