Phase 2 — Server hexagonal restructure

docs/plans/refactor/phase-2.md

@@ -4,6 +4,29 @@ Scope: `server/` only. Assumes P1 delivered `server/ + shared/ + client/` with `
 
 Consolidation deltas that modify this plan: S1 reduces to `shared/domain/provider.ts` consolidation only (P1 owns the schemas move); the schemas re-export shim does not exist so S12's shim deletion is moot; S1+S2+S3 land as an early `phase-2-foundations` PR merged to master so P3 can rebase; service unit tests are written red before each extraction (TDD delta 9).
 
+## 0. Reconciled during execution
+
+Facts found in the code that contradict what this plan was written against. The plan below is corrected in place; this list records what moved and why, so the diff between plan and reality is never silent.
+
+1. **Test baseline.** Section 9 was written against a post-P0 count of 232. Phase 1 landed more tests, so the real baseline on `master` before this branch is 315. Stage S4 took it to 479, and the stage 2 items below took it to 481. The phase gate is measured against 481 plus the new adapter and service tests, not 232.
+2. **`books.ts` size.** Read at 2,244 lines when this plan was written. The foundations PR moved constants, HTTP helpers, and the error handler out, so it is 2,176 lines and 47 route registrations at the point S6b splits it. Every line reference in sections 2 and 3 is therefore approximate and must be re-derived from the file rather than trusted.
+3. **Sanctioned change 1 has no frozen assertion to edit.** Risk 4 assumed the eight MCP authoring routes had P0 characterization assertions locking their current 500. They do not. Those eight routes have zero test coverage of any kind, verified by grepping every `*.test.ts` under `server/` for their paths. So the 500 to 400 flip edits nothing. Instead this phase must ADD tests asserting the new 400, because an unsanctioned regression there would otherwise be invisible.
+4. **DiagramRenderer failure contract.** The port shipped in S4 said a failed chart yields `''`. The two real implementations disagreed: kroki pushed `''`, Electron pushed an escaped mermaid code block holding the chart source. The architect standardized on the Electron behaviour, so a failed chart now yields `diagramSourceFallback(source)` and never `''`. The kroki adapter adopts it, which is sanctioned change 5, visible only in dev web mode.
+5. **AudioAssembly cover embedding.** `ConcatToM4bRequest` gained an optional `coverPath`. Retrying the stitch without a cover when embedding fails stays adapter-internal resilience rather than a caller concern.
+6. **Port count.** Thirteen ports shipped in S4, not the eleven plus two extras this plan estimated: TextGeneration, KeyVault, ImageGeneration, BookRepository, ArtifactStore, SpeechSynthesis, AudioAssembly, DiagramRenderer, EpubImport, EpubExport, BackgroundTasks, Clock, OsFileManager.
+7. **`services/mermaid-renderer.ts` is confirmed dead.** Only its own test imports it. Every other `mermaid-renderer` hit in the tree is a log prefix string or a temp file name, not an import.
+8. **Two ports were widened while their real adapters were built.** `AudioAssembly.concatToM4b` gained `bookTitle` alongside the architect's `coverPath`, because the real M4B stitch tags the file with the book's title as container metadata and as the FFMETADATA1 title line, and `AudiobookChapterEntry` carries only per-chapter titles. Without it the adapter could not reproduce today's tagging.
+9. **`generation-manager.ts` and the standalone `generate-quiz.ts` were not duplicates.** The generation-manager copy appended `MARKDOWN_FORMATTING_RULES` to the quiz prompt and the standalone one did not. Reconciling them onto one implementation therefore needed an explicit `includeFormattingRules` flag rather than a straight merge, so each caller keeps the prompt it has always sent.
+10. **The route split produced 46 route registrations, not 47.** Counted by grep across the original file, and the eight target modules account for all 46.
+11. **Three `server/domain/` modules were not pure.** `profile-context.ts`, `chapter-range.ts`, and `skill-progress-report.ts` all reached the filesystem through the `book-store` singleton. Each was split into a pure core plus a port-taking wrapper in `server/services/`, which is what section 6 intended by "server-only pure helpers" but the mechanical split alone did not achieve.
+12. **The singleton-to-factory risk was handled with deliberate temporary shims.** Rather than converting call sites and factories in one step across five parallel slices, each singleton became a thin shim over its new adapter in a single atomic commit, callers migrated onto ports slice by slice, and the shims were deleted only once nothing imported them. No half-converted state ever existed, so the production data directory was never at risk.
+
+## 0b. Final shape
+
+13 ports, 15 adapters, 52 services, 8 domain modules, 16 route modules, none over 200 lines. `createPorts(overrides)` builds all thirteen, `createSharedServices(ports)` builds the in-memory state two route modules must share, and `buildServer(overrides)` threads both into every route plugin as plugin options.
+
+`server/composition-root.ts` is the only module that names a concrete adapter, with two documented exceptions. `server/services/recover-from-crash.ts` imports `cleanTmpArtifacts` from the filesystem book repository, because sweeping the stray `.tmp` files left by an interrupted write has no equivalent on the `BookRepository` port, which that function's own doc comment explains. Outside `server/`, `electron/main.ts` names the Electron diagram renderer, because Electron is its own composition point and hands the renderer to `startServer` as a port override.
+
 ## 1. Port catalog
 
 Location: `server/ports/<port>.ts` — interface + fake + contract test per port. All signatures derived from actual call sites (line refs are current `server/`).
@@ -77,7 +100,7 @@ get(p: ProviderId): string|null;  set(p, key): void;  remove(p): void;  has(p):
 status(): Record<ProviderId, boolean>
 
 // ports/diagram-renderer.ts
-render(charts: string[]): Promise<string[]>     // markup per chart; '' = failed
+render(charts: string[]): Promise<string[]>     // markup per chart; failed = diagramSourceFallback(source), never ''
 
 // ports/epub-import.ts  — MUST NOT persist (today epub-importer.ts:10 imports book-store)
 preview(bytes: Buffer): Promise<EpubPreview>
@@ -206,16 +229,16 @@ Each port ships `ports/<name>.fake.ts` (in-memory, recording) and `ports/<name>.
 1. **SSE regressions** (`reply.hijack`, `raw.writeHead`, `request.raw.on('close')`) are invisible to unit tests. Require ≥1 P0 characterization test per streaming route asserting event order; manual boot per slice.
 2. **Timeout semantics move** into the adapter. Verify `AbortSignal.any` (Node 24 ✓, Electron 40 ✓); keep manual controller-combining as fallback.
 3. **Electron packaging**: adapters change the rollup `external()` list in `vite.config.ts`; `epub-gen-memory`'s double-default and `kokoro-js`/`@huggingface/transformers` must stay external. Gate `electron:preview` after S5 and S12.
