diff --git a/docs/plans/refactor/phase-2.md b/docs/plans/refactor/phase-2.md index b50d030..8a1c4d4 100644 --- a/docs/plans/refactor/phase-2.md +++ b/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/.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 // ports/diagram-renderer.ts -render(charts: string[]): Promise // markup per chart; '' = failed +render(charts: string[]): Promise // 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 @@ -206,7 +229,7 @@ Each port ships `ports/.fake.ts` (in-memory, recording) and `ports/. 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. @@ -214,8 +237,8 @@ Each port ships `ports/.fake.ts` (in-memory, recording) and `ports/. ## 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). diff --git a/electron/main.ts b/electron/main.ts index 32c3ac0..04cfd58 100644 --- a/electron/main.ts +++ b/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 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 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, ` - - - -
-