v0.42.4.0 fix: think --model fails loud — slash-form ids + never persist empty synthesis (#1698) (#1736)

* 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-27 22:15:33 +00:00 · 2026-06-01 21:54:13 -07:00
parent d9eadfec13
commit 5911072aec
21 changed files with 934 additions and 122 deletions
@@ -0,0 +1,64 @@
+/**
+ * #1698 — shared `hasAnthropicKey` (consolidated from 3 private copies).
+ *
+ * Hermetic: every case isolates env + GBRAIN_HOME via `withEnv` (R1) so the
+ * dev machine's real ~/.gbrain/config.json never leaks into the "neither" case.
+ */
+
+import { describe, test, expect, afterEach } from 'bun:test';
+import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { withEnv } from '../helpers/with-env.ts';
+import { hasAnthropicKey } from '../../src/core/ai/anthropic-key.ts';
+
+const tmpDirs: string[] = [];
+function freshHome(withConfig?: Record<string, unknown>): 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);
+      },
+    );
+  });
+});
@@ -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);
+    });
+  });
+});
@@ -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']));
@@ -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', () => {
@@ -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*\(/);
+  });
+});
@@ -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');
@@ -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;
+    }
+  });
+});