-4. **Sanctioned behavior change**: 8 MCP CRUD routes (books.ts:2065, 2089, 2106, 2123, 2145, 2171, 2194, 2237) call `.parse()` unguarded and currently return **500** on bad input; `parseBody` makes them **400**. Architect accepted: document in the PR, update those P0 assertions — the only assertion edit allowed this phase.
+4. **Sanctioned behavior change**: 8 MCP CRUD routes call `.parse()` unguarded and currently return **500** on bad input; `parseBody` makes them **400**. Architect accepted. At the current file size those unguarded parses sit at books.ts:1997, 2021, 2038, 2055, 2077, 2103, 2126, 2169. They have no test coverage at all, so nothing is edited and new tests assert the 400 instead (section 0 item 3).
 5. **Singleton→factory**: `epub-importer`, `audiobook-generator`, `mcp-server` import `* as store` at module scope. All must convert in S5 or keep a shim; a half-converted state silently writes to the production data dir.
 6. **Test-mock debt**: `vi.mock('../../lib/data-dir.js')` must be deleted when the factory lands, otherwise tests pass against the wrong directory.
 7. `electron/main.ts:385` sets `fastify.mermaidRenderer`; switching to `setDiagramRenderer` requires an `electron/` edit — coordinate with the orchestrator on that file.
 8. **Scope**: ~40 new files. Every slice must be independently mergeable and leave the app green.
 
 ## 9. Phase gate
 
-- `pnpm test` green, count ≥ 232 + new contract/service tests (Phase 0 corrected the inflated 384 baseline — vitest was collecting nested worktree copies; the true post-P0 count is 232); `tsc --noEmit` clean; ESLint 0 warnings including new boundary rules.
+- `pnpm test` green, count ≥ 481 + new adapter/service tests (see section 0 item 1 for how the baseline moved: 315 on master, 479 at S4, 481 after the stage 2 items); `tsc --noEmit` clean; ESLint 0 warnings including new boundary rules.
 - Grep gates: `rg "from 'ai'" server/{routes,services}` = 0 · `rg "node:fs" server/routes` = 0 · `rg "5 \* 60 \* 1000" server` = 0 · `rg "instanceof ZodError" server` = 0 · `rg "await import\(" server/routes` = 0 · no route file > 200 lines · `server/routes/books.ts` gone.
 - Contract test exists for all mandated ports; hermetic ones also run against the real adapter.
 - `electron:dev` + `electron:preview` boot; manual E2E once: create book (SSE) → read → feedback+quiz → generate next → export EPUB → audiobook range request returns 206.
-- P0 characterization assertions unchanged except the documented 400/500 item.
+- P0 characterization assertions unchanged. The 400/500 item edits none of them, because those eight routes had no coverage to edit, so it ships with new tests instead (section 0 item 3).

electron/main.ts

@@ -6,6 +6,7 @@ import { readFile, writeFile, mkdir, rm } from 'node:fs/promises'
 import { existsSync } from 'node:fs'
 import { getDataDir } from '@shared/node/data-dir.js'
 import { startServer } from '../server/index.js'
+import { createElectronDiagramRenderer } from '../server/adapters/electron-diagram-renderer.js'
 
 const __dirname = path.dirname(fileURLToPath(import.meta.url))
 const esmRequire = createRequire(import.meta.url)
@@ -323,8 +324,20 @@ let apiPort = 0
 
 app.whenReady().then(async () => {
   await migrateLegacyPlaintextKeys()
-  // Start the embedded API server on a random free port (localhost only — no firewall prompt)
-  const server = await startServer(0, '127.0.0.1')
+  // Start the embedded API server on a random free port (localhost only — no firewall prompt).
+  // The diagram renderer goes in as a port override rather than being
+  // assigned onto the built server afterwards, so the server is never
+  // briefly live on the kroki.io fallback it will not use, and Electron
+  // never has to cast away the instance's type to reach a decoration.
+  // Renders to PNG <img> tags — SVGs render poorly in most e-readers, and
+  // this one works offline, unlike the kroki.io default.
+  const server = await startServer(0, '127.0.0.1', {
+    diagramRenderer: createElectronDiagramRenderer({
+      BrowserWindow,
+      dataDir,
+      resolveMermaidPath: () => esmRequire.resolve('mermaid/dist/mermaid.min.js'),
+    }),
+  })
   const addr = server.server.address()
   apiPort = typeof addr === 'object' && addr ? addr.port : 0
 
@@ -376,86 +389,6 @@ app.whenReady().then(async () => {
     } catch { /* swallow */ }
   })
 
-  // Override mermaid renderer with Electron BrowserWindow-based renderer
-  // (faster and works offline, unlike the kroki.io API fallback).
-  // Renders to PNG <img> tags — SVGs render poorly in most e-readers.
-  const { sanitizeMermaidChart } = await import('@shared/sanitize-mermaid.js')
-  const { mermaidInitConfig } = await import('@shared/mermaid-theme.js')
-
-  ;(server as unknown as { mermaidRenderer: unknown }).mermaidRenderer = async (charts: string[]) => {
-    if (charts.length === 0) return []
-
-    const win = new BrowserWindow({
-      show: false,
-      width: 1600,
-      height: 1200,
-      webPreferences: { offscreen: true },
-    })
-
-    try {
-      const mermaidPath = esmRequire.resolve('mermaid/dist/mermaid.min.js')
-      const mermaidJs = await readFile(mermaidPath, 'utf-8')
-
-      const tmpHtml = path.join(dataDir, 'mermaid-renderer.html')
-      // Safe: mermaidJs is from a trusted local npm package, not user input
-      await writeFile(tmpHtml, `<!DOCTYPE html>
-<html><head><meta charset="utf-8">
-<style>body { margin: 0; background: white; }</style>
-</head><body>
-<div id="output"></div>
-<script>${mermaidJs}<` + `/script>
-<script>
-  mermaid.initialize(${JSON.stringify({ ...mermaidInitConfig, theme: 'default' })});
-<` + `/script>
-</body></html>`, 'utf-8')
-
-      await win.loadFile(tmpHtml)
-
-      const results: string[] = []
-      for (let i = 0; i < charts.length; i++) {
-        const sanitized = sanitizeMermaidChart(charts[i])
-        try {
-          // Render mermaid SVG, insert into DOM, then capture page as PNG
-          const dimensions: { width: number; height: number } = await Promise.race([
-            win.webContents.executeJavaScript(`
-              (async () => {
-                const { svg } = await mermaid.render('epub-chart-${i}', ${JSON.stringify(sanitized)});
-                const output = document.getElementById('output');
-                output.replaceChildren();
-                output.insertAdjacentHTML('afterbegin', svg);
-                const svgEl = output.querySelector('svg');
-                const rect = svgEl.getBoundingClientRect();
-                return { width: Math.ceil(rect.width) + 20, height: Math.ceil(rect.height) + 20 };
-              })()
-            `),
-            new Promise<never>((_, reject) =>
-              setTimeout(() => reject(new Error('Mermaid render timeout')), 10_000)
-            ),
-          ])
-
-          // Resize to fit diagram and capture as PNG
-          win.setContentSize(Math.max(dimensions.width, 200), Math.max(dimensions.height, 100))
-          await new Promise(r => setTimeout(r, 100))
-          const image = await win.webContents.capturePage()
-          const pngBuffer = image.toPNG()
-          // Save to temp file — epub-gen-memory doesn't support data: URLs
-          const tmpPng = path.join(dataDir, `mermaid-chart-${i}.png`)
-          await writeFile(tmpPng, pngBuffer)
-          const { pathToFileURL } = await import('node:url')
-          results.push(`<img src="${pathToFileURL(tmpPng).href}" alt="diagram" style="max-width:100%"/>`)
-        } catch (err) {
-          console.warn('[mermaid-renderer] Chart ' + i + ' failed:', err)
-          results.push('<pre><code class="language-mermaid">' + sanitized.replace(/</g, '&lt;').replace(/>/g, '&gt;') + '</code></pre>')
-        }
-      }
-
-      await rm(tmpHtml).catch(() => {})
-      return results
-    } finally {
-      win.destroy()
-    }
-  }
-
   // POST all saved API keys to the server's key store. loadApiKey handles
   // identity-suffix paths, legacy fallback, and migration; failures are
   // logged but never delete the .enc file.

server/adapters/ai-sdk-text-generation.test.ts

@@ -0,0 +1,123 @@
+import { describe, expect, it } from 'vitest'
+import type { LanguageModel } from 'ai'
+import { AI_GENERATION_TIMEOUT_MS } from '../constants.js'
+import { createFakeKeyVault } from '../ports/key-vault.fake.js'
+import { composeAbortSignal, resolveModelClient } from './ai-sdk-text-generation.js'
+
+// resolveModelClient's return type, LanguageModel, is a union that also
+// allows a bare model-id string, for a registry-style global provider. The
+// real implementation never returns that variant. It always calls one of
+// the three provider factories and returns the resulting model object.
+// This narrows that union back down so tests can read `.modelId` off it.
+function modelIdOf(model: LanguageModel): string {
+  if (typeof model === 'string') {
+    throw new Error('expected a resolved model object, got a bare model id string')
+  }
+  return model.modelId
+}
+
+/**
+ * The TextGeneration contract (server/ports/text-generation.contract.ts) is
+ * fake-only and is never run against this adapter here. Doing so would
+ * spend real money against a live provider and reach the network. These
+ * tests instead cover the two parts of the adapter that are hermetically
+ * testable without a network call or an API key. Those are provider and
+ * model resolution, and the timeout signal composition. Constructing a
+ * model client below never phones home. Every AI SDK provider factory is
+ * lazy, only making a request once the returned model is actually used to
+ * generate or stream something, which none of these tests do.
+ */
+
+describe('resolveModelClient', () => {
+  it('throws for a provider the app does not know about', () => {
+    const keyVault = createFakeKeyVault({ anthropic: 'sk-test' })
+
+    expect(() => resolveModelClient(keyVault, 'not-a-real-provider', 'claude-sonnet-4-6'))
+      .toThrow('Invalid provider: not-a-real-provider')
+  })
+
+  it('throws for a model identifier with characters outside MODEL_REGEX', () => {
+    const keyVault = createFakeKeyVault({ anthropic: 'sk-test' })
+
+    expect(() => resolveModelClient(keyVault, 'anthropic', 'not a valid model!'))
+      .toThrow('Invalid model identifier: not a valid model!')
+  })
+
+  it('throws when the vault has no key for the requested provider', () => {
+    const keyVault = createFakeKeyVault()
+
+    expect(() => resolveModelClient(keyVault, 'anthropic', 'claude-sonnet-4-6'))
+      .toThrow('No API key configured for provider: anthropic')
+  })
+
+  it('resolves an anthropic model client once a key is present', () => {
+    const keyVault = createFakeKeyVault({ anthropic: 'sk-test-dummy' })
+
+    const model = resolveModelClient(keyVault, 'anthropic', 'claude-sonnet-4-6')
+
+    expect(model).toBeTruthy()
+    expect(modelIdOf(model)).toBe('claude-sonnet-4-6')
+  })
+
+  it('resolves an openai model client once a key is present', () => {
+    const keyVault = createFakeKeyVault({ openai: 'sk-test-dummy' })
+
+    const model = resolveModelClient(keyVault, 'openai', 'gpt-4o')
+
+    expect(model).toBeTruthy()
+    expect(modelIdOf(model)).toBe('gpt-4o')
+  })
+
+  it('resolves a google model client once a key is present', () => {
+    const keyVault = createFakeKeyVault({ google: 'test-dummy-key' })
+
+    const model = resolveModelClient(keyVault, 'google', 'gemini-2.0-flash')
+
+    expect(model).toBeTruthy()
+    expect(modelIdOf(model)).toBe('gemini-2.0-flash')
+  })
+})
+
+describe('composeAbortSignal', () => {
+  it('is not aborted when neither the caller signal nor the timeout has fired', () => {
+    const combined = composeAbortSignal(undefined, AI_GENERATION_TIMEOUT_MS)
+
+    expect(combined.aborted).toBe(false)
+  })
+
+  it('aborts the composed signal when the caller signal aborts', () => {
+    const controller = new AbortController()
+    const combined = composeAbortSignal(controller.signal, AI_GENERATION_TIMEOUT_MS)
+
+    expect(combined.aborted).toBe(false)
+    controller.abort()
+    expect(combined.aborted).toBe(true)
+  })
+
+  // AbortSignal.timeout()'s countdown runs on the runtime's own native
+  // timer, not the global setTimeout that vi.useFakeTimers() intercepts.
+  // This was verified empirically, advancing fake timers by the full
+  // timeout duration never fires a real AbortSignal.timeout(). So the
+  // "internal timeout also aborts the composed signal" behaviour below is
+  // exercised with a real, but tiny, timeout instead of a faked
+  // five-minute one. The `timeoutMs` parameter exists for exactly this.
+  // Production code never passes it, and always relies on the
+  // AI_GENERATION_TIMEOUT_MS default.
+
+  it('aborts the composed signal on its own once the internal timeout elapses', async () => {
+    const combined = composeAbortSignal(undefined, 10)
+
+    expect(combined.aborted).toBe(false)
+    await new Promise((resolve) => setTimeout(resolve, 50))
+    expect(combined.aborted).toBe(true)
+  })
+
+  it('aborts on its own timeout even when a caller signal is provided and never fires', async () => {
+    const controller = new AbortController()
+    const combined = composeAbortSignal(controller.signal, 10)
+
+    await new Promise((resolve) => setTimeout(resolve, 50))
+
+    expect(combined.aborted).toBe(true)
+  })
+})